Skip to content

hot-restarter.py: revert SIGCHLD handler to force kill instead of term#2640

Merged
htuch merged 1 commit intoenvoyproxy:masterfrom
mpuncel:mpuncel/kill-children-sigchld
Feb 22, 2018
Merged

hot-restarter.py: revert SIGCHLD handler to force kill instead of term#2640
htuch merged 1 commit intoenvoyproxy:masterfrom
mpuncel:mpuncel/kill-children-sigchld

Conversation

@mpuncel
Copy link
Contributor

@mpuncel mpuncel commented Feb 19, 2018

PR: #2596 changed the behavior of the SIGTERM and SIGCHLD handlers to
attempt to allow child processes to exit gracefully before force killing
them. This PR reverts the behavior of the SIGCHLD handler back to force
killing children if a child exits uncleanly. This should allow the
supervisor of the python process (e.g. runit) to restart envoy with a
shorter delay (whereas an attempt at graceful TERM might delay up to
TERM_WAIT_SECONDS).

Note: If the child process of hot-restarter.py is a container framework
(e.g. runc), the force kill might result in container state being
leaked. This should hopefully be a rare occurrence.

Signed-off-by: Michael Puncel mpuncel@squareup.com

title: Revert SIGCHLD handler in hot-restarter.py to force killing children instead of attempting graceful shutdown

Description:
Reverts the behavior of the hot-restarter.py
Risk Level: Low | Medium | High
Low

Testing:
I ran hot-restarter.py manually with /usr/bin/false and saw it print that it was sending a TERM
ran it with /usr/bin/true and saw that it exited cleanly without terminating children

Docs Changes:
no docs changes

Release Notes:
no release notes

PR: envoyproxy#2596 changed the behavior of the SIGTERM and SIGCHLD handlers to
attempt to allow child processes to exit gracefully before force killing
them. This PR reverts the behavior of the SIGCHLD handler back to force
killing children if a child exits uncleanly. This should allow the
supervisor of the python process (e.g. runit) to restart envoy with a
shorter delay (whereas an attempt at graceful TERM might delay up to
TERM_WAIT_SECONDS).

Note: If the child process of hot-restarter.py is a container framework
(e.g. runc), the force kill might result in container state being
leaked. This should hopefully be a rare occurrence.

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@mpuncel mpuncel force-pushed the mpuncel/kill-children-sigchld branch from 2f0040e to 9db2974 Compare February 19, 2018 00:48
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpuncel seems reasonable as per @mattklein123 's ask. Thoughts on adding an integration sh_test with this?

@mpuncel
Copy link
Contributor Author

mpuncel commented Feb 20, 2018

sounds good to me, FYI it will probably take me a bit to get the developer environment set up

@mattklein123
Copy link
Member

@htuch @danielhochman @mpuncel IMO this is somewhat of a regression. Can we merge this and then file a follow up issue for adding tests?

@mpuncel
Copy link
Contributor Author

mpuncel commented Feb 21, 2018

if you'd prefer, I can also revert my previous PR and submit a new one with tests once I get the chance

@mattklein123
Copy link
Member

I will defer to @htuch @danielhochman. I don't really think the other PR needs to get reverted but I would rather merge this sooner rather than later.

@htuch
Copy link
Member

htuch commented Feb 22, 2018

Fair enough, tests welcome in a followup PR.

@htuch htuch merged commit ade5deb into envoyproxy:master Feb 22, 2018
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* prototype configurable metrics

Signed-off-by: Kuat Yessenov <kuat@google.com>

* fix license

Signed-off-by: Kuat Yessenov <kuat@google.com>

* format and regenerate

Signed-off-by: Kuat Yessenov <kuat@google.com>

* fix non-det config

Signed-off-by: Kuat Yessenov <kuat@google.com>

* generalize metric overrides

Signed-off-by: Kuat Yessenov <kuat@google.com>

* merge fix

Signed-off-by: Kuat Yessenov <kuat@google.com>

* wip

Signed-off-by: Kuat Yessenov <kuat@google.com>

* update PR

Signed-off-by: Kuat Yessenov <kuat@google.com>

* update

Signed-off-by: Kuat Yessenov <kuat@google.com>

* make example more complicated

Signed-off-by: Kuat Yessenov <kuat@google.com>

* asan debugging

Signed-off-by: Kuat Yessenov <kuat@google.com>

* stats golint

Signed-off-by: Kuat Yessenov <kuat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants