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.

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

Kill exec process with SIGTERM instead of SIGKILL #21

Open
inoc603 opened this issue Jul 10, 2017 · 4 comments
Open

Kill exec process with SIGTERM instead of SIGKILL #21

inoc603 opened this issue Jul 10, 2017 · 4 comments

Comments

@inoc603
Copy link

inoc603 commented Jul 10, 2017

In runc.Exec, here the exec command is created with exec.CommandContext, which will kill the runc command with os.Process.Kill when the context is done. And when runc is killed by SIGKILL, the exec process inside the container is not stopped. I think we should send SIGTERM manually to stop the runc command when we're doing exec.

I only tested this with docker-runc. Can anyone confirm whether it is the same with runc?

@crosbymichael
Copy link
Member

Why do you think this is not the correct functionality? When a context closes, that means the request or parent crashed/ended so we don't want to orphan the runc process.

@inoc603
Copy link
Author

inoc603 commented Jul 12, 2017

Yes the runc process should be killed when the context closes. My point is that it should be killed by SIGTERM rather than SIGKILL, so runc itself can clean up the exec process in the container. Current mechanism will orphan the exec process inside the container.

What I'm suggesting is listening for ctx.Done() manually, instead of letting the exec package handle it.

@crosbymichael
Copy link
Member

crosbymichael commented Jul 12, 2017

@inoc603 ok, the hard part is that Go is the one that sends the SIGKILL. We would have to rewrite all the logic for CommandContext to make this work like you said.

dqminh added a commit to dqminh/go-runc that referenced this issue Sep 21, 2017
instead of defering to Go's stdlib to handle context, which only sends
SIGKILL when the context timed out, handle the context timeout ourselves
so we can inject a custom signal first ( default to SIGTERM, and send
SIGKILL after 10 seconds) to stop container more gracefully.

Fix containerd#21
dqminh added a commit to dqminh/go-runc that referenced this issue Sep 21, 2017
instead of defering to Go's stdlib to handle context, which only sends
SIGKILL when the context timed out, handle the context timeout ourselves
so we can inject a custom signal first ( default to SIGTERM, and send
SIGKILL after 10 seconds) to stop container more gracefully.

If no signal is specified, then fall back to sending SIGKILL just like
stdlib.

Fix containerd#21
@dmcgowan
Copy link
Member

Closed #28 for staleness, is this issue still relevant though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants