From 254d23b964f1f32ccd93bca1c3e25c173e75ed7c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jul 2020 15:44:17 -0700 Subject: [PATCH 1/4] libc/cgroups: empty map in RemovePaths RemovePaths() deletes elements from the paths map for paths that has been successfully removed. Although, it does not empty the map itself (which is needed that AFAIK Go garbage collector does not shrink the map), but all its callers do. Move this operation from callers to RemovePaths. No functional change, except the old map should be garbage collected now. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs.go | 6 +----- libcontainer/cgroups/systemd/v1.go | 9 +++------ libcontainer/cgroups/utils.go | 1 + 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index f9034f09f..3de8f9d58 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -233,11 +233,7 @@ func (m *manager) Destroy() error { } m.mu.Lock() defer m.mu.Unlock() - if err := cgroups.RemovePaths(m.paths); err != nil { - return err - } - m.paths = make(map[string]string) - return nil + return cgroups.RemovePaths(m.paths) } func (m *manager) Path(subsys string) string { diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index c3cc9c8b3..2d92dc279 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -223,17 +223,14 @@ func (m *legacyManager) Destroy() error { } unitName := getUnitName(m.cgroups) - err = stopUnit(dbusConnection, unitName) + stopErr := stopUnit(dbusConnection, unitName) // 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 { return err } - if err != nil { - return err - } - m.paths = make(map[string]string) - return nil + + return stopErr } func (m *legacyManager) Path(subsys string) string { diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 6e88b5dff..222a513ee 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -230,6 +230,7 @@ func RemovePaths(paths map[string]string) (err error) { } } if len(paths) == 0 { + paths = make(map[string]string) return nil } } From 3f14242e0a3b01a18621aa5efd91f87d915bd947 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jul 2020 17:30:58 -0700 Subject: [PATCH 2/4] libct/cgroups: move RemovePath from fs2 This is to be used by RemovePaths. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/fs2.go | 41 +-------------------------------- libcontainer/cgroups/utils.go | 37 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 40 deletions(-) diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 7be26211a..0975064f2 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -4,14 +4,12 @@ package fs2 import ( "io/ioutil" - "os" "path/filepath" "strings" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" "github.com/pkg/errors" - "golang.org/x/sys/unix" ) type manager struct { @@ -157,45 +155,8 @@ func (m *manager) Freeze(state configs.FreezerState) error { return nil } -func rmdir(path string) error { - err := unix.Rmdir(path) - if err == nil || err == unix.ENOENT { - return nil - } - return &os.PathError{Op: "rmdir", Path: path, Err: err} -} - -// removeCgroupPath aims to remove cgroup path recursively -// Because there may be subcgroups in it. -func removeCgroupPath(path string) error { - // try the fast path first - if err := rmdir(path); err == nil { - return nil - } - - infos, err := ioutil.ReadDir(path) - if err != nil { - if os.IsNotExist(err) { - err = nil - } - return err - } - for _, info := range infos { - if info.IsDir() { - // We should remove subcgroups dir first - if err = removeCgroupPath(filepath.Join(path, info.Name())); err != nil { - break - } - } - } - if err == nil { - err = rmdir(path) - } - return err -} - func (m *manager) Destroy() error { - return removeCgroupPath(m.dirPath) + return cgroups.RemovePath(m.dirPath) } func (m *manager) Path(_ string) string { diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 222a513ee..dc6aa683c 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -207,6 +207,43 @@ func EnterPid(cgroupPaths map[string]string, pid int) error { return nil } +func rmdir(path string) error { + err := unix.Rmdir(path) + if err == nil || err == unix.ENOENT { + return nil + } + return &os.PathError{Op: "rmdir", Path: path, Err: err} +} + +// RemovePath aims to remove cgroup path. It does so recursively, +// by removing any subdirectories (sub-cgroups) first. +func RemovePath(path string) error { + // try the fast path first + if err := rmdir(path); err == nil { + return nil + } + + infos, err := ioutil.ReadDir(path) + if err != nil { + if os.IsNotExist(err) { + err = nil + } + return err + } + for _, info := range infos { + if info.IsDir() { + // We should remove subcgroups dir first + if err = RemovePath(filepath.Join(path, info.Name())); err != nil { + break + } + } + } + if err == nil { + err = rmdir(path) + } + return err +} + // RemovePaths iterates over the provided paths removing them. // We trying to remove all paths five times with increasing delay between tries. // If after all there are not removed cgroups - appropriate error will be From 19be8e5ba56df7a91c71c05649c8619ecf0a4ade Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jul 2020 17:37:42 -0700 Subject: [PATCH 3/4] 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 From 335f0806c0d5d18a6cf7b2d8c222f33b2233032d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jul 2020 17:53:13 -0700 Subject: [PATCH 4/4] tests/int/delete: cgroupv1 with sub-cgroups removal case This is similar to what we did before for v2. Signed-off-by: Kir Kolyshkin --- tests/integration/delete.bats | 54 ++++++++++++++++++++++++++++++++++ tests/integration/helpers.bash | 5 ++++ 2 files changed, 59 insertions(+) diff --git a/tests/integration/delete.bats b/tests/integration/delete.bats index b356e7647..48264e1a8 100644 --- a/tests/integration/delete.bats +++ b/tests/integration/delete.bats @@ -52,6 +52,60 @@ function teardown() { [ "$status" -eq 0 ] } +@test "runc delete --force in cgroupv1 with subcgroups" { + requires cgroups_v1 root cgroupns + set_cgroups_path "$BUSYBOX_BUNDLE" + set_cgroup_mount_writable "$BUSYBOX_BUNDLE" + # enable cgroupns + update_config '.linux.namespaces += [{"type": "cgroup"}]' + + local subsystems="memory freezer" + + for i in $(seq 1); do + runc run -d --console-socket $CONSOLE_SOCKET test_busybox + [ "$status" -eq 0 ] + + testcontainer test_busybox running + + __runc exec -d test_busybox sleep 1d + + # find the pid of sleep + pid=$(__runc exec test_busybox ps -a | grep 1d | awk '{print $1}') + [[ ${pid} =~ [0-9]+ ]] + + # create a sub-cgroup + cat < tasks + cat tasks +done +EOF + [ "$status" -eq 0 ] + [[ "$output" =~ [0-9]+ ]] + + for s in ${subsystems}; do + name=CGROUP_${s^^} + eval path=\$${name}/foo + echo $path + [ -d ${path} ] || fail "test failed to create memory sub-cgroup" + done + + runc delete --force test_busybox + + runc state test_busybox + [ "$status" -ne 0 ] + + run find /sys/fs/cgroup -wholename '*testbusyboxdelete*' -type d + [ "$status" -eq 0 ] + [ "$output" = "" ] || fail "cgroup not cleaned up correctly: $output" + + done +} + @test "runc delete --force in cgroupv2 with subcgroups" { requires cgroups_v2 root set_cgroups_path "$BUSYBOX_BUNDLE" diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index b62b6ba49..62e2a464c 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -286,6 +286,11 @@ function requires() { skip_me=1 fi ;; + cgroupns) + if [ ! -e "/proc/self/ns/cgroup" ]; then + skip_me=1 + fi + ;; cgroups_v1) init_cgroup_paths if [ "$CGROUP_UNIFIED" != "no" ]; then