oci: fix runc kill usage #305
Conversation
oci/oci.go
Outdated
| c.opLock.Lock() | ||
| defer c.opLock.Unlock() | ||
| if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, os.Stderr, r.path, "kill", c.name); err != nil { | ||
| if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, os.Stderr, r.path, "kill", c.name, "KILL"); err != nil { |
There was a problem hiding this comment.
Is this sending a SIGKILL to the container? If so, it seems more "polite" to send a SIGINT first, to give the application time to clean up, before sending SIGKILL to forcibly kill the processes. Is a forcible/uncatchable termination signal required per the CRI spec?
There was a problem hiding this comment.
The old code used to send SIGKILL. I'm just making it so that the code actually works. If you want to fix the behaviour, we can discuss that separately.
If so, it seems more "polite" to send a SIGINT first, to give the application time to clean up,
Did you mean SIGTERM? SIGINT is for <c-c> interrupts, and usually isn't handled gracefully by daemons.
There was a problem hiding this comment.
Makes sense, this change LGTM then. +1 on making this work as intended, will open an issue to discuss whether the current behavior is still what we want.
Did you mean SIGTERM?
Yeah, I guess so - anything kinder than terminating the process with extreme prejudice ;)
There was a problem hiding this comment.
@cyphar My intention in the original code was to sent TERM first and then send KILL if the process is still around after 10 ms. Maybe we could adjust that time or make it configurable but we should send TERM first.
|
LGTM, @mrunalp PTAL and merge if it's ok for you as well |
In later versions of runC, `runc kill` *requires* the signal parameter to know what signal needs to be sent. Signed-off-by: Aleksa Sarai <asarai@suse.com>
|
LGTM |
1 similar comment
|
LGTM |
In later versions of runC,
runc killrequires the signal parameterto know what signal needs to be sent.
Signed-off-by: Aleksa Sarai asarai@suse.com