Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: drop -a from runc kill #3864

Closed
kolyshkin opened this issue May 12, 2023 · 4 comments · Fixed by #3825
Closed

RFC: drop -a from runc kill #3864

kolyshkin opened this issue May 12, 2023 · 4 comments · Fixed by #3825
Milestone

Comments

@kolyshkin
Copy link
Contributor

kolyshkin commented May 12, 2023

Option -a for runc kill is only usable when we are trying to kill -9 the container which does not have its own PID namespace. All other use cases (like sending SIGTERM to all container processes) are questionable to say at least.

I propose we deprecate -a, and handle the above use case (sending SIGKILL to init of the container which does not have its own pidns) automatically (in fact, this is already done in libcontainer, but not in runc binary).

The deprecation can be done in steps:

  1. In runc 1.2, emit a warning when -a is used with runc kill.
  2. In runc 1.3 (or later version), upgrade the warning to error.
  3. In runc 1.4, drop the flag.
@kolyshkin
Copy link
Contributor Author

@giuseppe @cyphar @AkihiroSuda WDYT?

@AkihiroSuda
Copy link
Member

automatically

Probably this is fine but we might need to add another flag to disable it

drop the flag

For compatibility sake, the flag will have to remain as a NOP flag

@kolyshkin
Copy link
Contributor Author

automatically

Probably this is fine but we might need to add another flag to disable it

libcontainer works this way since 2005:

func (p *initProcess) wait() (*os.ProcessState, error) {
err := p.cmd.Wait()
// we should kill all processes in cgroup when init is died if we use host PID namespace
if p.sharePidns {
_ = signalAllProcesses(p.manager, unix.SIGKILL)
}

(Yes, the other peculiarity here is we call kill from wait which is counterintuitive to say at least).

For some reason, runc kill doesn't work this way, thus the need for -a.

I don't think we need a flag to disable this behavior -- there is no use case when you want to kill -9 container init but leave all other processes around.

drop the flag

For compatibility sake, the flag will have to remain as a NOP flag

Sure

@giuseppe
Copy link
Member

@giuseppe @cyphar @AkihiroSuda WDYT?

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants