From 33c9f8b9c7818b47f377d11383da4d086d6ee3cd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 7 May 2021 15:22:47 -0700 Subject: [PATCH] libct/cg/sd: return error from stopUnit Historically, we never returned an error from failed startUnit or stopUnit. The startUnit case was fixed by commit 3844789. It is time to fix stopUnit, too. The reasons are: 1. Ignoring an error from stopUnit means an unexpected trouble down the road, for example a failure to create a container with the same name: > time="2021-05-07T19:51:27Z" level=error msg="container_linux.go:380: starting container process caused: process_linux.go:385: applying cgroup configuration for process caused: Unit runc-test_busybox.scope already exists." 2. A somewhat short timeout of 1 second means the cgroup might actually be removed a few seconds later but we might have a race between removing the cgroup and creating another one with the same name, resulting in the same error as amove. So, return an error if removal failed, and increase the timeout. Now, modify the systemd cgroup v1 manager to not mask the error from stopUnit (stopErr) with the subsequent one from cgroups.RemovePath, as stopErr is most probably the reason why RemovePath failed. Note that for v1 we do want to remove the paths even in case of a failure from stopUnit, as some were not created by systemd. There's no need to do that for v2, thanks to unified hierarchy, so no changes there. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/common.go | 7 +++++-- libcontainer/cgroups/systemd/v1.go | 7 ++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go index 91c314e09..563afc9f3 100644 --- a/libcontainer/cgroups/systemd/common.go +++ b/libcontainer/cgroups/systemd/common.go @@ -355,6 +355,9 @@ func stopUnit(cm *dbusConnManager, unitName string) error { return err }) if err == nil { + timeout := time.NewTimer(30 * time.Second) + defer timeout.Stop() + select { case s := <-statusChan: close(statusChan) @@ -362,8 +365,8 @@ func stopUnit(cm *dbusConnManager, unitName string) error { if s != "done" { logrus.Warnf("error removing unit `%s`: got `%s`. Continuing...", unitName, s) } - case <-time.After(time.Second): - logrus.Warnf("Timed out while waiting for StopUnit(%s) completion signal from dbus. Continuing...", unitName) + case <-timeout.C: + return errors.New("Timed out while waiting for systemd to remove " + unitName) } } return nil diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 41de6e8b7..b6a70e024 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -207,9 +207,10 @@ func (m *legacyManager) Destroy() error { stopErr := stopUnit(m.dbus, getUnitName(m.cgroups)) - // Both on success and on error, cleanup all the cgroups we are aware of. - // Some of them were created directly by Apply() and are not managed by systemd. - if err := cgroups.RemovePaths(m.paths); err != nil { + // Both on success and on error, cleanup all the cgroups + // we are aware of, as some of them were created directly + // by Apply() and are not managed by systemd. + if err := cgroups.RemovePaths(m.paths); err != nil && stopErr == nil { return err }