Repository: opencontainers/runc
Pull request #2386 (2020-05-07 01:35:33)

Simplify cgroup paths handling in v2 via unified v1/v2 API
This unties the Gordian Knot of using GetPaths in cgroupv2 code.

The problem is, the current code uses GetPaths for three kinds of things:

1. Get all the paths to cgroup v1 controllers to save its state (see
   (*linuxContainer).currentState(), (*LinuxFactory).loadState()
   methods).

2. Get all the paths to cgroup v1 controllers to have the setns process
    enter the proper cgroups in `(*setnsProcess).start()`.

3. Get the path to a specific controller (for example,
   `m.GetPaths()["devices"]`).

Now, for cgroup v2 instead of a set of per-controller paths, we have only
one single unified path, and a dedicated function `GetUnifiedPath()` to get it.

This discrepancy between v1 and v2 cgroupManager API leads to the
following problems with the code:

 - multiple if/else code blocks that have to treat v1 and v2 separately;

 - backward-compatible GetPaths() methods in v2 controllers;

 - repeated writing of the PID into the same cgroup for v2;

Overall, it's hard to write the right code with all this, and the code
that is written is kinda hard to follow.

The solution is to slightly change the API to do the 3 things outlined
above in the same manner for v1 and v2:

1. Use `GetPaths()` for state saving and setns process cgroups entering.

2. Introduce and use `Path(subsys string)` to obtain a path to a
   subsystem. For v2, the argument is ignored and the unified path is
   returned.

This commit converts all the controllers to the new API, and modifies
all the users to use it.


kolyshkin: The only ugliness that is left is
1. function `func getUnifiedPath()` in `libcontainer/factory_linux.go`. It can be simplified but we'll lose backward compatibility (not sure if we need it so I'm not touching it).
2. a requirement to pass a string parameter to `Path()` for v2. I think it's an OK price to pay for the API which works for v1 and v2, otherwise we'd need an if/else and it will be uglier.
3. Public members of fs.Manager and systemd.LegacyManager. Will address it later.
kolyshkin: @AkihiroSuda PTAL
AkihiroSuda: LGTM

kolyshkin: I think I'll split it into two PRs for the sake of easier review.

Part I: https://github.com/opencontainers/runc/pull/2387 (relatively easy to review)
Part II is this one.

Patch:
diff --git a/libcontainer/cgroups/cgroups.go b/libcontainer/cgroups/cgroups.go
index c0a96592..cbc430bb 100644
--- a/libcontainer/cgroups/cgroups.go
+++ b/libcontainer/cgroups/cgroups.go
@@ -27,27 +27,22 @@ type Manager interface {
 	// Destroys the cgroup set
 	Destroy() error
 
-	// The option func SystemdCgroups() and Cgroupfs() require following attributes:
-	// 	Paths   map[string]string
-	// 	Cgroups *configs.Cgroup
-	// Paths maps cgroup subsystem to path at which it is mounted.
-	// Cgroups specifies specific cgroup settings for the various subsystems
-
-	// Returns cgroup paths to save in a state file and to be able to
-	// restore the object later.
-	GetPaths() map[string]string
-
-	// GetUnifiedPath returns the unified path when running in unified mode.
-	// The value corresponds to the all values of GetPaths() map.
-	//
-	// GetUnifiedPath returns error when running in hybrid mode as well as
-	// in legacy mode.
-	GetUnifiedPath() (string, error)
+	// Path returns a cgroup path to the specified controller/subsystem.
+	// For cgroupv2, the argument is unused and can be empty.
+	Path(string) string
 
 	// Sets the cgroup as configured.
 	Set(container *configs.Config) error
 
-	// Gets the cgroup as configured.
+	// GetPaths returns cgroup path(s) to save in a state file in order to restore later.
+	//
+	// For cgroup v1, a key is cgroup subsystem name, and the value is the path
+	// to the cgroup for this subsystem.
+	//
+	// For cgroup v2 unified hierarchy, a key is "", and the value is the unified path.
+	GetPaths() map[string]string
+
+	// GetCgroups returns the cgroup data as configured.
 	GetCgroups() (*configs.Cgroup, error)
 }
 
diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go
index 760d4e6e..c3060145 100644
--- a/libcontainer/cgroups/fs/apply_raw.go
+++ b/libcontainer/cgroups/fs/apply_raw.go
@@ -209,15 +209,10 @@ func (m *manager) Destroy() error {
 	return nil
 }
 
-func (m *manager) GetPaths() map[string]string {
+func (m *manager) Path(subsys string) string {
 	m.mu.Lock()
-	paths := m.paths
-	m.mu.Unlock()
-	return paths
-}
-
-func (m *manager) GetUnifiedPath() (string, error) {
-	return "", errors.New("unified path is only supported when running in unified mode")
+	defer m.mu.Unlock()
+	return m.paths[subsys]
 }
 
 func (m *manager) GetStats() (*cgroups.Stats, error) {
@@ -299,13 +294,11 @@ func (m *manager) Freeze(state configs.FreezerState) error {
 }
 
 func (m *manager) GetPids() ([]int, error) {
-	paths := m.GetPaths()
-	return cgroups.GetPids(paths["devices"])
+	return cgroups.GetPids(m.Path("devices"))
 }
 
 func (m *manager) GetAllPids() ([]int, error) {
-	paths := m.GetPaths()
-	return cgroups.GetAllPids(paths["devices"])
+	return cgroups.GetAllPids(m.Path("devices"))
 }
 
 func getCgroupData(c *configs.Cgroup, pid int) (*cgroupData, error) {
@@ -411,6 +404,12 @@ func CheckCpushares(path string, c uint64) error {
 	return nil
 }
 
+func (m *manager) GetPaths() map[string]string {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	return m.paths
+}
+
 func (m *manager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go
index fc469e10..a0881dac 100644
--- a/libcontainer/cgroups/fs2/fs2.go
+++ b/libcontainer/cgroups/fs2/fs2.go
@@ -164,22 +164,8 @@ func (m *manager) Destroy() error {
 	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,
-		"freezer": m.dirPath,
-	}
-	for c := range m.controllers {
-		paths[c] = m.dirPath
-	}
-	return paths
-}
-
-func (m *manager) GetUnifiedPath() (string, error) {
-	return m.dirPath, nil
+func (m *manager) Path(_ string) string {
+	return m.dirPath
 }
 
 func (m *manager) Set(container *configs.Config) error {
@@ -245,6 +231,12 @@ func (m *manager) Set(container *configs.Config) error {
 	return nil
 }
 
+func (m *manager) GetPaths() map[string]string {
+	paths := make(map[string]string, 1)
+	paths[""] = m.dirPath
+	return paths
+}
+
 func (m *manager) GetCgroups() (*configs.Cgroup, error) {
 	return m.config, nil
 }
diff --git a/libcontainer/cgroups/systemd/unsupported.go b/libcontainer/cgroups/systemd/unsupported.go
index 5318a1e9..37fa5035 100644
--- a/libcontainer/cgroups/systemd/unsupported.go
+++ b/libcontainer/cgroups/systemd/unsupported.go
@@ -42,8 +42,8 @@ func (m *Manager) GetPaths() map[string]string {
 	return nil
 }
 
-func (m *Manager) GetUnifiedPath() (string, error) {
-	return "", fmt.Errorf("Systemd not supported")
+func (m *Manager) Path(_ string) string {
+	return ""
 }
 
 func (m *Manager) GetStats() (*cgroups.Stats, error) {
diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
index 26858517..70440ea6 100644
--- a/libcontainer/cgroups/systemd/v1.go
+++ b/libcontainer/cgroups/systemd/v1.go
@@ -236,15 +236,10 @@ func (m *legacyManager) Destroy() error {
 	return nil
 }
 
-func (m *legacyManager) GetPaths() map[string]string {
+func (m *legacyManager) Path(subsys string) string {
 	m.mu.Lock()
-	paths := m.paths
-	m.mu.Unlock()
-	return paths
-}
-
-func (m *legacyManager) GetUnifiedPath() (string, error) {
-	return "", errors.New("unified path is only supported when running in unified mode")
+	defer m.mu.Unlock()
+	return m.paths[subsys]
 }
 
 func join(c *configs.Cgroup, subsystem string, pid int) (string, error) {
@@ -434,6 +429,13 @@ func setKernelMemory(c *configs.Cgroup) error {
 	}
 	return fs.EnableKernelMemoryAccounting(path)
 }
+
+func (m *legacyManager) GetPaths() map[string]string {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	return m.paths
+}
+
 func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go
index 4bce0465..d43c855b 100644
--- a/libcontainer/cgroups/systemd/v2.go
+++ b/libcontainer/cgroups/systemd/v2.go
@@ -153,8 +153,7 @@ func (m *unifiedManager) Apply(pid int) error {
 		return errors.Wrapf(err, "error while starting unit %q with properties %+v", unitName, properties)
 	}
 
-	_, err = m.GetUnifiedPath()
-	if err != nil {
+	if err = m.initPath(); err != nil {
 		return err
 	}
 	if err := fs2.CreateCgroupPath(m.path, m.cgroups); err != nil {
@@ -188,22 +187,11 @@ func (m *unifiedManager) Destroy() error {
 	return nil
 }
 
-// this method is for v1 backward compatibility and will be removed
-func (m *unifiedManager) GetPaths() map[string]string {
-	_, _ = m.GetUnifiedPath()
-	paths := map[string]string{
-		"pids":    m.path,
-		"memory":  m.path,
-		"io":      m.path,
-		"cpu":     m.path,
-		"devices": m.path,
-		"cpuset":  m.path,
-		"freezer": m.path,
-	}
-	return paths
+func (m *unifiedManager) Path(_ string) string {
+	return m.path
 }
 
-// getSliceFull value is used in GetUnifiedPath.
+// getSliceFull value is used in initPath.
 // The value is incompatible with systemdDbus.PropSlice.
 func (m *unifiedManager) getSliceFull() (string, error) {
 	c := m.cgroups
@@ -241,37 +229,35 @@ func (m *unifiedManager) getSliceFull() (string, error) {
 	return slice, nil
 }
 
-func (m *unifiedManager) GetUnifiedPath() (string, error) {
-	m.mu.Lock()
-	defer m.mu.Unlock()
+func (m *unifiedManager) initPath() error {
 	if m.path != "" {
-		return m.path, nil
+		return nil
 	}
 
 	sliceFull, err := m.getSliceFull()
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	c := m.cgroups
 	path := filepath.Join(sliceFull, getUnitName(c))
 	path, err = securejoin.SecureJoin(fs2.UnifiedMountpoint, path)
 	if err != nil {
-		return "", err
+		return err
 	}
-	m.path = path
 
 	// an example of the final path in rootless:
 	// "/sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/user.slice/libpod-132ff0d72245e6f13a3bbc6cdc5376886897b60ac59eaa8dea1df7ab959cbf1c.scope"
-	return m.path, nil
+	m.path = path
+
+	return nil
 }
 
 func (m *unifiedManager) fsManager() (cgroups.Manager, error) {
-	path, err := m.GetUnifiedPath()
-	if err != nil {
+	if err := m.initPath(); err != nil {
 		return nil, err
 	}
-	return fs2.NewManager(m.cgroups, path, m.rootless)
+	return fs2.NewManager(m.cgroups, m.path, m.rootless)
 }
 
 func (m *unifiedManager) Freeze(state configs.FreezerState) error {
@@ -283,19 +269,17 @@ func (m *unifiedManager) Freeze(state configs.FreezerState) error {
 }
 
 func (m *unifiedManager) GetPids() ([]int, error) {
-	path, err := m.GetUnifiedPath()
-	if err != nil {
+	if err := m.initPath(); err != nil {
 		return nil, err
 	}
-	return cgroups.GetPids(path)
+	return cgroups.GetPids(m.path)
 }
 
 func (m *unifiedManager) GetAllPids() ([]int, error) {
-	path, err := m.GetUnifiedPath()
-	if err != nil {
+	if err := m.initPath(); err != nil {
 		return nil, err
 	}
-	return cgroups.GetAllPids(path)
+	return cgroups.GetAllPids(m.path)
 }
 
 func (m *unifiedManager) GetStats() (*cgroups.Stats, error) {
@@ -326,6 +310,12 @@ func (m *unifiedManager) Set(container *configs.Config) error {
 	return fsMgr.Set(container)
 }
 
+func (m *unifiedManager) GetPaths() map[string]string {
+	paths := make(map[string]string, 1)
+	paths[""] = m.path
+	return paths
+}
+
 func (m *unifiedManager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index d4cc35d4..5cea3d6c 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -64,8 +64,12 @@ type State struct {
 	// Set to true if BaseState.Config.RootlessEUID && BaseState.Config.RootlessCgroups
 	Rootless bool `json:"rootless"`
 
-	// Path to all the cgroups setup for a container. Key is cgroup subsystem name
-	// with the value as the path.
+	// Paths to all the container's cgroups, as returned by (*cgroups.Manager).GetPaths
+	//
+	// For cgroup v1, a key is cgroup subsystem name, and the value is the path
+	// to the cgroup for this subsystem.
+	//
+	// For cgroup v2 unified hierarchy, a key is "", and the value is the unified path.
 	CgroupPaths map[string]string `json:"cgroup_paths"`
 
 	// NamespacePaths are filepaths to the container's namespaces. Key is the namespace type
@@ -648,14 +652,11 @@ func (c *linuxContainer) NotifyOOM() (<-chan struct{}, error) {
 	if c.config.RootlessCgroups {
 		logrus.Warn("getting OOM notifications may fail if you don't have the full access to cgroups")
 	}
+	path := c.cgroupManager.Path("memory")
 	if cgroups.IsCgroup2UnifiedMode() {
-		path, err := c.cgroupManager.GetUnifiedPath()
-		if err != nil {
-			return nil, err
-		}
 		return notifyOnOOMV2(path)
 	}
-	return notifyOnOOM(c.cgroupManager.GetPaths()["memory"])
+	return notifyOnOOM(path)
 }
 
 func (c *linuxContainer) NotifyMemoryPressure(level PressureLevel) (<-chan struct{}, error) {
@@ -663,7 +664,7 @@ func (c *linuxContainer) NotifyMemoryPressure(level PressureLevel) (<-chan struc
 	if c.config.RootlessCgroups {
 		logrus.Warn("getting memory pressure notifications may fail if you don't have the full access to cgroups")
 	}
-	return notifyMemoryPressure(c.cgroupManager.GetPaths()["memory"], level)
+	return notifyMemoryPressure(c.cgroupManager.Path("memory"), level)
 }
 
 var criuFeatures *criurpc.CriuFeatures
@@ -1023,23 +1024,12 @@ func (c *linuxContainer) Checkpoint(criuOpts *CriuOpts) error {
 
 	// CRIU can use cgroup freezer; when rpcOpts.FreezeCgroup
 	// is not set, CRIU uses ptrace() to pause the processes.
-	var fcg string
-	switch cgroups.IsCgroup2UnifiedMode() {
-	case false:
-		fcg = c.cgroupManager.GetPaths()["freezer"]
-	case true:
-		// cgroup v2 freezer is only supported since CRIU release 3.14
-		if c.checkCriuVersion(31400) == nil {
-			fcg, err = c.cgroupManager.GetUnifiedPath()
-			if err != nil {
-				// should not happen
-				return err
-			}
+	// Note cgroup v2 freezer is only supported since CRIU release 3.14.
+	if !cgroups.IsCgroup2UnifiedMode() || c.checkCriuVersion(31400) == nil {
+		if fcg := c.cgroupManager.Path("freezer"); fcg != "" {
+			rpcOpts.FreezeCgroup = proto.String(fcg)
 		}
 	}
-	if fcg != "" {
-		rpcOpts.FreezeCgroup = proto.String(fcg)
-	}
 
 	// append optional criu opts, e.g., page-server and port
 	if criuOpts.PageServer.Address != "" && criuOpts.PageServer.Port != 0 {
@@ -1857,23 +1847,17 @@ func (c *linuxContainer) runType() Status {
 }
 
 func (c *linuxContainer) isPaused() (bool, error) {
-	var fcg, filename, pausedState string
+	var filename, pausedState string
 
+	fcg := c.cgroupManager.Path("freezer")
 	if !cgroups.IsCgroup2UnifiedMode() {
-		fcg = c.cgroupManager.GetPaths()["freezer"]
 		if fcg == "" {
-			// A container doesn't have a freezer cgroup
+			// container doesn't have a freezer cgroup
 			return false, nil
 		}
 		filename = "freezer.state"
 		pausedState = "FROZEN"
 	} else {
-		var err error
-		fcg, err = c.cgroupManager.GetUnifiedPath()
-		if err != nil {
-			// should not happen
-			return false, err
-		}
 		filename = "cgroup.freeze"
 		pausedState = "1"
 	}


Test patch:
diff --git a/libcontainer/container_linux_test.go b/libcontainer/container_linux_test.go
index 583c1c0c..356e8f1a 100644
--- a/libcontainer/container_linux_test.go
+++ b/libcontainer/container_linux_test.go
@@ -15,11 +15,10 @@ import (
 )
 
 type mockCgroupManager struct {
-	pids        []int
-	allPids     []int
-	stats       *cgroups.Stats
-	paths       map[string]string
-	unifiedPath string
+	pids    []int
+	allPids []int
+	stats   *cgroups.Stats
+	paths   map[string]string
 }
 
 type mockIntelRdtManager struct {
@@ -55,8 +54,8 @@ func (m *mockCgroupManager) GetPaths() map[string]string {
 	return m.paths
 }
 
-func (m *mockCgroupManager) GetUnifiedPath() (string, error) {
-	return m.unifiedPath, nil
+func (m *mockCgroupManager) Path(subsys string) string {
+	return m.paths[subsys]
 }
 
 func (m *mockCgroupManager) Freeze(state configs.FreezerState) error {