Page MenuHomePhabricator

Add daemon overseer modules to allow daemons to be externally reloaded
ClosedPublic

Authored by joshuaspence on Nov 10 2015, 10:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 8:13 PM
Unknown Object (File)
Fri, Nov 22, 6:32 AM
Unknown Object (File)
Tue, Nov 19, 8:05 PM
Unknown Object (File)
Fri, Nov 15, 9:50 PM
Unknown Object (File)
Mon, Nov 11, 4:54 PM
Unknown Object (File)
Sun, Nov 10, 2:12 AM
Unknown Object (File)
Sun, Nov 10, 12:22 AM
Unknown Object (File)
Sat, Nov 9, 11:39 PM
Subscribers

Details

Summary

Ref T7053. Add a PhutilDaemonOverseerModule class which can be used to externally influence daemons.

Test Plan

See D14458.

Diff Detail

Repository
rPHU libphutil
Branch
master
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/daemon/PhutilDaemonOverseer.php:233PHL1Unknown Symbol
Unit
Tests Passed
Build Status
Buildable 8747
Build 10169: Run Core Tests
Build 10168: arc lint + arc unit

Event Timeline

joshuaspence retitled this revision from to Reload daemons when config chanes.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

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

Using posix_kill instead of exec('kill ...') seemed to fix this.

joshuaspence retitled this revision from Reload daemons when config chanes to Reload daemons when config changes.Nov 10 2015, 10:12 AM
joshuaspence edited edge metadata.
src/daemon/PhutilDaemonOverseer.php
319

Probably "...not fully used all the available capacity." or something.

joshuaspence retitled this revision from Reload daemons when config changes to Reload daemons when config chanes.
joshuaspence updated this object.

Change a few more exec('kill ...')

src/daemon/PhutilDaemonOverseer.php
232–238

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.

joshuaspence retitled this revision from Reload daemons when config chanes to Automagically reload daemons when configuration chanes.Nov 10 2015, 7:49 PM
joshuaspence updated this object.

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.

joshuaspence retitled this revision from Automagically reload daemons when configuration chanes to Automagically reload daemons when configuration changes.Nov 10 2015, 7:53 PM
joshuaspence updated this object.
joshuaspence edited edge metadata.

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
191–195

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
joshuaspence retitled this revision from Automagically reload daemons when configuration changes to Add daemon overseer modules to allow daemons to be externally reloaded.Nov 10 2015, 8:38 PM
joshuaspence updated this object.
joshuaspence retitled this revision from Add daemon overseer modules to allow daemons to be externally reloaded to Automagically reload daemons when configuration changes.
joshuaspence updated this object.

Various fixes

joshuaspence retitled this revision from Automagically reload daemons when configuration changes to Add daemon overseer modules to allow daemons to be externally reloaded.Nov 10 2015, 8:38 PM
joshuaspence updated this object.

I will fill out the diff summary once this stuff is more concrete.

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

Doesn't work for me at master on Ubuntu 14.04.

Hmm, it works on our production install running Ubuntu 12.04.

Ah I see... it works if I launch PhutilProcessGroupDaemon, but not if I run it with debug.

joshuaspence marked 4 inline comments as done.

Fix process group stuff

epriestley edited edge metadata.

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.

This revision is now accepted and ready to land.Nov 10 2015, 9:30 PM
This revision was automatically updated to reflect the committed changes.