Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix close a nil chan err #872

Merged
merged 1 commit into from May 19, 2017
Merged

Fix close a nil chan err #872

merged 1 commit into from May 19, 2017

Conversation

keloyang
Copy link
Contributor

If docker exec to a container, and then kill the runtime's shim process,it will lead to close a nil chan in Supervisor' execExit interface.

why ?

default

what I do ?

split getDeleteExecSyncMap to getExecSyncMap and deleteExecSyncMap to make sure delete operation come after the chan's close.

how to reproduce ?

following the steps below, the problem can reproduced every time.
first run a container,

docker run -ti busybox ash

secondly docker exec to the container,

docker exec -ti d2a397b12d19 ash

then the kill the runtime's shim proccess,

 ps -eaf|grep shim
root      1180  1173  1 19:27 ?        00:00:00 docker-containerd -l unix:///var/run/docker/libcontainerd/docker-containerd.sock --shim docker-containerd-shim --start-timeout 2m --state-dir /var/run/docker/libcontainerd/containerd --runtime docker-runc --metrics-interval=0
root      1387  1180  1 19:28 ?        00:00:00 docker-containerd-shim d2a397b12d19113bb48284a451095b24d646d1941bd28d6d97cb886571327c73 /var/run/docker/libcontainerd/d2a397b12d19113bb48284a451095b24d646d1941bd28d6d97cb886571327c73 docker-runc
root      1552  1180  2 19:28 ?        00:00:00 docker-containerd-shim d2a397b12d19113bb48284a451095b24d646d1941bd28d6d97cb886571327c73 /var/run/docker/libcontainerd/d2a397b12d19113bb48284a451095b24d646d1941bd28d6d97cb886571327c73 docker-runc
root      1611  8696  0 19:28 pts/4    00:00:00 grep --color=auto shim
V2R1C00-GUESTOS-VS-VMWARE-X64:~/workspace/docker-bin # kill -9 1387

and you will see the panic info from containerd like this,

panic: close of nil channel

goroutine 146 [running]:
panic(0x90a8c0, 0xc8204f4d90)
        /usr/local/go/src/runtime/panic.go:481 +0x3e6
github.com/docker/containerd/supervisor.(*Supervisor).execExit.func1(0xc820143450, 0xc8201a2f70, 0x0)
        /root/workspace/go/src/github.com/docker/containerd/supervisor/exit.go:95 +0x11d
created by github.com/docker/containerd/supervisor.(*Supervisor).execExit
        /root/workspace/go/src/github.com/docker/containerd/supervisor/exit.go:96 +0x2bf

Signed-off-by: yangshukui yangshukui@huawei.com

Fix close a nil chan err
de0e943 
If docker exec to a container, and then kill the runtime's shim process
it will close a nil chan in Supervisor' execExit interface.

Supervisor.exit
            |---> s.delete
            |         |
            |         V
            |     execMap := s.getDeleteExecSyncMap(t.ID) make getExecSyncChannel return nil
            |
            |--->s.execExit
            |         |
            |         V
            |     synCh := s.getExecSyncChannel(t.ID, t.PID)
            |         |
            |         V
            |     close(synCh) close a nil chan make container panic

Signed-off-by: yangshukui <yangshukui@huawei.com>
@hqhq
Copy link
Contributor

hqhq commented May 19, 2017

LGTM

mlaventure
mlaventure approved these changes May 19, 2017
View reviewed changes
Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mlaventure mlaventure merged commit dbe1bdd into containerd:v0.2.x May 19, 2017
@mlaventure mlaventure mentioned this pull request May 23, 2017
Closed
This was referenced May 30, 2017
Merged
Merged
mlaventure added a commit to mlaventure/containerd that referenced this pull request Jul 7, 2017
@mlaventure
Add a test for containerd#872
4b0c324 
This test that we don't panic if a container init process shim is
SiGKILL'ed while having an exec attach to that container.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
crosbymichael added a commit that referenced this pull request Aug 2, 2017
@crosbymichael
Merge pull request #1140 from mlaventure/test-exec-shim-sigkill-panic
474dbe2 
Add a test for #872
@linxiulei linxiulei mentioned this pull request Jun 12, 2018
Closed
This was referenced Oct 4, 2018
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlaventure mlaventure mlaventure approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@keloyang @hqhq @mlaventure @crosbymichael