Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7053: Restart daemons automagically
- Commits
- Restricted Diffusion Commit
rP321c61a853d9: Remove daemon envHash and envInfo
Saw no more setup issues.
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 8783 Build 10230: Run Core Tests Build 10229: arc lint + arc unit
Event Timeline
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.
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
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).
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.
- Can we get rid of calculateEnvironmentInfo() and calculateEnvironmentHash() completely now?
- Can we get rid of phd.variant-config (and add it to PhabricatorExtraConfigSetupCheck)?