Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deb systemd should use control-group default for killmode #36806

Closed
wants to merge 1 commit into from
Closed

Deb systemd should use control-group default for killmode #36806

wants to merge 1 commit into from

Conversation

l2ol33rt
Copy link
Contributor

@l2ol33rt l2ol33rt commented Oct 5, 2016

What does this PR do?

Fix deb systemd service files to not use KillMode=process and instead use the default (KillMode=control-group)

What issues does this PR fix or reference?

#33665

The above issue includes commits to fix the salt-master in the develop branch but Im seeing this issue on the salt-minion service in debian 8 on the 2016.3 branch as well. Im assuming this affects any salt service.

Previous Behavior

With a salt-* service running, issue a systemctl stop salt-{minion, master}. That service will hang for around a minute then systemd force kills it:

On the minion:

(salt) root@:vagrant # systemctl status salt-minion
● salt-minion.service - The Salt Minion daemon
   Loaded: loaded (/lib/systemd/system/salt-minion.service; enabled)
   Active: failed (Result: signal) since Wed 2016-10-05 18:13:39 GMT; 19s ago
  Process: 13492 ExecStart=/usr/lib/virtualenvs/salt/bin/salt-minion (code=killed, signal=KILL)
 Main PID: 13492 (code=killed, signal=KILL)

Oct 05 18:12:08 saltmaster systemd[1]: Stopping The Salt Minion daemon...
Oct 05 18:12:08 saltmaster salt-minion[13492]: [WARNING ] Minion received a SIGTERM. Exiting.
Oct 05 18:13:39 saltmaster systemd[1]: salt-minion.service stop-sigterm timed out. Killing.
Oct 05 18:13:39 saltmaster systemd[1]: salt-minion.service: main process exited, code=killed, status=9/KILL
Oct 05 18:13:39 saltmaster systemd[1]: Stopped The Salt Minion daemon.
Oct 05 18:13:39 saltmaster systemd[1]: Unit salt-minion.service entered failed state.
Oct 05 18:13:41 saltmaster salt-minion[13492]: The salt minion is shutdown. Minion received a SIGTERM. Exited.[ERROR   ] Minion process encountered exception: [Errno 3] No such process

On the master:

● salt-master.service - The Salt Master daemon
   Loaded: loaded (/lib/systemd/system/salt-master.service; enabled)
   Active: failed (Result: signal) since Wed 2016-10-05 18:18:04 GMT; 41s ago
  Process: 13547 ExecStart=/usr/lib/virtualenvs/salt/bin/salt-master (code=killed, signal=KILL)
 Main PID: 13547 (code=killed, signal=KILL)

Oct 05 18:16:33 saltmaster systemd[1]: Stopping The Salt Master daemon...
Oct 05 18:16:33 saltmaster salt-master[13547]: [WARNING ] Master received a SIGTERM. Exiting.
Oct 05 18:18:04 saltmaster systemd[1]: salt-master.service stop-sigterm timed out. Killing.
Oct 05 18:18:04 saltmaster systemd[1]: salt-master.service: main process exited, code=killed, status=9/KILL
Oct 05 18:18:04 saltmaster systemd[1]: Stopped The Salt Master daemon.
Oct 05 18:18:04 saltmaster systemd[1]: Unit salt-master.service entered failed state.

New Behavior

With the changes systemctl stop salt-{minion, master} no longer hangs and seems to correct stop.

On the minion:

● salt-minion.service - The Salt Minion daemon
   Loaded: loaded (/lib/systemd/system/salt-minion.service; enabled)
   Active: inactive (dead) since Wed 2016-10-05 20:23:03 GMT; 1s ago
  Process: 15439 ExecStart=/usr/lib/virtualenvs/salt/bin/salt-minion (code=exited, status=0/SUCCESS)
 Main PID: 15439 (code=exited, status=0/SUCCESS)

Oct 05 20:23:03 saltmaster systemd[1]: Stopping The Salt Minion daemon...
Oct 05 20:23:03 saltmaster salt-minion[15439]: [WARNING ] Minion received a SIGTERM. Exiting.
Oct 05 20:23:03 saltmaster salt-minion[15439]: The salt minion is shutdown. Minion received a SIGTERM. Exited.
Oct 05 20:23:03 saltmaster systemd[1]: Stopped The Salt Minion daemon.

On the master:

● salt-master.service - The Salt Master daemon
   Loaded: loaded (/lib/systemd/system/salt-master.service; enabled)
   Active: inactive (dead) since Wed 2016-10-05 20:21:25 GMT; 5s ago
  Process: 15156 ExecStart=/usr/lib/virtualenvs/salt/bin/salt-master (code=exited, status=0/SUCCESS)
 Main PID: 15156 (code=exited, status=0/SUCCESS)

Oct 05 20:21:24 saltmaster systemd[1]: Stopping The Salt Master daemon...
Oct 05 20:21:24 saltmaster salt-master[13547]: The salt master is shutdown. Master received a SIGTERM. Exited.
Oct 05 20:21:24 saltmaster salt-master[14448]: The salt master is shutdown. Master received a SIGTERM. Exited.
Oct 05 20:21:24 saltmaster salt-master[15156]: [WARNING ] Master received a SIGTERM. Exiting.
Oct 05 20:21:25 saltmaster salt-master[15156]: The salt master is shutdown. Master received a SIGTERM. Exited.
Oct 05 20:21:25 saltmaster systemd[1]: Stopped The Salt Master daemon.

Tests written?

No, manually tested against vagrant instance.

Please review Salt's Contributing Guide for best practices.

salt --versions
Salt Version:
           Salt: 2016.3.3-172-g8ff69bf

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
          gitdb: 0.6.4
      gitpython: 2.0.2
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 15.4.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.4.2
            ZMQ: 4.1.5

System Versions:
           dist: debian 8.2 
        machine: x86_64
        release: 3.16.0-4-amd64
         system: Linux
        version: debian 8.2

Let me know what you think,

~Rob

@cachedout
Copy link
Contributor

I am not sure we want to do this in a minor release. We have already done it in the upcoming carbon release after much discussion: #33792

cc: @BABILEN @terminalmage @thatch45 @dmurphy18

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Oct 6, 2016
@terminalmage
Copy link
Contributor

As far as I know, we already did this for 2015.8.12 and 2016.3.3, I wasn't aware that we had different unit files in the salt repo for debian when I opened #35577 to update the unit files after adding systemd-notify support and changing systemd service restarts to use scopes to take the service restart out of the cgroup in #35510.

We couldn't use the default of control-group in the past because when that is in effect, upon a systemctl stop a SIGTERM is sent to all processes in the cgroup. When upgrading the salt-minion package, a post-upgrade restart would be triggered by the package manager, causing systemd to kill all processes in the cgroup. This would interrupt the package manager command, leaving the package database in a bad state (particularly on Debian). The systemd-notify and scope support that I added in #35510 made it possible for us to use Type=notify for the service, and let systemd kill all procs in the cgroup on a systemctl stop without the possibility for negatively impacting the package database.

We absolutely should get this in, and backported to 2015.8. We also need to get the type changed to notify. I'll submit a PR to the OPs fork.

@terminalmage terminalmage mentioned this pull request Oct 6, 2016
Merged
@terminalmage
Copy link
Contributor

Rather than opening a PR against @l2ol33rt's fork, I just incorporated his commit into a new PR containing all the necessary changes to get these unit files updated. The new PR is #36823. Closing this one.

Thanks @l2ol33rt!

@dmurphy18
Copy link
Contributor

@l2ol33rt The Debian 8 systemd service unit files for releases 2015.8.12 and 2016.3.3 are correct, and are pulled from the pkg directory, that is, pkg/salt-minion.service.

Debian 8, as are all common Linux packages released by SaltStack, is built from the state files in salt-pack, openly available on github, https://github.com/saltstack/salt-pack. If you examine the contents of tarball https://github.com/saltstack/salt-pack/blob/develop/file_roots/pkg/salt/2016_3_3/debian8/spec/salt_debian.tar.xz you will see their use, for example: debian/salt-minion.install

@terminalmage
Copy link
Contributor

Yeah it looks like these unit files are only used for git installs via salt-bootstrap.

@terminalmage
Copy link
Contributor

Given @l2ol33rt's versions-report, that's very likely how he was installing it, so this is still a good change.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Oct 6, 2016 •
edited
Loading

agreed, best to have all unit service files correct, no matter were they reside.

Just wanted to make him aware of salt-packl

@l2ol33rt
Copy link
Contributor Author

l2ol33rt commented Oct 6, 2016

@terminalmage correct, Im using a variation of salt-bootstrap to install salt into a virtualenv on debian.

@dmurphy18 phew, Im glad to hear that the pkgs had different but working unit files.

Thanks for digging to this everyone!

@wwentland wwentland mentioned this pull request Sep 15, 2017
Closed
@rallytime rallytime mentioned this pull request Dec 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@l2ol33rt @cachedout @terminalmage @dmurphy18