Add '--signal' option to replace SIGTERM #83

mcclurmc · 2016-06-08T22:30:57Z

Many servers respond to other signals than SIGTERM for their "graceful shutdown" option, such as Unicorn which requires SIGQUIT to wait on outstanding connections. The docker stop command sends the SIGTERM signal to the container, and provides no option for modifying this behavior. The docker kill command has an -s option which allows one to modify the signal sent to the container, but orchestration frameworks such as Mesos don't provide a way to use this functionality.

This commit adds the -s/--signal option to dumb-init, which provides a replacement signal for SIGTERM. For instance, running dumb-init like so:

dumb-init -s 3 <command>

Will send SIGQUIT (3) to the process it spawns when dumb-init receives a SIGTERM. This allows Docker image writers the freedom to specify how the docker stop SIGTERM will behave in their containers.

Many servers respond to other signals than SIGTERM for their "soft shutdown" option, such as Unicorn which requires SIGQUIT to wait on outstanding connections. The 'docker stop' command sends the SIGTERM signal to the container, and provides no option for modifying this behavior. The 'docker kill' command has an '-s' option which allows one to modify the signal sent to the container, but orchestration frameworks such as Mesos don't provide a way to use this functionality. This commit adds the '-s/--signal' option to dumb-init, which provides a replacement signal for SIGTERM. For instance, running dumb-init like so: dumb-init -s 3 <command> Will send SIGQUIT (3) to the <command> process it spawns when it receives a SIGTERM. This allows Docker image writers the freedom to specify how SIGTERM will behave in their containers. A further improvement to this option could be to provide an arbirary mapping from signal to signal, but that would greatly complicate the code for a probably minor use case.

mcclurmc · 2016-06-08T22:32:06Z

Sorry for raising this PR without any prior development/design discussion. I'll be happy to work with the maintainers to get this PR in good shape to merge if there are any code review comments.

chriskuehl · 2016-06-08T22:35:47Z

dumb-init.c

@@ -136,15 +149,21 @@ void print_help(char *argv[]) {
 }


+void write_sigterm_replacement(char *arg) {
+    sigterm_replacement = strtol(arg, NULL, 10);
+}


I think this function is probably unnecessary and the line could just be put in the switch

chriskuehl · 2016-06-08T22:38:42Z

Thanks for the PR! This feature seems like a good idea to me. Just a couple nits in the code above. We'll also want to add a test or two to make sure this doesn't regress (it should be easy to copy and adapt an existing test, look maybe at proxies_signals_test.py for a start)

chriskuehl · 2016-06-08T22:42:31Z

One idea that just came up internally: we have a use-case for making this a little more generic (rewriting signal X to signal Y). How would you feel about an interface like...

dumb-init --rewrite 15:3 --rewrite 14:7 [...]

(where the number before the colon is the original signal, and after is the replacement signal)

This would still work for your case, but would be a bit more generic and potentially more useful. I don't think it would complicate the code too much, either.

mcclurmc · 2016-06-08T22:44:32Z

Oh, I didn't realize there were tests! Your suggestions are perfectly reasonable, so I'll make those changes now, and then get started on a test case. I'll also update the failing test, which seems to simply check the help docs, which I've modified.

Regarding your rewrite case: this was something I considered, but did not need for my use case. I can take a stab at implementing that if you'd like.

asottile · 2016-06-08T22:45:00Z

Could also use the symbolic names I imagine too:

--rewrite SIGTERM:SIGALRM or --rewrite TERM:ALRM

mcclurmc · 2016-06-08T22:46:28Z

@asottile I also considered that, and yes it would be far nicer. I'd have to cover the case that the argument was an int or a string, and then do the mapping. I wasn't sure if this would be considered code bloat, though.

chriskuehl · 2016-06-08T22:49:24Z

@mcclurmc sounds good, let us know if we can help.

I'd be happy with just numbers for now, we can always add names later without breaking backwards compatibility. This might be a start (going in the opposite direction, but I think still useful).

P.S. according to GitHub, dumb-init is 65% tests and only 22% code, heh:

We've decided to allow arbitrary signal rewriting, not just SIGTERM rewriting. To use, call dumb-init with the '-r s:r' option, which will rewrite signum s to signum r. Only signals 1-31 are allowed to be rewritten, which should cover all the signals we need to cover. Note that this commit does not add new tests, it only fixes the existing broken test.

mcclurmc · 2016-06-09T22:15:19Z

Hi all, the last commit I added changes this feature to do arbitrary signal rewriting, using the -r s:r syntax that @chriskuehl suggested. I haven't added a signal name dictionary as @asottile suggested, but that's an obvious improvement to usability.

I'm really not familiar with tox or pytest and I had a hell of a time trying to get tests to run. I think I've fixed the breaking test, but I wasn't able to write new tests. I'd be happy to write tests if someone wants to lend me a hand, but would be more happy to let someone else write tests for me :)

mcclurmc · 2016-06-13T19:29:00Z

Hi all, is there any more feedback on this pull request? I'd be happy to work on more tests with some assistance.

chriskuehl · 2016-06-13T20:28:43Z

@mcclurmc looks pretty good to me! Thanks a lot for doing this! I'll start some tests and send you a PR to get your thoughts.

chriskuehl · 2016-06-13T21:36:00Z

@mcclurmc made a PR against your branch at https://github.com/grnhse/dumb-init/pull/2

Add signal translation tests

mcclurmc · 2016-06-14T15:48:40Z

Thanks for the commits against my branch, @chriskuehl. This branch is now green with your new tests. Are we good to merge?

chriskuehl · 2016-06-14T17:18:28Z

Thanks again for the PR! I'll release a new version later today.

chriskuehl · 2016-06-14T18:53:31Z

Released in v1.1.0.

mcclurmc · 2016-06-14T21:25:34Z

Thanks!

chriskuehl reviewed Jun 8, 2016
View reviewed changes

mcclurmc mentioned this pull request Jun 9, 2016

Add '-deregister-on-stop' option gliderlabs/registrator#431

Open

Add signal translation tests

9e456ad

Merge pull request #2 from chriskuehl/translate-sigterm

38f6309

Add signal translation tests

chriskuehl merged commit 54a2fe4 into Yelp:master Jun 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add '--signal' option to replace SIGTERM #83

Add '--signal' option to replace SIGTERM #83

mcclurmc commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

chriskuehl Jun 8, 2016

chriskuehl commented Jun 8, 2016

chriskuehl commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

asottile commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

chriskuehl commented Jun 8, 2016

mcclurmc commented Jun 9, 2016

mcclurmc commented Jun 13, 2016

chriskuehl commented Jun 13, 2016

chriskuehl commented Jun 13, 2016

mcclurmc commented Jun 14, 2016

chriskuehl commented Jun 14, 2016

chriskuehl commented Jun 14, 2016

mcclurmc commented Jun 14, 2016

Add '--signal' option to replace SIGTERM #83

Add '--signal' option to replace SIGTERM #83

Conversation

mcclurmc commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

chriskuehl Jun 8, 2016

Choose a reason for hiding this comment

chriskuehl commented Jun 8, 2016

chriskuehl commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

asottile commented Jun 8, 2016

mcclurmc commented Jun 8, 2016

chriskuehl commented Jun 8, 2016

mcclurmc commented Jun 9, 2016

mcclurmc commented Jun 13, 2016

chriskuehl commented Jun 13, 2016

chriskuehl commented Jun 13, 2016

mcclurmc commented Jun 14, 2016

chriskuehl commented Jun 14, 2016

chriskuehl commented Jun 14, 2016

mcclurmc commented Jun 14, 2016