Page MenuHomePhabricator

Fix HHVM restarting routines
ClosedPublic

Authored by Joe on Oct 12 2016, 1:54 PM.

Details

Maniphest Tasks
T104352: Make scap able to depool/repool servers via the conftool API
T146656: remove hard-coded upstart commands
Reviewers
thcipriani
mmodell
fgiunchedi
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Release-Engineering-Team
Commits
rMSCA8eadbca15769: Fix HHVM restarting routines
Patch without arc
git checkout -b D411 && curl -L https://phabricator.wikimedia.org/D411?download=true | git apply
Summary

Fix the cli tool, also unbreak sync and hhvm-graceful

Subject: The cli tool is now supporting both upstart and systemd-based
distros. Commands "sync" and "hhvm-graceful" both try to use an
inexistent task "restart_hhvm", and implementing it is not exactly
an easy task. Raise and manage a NotImplementedError while
we have no implementation for it.

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Joe updated this revision to Diff 1092.Oct 12 2016, 1:54 PM
Joe retitled this revision from to Fix HHVM restarting routines.
Joe updated this object.
Joe edited the test plan for this revision. (Show Details)
Joe added reviewers: thcipriani, fgiunchedi.
Joe added a subscriber: Joe.
Restricted Application added a reviewer: Restricted Owners Package.Oct 12 2016, 1:54 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20161012135439.152~1.gbp044186
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/152/ for more details.

Joe updated this revision to Diff 1093.Oct 12 2016, 2:01 PM
Joe edited edge metadata.

Fix username

Restricted Application edited edge metadata.Oct 12 2016, 2:01 PM

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20161012140137.153~1.gbp057b2e
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/153/ for more details.

thcipriani accepted this revision.Oct 12 2016, 3:58 PM
thcipriani edited edge metadata.

Nice, addresses a few tasks: T104352: Make scap able to depool/repool servers via the conftool API and T146656: remove hard-coded upstart commands

Will likely need a rebase before landing since I landed D404: Restore restart_hhvm. I have a few questions inline (is_service_running may need to be tweaked).

scap/main.py
735

maybe just subprocess.CalledProcessError here.

737

Is there any way this could fail to depool after /usr/local/bin/depool calls out to confctl that we could/should handle here?

741

maybe just subprocess.CalledProcessError here

746

Good catch. Thank you.

scap/tasks.py
394 ↗(On Diff #1093)

This was previously implemented and erroneously removed. I restored in D404 which I just landed.

scap/utils.py
738

Seems like pid here would be something like MainPID=0. I think this may always return True.

This revision is now accepted and ready to land.Oct 12 2016, 3:58 PM
Joe marked an inline comment as done.Oct 17 2016, 9:23 AM

@thcipriani thanks for the comments, but I am going to rework this patch based on a) your fix of the graceful restart b) using the script me and mobrovac created for the more general case.

scap/main.py
737

Not really, but in the meantime me and mobrovac created a script that handles all of this way better than scap does.

For simplicity and consistency, I'll probably rework this change into using it

scap/tasks.py
394 ↗(On Diff #1093)

Heh, ok, Didn't really see it :)

scap/utils.py
738

yeah this was also a bad idea in general, I'll rewrite this.

Joe updated this revision to Diff 1107.Oct 17 2016, 9:50 AM
Joe edited edge metadata.
  • Use restart-hhvm which already DTRT
  • Use systemctl is-active instead of searching for the main pid
  • Remove definition of tasks.restart_hhvm

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20161017095017.160~1.gbpd13a30
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/160/ for more details.

One comment inline about is_service_running raising CalledProcessError. One nitpick about an exception that should no longer raise. Both minor issues.

scap/main.py
405

can probably remove since it won't raise this exception any more.

scap/utils.py
736

This seems to exit non-zero when service is inactive which raises subprocess.CalledProcessError.

738

I think this should be (pid == 'active')

mmodell accepted this revision.Oct 28 2016, 6:23 PM
mmodell edited edge metadata.
mmodell added inline comments.
scap/utils.py
739–746

I tested this on an ubuntu host in labs and it seems to work as intended.

This revision was automatically updated to reflect the committed changes.