Page MenuHomePhabricator

Remove daemon envHash and envInfo
ClosedPublic

Authored by joshuaspence on Nov 9 2015, 11:04 AM.
Tags
None
Referenced Files
F14061545: D14446.diff
Mon, Nov 18, 7:10 AM
F14058755: D14446.id34925.diff
Sun, Nov 17, 2:29 PM
F14058754: D14446.id34938.diff
Sun, Nov 17, 2:29 PM
F14058753: D14446.id34973.diff
Sun, Nov 17, 2:29 PM
F14058752: D14446.id34913.diff
Sun, Nov 17, 2:29 PM
F14049282: D14446.diff
Thu, Nov 14, 12:17 PM
F14035364: D14446.diff
Sun, Nov 10, 5:37 AM
F14029319: D14446.id34974.diff
Fri, Nov 8, 8:26 PM
Tokens
"Mountain of Wealth" token, awarded by epriestley.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T7053: Restart daemons automagically
Commits
Restricted Diffusion Commit
rP321c61a853d9: Remove daemon envHash and envInfo
Summary

Ref T7053. Remove the envHash and envInfo fields, which are no longer used now that the daemons restart automagically. Depends on D14458.

Test Plan

Saw no more setup issues.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Don't cache config.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)

I'm hesitant about this approach because:

  • Load scales with the number of daemons, and I expect some Harbormaster/Drydock installs to have a very large number of active daemons in a few years.
  • Some configuration has already been processed by the time we reach run().

Now that we have a single overseer per daemon group, I think we can reasonably do this to get slightly better behavior on each dimension:

  • Have it emit some sort of periodic status event (maybe just call doOverseerStuff() on all subclasses of some new PhutilDaemonOverseerExtension or whatever -- I don't think we need to use real events for this).
  • Phabricator listens for that event/call and does a SELECT version FROM config sort of call.
  • If the database version is newer than the overseer version, make some call which tells the overseer to SIGHUP all the subprocesses to restart them.

Then we have to version config in the database, but that's straightforward.

This gives us better scalability (smaller, more efficient query with fewer processes issuing it less often) and more complete restart behavior (entire processes restart). This is also a little more flexible, although I'm not totally sure it's all that useful -- but it would give us a reasonable workaround for cases like T3848, where you drop a RestartTheDaemonsEverySoOften.php extension into src/extensions/ if you have a mysterious ghost problem that we can't reproduce.

A small amount of configuration (like phd.taskmasters and phd.user) would still require restarts to take effect, but I think the number of options would be minimal at that point and we could reasonably just enumerate all of it and have warnings in the UI.

joshuaspence edited edge metadata.

Attempt to restart daemons every 5 seconds.

I"m just playing around with getting the daemons to reload, but this doesn't seem to work... any thoughts?

sh: 1: kill: No such process
joshuaspence retitled this revision from Don't cache config to Restart daemons automagically.Nov 9 2015, 8:11 PM

How did you imagine the versioning might work? I was thinking we could add a magic config.hash configuration which would be updated whenever some configuration is changed and which is added to phd.variant-config.

Hmm, I'm not sure why this doesn't work -- I'd expect this to work correctly as written.

For the version stuff, I think we can just stick an integer number somewhere and bump it by one on any config change (even just the maximum ID in the config_transaction table would be fine as a v0).

The hash stuff was to capture config changes from many different possible sources, but only the web source has a meaningful support/confusion cost. I think it's fine if editing a config file on disk does not automatically restart the daemons (I wouldn't expect it to either, and doing so would be weird/confusing for administrators in some cases, I think).

joshuaspence changed the repository for this revision from rP Phabricator to rPHU libphutil.
joshuaspence retitled this revision from Restart daemons automagically to Add a `config.hash` config entry and remove daemon envHash.Nov 10 2015, 10:33 AM
joshuaspence updated this object.

Can we just use SELECT MAX(id) FROM config_transaction (at least, for now, with some reasonable wrapper around it) instead of introducing config.hash? Rest of this looks good, the config.hash stuff just seems more complex than needbe. That would also let us throw away calculateEnvironmentHash(), I think, although that method isn't terribly complex.

The MAX(id) method is a little flimsy but I can't actually come up with anything that's wrong with it. Worst case is that it dirties too often if we start letting you subscribe to config or something, I think, but that doesn't seem like a big deal.

joshuaspence retitled this revision from Add a `config.hash` config entry and remove daemon envHash to Remove daemon envHash and envInfo.Nov 10 2015, 7:46 PM
  • Can we get rid of calculateEnvironmentInfo() and calculateEnvironmentHash() completely now?
  • Can we get rid of phd.variant-config (and add it to PhabricatorExtraConfigSetupCheck)?
This revision is now accepted and ready to land.Nov 10 2015, 9:54 PM
This revision was automatically updated to reflect the committed changes.