Support for killing the whole child process group

[dpw]

tini only kills the immediate child process. This means that if you
do, for example,

docker run krallin/ubuntu-tini sh -c 'sleep 10'

and ctrl-C it, nothing happens: SIGINT is sent to the 'sh' process,
but that shell won't react to it while it is waiting for the 'sleep'
to finish.

This change adds a -g option to put the child process of tini into a
new process group, and sends signals to that group, so that every
process in the group gets a signal. This corresponds more closely to
what happens when you do ctrl-C etc. in a terminal: The signal is sent
to the foreground process group.

So if you try the example above with a container image that passes -g
to tini, the SIGINT will be received by the 'sleep', and the container
promptly exits.

[krallin]

Hey @dpw; thanks for taking the time to submit this PR!

I'm not really against or for adding this feature, but it looks like something that could be built using another tool that you'd place between Tini and your shell (or whatever process you're using), which would basically receive signals and forward them to a process group.

Would you mind sharing a real-world use case where this change helps? This would help me understand better why we might want to make this change.

In other words:

• As a general rule I'd rather keep Tini minimal and avoid adding features that aren't necessary.
• On the other hand, I'm interested in adding functionality that is simple to build in Tini but would otherwise require another tool, provided there is a significant benefit in having them (e.g. that's what I did when I added subreaper support in Add child subreaper support #7).

Knowing about where you'd use this feature would help me make a more informed decision : ).

Thanks again,

Thomas

A couple side notes:

• We'd need to add tests before merging this in. I can take care of that.
• Not sure about changing fork to vfork in this patch (it doesn't seem related to the functionality being implemented, right?)

[dpw]

Hey @dpw; thanks for taking the time to submit this PR!

No problem. I'll respond out of order:

Would you mind sharing a real-world use case where this change helps? This would help me understand better why we might want to make this change.

Sure. I'll explain the use case that prompted it.

An annoyance I've encountered repeatedly when running docker containers in non-tty mode is that control-C and control-\ don't propagate down into the container: Docker just sends the signal to pid 1 in the container, which often isn't very useful, due to the special signal rules for pid 1. I hit this again last week when I was containerizing the build and test suite for a project, and I decided to try to find a good solution.

Some searching led me to tini, and it looked like just what I needed. So I changed the FROM line in my Dockerfile to point to one of your tini images. And I found it made no difference when I did control-C :(

The problem is that what I want to run inside the container is a shell script. So tini starts the shell, and the shell runs commands from the script. If I do control-C, tini gets the SIGINT, and sends it to the shell process. But dash (and I'm using images based on ubuntu, so /bin/sh is dash) does nothing while it is waiting on a command; it will only process the signal when the command completes.

It's easy to see this behaviour by doing

docker run --rm krallin/ubuntu-tini sh -c 'sleep 10'

and typing control-C straight away. It waits the full 10 seconds.

(In contrast, bash does handle signals while waiting on commands, so if you replace sh with bash above, control-C takes effect promptly.)

Normally, when running commands in a terminal without docker involved, this doesn't matter. Because typing control-C sends SIGINT to the foreground process group. So if in a terminal you do

sh -c 'sleep 10'

and control-C, SIGINT is sent to the sh process and to the sleep process. The sleep process exits, and so it takes effect immediately.

I want that behaviour to extend inside batch containers. And that's what the -g option does, by killing the process group.

I'm not really against or for adding this feature, but it looks like something that could be built using another tool that you'd place between Tini and your shell (or whatever process you're using), which would basically receive signals and forward them to a process group.

Such a tool would have to run the subprocess in a process group, and then wait for the subprocess to complete, or until a signal arrives, which it would relay to the process group. In other words, it would be equivalent to tini with the -g option. If I write such a thing, I will have no reason to use tini.

We'd need to add tests before merging this in. I can take care of that.

Ok. Or I can add a test, if you are otherwise happy with the PR.

Not sure about changing fork to vfork in this patch (it doesn't seem related to the functionality being implemented, right?)

Yes, I can pull that out. Do you want it in a separate commit, or in a separate PR?

[krallin]

Thanks for taking the time to expand on you use case, @dpw!

I agree that rewriting a tool that did only the signal "multiplexing" would indeed be the same thing as tini -g (except without the zombie reaping bits).

This probably makes sense to include in Tini since the goal is to make it a valid init for containers (not just a zombie reaper!). I'll look into adding some tests now (unless you really want to, but the test code isn't the cleanest..!).

---

As far as fork vs. vfork goes, I'm not sure I'd like to pull that in without being confident that it's legal to call sigprocmask (and setpgid) after vfork before calling execvp (I should also refactor this branch not to return). I haven't really been able to find definitive documentation, but let me know if you have.

Cheers,

[krallin]

I added some tests here:

fa87523

I'll play around with this for a little while and then either merge this PR (if you remove the vfork bit :) ) or amend your commit and push it to master directly (though it'll still be your commit).

Cheers,

[dpw]

As far as fork vs. vfork goes, I'm not sure I'd like to pull that in without being confidench t that it's legal to call sigprocmask (and setpgid) after vfork before calling execvp (I should also refactor this branch not to return). I haven't really been able to find definitive documentation, but let me know if you have.

That's a fair point. The return would certainly need attention.

The linux vork syscall implementation is a simple variant of fork which passes CLONE_VM, and in which the parent waits until the child relinquishes the struct_mm. sigprocmask and setpgid are both signal-safe, so they must be plain syscalls. So it seems fine under linux.

But POSIX 2001 clearly doesn't allow it (http://pubs.opengroup.org/onlinepubs/009695399/). And the glibc implementation of posix_spawn avoids vfork if POSIX_SPAWN_SETSIGMASK or POSIX_SPAWN_SETPGROUP is passed (https://github.com/andikleen/glibc/blob/rtm-devel9/sysdeps/posix/spawni.c#L102).

So I've removed the vfork change from my branch.

[krallin]

Merged in #16. Thanks!

[krallin]

@dpw also issued a new release: https://github.com/krallin/tini/releases/tag/v0.8.0 (Tini images will be updated soon)

[dpw]

Great!

[krallin]

@dpw ,

Just a heads up that making the child process foreground on the terminal seems to solve this issue as well, in a much more elegant manner (that's what I'm doing in #21), since you no longer need to pass an argument.

I might go with that instead, and remove the -g argument (Tini is not 1.0 yet). Thoughts?

[dpw]

Just a heads up that making the child process foreground on the terminal seems to solve this issue as well, in a much more elegant manner (that's what I'm doing in #21), since you no longer need to pass an argument.

I might go with that instead, and remove the -g argument (Tini is not 1.0 yet). Thoughts?

I wonder how that works if stdin is not a terminal. E.g., docker run without the -t option.

[krallin]

@dpw,

The child will be spawned properly (in #21, Tini moves on if there is no tty). Signals sent to the container will get to Tini and be propagated to the immediate child (not the group).

However, without the -t option, Ctrl+C wouldn't work anyway, which I think was your use case. Do let me know if I'm missing something, though!

[dpw]

However, without the -t option, Ctrl+C wouldn't work anyway, which I think was your use case. Do let me know if I'm missing something, though!

Ctrl+C does work without the -t option. The docker client receives SIGINT, and it relays it to pid 1 within the container.

For example, if I do:

docker run krallin/ubuntu-tini sh -c 'sleep 10'

It waits the full 10 seconds after I type ctrl-C promptly.

In contrast, if I do:

docker run --entrypoint=/usr/bin/tini krallin/ubuntu-tini -g -- sh -c 'sleep 10'

it exits immediately when I type ctrl-C. Do you not see the same behaviour? I happen to be using docker-1.7.1 on this machine, but I'm not aware of relevant changes in docker recently.

[krallin]

@dpw,

Sorry, my point was that it wouldn't work if Tini wasn't involved in the equation. The signal does get sent, but it doesn't get to your sleep process unless Tini is here to forward it to the entire process group.

For example, if you run

docker run ubuntu sh -c 'sleep 10'

and then press CTRL+C, the shell doesn't exit.

However, if you run:

docker run -t ubuntu sh -c 'sleep 10'

Then it does exit (which is the behavior you wanted to reproduce with Tini).

---

It does currently exit without a tty because that's how we built it, but I think emulating what happens when Tini isn't part of the equation (i.e. behaving like a regular shell would) makes the most sense. Thoughts?

Cheers,

[dpw]

So you are suggesting I should always pass the -t option to docker run? That's not really a satisfactory solution, because programs might think their output is destined for a tty when it isn't, altering their behaviour in unwelcome ways.

[...] but I think emulating what happens when Tini isn't part of the equation (i.e. behaving like a regular shell would) makes the most sense.

I'm not sure what you mean by "behaving like a regular shell would" here. My motivation comes from writing build systems and test suites that involve docker containers. When running those from a terminal I want to be able to hit ctrl-C and have the build/tests stop, just as they would without the use of containers. That's what I get from the -g option, so from my perspective the use of tini with -g does emulate what happens when tini+containers are not part of the equation.

[krallin]

By "behaving like a regular shell would", I mean that I think Docker + Tini + Dash would behave just like Docker + Dash do.

I'm not sure solving the fact that Dash doesn't forward signals to a child process group when there is no tty is really something Tini should be solving, but I'll take a bit more time to think about it.

I'll go ahead with #21, but won't remove the -g flag for now. If you find time to try using the new version (when released) without the -g flag, let me know how that works for you.

Cheers,

[dpw]

I'll go ahead with #21, but won't remove the -g flag for now. If you find time to try using the new version (when released) without the -g flag, let me know how that works for you.

But that would mean I have to use docker run -t, right?

[krallin]

@dpw,

You can continue using the -g flag, but yes, running docker run -t will be enough to ensure your entire process group gets signalled (without needing -g).

[dpw]

You can continue using the -g flag, but yes, running docker run -t will be enough to ensure your entire process group gets signalled (without needing -g).

Ok. As I said above, using -t is not really satisfactory for me, because it can have other side effects.

It seems that you have a strong view on the scope of tini, and remain uncomfortable with -g. I don't want to force it on you. If you are thinking of removing it, I'd prefer that you did it now, so that I can proceed on that basis.

[krallin]

@dpw,

I realize you said this option didn't seem satisfactory to you. All I asked was to try it out if you have time : ). You don't have to if you don't want to.

I'm not really uncomfortable with the -g flag, I just want to make sure it's the right way to approach the problem :)

(which is why I was asking for you to try the other option when it's out, but again, you don't have to if you don't want to)

Cheers,

[krallin]

To your question that I let you know whether I plan on removing it: not if it's useful.

There is test coverage for it and the implementation for it is just a few lines and I don't see how it might ever get more complex (it's just handling the CLI flag, and the ternary expression in the kill call).

---

I apologize if I made you feel like I felt this flag was useless / your use case wasn't valid for Tini. I should have made it clearer that I was just asking for your input (since you have the use case for the -g flag).

Cheers,

[dpw]

I apologize if I made you feel like I felt this flag was useless / your use case wasn't valid for Tini. I should have made it clearer that I was just asking for your input (since you have the use case for the -g flag).

No problem, I'm sorry that I misinterpreted you.

I tried the fix-19-pass-tty branch. I'll add a comment to the pull request.