CreateCgroupPath: only enable needed controllers

1. Instead of enabling all available controllers, figure out which
   ones are required, and only enable those.

2. Amend all setFoo() functions to call isFooSet(). While this might
   seem unnecessary, it might actually help to uncover a bug.
   Imagine someone:
    - adds a cgroup.Resources.CpuFoo setting;
    - modifies setCpu() to apply the new setting;
    - but forgets to amend isCpuSet() accordingly <-- BUG

   In this case, a test case modifying CpuFoo will help
   to uncover the BUG. This is the reason why it's added.

This patch *could be* amended by enabling controllers on a best-effort
basis, i.e. :

 - do not return an error early if we can't enable some controllers;
 - if we fail to enable all controllers at once (usually because one
   of them can't be enabled), try enabling them one by one.

Currently this is not implemented, and it's not clear whether this
would be a good way to go or not.

[v2: add/use is${Controller}Set() functions]
[v3: document neededControllers()]
[v4: drop "best-effort" part]

Signed-off-by: Kir Kolyshkin <[email protected]>

libcontainer/cgroups/fs2/cpu.go

@@ -13,7 +13,15 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
+func isCpuSet(cgroup *configs.Cgroup) bool {
+	return cgroup.Resources.CpuWeight != 0 || cgroup.Resources.CpuMax != ""
+}
+
 func setCpu(dirPath string, cgroup *configs.Cgroup) error {
+	if !isCpuSet(cgroup) {
+		return nil
+	}
+
 	// NOTE: .CpuShares is not used here. Conversion is the caller's responsibility.
 	if cgroup.Resources.CpuWeight != 0 {
 		if err := fscommon.WriteFile(dirPath, "cpu.weight", strconv.FormatUint(cgroup.Resources.CpuWeight, 10)); err != nil {

libcontainer/cgroups/fs2/cpuset.go

@@ -7,7 +7,15 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
+func isCpusetSet(cgroup *configs.Cgroup) bool {
+	return cgroup.Resources.CpusetCpus != "" || cgroup.Resources.CpusetMems != ""
+}
+
 func setCpuset(dirPath string, cgroup *configs.Cgroup) error {
+	if !isCpusetSet(cgroup) {
+		return nil
+	}
+
 	if cgroup.Resources.CpusetCpus != "" {
 		if err := fscommon.WriteFile(dirPath, "cpuset.cpus", cgroup.Resources.CpusetCpus); err != nil {
 			return err

libcontainer/cgroups/fs2/create.go

@@ -1,29 +1,77 @@
 package fs2
 
 import (
-	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
+
+	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-// CreateCgroupPath creates cgroupv2 path, enabling all the
-// available controllers in the process.
-func CreateCgroupPath(path string) (Err error) {
-	if !strings.HasPrefix(path, UnifiedMountpoint) {
-		return fmt.Errorf("invalid cgroup path %s", path)
+// neededControllers returns the string to write to cgroup.subtree_control,
+// containing the list of controllers to enable (for example, "+cpu +pids"),
+// based on (1) controllers available and (2) resources that are being set.
+//
+// The resulting string does not include "pseudo" controllers such as
+// "freezer" and "devices".
+func neededControllers(cgroup *configs.Cgroup) []string {
+	var list []string
+
+	if cgroup == nil {
+		return list
 	}
 
+	// list of all available controllers
 	const file = UnifiedMountpoint + "/cgroup.controllers"
 	content, err := ioutil.ReadFile(file)
 	if err != nil {
-		return err
+		return list
+	}
+	avail := make(map[string]struct{})
+	for _, ctr := range strings.Split(string(content), " ") {
+		avail[ctr] = struct{}{}
+	}
+
+	// add the controller if available
+	add := func(controller string) {
+		if _, ok := avail[controller]; ok {
+			list = append(list, "+"+controller)
+		}
+	}
+
+	if isPidsSet(cgroup) {
+		add("pids")
+	}
+	if isMemorySet(cgroup) {
+		add("memory")
 	}
+	if isIoSet(cgroup) {
+		add("io")
+	}
+	if isCpuSet(cgroup) {
+		add("cpu")
+	}
+	if isCpusetSet(cgroup) {
+		add("cpuset")
+	}
+	if isHugeTlbSet(cgroup) {
+		add("hugetlb")
+	}
+
+	return list
+}
 
-	ctrs := bytes.Fields(content)
-	res := append([]byte("+"), bytes.Join(ctrs, []byte(" +"))...)
+// CreateCgroupPath creates cgroupv2 path, enabling all the
+// needed controllers in the process.
+func CreateCgroupPath(path string, c *configs.Cgroup) (Err error) {
+	if !strings.HasPrefix(path, UnifiedMountpoint) {
+		return fmt.Errorf("invalid cgroup path %s", path)
+	}
+
+	ctrs := neededControllers(c)
+	allCtrs := strings.Join(ctrs, " ")
 
 	elements := strings.Split(path, "/")
 	elements = elements[3:]
@@ -45,11 +93,16 @@ func CreateCgroupPath(path string) (Err error) {
 				}()
 			}
 		}
+		// enable needed controllers
 		if i < len(elements)-1 {
-			if err := ioutil.WriteFile(filepath.Join(current, "cgroup.subtree_control"), res, 0755); err != nil {
+			file := filepath.Join(current, "cgroup.subtree_control")
+			if err := ioutil.WriteFile(file, []byte(allCtrs), 0755); err != nil {
+				// XXX: we can enable _some_ controllers doing it one-by one
+				// instead of erroring out -- does it makes sense to do so?
 				return err
 			}
 		}
 	}
+
 	return nil
 }

libcontainer/cgroups/fs2/fs2.go

@@ -66,7 +66,7 @@ func (m *manager) getControllers() error {
 }
 
 func (m *manager) Apply(pid int) error {
-	if err := CreateCgroupPath(m.dirPath); err != nil {
+	if err := CreateCgroupPath(m.dirPath, m.config); err != nil {
 		return err
 	}
 	if err := cgroups.WriteCgroupProc(m.dirPath, pid); err != nil && !m.rootless {

libcontainer/cgroups/fs2/hugetlb.go

@@ -15,7 +15,14 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
+func isHugeTlbSet(cgroup *configs.Cgroup) bool {
+	return len(cgroup.Resources.HugetlbLimit) > 0
+}
+
 func setHugeTlb(dirPath string, cgroup *configs.Cgroup) error {
+	if !isHugeTlbSet(cgroup) {
+		return nil
+	}
 	for _, hugetlb := range cgroup.Resources.HugetlbLimit {
 		if err := fscommon.WriteFile(dirPath, strings.Join([]string{"hugetlb", hugetlb.Pagesize, "max"}, "."), strconv.FormatUint(hugetlb.Limit, 10)); err != nil {
 			return err

libcontainer/cgroups/fs2/io.go

@@ -14,7 +14,19 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
+func isIoSet(cgroup *configs.Cgroup) bool {
+	return cgroup.Resources.BlkioWeight != 0 ||
+		len(cgroup.Resources.BlkioThrottleReadBpsDevice) > 0 ||
+		len(cgroup.Resources.BlkioThrottleWriteBpsDevice) > 0 ||
+		len(cgroup.Resources.BlkioThrottleReadIOPSDevice) > 0 ||
+		len(cgroup.Resources.BlkioThrottleWriteIOPSDevice) > 0
+}
+
 func setIo(dirPath string, cgroup *configs.Cgroup) error {
+	if !isIoSet(cgroup) {
+		return nil
+	}
+
 	if cgroup.Resources.BlkioWeight != 0 {
 		filename := "io.bfq.weight"
 		if err := fscommon.WriteFile(dirPath, filename,

libcontainer/cgroups/fs2/memory.go

@@ -32,7 +32,15 @@ func numToStr(value int64) (ret string) {
 	return ret
 }
 
+func isMemorySet(cgroup *configs.Cgroup) bool {
+	return cgroup.Resources.MemoryReservation != 0 ||
+		cgroup.Resources.Memory != 0 || cgroup.Resources.MemorySwap != 0
+}
+
 func setMemory(dirPath string, cgroup *configs.Cgroup) error {
+	if !isMemorySet(cgroup) {
+		return nil
+	}
 	swap, err := cgroups.ConvertMemorySwapToCgroupV2Value(cgroup.Resources.MemorySwap, cgroup.Resources.Memory)
 	if err != nil {
 		return err

libcontainer/cgroups/fs2/pids.go

@@ -14,7 +14,14 @@ import (
 	"golang.org/x/sys/unix"
 )
 
+func isPidsSet(cgroup *configs.Cgroup) bool {
+	return cgroup.Resources.PidsLimit != 0
+}
+
 func setPids(dirPath string, cgroup *configs.Cgroup) error {
+	if !isPidsSet(cgroup) {
+		return nil
+	}
 	if val := numToStr(cgroup.Resources.PidsLimit); val != "" {
 		if err := fscommon.WriteFile(dirPath, "pids.max", val); err != nil {
 			return err

libcontainer/cgroups/systemd/v2.go

@@ -151,7 +151,7 @@ func (m *unifiedManager) Apply(pid int) error {
 	if err != nil {
 		return err
 	}
-	if err := fs2.CreateCgroupPath(m.path); err != nil {
+	if err := fs2.CreateCgroupPath(m.path, m.cgroups); err != nil {
 		return err
 	}
 	return nil