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.

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

Improve stop by adding step to first send TERM to just the process #198

Closed
benhoyt opened this issue Feb 28, 2023 · 2 comments
Closed

Improve stop by adding step to first send TERM to just the process #198

benhoyt opened this issue Feb 28, 2023 · 2 comments
Labels
Feature A feature request Low Priority The opposite of "Priority"

Comments

@benhoyt
Copy link
Contributor

benhoyt commented Feb 28, 2023

Currently when a service is stopped, Pebble does the following:

  1. Send SIGTERM to the process group (negative pid). If the process pid exits, consider that a success.
  2. If the pid hasn't exited before a kill delay, send SIGKILL to the process group.

We want to make that a bit more graceful by changing it to the following:

  1. Send SIGTERM to only the individual process pid. If all the pids in the process tree exit, consider that a success.
  2. If some of the pids in the tree don't exit, send SIGTERM to each process in the tree that is still alive.
  3. If some of the pids are still alive after the kill delay, send SIGKILL to each process in the tree that is still alive.

Ideally we'd use the cgroup process tree to enumerate subprocesses, as that includes things like daemon processes. However, if that's too time-consuming, we could start with a simplification: for steps 2 and 3, wait for the process group and send signals to the process group (negative pid) instead of the tree.

Context: this is in part to better handle the issue in https://bugs.launchpad.net/juju/+bug/2008443 (but should also make stopping nicer in general).

See also my "Using cgroups in Pebble - design notes" doc.

See also #149.

@benhoyt
Copy link
Contributor Author

benhoyt commented Aug 21, 2023

It turns out that cgroups are hard to use in containers (in Docker they require privileged containers, in K8s/containerd they require a special setting), so while the cgroups approach might be good for using Pebble on machines, it's no good for the K8s use case (eg: Juju sidecar charms). We have fixed the initial issue surfaced by Patroni that brought this up in #149. But leaving this open to consider future work improving the process tree and termination handling (whether it's via cgroups, /proc, or an injected environment variable).

@benhoyt benhoyt added Feature A feature request Low Priority The opposite of "Priority" labels Mar 13, 2024
@benhoyt
Copy link
Contributor Author

benhoyt commented Sep 30, 2024

This relies on improving the process tree handling with cgroups (which we decided not to do), and with the improvements we've made earlier to service handling, this hasn't been asked for or needed. Going to close for now -- we can also revisit in future.

@benhoyt benhoyt closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A feature request Low Priority The opposite of "Priority"
Projects
None yet
Development

No branches or pull requests

1 participant