Merge pull request opencontainers#2299 from kolyshkin/fs2-init-ctrl

cgroupv2: fix fs2 driver initialization

.travis.yml

@@ -36,6 +36,8 @@ matrix:
         - sudo ssh default sudo podman run --privileged --cgroupns=private -v /lib/modules:/lib/modules:ro test make localunittest
         # cgroupv2+systemd: test on vagrant host itself as we need systemd
         - sudo ssh default -t 'cd /vagrant && sudo make localintegration RUNC_USE_SYSTEMD=yes'
+        # same setup but with fs2 driver instead of systemd
+        - sudo ssh default -t 'cd /vagrant && sudo make localintegration'
   allow_failures:
     - go: tip
 

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

@@ -0,0 +1,111 @@
+package fs2
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"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"),
+// 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, error) {
+	var list []string
+
+	if cgroup == nil {
+		return list, nil
+	}
+
+	// list of all available controllers
+	const file = UnifiedMountpoint + "/cgroup.controllers"
+	content, err := ioutil.ReadFile(file)
+	if err != nil {
+		return list, 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)
+		}
+	}
+
+	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, nil
+}
+
+// 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, err := neededControllers(c)
+	if err != nil {
+		return err
+	}
+	allCtrs := strings.Join(ctrs, " ")
+
+	elements := strings.Split(path, "/")
+	elements = elements[3:]
+	current := "/sys/fs"
+	for i, e := range elements {
+		current = filepath.Join(current, e)
+		if i > 0 {
+			if err := os.Mkdir(current, 0755); err != nil {
+				if !os.IsExist(err) {
+					return err
+				}
+			} else {
+				// If the directory was created, be sure it is not left around on errors.
+				current := current
+				defer func() {
+					if Err != nil {
+						os.Remove(current)
+					}
+				}()
+			}
+		}
+		// enable needed controllers
+		if i < len(elements)-1 {
+			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

@@ -8,75 +8,67 @@ import (
 	"path/filepath"
 	"strings"
 
-	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/pkg/errors"
 )
 
+type manager struct {
+	config *configs.Cgroup
+	// dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope"
+	dirPath string
+	// controllers is content of "cgroup.controllers" file.
+	// excludes pseudo-controllers ("devices" and "freezer").
+	controllers map[string]struct{}
+	rootless    bool
+}
+
 // NewManager creates a manager for cgroup v2 unified hierarchy.
 // dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope".
 // If dirPath is empty, it is automatically set using config.
 func NewManager(config *configs.Cgroup, dirPath string, rootless bool) (cgroups.Manager, error) {
 	if config == nil {
 		config = &configs.Cgroup{}
 	}
-	if dirPath != "" {
-		if filepath.Clean(dirPath) != dirPath || !filepath.IsAbs(dirPath) {
-			return nil, errors.Errorf("invalid dir path %q", dirPath)
-		}
-	} else {
+	if dirPath == "" {
 		var err error
 		dirPath, err = defaultDirPath(config)
 		if err != nil {
 			return nil, err
 		}
 	}
-	controllers, err := detectControllers(dirPath)
-	if err != nil && !rootless {
-		return nil, err
-	}
 
 	m := &manager{
-		config:      config,
-		dirPath:     dirPath,
-		controllers: controllers,
-		rootless:    rootless,
+		config:   config,
+		dirPath:  dirPath,
+		rootless: rootless,
 	}
 	return m, nil
 }
 
-func detectControllers(dirPath string) (map[string]struct{}, error) {
-	if err := os.MkdirAll(dirPath, 0755); err != nil {
-		return nil, err
-	}
-	controllersPath, err := securejoin.SecureJoin(dirPath, "cgroup.controllers")
-	if err != nil {
-		return nil, err
+func (m *manager) getControllers() error {
+	if m.controllers != nil {
+		return nil
 	}
-	controllersData, err := ioutil.ReadFile(controllersPath)
-	if err != nil {
-		return nil, err
+
+	file := filepath.Join(m.dirPath, "cgroup.controllers")
+	data, err := ioutil.ReadFile(file)
+	if err != nil && !m.rootless {
+		return err
 	}
-	controllersFields := strings.Fields(string(controllersData))
-	controllers := make(map[string]struct{}, len(controllersFields))
-	for _, c := range controllersFields {
-		controllers[c] = struct{}{}
+	fields := strings.Fields(string(data))
+	m.controllers = make(map[string]struct{}, len(fields))
+	for _, c := range fields {
+		m.controllers[c] = struct{}{}
 	}
-	return controllers, nil
-}
 
-type manager struct {
-	config *configs.Cgroup
-	// dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope"
-	dirPath string
-	// controllers is content of "cgroup.controllers" file.
-	// excludes pseudo-controllers ("devices" and "freezer").
-	controllers map[string]struct{}
-	rootless    bool
+	return nil
 }
 
 func (m *manager) Apply(pid int) error {
+	if err := CreateCgroupPath(m.dirPath, m.config); err != nil {
+		return err
+	}
 	if err := cgroups.WriteCgroupProc(m.dirPath, pid); err != nil && !m.rootless {
 		return err
 	}
@@ -97,6 +89,9 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
 	)
 
 	st := cgroups.NewStats()
+	if err := m.getControllers(); err != nil {
+		return st, err
+	}
 
 	// pids (since kernel 4.5)
 	if _, ok := m.controllers["pids"]; ok {
@@ -147,11 +142,15 @@ func (m *manager) Freeze(state configs.FreezerState) error {
 }
 
 func (m *manager) Destroy() error {
-	return os.RemoveAll(m.dirPath)
+	if err := os.Remove(m.dirPath); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	return nil
 }
 
 // GetPaths is for compatibility purpose and should be removed in future
 func (m *manager) GetPaths() map[string]string {
+	_ = m.getControllers()
 	paths := map[string]string{
 		// pseudo-controller for compatibility
 		"devices": m.dirPath,
@@ -171,6 +170,9 @@ func (m *manager) Set(container *configs.Config) error {
 	if container == nil || container.Cgroups == nil {
 		return nil
 	}
+	if err := m.getControllers(); err != nil {
+		return err
+	}
 	var errs []error
 	// pids (since kernel 4.5)
 	if _, ok := m.controllers["pids"]; ok {

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