[19.03 backport] Do not disable sig-proxy when using a TTY #2177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This partially reverts moby/moby@e0b59ab, and does not automatically disable proxying signals in TTY-mode Before this change: ------------------------------------ Start a container with a TTY in one shell: ``` docker run -it --init --name repro-28872 busybox sleep 30 ``` then, in another shell, kill the docker cli: ``` kill `pgrep -f repro-28872` ``` Notice that the CLI was killed, but the signal not forwarded to the container; the container continues running ``` docker container inspect --format '{{ .State.Status }}' repro-28872 running docker container rm -f repro-28872 ``` After this change: ------------------------------------ Start a container with a TTY in one shell: ``` docker run -it --init --name repro-28872 busybox sleep 30 ``` then, in another shell, kill the docker cli: ``` kill `pgrep -f repro-28872` ``` Verify that the signal was forwarded to the container, and the container exited ``` docker container inspect --format '{{ .State.Status }}' repro-28872 exited docker container rm -f repro-28872 ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit ee29504) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add a test to verify that killing the docker CLI forwards the signal to the container. Test-case for moby/moby 28872 Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 7cf1a8d) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
exec.CombinedOutput should not be used here because: - it redirects cmd Stdout and Stderr and we want it to be the tty - it calls cmd.Run which we already did While at it - use pty.Start() as it is cleaner - make sure we don't leave a zombie running, by calling Wait() in defer - use test.Name() for containerName Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit bc4ed69) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
Note that this change in behavior could be a very unpleasant surprise for people, particularly as it relates to SIGHUP. I am used to running very long-term jobs on a remote machine, under docker. I would ssh into the machine, start a container, suspend my laptop, and then later ssh back in and attach to the still-running container to see the job status. But suddenly all my jobs started disappearing! Turned out that this change suddenly meant that SIGHUPs from ssh connections dying were propagating into the containers and killing them! I would recommend specifically mentioning this difference in changelog. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
backport of:
This partially reverts moby/moby@e0b59ab (moby/moby#2426), and does not automatically disable proxying signals in TTY-mode
fixes moby/moby#28872 (docker client doesn't pass signals when a terminal is attached)
fixes moby/moby#3793 docker client not passing signals to dockerd
relates to:
docker execcommand will not terminate the spawned process moby/moby#9098 Killdocker execcommand will not terminate the spawned processdocker execcase; it looks like there's no API to kill an exec'd process, so there's no signal-proxy for this yetBefore this change:
Start a container with a TTY in one shell:
then, in another shell, kill the docker cli:
Notice that the CLI was killed, but the signal not forwarded to the container;
the container continues running
After this change:
Start a container with a TTY in one shell:
then, in another shell, kill the docker cli:
Verify that the signal was forwarded to the container, and the container exited
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)