From 19be8e5ba56df7a91c71c05649c8619ecf0a4ade Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jul 2020 17:37:42 -0700 Subject: [PATCH] libct/cgroups.RemovePaths: speedup Using os.RemoveAll has the following two issues: 1. it tries to remove all files, which does not make sense for cgroups; 2. it tries rm(2) which fails to directories, and then rmdir(2). Let's reuse our RemovePath instead, and add warnings and errors logging. PS I am somewhat hesitant to remove the weird checking my means of stat, as it might break something. Unfortunately, neither commit 6feb7bda04b31 nor the PR it contains [1] do not explain what kind of weird errors were seen from os.RemoveAll. Most probably our code won't return any bogus errors, but let's keep the old code to be on the safe side. [1] https://github.com/docker-archive/libcontainer/pull/308 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/utils.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index dc6aa683c..58cc4e19c 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -16,6 +16,7 @@ import ( "time" units "github.com/docker/go-units" + "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -249,15 +250,24 @@ func RemovePath(path string) error { // If after all there are not removed cgroups - appropriate error will be // returned. func RemovePaths(paths map[string]string) (err error) { + const retries = 5 delay := 10 * time.Millisecond - for i := 0; i < 5; i++ { + for i := 0; i < retries; i++ { if i != 0 { time.Sleep(delay) delay *= 2 } for s, p := range paths { - os.RemoveAll(p) - // TODO: here probably should be logging + if err := RemovePath(p); err != nil { + // do not log intermediate iterations + switch i { + case 0: + logrus.WithError(err).Warnf("Failed to remove cgroup (will retry)") + case retries - 1: + logrus.WithError(err).Error("Failed to remove cgroup") + } + + } _, err := os.Stat(p) // We need this strange way of checking cgroups existence because // RemoveAll almost always returns error, even on already removed