diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 3ccf3a3c6..531426084 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -274,11 +274,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/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/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 7217b0af7..aaf351df1 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -212,17 +212,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..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" ) @@ -207,20 +208,66 @@ 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 // 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 @@ -230,6 +277,7 @@ func RemovePaths(paths map[string]string) (err error) { } } if len(paths) == 0 { + paths = make(map[string]string) return nil } } diff --git a/tests/integration/delete.bats b/tests/integration/delete.bats index 43f6b9de8..57d08a48a 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"