Ref T7053. Add a PhutilDaemonOverseerModule class which can be used to externally influence daemons.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7053: Restart daemons automagically
- Commits
- rPHU66bf71f94817: Add daemon overseer modules to allow daemons to be externally reloaded
See D14458.
Diff Detail
- Repository
- rPHU libphutil
- Branch
- master
- Lint
Lint Passed Severity Location Code Message Advice src/daemon/PhutilDaemonOverseerModule.php:4 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 8772 Build 10211: Run Core Tests Build 10210: arc lint + arc unit
Event Timeline
src/daemon/PhutilDaemonOverseer.php | ||
---|---|---|
318 | Probably "...not fully used all the available capacity." or something. |
src/daemon/PhutilDaemonOverseer.php | ||
---|---|---|
239–245 | Yes I know that this is bad and wrong, but I'm not entirely sure how to do this properly because PhutilDaemonOverseer doesn't seem to emit any events. |
I'm thinking some kind of new module/extension class:
abstract class PhutilDaemonOverseerModule extends Phobject { // ... $modules = id(new PhutilClassMapQuery()) ->setAncestorClass(__CLASS__) ->execute() // ... }
And then build the listeners:
$this->modules = PhutilDaemonOverseerModule::getAllModules();
And then you can call them to change behavior:
foreach ($this->modules as $module) { if ($module->shouldRestartDaemons()) { $this->restartEverything(); } }
I think this should work because phabricator/scripts/daemon/phd-daemon loads the whole Phabricator environment.
We could also use real events but I think classtrees are probably a bit better here, probably. I generally prefer classtrees over events where we can reasonably use them, since I think the totality of their behaviors are much better.
Introduce PhutilDaemonOverseerModule (I don't think this is a great name for this class, but can't think of anytihng better at the moment)
src/daemon/PhutilDaemonHandle.php | ||
---|---|---|
260 | I guess this relies on the POSIX extension, which is probably why you originally used exec('kill ...'). I'll see if I can get this working without this change. |
I don't love the name either, I'll yell if I come up with anything better.
I think we need posix anyway (e.g., for posix_getpgid() and posix_setsid()).
The original kill probably was to avoid posix, but I think we're fine relying on it now.
src/daemon/PhutilDaemonOverseer.php | ||
---|---|---|
193–197 | Maybe try/catch each call (and phlog() + continue on exception, I guess?) |
src/daemon/PhutilDaemonHandle.php | ||
---|---|---|
260 | Oh, except I think this has changed the meaning. The original is -{$pgid} (note negative -). I think passing $pgid won't do the same thing; I'm not sure if passing -$pgid will have the same effect or not. You can probably test this by starting PhutilProcessGroupDaemon with bin/phd launch and then trying to stop it. If it stops -- and the resist-death.php script actually gets killed -- you should be good. If it doesn't stop or resist-death.php hangs around in the process list, the processgroup isn't being killed properly. Generally, the goal here is to make sure all the daemons' subprocesses get killed. Without this stuff, hung subprocesses like git and svn can stick around forever. |
src/daemon/PhutilDaemonHandle.php | ||
---|---|---|
260 | Yeah it didn't seem to work |
src/daemon/PhutilDaemonHandle.php | ||
---|---|---|
260 | Actually the original version doesn't seem to work for me either... vagrant@phabricator:/usr/src/phabricator$ sudo /usr/src/phabricator/bin/phd stop Interrupting process 2449... Terminating process 2449... Process 2449 exited. vagrant@phabricator:/usr/src/phabricator$ ps aux | grep resist phd 2454 0.2 0.6 271836 13808 ? S 20:21 0:00 php /usr/src/libphutil/scripts/daemon/torture/resist-death.php vagrant 2465 0.0 0.0 10460 936 pts/3 S+ 20:21 0:00 grep --color=auto resist |
master does seem to work properly for me locally (OS X):
epriestley@orbital ~/dev/phabricator $ ps auxwww | grep -i resist epriestley 89251 0.0 0.0 2432772 672 s025 S+ 12:38PM 0:00.00 grep -i resist epriestley 89245 0.0 0.1 2564196 11576 ?? S 12:37PM 0:00.02 php /Users/epriestley/dev/core/lib/libphutil/scripts/daemon/torture/resist-death.php epriestley@orbital ~/dev/phabricator $ ./bin/phd stop Interrupting process 89243... Terminating process 89243... Process 89243 exited. epriestley@orbital ~/dev/phabricator $ ps auxwww | grep -i resist epriestley 89258 0.4 0.0 2432772 672 s025 S+ 12:38PM 0:00.00 grep -i resist
Ah I see... it works if I launch PhutilProcessGroupDaemon, but not if I run it with debug.
Ah, yeah. With debug we don't do process group separation (since things are staying in the foreground), so that's expected.
Rest of this looks reasonable to me.