Repository: opencontainers/runc
Pull request #3626 (2022-10-06 12:37:17)

libcontainer/cgroups: return concrete types
It's more idiomatic Go to define interfaces on the receiver, and constructors to return concrete types.

This patch changes various constructors to return a concrete type, with the exceptions of NewWithPaths, which needs the abstraction as it switches between implementations.


thaJeztah: Not sure why centos-7 is failing (likely unrelated); still not figured out how to restart those builds; where do I do that?

Patch:
diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go
index be4dcc34..43c54783 100644
--- a/libcontainer/cgroups/fs/fs.go
+++ b/libcontainer/cgroups/fs/fs.go
@@ -53,13 +53,13 @@ type subsystem interface {
 	Set(path string, r *configs.Resources) error
 }
 
-type manager struct {
+type Manager struct {
 	mu      sync.Mutex
 	cgroups *configs.Cgroup
 	paths   map[string]string
 }
 
-func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
+func NewManager(cg *configs.Cgroup, paths map[string]string) (*Manager, error) {
 	// Some v1 controllers (cpu, cpuset, and devices) expect
 	// cgroups.Resources to not be nil in Apply.
 	if cg.Resources == nil {
@@ -77,7 +77,7 @@ func NewManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, e
 		}
 	}
 
-	return &manager{
+	return &Manager{
 		cgroups: cg,
 		paths:   paths,
 	}, nil
@@ -104,7 +104,7 @@ func isIgnorableError(rootless bool, err error) bool {
 	return false
 }
 
-func (m *manager) Apply(pid int) (err error) {
+func (m *Manager) Apply(pid int) (err error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -138,19 +138,19 @@ func (m *manager) Apply(pid int) (err error) {
 	return nil
 }
 
-func (m *manager) Destroy() error {
+func (m *Manager) Destroy() error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return cgroups.RemovePaths(m.paths)
 }
 
-func (m *manager) Path(subsys string) string {
+func (m *Manager) Path(subsys string) string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return m.paths[subsys]
 }
 
-func (m *manager) GetStats() (*cgroups.Stats, error) {
+func (m *Manager) GetStats() (*cgroups.Stats, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	stats := cgroups.NewStats()
@@ -166,7 +166,7 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
 	return stats, nil
 }
 
-func (m *manager) Set(r *configs.Resources) error {
+func (m *Manager) Set(r *configs.Resources) error {
 	if r == nil {
 		return nil
 	}
@@ -201,7 +201,7 @@ func (m *manager) Set(r *configs.Resources) error {
 
 // Freeze toggles the container's freezer cgroup depending on the state
 // provided
-func (m *manager) Freeze(state configs.FreezerState) error {
+func (m *Manager) Freeze(state configs.FreezerState) error {
 	path := m.Path("freezer")
 	if path == "" {
 		return errors.New("cannot toggle freezer: cgroups not configured for container")
@@ -217,25 +217,25 @@ func (m *manager) Freeze(state configs.FreezerState) error {
 	return nil
 }
 
-func (m *manager) GetPids() ([]int, error) {
+func (m *Manager) GetPids() ([]int, error) {
 	return cgroups.GetPids(m.Path("devices"))
 }
 
-func (m *manager) GetAllPids() ([]int, error) {
+func (m *Manager) GetAllPids() ([]int, error) {
 	return cgroups.GetAllPids(m.Path("devices"))
 }
 
-func (m *manager) GetPaths() map[string]string {
+func (m *Manager) GetPaths() map[string]string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return m.paths
 }
 
-func (m *manager) GetCgroups() (*configs.Cgroup, error) {
+func (m *Manager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
 
-func (m *manager) GetFreezerState() (configs.FreezerState, error) {
+func (m *Manager) GetFreezerState() (configs.FreezerState, error) {
 	dir := m.Path("freezer")
 	// If the container doesn't have the freezer cgroup, say it's undefined.
 	if dir == "" {
@@ -245,7 +245,7 @@ func (m *manager) GetFreezerState() (configs.FreezerState, error) {
 	return freezer.GetState(dir)
 }
 
-func (m *manager) Exists() bool {
+func (m *Manager) Exists() bool {
 	return cgroups.PathExists(m.Path("devices"))
 }
 
@@ -253,7 +253,7 @@ func OOMKillCount(path string) (uint64, error) {
 	return fscommon.GetValueByKey(path, "memory.oom_control", "oom_kill")
 }
 
-func (m *manager) OOMKillCount() (uint64, error) {
+func (m *Manager) OOMKillCount() (uint64, error) {
 	c, err := OOMKillCount(m.Path("memory"))
 	// Ignore ENOENT when rootless as it couldn't create cgroup.
 	if err != nil && m.cgroups.Rootless && os.IsNotExist(err) {
diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go
index d5208d77..7d366d0f 100644
--- a/libcontainer/cgroups/fs2/fs2.go
+++ b/libcontainer/cgroups/fs2/fs2.go
@@ -13,7 +13,7 @@ import (
 
 type parseError = fscommon.ParseError
 
-type manager struct {
+type Manager struct {
 	config *configs.Cgroup
 	// dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope"
 	dirPath string
@@ -25,7 +25,7 @@ type manager struct {
 // 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) (cgroups.Manager, error) {
+func NewManager(config *configs.Cgroup, dirPath string) (*Manager, error) {
 	if dirPath == "" {
 		var err error
 		dirPath, err = defaultDirPath(config)
@@ -34,14 +34,14 @@ func NewManager(config *configs.Cgroup, dirPath string) (cgroups.Manager, error)
 		}
 	}
 
-	m := &manager{
+	m := &Manager{
 		config:  config,
 		dirPath: dirPath,
 	}
 	return m, nil
 }
 
-func (m *manager) getControllers() error {
+func (m *Manager) getControllers() error {
 	if m.controllers != nil {
 		return nil
 	}
@@ -62,7 +62,7 @@ func (m *manager) getControllers() error {
 	return nil
 }
 
-func (m *manager) Apply(pid int) error {
+func (m *Manager) Apply(pid int) error {
 	if err := CreateCgroupPath(m.dirPath, m.config); err != nil {
 		// Related tests:
 		// - "runc create (no limits + no cgrouppath + no permission) succeeds"
@@ -84,15 +84,15 @@ func (m *manager) Apply(pid int) error {
 	return nil
 }
 
-func (m *manager) GetPids() ([]int, error) {
+func (m *Manager) GetPids() ([]int, error) {
 	return cgroups.GetPids(m.dirPath)
 }
 
-func (m *manager) GetAllPids() ([]int, error) {
+func (m *Manager) GetAllPids() ([]int, error) {
 	return cgroups.GetAllPids(m.dirPath)
 }
 
-func (m *manager) GetStats() (*cgroups.Stats, error) {
+func (m *Manager) GetStats() (*cgroups.Stats, error) {
 	var errs []error
 
 	st := cgroups.NewStats()
@@ -128,7 +128,7 @@ func (m *manager) GetStats() (*cgroups.Stats, error) {
 	return st, nil
 }
 
-func (m *manager) Freeze(state configs.FreezerState) error {
+func (m *Manager) Freeze(state configs.FreezerState) error {
 	if m.config.Resources == nil {
 		return errors.New("cannot toggle freezer: cgroups not configured for container")
 	}
@@ -139,15 +139,15 @@ func (m *manager) Freeze(state configs.FreezerState) error {
 	return nil
 }
 
-func (m *manager) Destroy() error {
+func (m *Manager) Destroy() error {
 	return cgroups.RemovePath(m.dirPath)
 }
 
-func (m *manager) Path(_ string) string {
+func (m *Manager) Path(_ string) string {
 	return m.dirPath
 }
 
-func (m *manager) Set(r *configs.Resources) error {
+func (m *Manager) Set(r *configs.Resources) error {
 	if r == nil {
 		return nil
 	}
@@ -213,7 +213,7 @@ func setDevices(dirPath string, r *configs.Resources) error {
 	return cgroups.DevicesSetV2(dirPath, r)
 }
 
-func (m *manager) setUnified(res map[string]string) error {
+func (m *Manager) setUnified(res map[string]string) error {
 	for k, v := range res {
 		if strings.Contains(k, "/") {
 			return fmt.Errorf("unified resource %q must be a file name (no slashes)", k)
@@ -239,21 +239,21 @@ func (m *manager) setUnified(res map[string]string) error {
 	return nil
 }
 
-func (m *manager) GetPaths() map[string]string {
+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) {
+func (m *Manager) GetCgroups() (*configs.Cgroup, error) {
 	return m.config, nil
 }
 
-func (m *manager) GetFreezerState() (configs.FreezerState, error) {
+func (m *Manager) GetFreezerState() (configs.FreezerState, error) {
 	return getFreezer(m.dirPath)
 }
 
-func (m *manager) Exists() bool {
+func (m *Manager) Exists() bool {
 	return cgroups.PathExists(m.dirPath)
 }
 
@@ -261,7 +261,7 @@ func OOMKillCount(path string) (uint64, error) {
 	return fscommon.GetValueByKey(path, "memory.events", "oom_kill")
 }
 
-func (m *manager) OOMKillCount() (uint64, error) {
+func (m *Manager) OOMKillCount() (uint64, error) {
 	c, err := OOMKillCount(m.dirPath)
 	if err != nil && m.config.Rootless && os.IsNotExist(err) {
 		err = nil
diff --git a/libcontainer/cgroups/systemd/devices.go b/libcontainer/cgroups/systemd/devices.go
index edd1f158..d8c572b4 100644
--- a/libcontainer/cgroups/systemd/devices.go
+++ b/libcontainer/cgroups/systemd/devices.go
@@ -17,7 +17,7 @@ import (
 // (unlike our fs driver, they will happily write deny-all rules to running
 // containers). So we have to freeze the container to avoid the container get
 // an occasional "permission denied" error.
-func (m *legacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (needsFreeze, needsThaw bool, err error) {
+func (m *LegacyManager) freezeBeforeSet(unitName string, r *configs.Resources) (needsFreeze, needsThaw bool, err error) {
 	// Special case for SkipDevices, as used by Kubernetes to create pod
 	// cgroups with allow-all device policy).
 	if r.SkipDevices {
diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
index 1e4c5821..ab9333cb 100644
--- a/libcontainer/cgroups/systemd/v1.go
+++ b/libcontainer/cgroups/systemd/v1.go
@@ -15,14 +15,14 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-type legacyManager struct {
+type LegacyManager struct {
 	mu      sync.Mutex
 	cgroups *configs.Cgroup
 	paths   map[string]string
 	dbus    *dbusConnManager
 }
 
-func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Manager, error) {
+func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) (*LegacyManager, error) {
 	if cg.Rootless {
 		return nil, errors.New("cannot use rootless systemd cgroups manager on cgroup v1")
 	}
@@ -36,7 +36,7 @@ func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Mana
 			return nil, err
 		}
 	}
-	return &legacyManager{
+	return &LegacyManager{
 		cgroups: cg,
 		paths:   paths,
 		dbus:    newDbusConnManager(false),
@@ -46,7 +46,7 @@ func NewLegacyManager(cg *configs.Cgroup, paths map[string]string) (cgroups.Mana
 type subsystem interface {
 	// Name returns the name of the subsystem.
 	Name() string
-	// Returns the stats, as 'stats', corresponding to the cgroup under 'path'.
+	// GetStats returns the stats, as 'stats', corresponding to the cgroup under 'path'.
 	GetStats(path string, stats *cgroups.Stats) error
 	// Set sets cgroup resource limits.
 	Set(path string, r *configs.Resources) error
@@ -157,7 +157,7 @@ func initPaths(c *configs.Cgroup) (map[string]string, error) {
 	return paths, nil
 }
 
-func (m *legacyManager) Apply(pid int) error {
+func (m *LegacyManager) Apply(pid int) error {
 	var (
 		c          = m.cgroups
 		unitName   = getUnitName(c)
@@ -215,7 +215,7 @@ func (m *legacyManager) Apply(pid int) error {
 	return nil
 }
 
-func (m *legacyManager) Destroy() error {
+func (m *LegacyManager) Destroy() error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -231,13 +231,13 @@ func (m *legacyManager) Destroy() error {
 	return stopErr
 }
 
-func (m *legacyManager) Path(subsys string) string {
+func (m *LegacyManager) Path(subsys string) string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return m.paths[subsys]
 }
 
-func (m *legacyManager) joinCgroups(pid int) error {
+func (m *LegacyManager) joinCgroups(pid int) error {
 	for _, sys := range legacySubsystems {
 		name := sys.Name()
 		switch name {
@@ -281,7 +281,7 @@ func getSubsystemPath(slice, unit, subsystem string) (string, error) {
 	return filepath.Join(mountpoint, initPath, slice, unit), nil
 }
 
-func (m *legacyManager) Freeze(state configs.FreezerState) error {
+func (m *LegacyManager) Freeze(state configs.FreezerState) error {
 	err := m.doFreeze(state)
 	if err == nil {
 		m.cgroups.Resources.Freezer = state
@@ -291,7 +291,7 @@ func (m *legacyManager) Freeze(state configs.FreezerState) error {
 
 // doFreeze is the same as Freeze but without
 // changing the m.cgroups.Resources.Frozen field.
-func (m *legacyManager) doFreeze(state configs.FreezerState) error {
+func (m *LegacyManager) doFreeze(state configs.FreezerState) error {
 	path, ok := m.paths["freezer"]
 	if !ok {
 		return errSubsystemDoesNotExist
@@ -301,7 +301,7 @@ func (m *legacyManager) doFreeze(state configs.FreezerState) error {
 	return freezer.Set(path, resources)
 }
 
-func (m *legacyManager) GetPids() ([]int, error) {
+func (m *LegacyManager) GetPids() ([]int, error) {
 	path, ok := m.paths["devices"]
 	if !ok {
 		return nil, errSubsystemDoesNotExist
@@ -309,7 +309,7 @@ func (m *legacyManager) GetPids() ([]int, error) {
 	return cgroups.GetPids(path)
 }
 
-func (m *legacyManager) GetAllPids() ([]int, error) {
+func (m *LegacyManager) GetAllPids() ([]int, error) {
 	path, ok := m.paths["devices"]
 	if !ok {
 		return nil, errSubsystemDoesNotExist
@@ -317,7 +317,7 @@ func (m *legacyManager) GetAllPids() ([]int, error) {
 	return cgroups.GetAllPids(path)
 }
 
-func (m *legacyManager) GetStats() (*cgroups.Stats, error) {
+func (m *LegacyManager) GetStats() (*cgroups.Stats, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	stats := cgroups.NewStats()
@@ -334,7 +334,7 @@ func (m *legacyManager) GetStats() (*cgroups.Stats, error) {
 	return stats, nil
 }
 
-func (m *legacyManager) Set(r *configs.Resources) error {
+func (m *LegacyManager) Set(r *configs.Resources) error {
 	if r == nil {
 		return nil
 	}
@@ -382,17 +382,17 @@ func (m *legacyManager) Set(r *configs.Resources) error {
 	return nil
 }
 
-func (m *legacyManager) GetPaths() map[string]string {
+func (m *LegacyManager) GetPaths() map[string]string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return m.paths
 }
 
-func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) {
+func (m *LegacyManager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
 
-func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) {
+func (m *LegacyManager) GetFreezerState() (configs.FreezerState, error) {
 	path, ok := m.paths["freezer"]
 	if !ok {
 		return configs.Undefined, nil
@@ -401,10 +401,10 @@ func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) {
 	return freezer.GetState(path)
 }
 
-func (m *legacyManager) Exists() bool {
+func (m *LegacyManager) Exists() bool {
 	return cgroups.PathExists(m.Path("devices"))
 }
 
-func (m *legacyManager) OOMKillCount() (uint64, error) {
+func (m *LegacyManager) OOMKillCount() (uint64, error) {
 	return fs.OOMKillCount(m.Path("memory"))
 }
diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go
index de0cb974..48e9750c 100644
--- a/libcontainer/cgroups/systemd/v2.go
+++ b/libcontainer/cgroups/systemd/v2.go
@@ -20,7 +20,7 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-type unifiedManager struct {
+type UnifiedManager struct {
 	mu      sync.Mutex
 	cgroups *configs.Cgroup
 	// path is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope"
@@ -29,8 +29,8 @@ type unifiedManager struct {
 	fsMgr cgroups.Manager
 }
 
-func NewUnifiedManager(config *configs.Cgroup, path string) (cgroups.Manager, error) {
-	m := &unifiedManager{
+func NewUnifiedManager(config *configs.Cgroup, path string) (*UnifiedManager, error) {
+	m := &UnifiedManager{
 		cgroups: config,
 		path:    path,
 		dbus:    newDbusConnManager(config.Rootless),
@@ -237,7 +237,7 @@ func genV2ResourcesProperties(r *configs.Resources, cm *dbusConnManager) ([]syst
 	return properties, nil
 }
 
-func (m *unifiedManager) Apply(pid int) error {
+func (m *UnifiedManager) Apply(pid int) error {
 	var (
 		c          = m.cgroups
 		unitName   = getUnitName(c)
@@ -340,7 +340,7 @@ func cgroupFilesToChown() ([]string, error) {
 	return filesToChown, nil
 }
 
-func (m *unifiedManager) Destroy() error {
+func (m *UnifiedManager) Destroy() error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -359,13 +359,13 @@ func (m *unifiedManager) Destroy() error {
 	return nil
 }
 
-func (m *unifiedManager) Path(_ string) string {
+func (m *UnifiedManager) Path(_ string) string {
 	return m.path
 }
 
 // getSliceFull value is used in initPath.
 // The value is incompatible with systemdDbus.PropSlice.
-func (m *unifiedManager) getSliceFull() (string, error) {
+func (m *UnifiedManager) getSliceFull() (string, error) {
 	c := m.cgroups
 	slice := "system.slice"
 	if c.Rootless {
@@ -393,7 +393,7 @@ func (m *unifiedManager) getSliceFull() (string, error) {
 	return slice, nil
 }
 
-func (m *unifiedManager) initPath() error {
+func (m *UnifiedManager) initPath() error {
 	if m.path != "" {
 		return nil
 	}
@@ -417,23 +417,23 @@ func (m *unifiedManager) initPath() error {
 	return nil
 }
 
-func (m *unifiedManager) Freeze(state configs.FreezerState) error {
+func (m *UnifiedManager) Freeze(state configs.FreezerState) error {
 	return m.fsMgr.Freeze(state)
 }
 
-func (m *unifiedManager) GetPids() ([]int, error) {
+func (m *UnifiedManager) GetPids() ([]int, error) {
 	return cgroups.GetPids(m.path)
 }
 
-func (m *unifiedManager) GetAllPids() ([]int, error) {
+func (m *UnifiedManager) GetAllPids() ([]int, error) {
 	return cgroups.GetAllPids(m.path)
 }
 
-func (m *unifiedManager) GetStats() (*cgroups.Stats, error) {
+func (m *UnifiedManager) GetStats() (*cgroups.Stats, error) {
 	return m.fsMgr.GetStats()
 }
 
-func (m *unifiedManager) Set(r *configs.Resources) error {
+func (m *UnifiedManager) Set(r *configs.Resources) error {
 	if r == nil {
 		return nil
 	}
@@ -449,24 +449,24 @@ func (m *unifiedManager) Set(r *configs.Resources) error {
 	return m.fsMgr.Set(r)
 }
 
-func (m *unifiedManager) GetPaths() map[string]string {
+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) {
+func (m *UnifiedManager) GetCgroups() (*configs.Cgroup, error) {
 	return m.cgroups, nil
 }
 
-func (m *unifiedManager) GetFreezerState() (configs.FreezerState, error) {
+func (m *UnifiedManager) GetFreezerState() (configs.FreezerState, error) {
 	return m.fsMgr.GetFreezerState()
 }
 
-func (m *unifiedManager) Exists() bool {
+func (m *UnifiedManager) Exists() bool {
 	return cgroups.PathExists(m.path)
 }
 
-func (m *unifiedManager) OOMKillCount() (uint64, error) {
+func (m *UnifiedManager) OOMKillCount() (uint64, error) {
 	return m.fsMgr.OOMKillCount()
 }


Test patch:
diff --git a/libcontainer/cgroups/systemd/freeze_test.go b/libcontainer/cgroups/systemd/freeze_test.go
index 76057865..ed71603c 100644
--- a/libcontainer/cgroups/systemd/freeze_test.go
+++ b/libcontainer/cgroups/systemd/freeze_test.go
@@ -139,10 +139,9 @@ func TestFreezeBeforeSet(t *testing.T) {
 				t.Fatal(err)
 			}
 			defer m.Destroy() //nolint:errcheck
-			lm := m.(*legacyManager)
 
 			// Checks for a non-existent unit.
-			freeze, thaw, err := lm.freezeBeforeSet(getUnitName(tc.cg), tc.cg.Resources)
+			freeze, thaw, err := m.freezeBeforeSet(getUnitName(tc.cg), tc.cg.Resources)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -176,7 +175,7 @@ func TestFreezeBeforeSet(t *testing.T) {
 					return // no more checks
 				}
 			}
-			freeze, thaw, err = lm.freezeBeforeSet(getUnitName(tc.cg), tc.cg.Resources)
+			freeze, thaw, err = m.freezeBeforeSet(getUnitName(tc.cg), tc.cg.Resources)
 			if err != nil {
 				t.Error(err)
 				return // no more checks