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.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 7:16 PM
Unknown Object (File)
Tue, Mar 26, 7:16 PM
Unknown Object (File)
Tue, Mar 26, 7:15 PM
Unknown Object (File)
Tue, Mar 26, 7:15 PM
Unknown Object (File)
Tue, Mar 26, 7:15 PM
Unknown Object (File)
Tue, Mar 26, 7:15 PM
Unknown Object (File)
Tue, Mar 26, 7:15 PM
Unknown Object (File)
Feb 16 2024, 5:31 AM
Subscribers
None

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 Passed
Unit
Tests Passed
Build Status
Buildable 23011
Build 31591: Run Core Tests
Build 31590: arc lint + arc unit

Event Timeline

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
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.

  • Wordsmithing: "any instance" -> "all instances"