Merge pull request #2395 from lifubang/updateCgroupv2

Partially revert "CreateCgroupPath: only enable needed controllers"

libcontainer/cgroups/fs2/create.go

@@ -1,6 +1,7 @@
 package fs2
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -10,57 +11,56 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-// neededControllers returns the string to write to cgroup.subtree_control,
-// containing the list of controllers to enable (for example, "+cpu +pids"),
+func supportedControllers(cgroup *configs.Cgroup) ([]byte, error) {
+	const file = UnifiedMountpoint + "/cgroup.controllers"
+	return ioutil.ReadFile(file)
+}
+
+// needAnyControllers returns whether we enable some supported controllers or not,
 // based on (1) controllers available and (2) resources that are being set.
-//
-// The resulting string does not include "pseudo" controllers such as
+// We don't check "pseudo" controllers such as
 // "freezer" and "devices".
-func neededControllers(cgroup *configs.Cgroup) ([]string, error) {
-	var list []string
-
+func needAnyControllers(cgroup *configs.Cgroup) (bool, error) {
 	if cgroup == nil {
-		return list, nil
+		return false, nil
 	}
 
 	// list of all available controllers
-	const file = UnifiedMountpoint + "/cgroup.controllers"
-	content, err := ioutil.ReadFile(file)
+	content, err := supportedControllers(cgroup)
 	if err != nil {
-		return list, err
+		return false, err
 	}
 	avail := make(map[string]struct{})
 	for _, ctr := range strings.Fields(string(content)) {
 		avail[ctr] = struct{}{}
 	}
 
-	// add the controller if available
-	add := func(controller string) {
-		if _, ok := avail[controller]; ok {
-			list = append(list, "+"+controller)
-		}
+	// check whether the controller if available or not
+	have := func(controller string) bool {
+		_, ok := avail[controller]
+		return ok
 	}
 
-	if isPidsSet(cgroup) {
-		add("pids")
+	if isPidsSet(cgroup) && have("pids") {
+		return true, nil
 	}
-	if isMemorySet(cgroup) {
-		add("memory")
+	if isMemorySet(cgroup) && have("memory") {
+		return true, nil
 	}
-	if isIoSet(cgroup) {
-		add("io")
+	if isIoSet(cgroup) && have("io") {
+		return true, nil
 	}
-	if isCpuSet(cgroup) {
-		add("cpu")
+	if isCpuSet(cgroup) && have("cpu") {
+		return true, nil
 	}
-	if isCpusetSet(cgroup) {
-		add("cpuset")
+	if isCpusetSet(cgroup) && have("cpuset") {
+		return true, nil
 	}
-	if isHugeTlbSet(cgroup) {
-		add("hugetlb")
+	if isHugeTlbSet(cgroup) && have("hugetlb") {
+		return true, nil
 	}
 
-	return list, nil
+	return false, nil
 }
 
 // containsDomainController returns whether the current config contains domain controller or not.
@@ -70,18 +70,19 @@ func containsDomainController(cg *configs.Cgroup) bool {
 	return isMemorySet(cg) || isIoSet(cg) || isCpuSet(cg) || isHugeTlbSet(cg)
 }
 
-// CreateCgroupPath creates cgroupv2 path, enabling all the
-// needed controllers in the process.
+// CreateCgroupPath creates cgroupv2 path, enabling all the supported controllers.
 func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) {
 	if !strings.HasPrefix(path, UnifiedMountpoint) {
 		return fmt.Errorf("invalid cgroup path %s", path)
 	}
 
-	ctrs, err := neededControllers(c)
+	content, err := supportedControllers(c)
 	if err != nil {
 		return err
 	}
-	allCtrs := strings.Join(ctrs, " ")
+
+	ctrs := bytes.Fields(content)
+	res := append([]byte("+"), bytes.Join(ctrs, []byte(" +"))...)
 
 	elements := strings.Split(path, "/")
 	elements = elements[3:]
@@ -131,13 +132,14 @@ func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) {
 				}
 			}
 		}
-		// enable needed controllers
+		// enable all supported controllers
 		if i < len(elements)-1 {
 			file := filepath.Join(current, "cgroup.subtree_control")
-			if err := ioutil.WriteFile(file, []byte(allCtrs), 0644); err != nil {
+			if err := ioutil.WriteFile(file, res, 0644); err != nil {
 				// try write one by one
-				for _, ctr := range ctrs {
-					_ = ioutil.WriteFile(file, []byte(ctr), 0644)
+				allCtrs := bytes.Split(res, []byte(" "))
+				for _, ctr := range allCtrs {
+					_ = ioutil.WriteFile(file, ctr, 0644)
 				}
 			}
 			// Some controllers might not be enabled when rootless or containerized,

libcontainer/cgroups/fs2/fs2.go

@@ -76,8 +76,7 @@ func (m *manager) Apply(pid int) error {
 		// - "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
 		if m.rootless {
 			if m.config.Path == "" {
-				cl, clErr := neededControllers(m.config)
-				if clErr == nil && len(cl) == 0 {
+				if blNeed, nErr := needAnyControllers(m.config); nErr == nil && !blNeed {
 					return nil
 				}
 				return errors.Wrap(err, "rootless needs no limits + no cgrouppath when no permission is granted for cgroups")
@@ -175,57 +174,41 @@ func (m *manager) Set(container *configs.Config) error {
 	if err := m.getControllers(); err != nil {
 		return err
 	}
-	var errs []error
 	// pids (since kernel 4.5)
-	if _, ok := m.controllers["pids"]; ok {
-		if err := setPids(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setPids(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// memory (since kernel 4.5)
-	if _, ok := m.controllers["memory"]; ok {
-		if err := setMemory(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setMemory(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// io (since kernel 4.5)
-	if _, ok := m.controllers["io"]; ok {
-		if err := setIo(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setIo(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// cpu (since kernel 4.15)
-	if _, ok := m.controllers["cpu"]; ok {
-		if err := setCpu(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setCpu(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// devices (since kernel 4.15, pseudo-controller)
 	//
 	// When m.Rootless is true, errors from the device subsystem are ignored because it is really not expected to work.
 	// However, errors from other subsystems are not ignored.
 	// see @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error"
 	if err := setDevices(m.dirPath, container.Cgroups); err != nil && !m.rootless {
-		errs = append(errs, err)
+		return err
 	}
 	// cpuset (since kernel 5.0)
-	if _, ok := m.controllers["cpuset"]; ok {
-		if err := setCpuset(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setCpuset(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// hugetlb (since kernel 5.6)
-	if _, ok := m.controllers["hugetlb"]; ok {
-		if err := setHugeTlb(m.dirPath, container.Cgroups); err != nil {
-			errs = append(errs, err)
-		}
+	if err := setHugeTlb(m.dirPath, container.Cgroups); err != nil {
+		return err
 	}
 	// freezer (since kernel 5.2, pseudo-controller)
 	if err := setFreezer(m.dirPath, container.Cgroups.Freezer); err != nil {
-		errs = append(errs, err)
-	}
-	if len(errs) > 0 {
-		return errors.Errorf("error while setting cgroup v2: %+v", errs)
+		return err
 	}
 	m.config = container.Cgroups
 	return nil

tests/integration/cgroups.bats

@@ -110,6 +110,17 @@ EOF
 
     runc run -d --console-socket $CONSOLE_SOCKET test_cgroups_permissions
     [ "$status" -eq 0 ]
+    if [ "$CGROUP_UNIFIED" != "no" ]; then
+        if [ -n "${RUNC_USE_SYSTEMD}" ] ; then
+            if [ $(id -u) = "0" ]; then
+                check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/machine.slice/cgroup.controllers)"
+            else
+                check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers)"
+            fi
+        else
+            check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/cgroup.controllers)"
+        fi
+    fi
 }
 
 @test "runc exec (limits + cgrouppath + permission on the cgroup dir) succeeds" {