Page MenuHomePhabricator

Drive "phd stop" entirely from the process list, not PID files on disk
ClosedPublic

Authored by epriestley on Jun 20 2019, 9:14 PM.

Details

Summary

Ref T13321. Depends on D20600. Make bin/phd stop mean:

  • bin/phd stop: Stop all processes which have daemon process titles. If we're instanced, only stop daemons for the current instance.
  • bin/phd stop --force: Stop all processes which have deamon process titles for any instance.

We no longer read or care about PID files on disk, and this moves us away from PID files.

This makes unusual flag --gently do nothing. A followup will update the documentation and flags to reflect actual usage/behavior.

This also removes the ability to stop specific PIDs. This was somewhat useful long, long ago when you might explicitly run different copies of the PullLocal daemon with flags to control which repositories they updated, but with the advent of clustering it's no longer valid to run custom daemon loadouts.

Test Plan

Ran bin/phd start, then bin/phd stop. Saw instance daemons stop. Ran bin/phd stop --force, saw all daemons stop.

Diff Detail

Repository
rP Phabricator
Branch
pquery2
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 23008
Build 31585: Run Core Tests
Build 31584: arc lint + arc unit

Event Timeline

epriestley created this revision.Jun 20 2019, 9:14 PM
epriestley requested review of this revision.Jun 20 2019, 9:15 PM
amckinley accepted this revision.Jun 20 2019, 10:22 PM
amckinley added inline comments.
src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
417

Don't you think we should distinguish between "no daemons found" and "we couldn't run the query"? If for no other reason than to more-quickly shake out weird distros where our ps invocation doesn't work. (Incidentally, it does work on my Mac, so hooray for that).

422

"for all instances"

This revision is now accepted and ready to land.Jun 20 2019, 10:22 PM
epriestley added inline comments.Jun 20 2019, 10:40 PM
src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
417

We'll still phlog($ex), so Solaris should get something like this:

EXCEPTION: ps doesn't take these flags on solaris ha ha ha
NO DAEMONS

...which should be reasonably clear, and if anyone hits this maybe we can write phutil_is_solaris() and find the right ps incantation on Solaris.

On Windows I think we just fatal instantly anyway since there's no posix or pcntl.

So, practically, I don't think any paths today will lead to messaging that just says "no stuff" when the root cause is really "ProcessQuery can't deal with it", although this is sort of a product of me arranging things to hit a convenient series of coincidences rather than an explicit rule of the system.

epriestley updated this revision to Diff 49165.Jun 24 2019, 5:58 PM
  • Wordsmithing: "any instance" -> "all instances"
epriestley updated this revision to Diff 49166.Jun 24 2019, 6:00 PM
  • Rebase to clear tests.