compose-cli icon indicating copy to clipboard operation
compose-cli copied to clipboard

Terminating compose-cli does not terminate main cli process

Open thaJeztah opened this issue 4 years ago • 16 comments

Description

Steps to reproduce the issue:

Start an interactive container in one shell:

docker run -it --rm --name mycontainer busybox

In another shell, find and kill the docker run process for the container:

ps aux | grep '\(docker\|docker.cli\)\ run'
sebastiaan       31278   0.0  0.1  4446388  23872 s007  S+   10:24AM   0:00.04 /usr/local/bin/com.docker.cli run -it --rm --name mycontainer busybox
sebastiaan       31275   0.0  0.1  5066904  25900 s007  S+   10:24AM   0:00.06 docker run -it --rm --name mycontainer busybox

kill -9 31275

Notice in the first shell that the docker run process was killed:

docker run -it --rm --name mycontainer busybox
/ # Killed: 9

However, only the compose-cli process was killed; the com.docker.cli process is still running;

ps aux | grep '\(docker\|docker.cli\)\ run'
sebastiaan       31278   0.0  0.1  4446644  23884 s007  S    10:24AM   0:00.04 /usr/local/bin/com.docker.cli run -it --rm --name mycontainer busybox

And so is the container:

docker ps
CONTAINER ID   IMAGE     COMMAND   CREATED              STATUS              PORTS     NAMES
471ca4feb7e6   busybox   "sh"      About a minute ago   Up About a minute             mycontainer

Describe the results you received:

Only the docker run process was killed, but the com.docker.cli run process was not

Describe the results you expected:

Both to be terminated, the same as when running com.docker.cli directly.

Additional information you deem important (e.g. issue happens only occasionally):

Output of docker version:

docker version
Client: Docker Engine - Community
 Cloud integration: 1.0.9
 Version:           20.10.5
 API version:       1.41
 Go version:        go1.13.15
 Git commit:        55c4c88
 Built:             Tue Mar  2 20:13:00 2021
 OS/Arch:           darwin/amd64
 Context:           default
 Experimental:      true

Server: Docker Engine - Community
 Engine:
  Version:          20.10.5
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.13.15
  Git commit:       363e9a8
  Built:            Tue Mar  2 20:15:47 2021
  OS/Arch:          linux/amd64
  Experimental:     true
 containerd:
  Version:          1.4.3
  GitCommit:        269548fa27e0089a8b8278fc4fc781d7f65a939b
 runc:
  Version:          1.0.0-rc92
  GitCommit:        ff819c7e9184c13b7c2607fe6c30ae19403a7aff
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

thaJeztah avatar Mar 30 '21 08:03 thaJeztah

Hmmm. We do forward signals, but I suppose with kill the parent process is killed before it can forward the signal here. Need to investigate more on this.

gtardif avatar Mar 30 '21 08:03 gtardif

Yes, we probably need some kind of "reaper" construct to handle this; also didn't look how to best handle this, but I ran into this when testing some things in our packaging, which had a build-container started without -it, so I wasn't able to terminate; then kill -9'd the process, only to discover my computer's fans were still making a lot of noise (because the container turned out to be still running) 😂

thaJeztah avatar Mar 30 '21 08:03 thaJeztah

Trying to investigate this issue, I noticed this also applies to CLI plugins

com.docker.cli compose up
[+] Running 2/1
 ⠿ Network truc_default    Created                                                                                                                        
 ⠿ Container truc_redis_1  Running                                                                                                                       Attaching to redis_1
...
(in another terminal) killall -9 com.docker.cli)
Killed: 9

Cli plugin process docker-compose compose up is still running (and so does the service containers)

Also applies to com.docker.cli run nginx: kill -9 on the CLI command doesn't kill the attached container. Not sure this later is expected anyway

ndeloof avatar Jun 23 '21 09:06 ndeloof

IIUC we could get child cli-plugin processes to set PR_SET_PDEATHSIG so it receive a kill signal when parent process dies. This could be integrated in the cli-plugin framework. But this probably only applies to Linux systems.

ndeloof avatar Jun 23 '21 09:06 ndeloof

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 03 '22 21:01 stale[bot]

.

thaJeztah avatar Jan 04 '22 09:01 thaJeztah

This issue has been automatically marked as not stale anymore due to the recent activity.

stale[bot] avatar Jan 04 '22 09:01 stale[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jul 10 '22 08:07 stale[bot]

.

thaJeztah avatar Jul 10 '22 17:07 thaJeztah

This issue has been automatically marked as not stale anymore due to the recent activity.

stale[bot] avatar Jul 10 '22 18:07 stale[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 08 '23 04:01 stale[bot]

.

thaJeztah avatar Jan 08 '23 04:01 thaJeztah

This issue has been automatically closed because it had not recent activity during the stale period.

stale[bot] avatar Jun 18 '23 19:06 stale[bot]

This issue has been automatically marked as not stale anymore due to the recent activity.

stale[bot] avatar Jun 20 '23 09:06 stale[bot]

AFAICT this "works as expected" and can be closed: kill -9 31275 will kill the process, but won't send kill signal to the whole process group, like Ctrl+C does in an interactive terminal. Need to run kill -9 -31275 to get both compose-cli docker and child process killed

ndeloof avatar Jun 20 '23 10:06 ndeloof

I think that's still a bug (which could be in this code and/or docker/cli itself), or at least something we should consider changing. The compose-cli being a wrapper for the CLI should be an implementation detail, and should not affect the behavior. I think there's still a similar issue with docker build as well, which (since the buildx integration) no longer cancels a build for this case.

thaJeztah avatar Jun 20 '23 10:06 thaJeztah