kill: allow to signal paused containers

[giuseppe]

regression introduced by 87a1889

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[cyphar]

The reason why we excluded Paused is because the runtime-spec explicitly states that

Attempting to send a signal to a container that is neither created nor running MUST have no effect on the container and MUST generate an error.

However the runtime spec doesn't support pause so it should be okay to add an extension to kill as well, so I'm somewhat LGTM.

@crosbymichael WDYT?

[Ace-Tang]

I get a question, from the original logic, function Signal send signal to all status container

if err := c.initProcess.signal(s); err != nil {
 		return newSystemErrorWithCause(err, "signaling init process")
}

so it also include Pausing status, the change 87a1889 fix to not kill stopped container. Now the pr add Paused status container, what about Pausing status container, should it also need to be add according to the original logic ?

[cyphar]

Pausing is a transient state -- effectively it's just a state before we write FROZEN to the freezer cgroup. You wouldn't consistently be able to hit that race anyway. For analogy, we don't allow kill on containers which are in the Creating state either.

[lifubang]

I have considered the paused state before I submitted the commit 87a1889 , I should tell the maintainer my thinking before the commit merged. My original thinking is that:
When we send the signal kill redis 9 to the container redis which is paussed, it tell us success, but when we use runc list, redis's state is also paused, we need to use runc resume redis, then it's state can transfer to stopped.
If there are two users look into container redis which is paused, one user A runc kill redis 9, aother user B use runc resume redis, the user B will be puzzle in the state stopped after resume.
That is my original idea.
So, I think both add or don't add paused are ok.

[giuseppe]

I've noticed the breaking change as we are using to signal a paused container in podman (to implement rm -f for paused containers) and one of our tests failed when using runc from master.

/cc @baude

[lifubang]

@giuseppe For https://github.com/opencontainers/runtime-spec/blob/7c4c8f63a63693f75cfa0f3f397151fb8d9732ad/runtime.md#Kill

This operation MUST generate an error if it is not provided the container ID. Attempting to send a signal to a container that is neither created nor running MUST have no effect on the container and MUST generate an error. This operation MUST send the specified signal to the container process.

I think we may not add Paused.

But, I think, for https://github.com/opencontainers/runtime-spec/blob/7c4c8f63a63693f75cfa0f3f397151fb8d9732ad/runtime.md#state

running: the container process has executed the user-specified program but has not exited (after step 5 in the lifecycle)

I think Paused should belongs to Running.

So, I think to follow the runtime-spec standard, I think we may need to add a new function to check the containers status, for example:

func (s Status) RuntimeSpecStatus() Status {
	switch s {
	case Created:
		return Created
	case Running:
		return Running
	case Pausing:
		return Running
	case Paused:
		return Running
	case Stopped:
		return Stopped
	default:
		return nil
	}
}

Then, in

runc/libcontainer/container_linux.go

Line 385 in 07d1ad4

if status == Running || status == Created || status == Paused {

, we can use:

runtimeSpecStatus := status.RuntimeSpecStatus()
if runtimeSpecStatus == Running || runtimeSpecStatus == Created {

[lifubang]

I think to follow runtime-spec standard, we can add a func to transfer runc container state to runtime-spec state.
Please see: #1943 (comment)

[cyphar]

@lifubang You're over-complicating things -- runc has a few extensions to the runtime-spec and one of those is the pause operation and the paused state. It's really not necessary to have separate logic about "runtime spec states" and "runc states" -- libcontainer is already complicated enough as it is.

Given that paused is an extension to the spec, we can add it to other operations. The part of the spec that I quoted earlier (and you quoted) is only talking about the states defined in the spec. paused is not defined in the spec and thus we can make our own semantics for it.

As @giuseppe said this is a regression, so I'm okay with fixing this.

LGTM.

[crosbymichael]

LGTM