Merge pull request #3554 from giuseppe/fix-cgroupfs-cleanup

cgroups: fix a leak when using cgroupfs

7 files changed, 62 insertions, 9 deletions

diff --git a/pkg/cgroups/blkio.go b/pkg/cgroups/blkio.go
index 9c2a811d9..bacd4eb93 100644
--- a/pkg/cgroups/blkio.go
+++ b/pkg/cgroups/blkio.go
@@ -37,7 +37,7 @@ func (c *blkioHandler) Create(ctr *CgroupControl) (bool, error) {
 
 // Destroy the cgroup
 func (c *blkioHandler) Destroy(ctr *CgroupControl) error {
-	return os.Remove(ctr.getCgroupv1Path(Blkio))
+	return rmDirRecursively(ctr.getCgroupv1Path(Blkio))
 }
 
 // Stat fills a metrics structure with usage stats for the controller
diff --git a/pkg/cgroups/cgroups.go b/pkg/cgroups/cgroups.go
index 1dad45d7f..081db772f 100644
--- a/pkg/cgroups/cgroups.go
+++ b/pkg/cgroups/cgroups.go
@@ -328,6 +328,13 @@ func Load(path string) (*CgroupControl, error) {
 		systemd: false,
 	}
 	if !cgroup2 {
+		controllers, err := getAvailableControllers(handlers, false)
+		if err != nil {
+			return nil, err
+		}
+		control.additionalControllers = controllers
+	}
+	if !cgroup2 {
 		for name := range handlers {
 			p := control.getCgroupv1Path(name)
 			if _, err := os.Stat(p); err != nil {
@@ -355,11 +362,40 @@ func (c *CgroupControl) Delete() error {
 	return c.DeleteByPath(c.path)
 }
 
+// rmDirRecursively delete recursively a cgroup directory.
+// It differs from os.RemoveAll as it doesn't attempt to unlink files.
+// On cgroupfs we are allowed only to rmdir empty directories.
+func rmDirRecursively(path string) error {
+	if err := os.Remove(path); err == nil || os.IsNotExist(err) {
+		return nil
+	}
+	entries, err := ioutil.ReadDir(path)
+	if err != nil {
+		return errors.Wrapf(err, "read %s", path)
+	}
+	for _, i := range entries {
+		if i.IsDir() {
+			if err := rmDirRecursively(filepath.Join(path, i.Name())); err != nil {
+				return err
+			}
+		}
+	}
+	if os.Remove(path); err != nil {
+		if !os.IsNotExist(err) {
+			return errors.Wrapf(err, "remove %s", path)
+		}
+	}
+	return nil
+}
+
 // DeleteByPath deletes the specified cgroup path
 func (c *CgroupControl) DeleteByPath(path string) error {
 	if c.systemd {
 		return systemdDestroy(path)
 	}
+	if c.cgroup2 {
+		return rmDirRecursively(filepath.Join(cgroupRoot, c.path))
+	}
 	var lastError error
 	for _, h := range handlers {
 		if err := h.Destroy(c); err != nil {
@@ -368,8 +404,11 @@ func (c *CgroupControl) DeleteByPath(path string) error {
 	}
 
 	for _, ctr := range c.additionalControllers {
+		if ctr.symlink {
+			continue
+		}
 		p := c.getCgroupv1Path(ctr.name)
-		if err := os.Remove(p); err != nil {
+		if err := rmDirRecursively(p); err != nil {
 			lastError = errors.Wrapf(err, "remove %s", p)
 		}
 	}
diff --git a/pkg/cgroups/cpu.go b/pkg/cgroups/cpu.go
index 1c8610cc4..03677f1ef 100644
--- a/pkg/cgroups/cpu.go
+++ b/pkg/cgroups/cpu.go
@@ -68,7 +68,7 @@ func (c *cpuHandler) Create(ctr *CgroupControl) (bool, error) {
 
 // Destroy the cgroup
 func (c *cpuHandler) Destroy(ctr *CgroupControl) error {
-	return os.Remove(ctr.getCgroupv1Path(CPU))
+	return rmDirRecursively(ctr.getCgroupv1Path(CPU))
 }
 
 // Stat fills a metrics structure with usage stats for the controller
diff --git a/pkg/cgroups/cpuset.go b/pkg/cgroups/cpuset.go
index 25d2f7f76..46d0484f2 100644
--- a/pkg/cgroups/cpuset.go
+++ b/pkg/cgroups/cpuset.go
@@ -3,7 +3,6 @@ package cgroups
 import (
 	"fmt"
 	"io/ioutil"
-	"os"
 	"path/filepath"
 	"strings"
 
@@ -77,7 +76,7 @@ func (c *cpusetHandler) Create(ctr *CgroupControl) (bool, error) {
 
 // Destroy the cgroup
 func (c *cpusetHandler) Destroy(ctr *CgroupControl) error {
-	return os.Remove(ctr.getCgroupv1Path(CPUset))
+	return rmDirRecursively(ctr.getCgroupv1Path(CPUset))
 }
 
 // Stat fills a metrics structure with usage stats for the controller
diff --git a/pkg/cgroups/memory.go b/pkg/cgroups/memory.go
index 80e88d17c..b3991f7e3 100644
--- a/pkg/cgroups/memory.go
+++ b/pkg/cgroups/memory.go
@@ -2,7 +2,6 @@ package cgroups
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 
 	spec "github.com/opencontainers/runtime-spec/specs-go"
@@ -33,7 +32,7 @@ func (c *memHandler) Create(ctr *CgroupControl) (bool, error) {
 
 // Destroy the cgroup
 func (c *memHandler) Destroy(ctr *CgroupControl) error {
-	return os.Remove(ctr.getCgroupv1Path(Memory))
+	return rmDirRecursively(ctr.getCgroupv1Path(Memory))
 }
 
 // Stat fills a metrics structure with usage stats for the controller
diff --git a/pkg/cgroups/pids.go b/pkg/cgroups/pids.go
index ffbde100d..65b9b5b34 100644
--- a/pkg/cgroups/pids.go
+++ b/pkg/cgroups/pids.go
@@ -3,7 +3,6 @@ package cgroups
 import (
 	"fmt"
 	"io/ioutil"
-	"os"
 	"path/filepath"
 
 	spec "github.com/opencontainers/runtime-spec/specs-go"
@@ -40,7 +39,7 @@ func (c *pidHandler) Create(ctr *CgroupControl) (bool, error) {
 
 // Destroy the cgroup
 func (c *pidHandler) Destroy(ctr *CgroupControl) error {
-	return os.Remove(ctr.getCgroupv1Path(Pids))
+	return rmDirRecursively(ctr.getCgroupv1Path(Pids))
 }
 
 // Stat fills a metrics structure with usage stats for the controller
diff --git a/test/e2e/pod_rm_test.go b/test/e2e/pod_rm_test.go
index 0d3f47f30..f0689f152 100644
--- a/test/e2e/pod_rm_test.go
+++ b/test/e2e/pod_rm_test.go
@@ -3,6 +3,8 @@ package integration
 import (
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	. "github.com/containers/libpod/test/utils"
 	. "github.com/onsi/ginkgo"
@@ -40,6 +42,21 @@ var _ = Describe("Podman pod rm", func() {
 		result := podmanTest.Podman([]string{"pod", "rm", podid})
 		result.WaitWithDefaultTimeout()
 		Expect(result.ExitCode()).To(Equal(0))
+
+		// Also check that we don't leak cgroups
+		err := filepath.Walk("/sys/fs/cgroup", func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			if !info.IsDir() {
+				Expect(err).To(BeNil())
+			}
+			if strings.Contains(info.Name(), podid) {
+				return fmt.Errorf("leaking cgroup path %s", path)
+			}
+			return nil
+		})
+		Expect(err).To(BeNil())
 	})
 
 	It("podman pod rm latest pod", func() {