Add the all option to kill operation by utam0k · Pull Request #1190 · opencontainers/runtime-spec

utam0k · 2023-03-21T13:28:40Z

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

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.

runtime.md

utam0k · 2023-03-22T05:52:13Z

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 · 2023-03-22T08:49:53Z

runtime.md

+#### <a name="runtimeKillAll" />KillAll
+
+- `--all` Kill all the processes in the container.
+


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

Or maybe Kill(containerID, signal, all)

Good idea. I have updated it.

AkihiroSuda · 2023-03-22T11:46:34Z

runtime.md

 This operation MUST send the specified signal to the container process.

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


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

I have picked up the first one. Thanks 😍

AkihiroSuda · 2023-03-22T12:06:22Z

Add the all option to kill command

s/command/operation/

Signed-off-by: utam0k <[email protected]>

utam0k · 2023-03-22T12:12:08Z

Add the all option to kill command

s/command/operation/

Indeed 👍

AkihiroSuda · 2023-05-12T02:00:35Z

RFC: drop -a from runc kill runc#3864

@kolyshkin What should we do with this spec PR?

AkihiroSuda · 2023-11-06T16:59:49Z

@opencontainers/runtime-spec-maintainers @lifubang PTAL

kolyshkin · 2023-11-06T23:00:19Z

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 · 2023-11-08T07:02:46Z

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 · 2023-11-08T09:45:18Z

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.

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 · 2023-11-08T09:47:12Z

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

utam0k requested review from AkihiroSuda, crosbymichael, cyphar, dqminh, giuseppe, hqhq, kolyshkin, mrunalp, thaJeztah, tianon and vbatts as code owners March 21, 2023 13:28

AkihiroSuda requested changes Mar 21, 2023

View reviewed changes

AkihiroSuda reviewed Mar 21, 2023

View reviewed changes

runtime.md Show resolved Hide resolved

utam0k force-pushed the kill-a-option branch from 216818d to 5c2418b Compare March 22, 2023 05:48

AkihiroSuda reviewed Mar 22, 2023

View reviewed changes

utam0k force-pushed the kill-a-option branch 2 times, most recently from e1ebe38 to 7fe4d32 Compare March 22, 2023 11:42

AkihiroSuda reviewed Mar 22, 2023

View reviewed changes

utam0k force-pushed the kill-a-option branch from 7fe4d32 to abe723e Compare March 22, 2023 11:50

AkihiroSuda approved these changes Mar 22, 2023

View reviewed changes

Add the all option to the kill operation

e873658

Signed-off-by: utam0k <[email protected]>

utam0k force-pushed the kill-a-option branch from abe723e to e873658 Compare March 22, 2023 12:11

AkihiroSuda changed the title ~~Add the all option to kill command~~ Add the all option to kill operation Mar 23, 2023

AkihiroSuda added the easy-to-review label Mar 27, 2023

AkihiroSuda added this to the v1.1.0 milestone Mar 28, 2023

AkihiroSuda mentioned this pull request Mar 28, 2023

Release v1.1.0-rc.2 #1192

Merged

AkihiroSuda modified the milestones: v1.1.0, v1.1.1 May 12, 2023

AkihiroSuda removed the easy-to-review label May 12, 2023

AkihiroSuda mentioned this pull request Oct 10, 2023

clarify kill and delete operation for shared pid namespace container #1234

Open

cyphar requested changes Nov 8, 2023

View reviewed changes

utam0k closed this Nov 8, 2023

AkihiroSuda removed this from the v1.1.1 milestone Dec 12, 2023

		#### <a name="runtimeKillAll" />KillAll

		- `--all` Kill all the processes in the container.

Conversation

utam0k commented Mar 21, 2023

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

utam0k commented Mar 22, 2023

Uh oh!

AkihiroSuda Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

utam0k Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

utam0k Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 22, 2023

Uh oh!

utam0k commented Mar 22, 2023

Uh oh!

AkihiroSuda commented May 12, 2023

Uh oh!

AkihiroSuda commented Nov 6, 2023

Uh oh!

kolyshkin commented Nov 6, 2023

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

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

Uh oh!

cyphar commented Nov 8, 2023

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

utam0k commented Nov 8, 2023

Uh oh!

utam0k commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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