Page MenuHomePhabricator

Make `./bin/aphlict` behave more like a service.
ClosedPublic

Authored by joshuaspence on May 20 2014, 10:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 7:05 PM
Unknown Object (File)
Thu, Dec 19, 12:55 AM
Unknown Object (File)
Wed, Dec 18, 6:11 PM
Unknown Object (File)
Tue, Dec 17, 8:23 PM
Unknown Object (File)
Sun, Dec 15, 4:54 PM
Unknown Object (File)
Fri, Dec 13, 10:20 AM
Unknown Object (File)
Wed, Dec 11, 10:59 AM
Unknown Object (File)
Tue, Dec 10, 8:44 PM

Details

Summary

Fixes T5126. Provide start, stop, restart, debug and status workflows for ./bin/aphlict. This makes it easier to manage Aphlict as if it were a service.

Test Plan
> sudo ./bin/aphlict status
Aphlict is not running.

> sudo ./bin/aphlict stop
Aphlict is not running.

> sudo ./bin/aphlict start
Aphlict Server started.

> sudo ./bin/aphlict status
Aphlict (12880) is running.

> sudo ./bin/aphlict restart
Stopping Aphlict Server (12880)...
Aphlict Server (12880) exited normally.
Aphlict Server started.

> sudo ./bin/aphlict stop
Stopping Aphlict Server (12895)...
Aphlict Server (12895) exited normally.

> sudo ./bin/aphlict debug
Starting Aphlict server in foreground...
Launching server:

    $ node '/usr/src/phabricator/src/applications/aphlict/management/../../../../support/aphlict/server/aphlict_server.js' --port='22280' --admin='22281' --host='localhost' --user='aphlict'

[Fri May 30 2014 09:56:14 GMT+0000 (UTC)] Started Server (PID 12911)

Diff Detail

Repository
rP Phabricator
Branch
aphlict-management
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 737
Build 737: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Make `./bin/aphlict` behave more like a service..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

Please make sure there's a way to run this in the old way. systemd automatically manages daemons so the old way of daemonizing processes isn't required on systemd-based systems.

I'm generally OK with this but it will break existing installs (by changing how aphlict is invoked) so I'd like to hold it until the next notification phase is a little further along and we can hopefully just do all the breaks at once with supporting documentation and such.

@hach-que, do you use systemd for the normal daemons (vs aphlict)? This is very similar to phd now. Is there something missing that allows phd to work but not aphlict?

At the moment phd is configured under systemd to have Type=forking, which is a fully supported mode and still works fine. Obviously having daemons not double fork is better architecturally and makes the daemon code much more simple (most normal services I configure under systemd I use a foregrounding option if they have it). Since all major Linux distributions are either already using systemd or are planning to move to it, I'm not sure how much sense it makes to add double fork logic / self management of PID files, etc these days.

In phd's case it has to fork multiple processes anyway, so it mostly makes sense as something that forks anyway (as an aside, there is a way that phd could use systemd to manage the daemons it spawns, and probably simplify a bunch of Phabricator's daemon management by doing so).

Can you just use Type=forking for this, too, then?

Managing our own daemons currently seems to work well for most users (the only complaint I'm aware of is that there's no standard way to get them started on system startup). It's also relatively easy to support, because you can't really misconfigure phd, I know how phd works, commands like phd status can print out exactly the right information, having control over the workflow lets us do things like freeing leases on phd start (D9256), we can manage which daemons start based on configuration, we can adjust how daemons start easily (e.g., with debug flags), and detect hung processes with a well-known heartbeat signal. I think these kinds of things would become far more challenging and complex if we relied on systemd instead, and it would only really solve one relatively small problem (daemon startup on machine start).

I can add Type=forking for this as well.

Okay, cool.

I do want to hold this for a bit so we don't have to hit users with two "a bunch of stuff changed, go read the documentation again" issues in short succession, and AFAIK this doesn't unblock anything on its own, but let's plan to move forward with this once T4324 is far enough along to justify a doc update.

joshuaspence edited edge metadata.
  • Various minor improvements.
  • Don't require root for things like ./bin/aphlict status.
  • Fix method name.
  • Launch in non-debug mode by default.
  • Fix method name
  • Fix aphlict_server.js path.
  • Fix broken conditional
  • Move some stuff from executeStartCommand to launch
  • Move code for removing PID file.
  • Fix inverted logic
  • Use global variable to hold state :(
  • Make sure the error messages actually display.
  • SIGTERM instead of SIGKILL
  • Add newlines to error messages.
  • Remove the PID file from the ./bin/aphlict stop workflow.
  • Use SIGINT rather than SIGTERM.
  • Use SIGKILL if the notifications server won't die.
  • Minor tidying.
  • Made some methods final.
  • Minor changes for consistency with ./bin/phd
  • Minor fix
  • Delete PID file on SIGINT/SIGTERM

If we wanted to land this earlier, we could just (temprarily) restore the existing functionality by adding something like this to aphlict_launcher.php:

if (!$argv) {
  $argv = array('start');
} 
if ($argv == array('--foreground')) { // or similar
  $argv = arraay('debug');
}
  • Various minor improvements.
  • Don't require root for things like ./bin/aphlict status.
  • Fix method name.
  • Launch in non-debug mode by default.
  • Fix method name
  • Fix aphlict_server.js path.
  • Fix broken conditional
  • Move some stuff from executeStartCommand to launch
  • Move code for removing PID file.
  • Fix inverted logic
  • Use global variable to hold state :(
  • Make sure the error messages actually display.
  • SIGTERM instead of SIGKILL
  • Add newlines to error messages.
  • Remove the PID file from the ./bin/aphlict stop workflow.
  • Use SIGINT rather than SIGTERM.
  • Use SIGKILL if the notifications server won't die.
  • Minor tidying.
  • Made some methods final.
  • Minor changes for consistency with ./bin/phd
  • Minor fix
  • Minor fix
  • Delete PID file on SIGINT/SIGTERM
  • Only give the aphlict server 5 seconds to terminate.
  • Improve output messages.
  • Improve check for node.
  • Make csprintf a bit safer.
epriestley edited edge metadata.

I think this can land now that we're changing protocols, bumping versions, and installs have to restart things anyway.

One minor inline but we can clean that up later.

src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
20–24

plz 2 use property

plz T.T

This revision is now accepted and ready to land.Jun 5 2014, 7:13 PM
epriestley updated this revision to Diff 22418.

Closed by commit rP8a7a7dcbf1cb (authored by @joshuaspence, committed by @epriestley).