Repository: opencontainers/runc
Pull request #2358 (2020-04-27 22:02:38)

cgroups/fs2: don't always parse /proc/self/cgroup
Function `defaultPath` always parses `/proc/self/cgroup`, but
the resulting value is not always used.

Avoid unnecessary reading/parsing by moving the code
to just before its use.

Modify the test case accordingly.

AkihiroSuda: Looks good but a single nit


Patch:
diff --git a/libcontainer/cgroups/fs2/defaultpath.go b/libcontainer/cgroups/fs2/defaultpath.go
index ab35e4b1..00912645 100644
--- a/libcontainer/cgroups/fs2/defaultpath.go
+++ b/libcontainer/cgroups/fs2/defaultpath.go
@@ -44,19 +44,10 @@ func defaultDirPath(c *configs.Cgroup) (string, error) {
 	cgParent := libcontainerUtils.CleanPath(c.Parent)
 	cgName := libcontainerUtils.CleanPath(c.Name)
 
-	ownCgroup, err := parseCgroupFile("/proc/self/cgroup")
-	if err != nil {
-		return "", err
-	}
-	// The current user scope most probably has tasks in it already,
-	// making it impossible to enable controllers for its sub-cgroup.
-	// A parent cgroup (with no tasks in it) is what we need.
-	ownCgroup = filepath.Dir(ownCgroup)
-
-	return _defaultDirPath(UnifiedMountpoint, cgPath, cgParent, cgName, ownCgroup)
+	return _defaultDirPath(UnifiedMountpoint, cgPath, cgParent, cgName)
 }
 
-func _defaultDirPath(root, cgPath, cgParent, cgName, ownCgroup string) (string, error) {
+func _defaultDirPath(root, cgPath, cgParent, cgName string) (string, error) {
 	if (cgName != "" || cgParent != "") && cgPath != "" {
 		return "", errors.New("cgroup: either Path or Name and Parent should be used")
 	}
@@ -67,6 +58,16 @@ func _defaultDirPath(root, cgPath, cgParent, cgName, ownCgroup string) (string,
 	if filepath.IsAbs(innerPath) {
 		return filepath.Join(root, innerPath), nil
 	}
+
+	ownCgroup, err := parseCgroupFile("/proc/self/cgroup")
+	if err != nil {
+		return "", err
+	}
+	// The current user scope most probably has tasks in it already,
+	// making it impossible to enable controllers for its sub-cgroup.
+	// A parent cgroup (with no tasks in it) is what we need.
+	ownCgroup = filepath.Dir(ownCgroup)
+
 	return filepath.Join(root, ownCgroup, innerPath), nil
 }
 


Test patch:
diff --git a/libcontainer/cgroups/fs2/defaultpath_test.go b/libcontainer/cgroups/fs2/defaultpath_test.go
index 6d5d1170..775adc09 100644
--- a/libcontainer/cgroups/fs2/defaultpath_test.go
+++ b/libcontainer/cgroups/fs2/defaultpath_test.go
@@ -17,8 +17,11 @@
 package fs2
 
 import (
+	"path/filepath"
 	"strings"
 	"testing"
+
+	"github.com/opencontainers/runc/libcontainer/cgroups"
 )
 
 func TestParseCgroupFromReader(t *testing.T) {
@@ -45,27 +48,35 @@ func TestParseCgroupFromReader(t *testing.T) {
 }
 
 func TestDefaultDirPath(t *testing.T) {
-	root := "/sys/fs/cgroup"
+	if !cgroups.IsCgroup2UnifiedMode() {
+		t.Skip("need cgroupv2")
+	}
+	// same code as in defaultDirPath()
+	ownCgroup, err := parseCgroupFile("/proc/self/cgroup")
+	if err != nil {
+		// Not a test failure, but rather some weird
+		// environment so we can't run this test.
+		t.Skipf("can't get own cgroup: %v", err)
+	}
+	ownCgroup = filepath.Dir(ownCgroup)
+
 	cases := []struct {
-		cgPath    string
-		cgParent  string
-		cgName    string
-		ownCgroup string
-		expected  string
+		cgPath   string
+		cgParent string
+		cgName   string
+		expected string
 	}{
 		{
-			cgPath:    "/foo/bar",
-			ownCgroup: "/apple/banana",
-			expected:  "/sys/fs/cgroup/foo/bar",
+			cgPath:   "/foo/bar",
+			expected: "/sys/fs/cgroup/foo/bar",
 		},
 		{
-			cgPath:    "foo/bar",
-			ownCgroup: "/apple/banana",
-			expected:  "/sys/fs/cgroup/apple/banana/foo/bar",
+			cgPath:   "foo/bar",
+			expected: filepath.Join(UnifiedMountpoint, ownCgroup, "foo/bar"),
 		},
 	}
 	for _, c := range cases {
-		got, err := _defaultDirPath(root, c.cgPath, c.cgParent, c.cgName, c.ownCgroup)
+		got, err := _defaultDirPath(UnifiedMountpoint, c.cgPath, c.cgParent, c.cgName)
 		if err != nil {
 			t.Fatal(err)
 		}