cli: allow to kill a stopped container and sandbox #1085
Conversation
| // container MUST be created, running or paused | ||
| if status.State.State == vc.StateReady || status.State.State == vc.StateRunning || status.State.State == vc.StatePaused { | ||
| if err := vci.KillContainer(ctx, sandboxID, containerID, signum, all); err != nil { | ||
| return err |
There was a problem hiding this comment.
If the sandbox/container has stopped, will the following vci.StopSandbox/vci.StopContainer return another err?
There was a problem hiding this comment.
No, it's ignored in sandbox.stop()
|
/test |
|
It seems there's something wrong with the CI. |
There was a problem hiding this comment.
@bergwolf
I understand what you're doing by making the kill more permissive and the Sandbox stop idempotent, but I'm worried we might not be OCI compliant anymore.
If you take a look here: https://github.com/opencontainers/runtime-spec/blob/master/runtime.md#kill, it specifies clearly that sending a signal to a container that is not running or created must return an error.
I'm wondering if instead we should try to fix the behavior at the crictl level, by making sure that crictl ignores an error coming from a kill, since the container might already be stopped. WDYT?
|
@sboeuf Good point! After taking a closer look, it is because of a tiny difference with how we handle I've updated the PR to include above change. PTAL. |
|
/test |
|
/test |
|
/test |
| return err | ||
| } | ||
| } else if !all { | ||
| return fmt.Errorf("container not running") |
There was a problem hiding this comment.
Could you add a unit test for this scenario (and the one above in the if)?
cri containerd calls kill on stopped sandbox and if we fail the call, it can cause `cri stopp` command to fail too. Fixes: kata-containers#1084 Signed-off-by: Peng Tao <[email protected]>
|
/test |
|
@jodh-intel unit test has been added, if this is okay for you, please merge the PR! |
|
Thanks @bergwolf! lgtm |
cri containerd calls kill on stopped sandbox and if we
fail the call, it can cause
cri stoppcommand to failtoo.
Also add a few logs to track container state transitioning.
Fixes: #1084