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

Make use of cgroup.kill #3135

Closed
kolyshkin opened this issue Aug 6, 2021 · 2 comments · Fixed by #3825
Closed

Make use of cgroup.kill #3135

kolyshkin opened this issue Aug 6, 2021 · 2 comments · Fixed by #3825

Comments

@kolyshkin
Copy link
Contributor

kolyshkin commented Aug 6, 2021

Cgroup v2 Kernel 5.14+ implements cgroup.kill which does the same thing that we try to do in signalAllProcesses

func signalAllProcesses(m cgroups.Manager, s os.Signal) error {

but in the kernel and reliably.

Since we're using signalAllProcesses mostly to send SIGKILL to container (with the exception of runc kill --all), this needs to be moved to cgroup managers to benefit from the new feature.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Aug 12, 2021

So, now we have this:

func destroy(c *linuxContainer) error {
if !c.config.Namespaces.Contains(configs.NEWPID) ||
c.config.Namespaces.PathOf(configs.NEWPID) != "" {
if err := signalAllProcesses(c.cgroupManager, unix.SIGKILL); err != nil {
logrus.Warn(err)
}
}
err := c.cgroupManager.Destroy()

and this:

runc/delete.go

Lines 18 to 27 in b114862

func killContainer(container libcontainer.Container) error {
_ = container.Signal(unix.SIGKILL, false)
for i := 0; i < 100; i++ {
time.Sleep(100 * time.Millisecond)
if err := container.Signal(unix.Signal(0), false); err != nil {
destroy(container)
return nil
}
}
return errors.New("container init still running")

and it would be nice to use cgroup.kill in both cases (possibly by adding Kill() method to cgroupManager, implementing it in a traditional way for v1, and use cgroup.kill for v2).

@kolyshkin
Copy link
Contributor Author

This is a relatively new addition and might not be available for older kernels: https://lwn.net/Articles/855924/

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