Merge pull request #2506 from kolyshkin/cgroup-fixes

cgroupv1 removal nits

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 {

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 {

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 {

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
 		}
 	}

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 <<EOF | runc exec test_busybox sh
+set -e -u -x
+for s in ${subsystems}; do
+  cd /sys/fs/cgroup/\$s
+  mkdir foo
+  cd foo
+  echo ${pid} > 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"