containerd / go-runc

runc bindings for Go
Apache License 2.0
161 stars 71 forks source link

Kill exec process with SIGTERM instead of SIGKILL #21

Open inoc603 opened 7 years ago

inoc603 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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.

dmcgowan commented 2 years ago

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