Add the all option to kill operation

[utam0k]

Since the --all option is already used a lot, why not include it in the specification?

• contained
  https://github.com/containerd/go-runc/blob/f5d58d02d6c8d10b17786c1da74e72aed01d4dfb/runc.go#L375-L378
• cri-o
  https://github.com/cri-o/cri-o/blob/488d8823f507fe7fb7c9041ed55ae957762b874c/internal/oci/runtime_oci.go#L1133-L1138

All of runc/crun/youki supported this option.

[AkihiroSuda]

L95:

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

[utam0k]

L95:

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

@AkihiroSuda Thanks for your review 🙏
Would you like to stop mentioning Options in the specs? But I think it is inevitable that some major options are already in use and not written. Is there anything that should be written on a separate page?

[AkihiroSuda]

kill-all <container-id> <signal>, or kill <container-id> <signal> <all> to clarify that this is not a CLI specification

[AkihiroSuda]

Or maybe Kill(containerID, signal, all)

[utam0k]

Good idea. I have updated it.

[AkihiroSuda]

### <a name="runtimeKill" />Kill
`kill <container-id> <signal> <all>`
This operation MUST [generate an error](#errors) if it is not provided the container ID.
Attempting to send a signal to a container that is neither [`created` nor `running`](#state) MUST have no effect on the container and MUST [generate an error](#errors).
This operation MUST send the specified signal to the container process.

If `<all>` is `true`, all the processes in the container are killed.

Or:

### <a name="runtimeKill" />Kill
`kill <container-id> <signal>
This operation MUST [generate an error](#errors) if it is not provided the container ID.
Attempting to send a signal to a container that is neither [`created` nor `running`](#state) MUST have no effect on the container and MUST [generate an error](#errors).
This operation MUST send the specified signal to the container process.

### <a name="runtimeKillAll" />KillAll
`kill-all <container-id> <signal>`

Kill all the processes in the container.

[utam0k]

I have picked up the first one. Thanks 😍

[AkihiroSuda]

Add the all option to kill command

s/command/operation/

[utam0k]

Add the all option to kill command

s/command/operation/

Indeed 👍

[AkihiroSuda]

• RFC: drop -a from runc kill runc#3864

@kolyshkin What should we do with this spec PR?

[AkihiroSuda]

@opencontainers/runtime-spec-maintainers @lifubang PTAL

[kolyshkin]

To my mind, -a is not needed. Let me explain why (this is reiterating #3864, to some extent).

runc kill -a has two use cases:

1. Killing the container (runc kill, runc delete -f).

Normally, if container's init is gone (i.e. exited, or we killed it), all other processes in the same PID ns are killed by the kernel. So there is nothing to do here, no special handing etc.

Except, this is not true for a container without own private PID namespace (which usually means host PID ns, or maybe a PID namespace of another container) -- in such case, we need to kill all processes manually. For that, we use container's cgroup (for newer kernels, we can use cgroup.kill file, for older, we have to freeze the cgroup, read all cgroup.procs (including in sub-cgroups) to get PIDs, send signal 9 to all those PIDs, and unfreeze).

My guess is, runc kill -a was introduced solely as a quick and dirty hack to kill all processes in a container which does not have its own private PID namespace.

2. Sending an arbitrary signal (not SIGKILL) to all processes in a container.

I doubt that anyone ever uses this feature (because otherwise kernel would introduce cgroup.kill API with an ability to choose the signal -- maybe @brauner can shed some light on this). Containers with complex lifecycle (such as running systemd, or any other sort of a program that runs and oversees other programs, let's call it a process manager) should have process manager relaying e.g. SIGTERM to all the processes. Containers with simple lifecycle should either exit on its own, or be killed.

---

Now, how do we handle these two use cases?

For (1), I propose that runtime should kill all processes in a container if it knows it does not have its own private PID ns (this is what runc does since #3825).

For (2), I don't think anyone uses it, so, unless I hear otherwise, I consider this functionality unused and not needed.

Ergo, we don't need kill -a.

[cyphar]

I agree with @kolyshkin. runc kill -a was a hack to avoid dealing more completely with the way that host-pidns containers behave. Conceptually, the "kill" operation (with SIGKILL) means to kill the container (which includes all processes spawned inside it, regardless of the pidns relationship).

I think I spoke with @brauner about cgroup.kill and other signals, but I don't remember the details. My guess is that the nice thing about SIGKILL is that there is no funny business with signal handlers or any other complications that might come about from having a multi-shot kill(2). In any case, there is no interest to add support for alternative signals in the kernel and so there's not much point adding text to the spec to allow for this in the future -- we can cross that bridge when we get to it.

[cyphar]

As discussed, I don't think this is necessary (regardless of whether runc kill -a exists -- but it has also been removed).

[utam0k]

I can follow runc's decision if runc drops it 👌 I just want to align the runtime-spec to reality in this PR.

Youki use this way to implement it actually.

For that, we use container's cgroup (for newer kernels, we can use cgroup.kill file, for older, we have to freeze the cgroup, read all cgroup.procs (including in sub-cgroups) to get PIDs, send signal 9 to all those PIDs, and unfreeze).

I agree with you.

2. Sending an arbitrary signal (not SIGKILL) to all processes in a container.
   I doubt that anyone ever uses this feature

@AkihiroSuda @kolyshkin @cyphar Thanks for your review 🙏

[utam0k]

@giuseppe FYI 👀 I guess this decision affects crun in the future.

• RFC: drop -a from runc kill runc#3864

@kolyshkin What should we do with this spec PR?