From dc42459130a7b6ab7439d82af040473734eb7f86 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 29 Sep 2020 18:43:28 -0700 Subject: [PATCH] libct/(*initProcess).start: fix removing cgroups on error In case cgroup configuration is invalid (some parameters can't be set etc.), p.manager.Set fails, the error is returned, and then we try to remove cgroups (by calling p.manager.Destroy) in a defer. The problem is, the container init is not yet killed (as it is killed in the caller, i.e. (*linuxContainer).start), so cgroup removal fails like this: > time="2020-09-26T07:46:25Z" level=warning msg="Failed to remove cgroup (will retry)" error="rmdir /sys/fs/cgroup/net_cls,net_prio/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod28ce6e74_694c_4b77_a953_dc01e182ac76.slice/crio-f6984c5eeb6c6b49ff3f036bdcb9ded317b3d0b2469ebbb35705442a2afd98c2.scope: device or resource busy" > ... > time="2020-09-26T07:46:27Z" level=error msg="Failed to remove cgroup" error="rmdir /sys/fs/cgroup/net_cls,net_prio/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod28ce6e74_694c_4b77_a953_dc01e182ac76.slice/crio-f6984c5eeb6c6b49ff3f036bdcb9ded317b3d0b2469ebbb35705442a2afd98c2.scope: device or resource busy" The above is repeated for every controller, and looks quite scary. To fix, move the init termination to the abovementioned defer. Do the same for (*setnsProcess).start() for uniformity. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 4 ---- libcontainer/process_linux.go | 13 +++++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 3531487c6..27b5a618e 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -363,10 +363,6 @@ func (c *linuxContainer) start(process *Process) error { } parent.forwardChildLogs() if err := parent.start(); err != nil { - // terminate the process to ensure that it properly is reaped. - if err := ignoreTerminateErrors(parent.terminate()); err != nil { - logrus.Warn(err) - } return newSystemErrorWithCause(err, "starting container process") } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 81004046e..1402b516d 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -95,6 +95,14 @@ func (p *setnsProcess) start() (retErr error) { if err != nil { return newSystemErrorWithCause(err, "starting setns process") } + defer func() { + if retErr != nil { + err := ignoreTerminateErrors(p.terminate()) + if err != nil { + logrus.WithError(err).Warn("unable to terminate setnsProcess") + } + } + }() if p.bootstrapData != nil { if _, err := io.Copy(p.messageSockPair.parent, p.bootstrapData); err != nil { return newSystemErrorWithCause(err, "copying bootstrap data to pipe") @@ -313,6 +321,11 @@ func (p *initProcess) start() (retErr error) { } defer func() { if retErr != nil { + // terminate the process to ensure we can remove cgroups + if err := ignoreTerminateErrors(p.terminate()); err != nil { + logrus.WithError(err).Warn("unable to terminate initProcess") + } + p.manager.Destroy() if p.intelRdtManager != nil { p.intelRdtManager.Destroy()