Enable signal proxying with TTY

[mheon]

Fixes #5547

At present, documentation indicates you can use signal proxying in TTY mode, but it's explicitly disabled in the code.

Removing the explicit checks to prevent proxying in TTY mode does not produce any bugs I can find, makes the behavior consistent with documentation, and makes the Docker client a bit more usable than it was before.

Of note: in TTY mode, SIGTTIN, SIGTTOU, SIGTSTP, and SIGINT are not proxied. SIGTTIN, SIGTTOU, and SIGTSTP are handled by the TTY allocated to the process, and consequently not being able to proxy them is not a significant issue. SIGINT is used by the TTY code to cause a graceful exit of the client, which seems to be a good behavior to maintain.

As mentioned previously, I can't find any bugs with this code. The only potential issue I can see is that SIGTERM will no longer stop the client by default, but will instead be proxied through to the container; however, SIGINT will still close only the client, if that is desired.

[vieux]

@mheon it's not meant to be explicitly disabled in the code. At least it wasn't and maybe we broke something.
In tty mode the signals should go through the TTY no need for the extra API call

[jamtur01]

Docs LGTM

[SvenDowideit]

aye, Docs LGTM too

[mheon]

@vieux That's mostly true, in that signals generated through the TTY are proxied (Control-C proxies a SIGINT, for example).

However, if I wanted to use a script to send signals to a container, in non-TTY mode I could
'kill -$SIGNAL $DOCKERCLIENT_PID'; this isn't an option in TTY mode at present. You could substitute 'docker kill -s $SIGNAL' to do the same thing, of course, but there are no downsides I can see to using the normal kill command instead (and a few advantages - 'docker kill' does not recognize signals higher than 31 at present, for example).

[rhatdan]

@crosbymichael @shykes Any chance we could look at this?

[rhatdan]

@crosbymichael @tianon @shykes Any chance someone can look at this.

[shykes]

Reviewing for #dockertuesday

[shykes]

Assigning to @jfrazelle, @crosbymichael offered to help. Merge or close at will.

[jessfraz]

I don't see this as being something that needs to be merged. I ran several tests with sending various signals through the tty and all worked as expected. I don't think this is behavior we should risk modifying and in turn possibly expose some regression in terms of how people use this today. Closing, but feel free to disagree with use cases as to why this is needed.

[miminar]

I must say that I'm in favor of this solution instead of #9144 which is just man-page fixing.

[cevich]

One possible use case: pgrep/pkill, signalling a group of processes that may be running in containers or not. It will not uniformly deliver the signal if this bug is not fixed. Especially since both pgrep and pkill support namespace based matching along with other criteria that may include contained and non-contained processes. For example if some container is in TTY mode, but another is not or security or resource-use purposes.

Process signalling is a low-level mechanism with almost universal expectations on certain behaviour and semantics. Especially where some app. would otherwise function as expected when simply executing on the host directly. This bug breaks those expectations/standards for any app that's contained, but restricted by external conditions beyond it's control (i.e. tty vs non-tty). It also makes troubleshooting this class of problems extraordinarily difficult.

[amouat]

I'm also in favour of this solution.

Unless I'm misunderstanding something, it does seem odd to have something not working as intended when the fix is trivial. I can't really imagine a case where fixing this would break stuff for someone and seems like leaving it as is will be more likely to confuse people.

[nh2]

Please consider reopening this issue.

We're bulding interactive tools on top of Docker, for example running the ghci interpreter inside a Docker container (see [related problem)[https://github.com/commercialhaskell/stack/issues/527]). The user need not care whether that ghci runs locally or in a container (it should be transparent to them), but Docker not forwarding signals if the settion is interactive (for which you need to be in TTY mode) breaks this transparency, and the user cannot kill the process as they would expect they can.

Process signalling is a low-level mechanism with almost universal expectations on certain behaviour and semantics. Especially where some app. would otherwise function as expected when simply executing on the host directly. This bug breaks those expectations/standards for any app that's contained, but restricted by external conditions beyond it's control (i.e. tty vs non-tty).

What @cevich says here is correct, and expresses my problems above in more general terms.

Using Docker somewhere in a process hierarchy creates a "virtual" process hierarchy (children of docker run are not its children, they are attached to the docker -d (usually running under root as child of init), which creates a separate real process hierarcy. Docker usually manages to hide this split quite well, with signal proxying being the thing that implements it.

It is common for well-behaved process hierarchies to implement the passing-on of signals like SIGTERM to their children. Docker allows this for many cases, but when in TTY mode, this suddenly breaks. This disallows implementing such well-behaved process hierarchies on top of Docker.

The original poster's (@mheon) PR description reads like "because we can", but that's misleading - passing on signals is the right behaviour, and it fixes a nasty bug and inconsistency.

It also makes troubleshooting this class of problems extraordinarily difficult.

Confirmed, we spent many days on problems related to this.

I ran several tests with sending various signals through the tty and all worked as expected.

@jfrazelle I don't think this is the right test - it's not that passing signals through the TTY doesn't work. The problem is that when you send a signal directly to the PID of docker run, it does not get passed on.

Please see commercialhaskell/stack#527 for a test that reflects this problem: A user starts a program, gets its PID, tries to kill it, but because the program is in Docker in TTY mode, his kill gets swallowed by docker run and never reaches the actual program.

[cevich]

@nh2 FWIW, in testing (where we discovered this issue) I am working around it by either not using tty mode or using docker kill -s $SIGNAL $CONT. This allows us to continue but it does not address the actual underlying problem. I agree with you it should be fixed for real, but given the track-record on sigproxy/signal handling issues/bugs, I would not get your hopes up to high.

FWIW, another avenue that may get you additional leverage is to go through Red Hat support (assuming you have it). My team in QE originally opened these issues, but tying direct customer feedback to them, often brings additional attention from engineering. It still may not result a fix anytime soon, but it will likely increase the "weight" pressing for a fix.

[neurogenesis]

@shykes @jfrazelle, i understand the decision to not fix the documentation instead of the problem (potential signalling regression problems). However, do you know of any other efforts or issues that address the underlying problem?

That said, are there any suggestions for a workaround to downstream issues because of the tty signaling problems?

We've been trying to use docker to provide CLI access to developers for things like rails consoles or database access. The fact that signals aren't passed in TTY mode means that containers launched with interactive+tty (docker run -it) aren't properly removed when the parent tty receives a SIGTERM/QUIT/EXIT (ssh connection termination for example, laptop losing wireless, going to sleep, etc.).

This wouldn't be a problem if docker had additional metadata to map an interactive container back to the originating PID. We could just use docker inspect to find a parent PID / interactive PID, and check to see if that PID exists (and if not we could remove the container). However, i can only see the PID of the docker container. A forward pstree from ssh connection to "docker run" doesn't result in the PID of the container, and a reverse lookup doesn't identify the ssh session. Because of this we can't accurately identify orphaned containers that should be removed.

Additionally, using wrapper shell scripts that trap signals and try and run docker rm / docker signal won't work because docker run interrupts their functionality. A cleanup trap appears to not run until docker run exits (which only happens when closed cleanly).