From 04389ae99b8cd8c94133030a5c540e26e2ca9149 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 6 Oct 2022 14:22:57 +0200 Subject: [PATCH] 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. Signed-off-by: Sebastiaan van Stijn --- libcontainer/cgroups/fs/fs.go | 32 ++++++++--------- libcontainer/cgroups/fs2/fs2.go | 36 +++++++++---------- libcontainer/cgroups/systemd/devices.go | 2 +- libcontainer/cgroups/systemd/freeze_test.go | 5 ++- libcontainer/cgroups/systemd/v1.go | 38 ++++++++++----------- libcontainer/cgroups/systemd/v2.go | 36 +++++++++---------- 6 files changed, 74 insertions(+), 75 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index be4dcc341..43c547838 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 d5208d778..7d366d0f3 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 edd1f1585..d8c572b4d 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/freeze_test.go b/libcontainer/cgroups/systemd/freeze_test.go index 760578654..ed71603ca 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 diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 1e4c58212..ab9333cb2 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 de0cb974d..48e9750cb 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() }