From 5ebcfa62816140c2a7a8972b4074db179694275b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 9 Aug 2023 16:14:00 -0700 Subject: [PATCH] [1.1] libct: rm intelrtd.Manager interface, NewIntelRdtManager [This is a manual port of commit dbd990d5559733bd0f33e2 to release-1.1 branch. Original description follows.] Remove intelrtd.Manager interface, since we only have a single implementation, and do not expect another one. Rename intelRdtManager to Manager, and modify its users accordingly. Remove NewIntelRdtManager from factory. Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the feature is not available. Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew. Add internal function newManager to be used for tests (to make sure some testing is done even when the feature is not available in kernel/hardware). Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 2 +- libcontainer/factory_linux.go | 43 ++++++---------------- libcontainer/factory_linux_test.go | 24 +------------ libcontainer/intelrdt/intelrdt.go | 49 +++++++++++--------------- libcontainer/intelrdt/intelrdt_test.go | 10 +++--- libcontainer/process_linux.go | 2 +- utils_linux.go | 4 +-- 7 files changed, 41 insertions(+), 93 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 5f1a494b4a8..2f17e37d43d 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -40,7 +40,7 @@ type linuxContainer struct { root string config *configs.Config cgroupManager cgroups.Manager - intelRdtManager intelrdt.Manager + intelRdtManager *intelrdt.Manager initPath string initArgs []string initProcess parentProcess diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index a1fa7de2d24..546d0eb000c 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -48,20 +48,6 @@ func InitArgs(args ...string) func(*LinuxFactory) error { } } -// IntelRdtfs is an options func to configure a LinuxFactory to return -// containers that use the Intel RDT "resource control" filesystem to -// create and manage Intel RDT resources (e.g., L3 cache, memory bandwidth). -func IntelRdtFs(l *LinuxFactory) error { - if !intelrdt.IsCATEnabled() && !intelrdt.IsMBAEnabled() { - l.NewIntelRdtManager = nil - } else { - l.NewIntelRdtManager = func(config *configs.Config, id string, path string) intelrdt.Manager { - return intelrdt.NewManager(config, id, path) - } - } - return nil -} - // TmpfsRoot is an option func to mount LinuxFactory.Root to tmpfs. func TmpfsRoot(l *LinuxFactory) error { mounted, err := mountinfo.Mounted(l.Root) @@ -136,9 +122,6 @@ type LinuxFactory struct { // Validator provides validation to container configurations. Validator validate.Validator - - // NewIntelRdtManager returns an initialized Intel RDT manager for a single container. - NewIntelRdtManager func(config *configs.Config, id string, path string) intelrdt.Manager } func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) { @@ -208,18 +191,16 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err return nil, err } c := &linuxContainer{ - id: id, - root: containerRoot, - config: config, - initPath: l.InitPath, - initArgs: l.InitArgs, - criuPath: l.CriuPath, - newuidmapPath: l.NewuidmapPath, - newgidmapPath: l.NewgidmapPath, - cgroupManager: cm, - } - if l.NewIntelRdtManager != nil { - c.intelRdtManager = l.NewIntelRdtManager(config, id, "") + id: id, + root: containerRoot, + config: config, + initPath: l.InitPath, + initArgs: l.InitArgs, + criuPath: l.CriuPath, + newuidmapPath: l.NewuidmapPath, + newgidmapPath: l.NewgidmapPath, + cgroupManager: cm, + intelRdtManager: intelrdt.NewManager(config, id, ""), } c.state = &stoppedState{c: c} return c, nil @@ -261,12 +242,10 @@ func (l *LinuxFactory) Load(id string) (Container, error) { newuidmapPath: l.NewuidmapPath, newgidmapPath: l.NewgidmapPath, cgroupManager: cm, + intelRdtManager: intelrdt.NewManager(&state.Config, id, state.IntelRdtPath), root: containerRoot, created: state.Created, } - if l.NewIntelRdtManager != nil { - c.intelRdtManager = l.NewIntelRdtManager(&state.Config, id, state.IntelRdtPath) - } c.state = &loadedState{c: c} if err := c.refreshState(); err != nil { return nil, err diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index d29c32e9dbf..47f3069953b 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -37,28 +37,6 @@ func TestFactoryNew(t *testing.T) { } } -func TestFactoryNewIntelRdt(t *testing.T) { - root := t.TempDir() - factory, err := New(root, IntelRdtFs) - if err != nil { - t.Fatal(err) - } - if factory == nil { - t.Fatal("factory should not be nil") - } - lfactory, ok := factory.(*LinuxFactory) - if !ok { - t.Fatal("expected linux factory returned on linux based systems") - } - if lfactory.Root != root { - t.Fatalf("expected factory root to be %q but received %q", root, lfactory.Root) - } - - if factory.Type() != "libcontainer" { - t.Fatalf("unexpected factory type: %q, expected %q", factory.Type(), "libcontainer") - } -} - func TestFactoryNewTmpfs(t *testing.T) { root := t.TempDir() factory, err := New(root, TmpfsRoot) @@ -157,7 +135,7 @@ func TestFactoryLoadContainer(t *testing.T) { if err := marshal(filepath.Join(root, id, stateFilename), expectedState); err != nil { t.Fatal(err) } - factory, err := New(root, IntelRdtFs) + factory, err := New(root) if err != nil { t.Fatal(err) } diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 1fe1ec3e86c..3953f930d25 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -146,34 +146,27 @@ import ( * } */ -type Manager interface { - // Applies Intel RDT configuration to the process with the specified pid - Apply(pid int) error - - // Returns statistics for Intel RDT - GetStats() (*Stats, error) - - // Destroys the Intel RDT container-specific 'container_id' group - Destroy() error - - // Returns Intel RDT path to save in a state file and to be able to - // restore the object later - GetPath() string - - // Set Intel RDT "resource control" filesystem as configured. - Set(container *configs.Config) error -} - -// This implements interface Manager -type intelRdtManager struct { +type Manager struct { mu sync.Mutex config *configs.Config id string path string } -func NewManager(config *configs.Config, id string, path string) Manager { - return &intelRdtManager{ +// NewManager returns a new instance of Manager, or nil, if the Intel RDT +// functionality is not available from hardware or not enabled in the kernel. +func NewManager(config *configs.Config, id string, path string) *Manager { + if _, err := Root(); err != nil { + // Intel RDT is not available. + return nil + } + return newManager(config, id, path) +} + +// newManager is the same as NewManager, except it does not check if the feature +// is actually available. Used by unit tests that mock intelrdt paths. +func newManager(config *configs.Config, id string, path string) *Manager { + return &Manager{ config: config, id: id, path: path, @@ -507,7 +500,7 @@ func IsMBAScEnabled() bool { } // Get the path of the clos group in "resource control" filesystem that the container belongs to -func (m *intelRdtManager) getIntelRdtPath() (string, error) { +func (m *Manager) getIntelRdtPath() (string, error) { rootPath, err := Root() if err != nil { return "", err @@ -522,7 +515,7 @@ func (m *intelRdtManager) getIntelRdtPath() (string, error) { } // Applies Intel RDT configuration to the process with the specified pid -func (m *intelRdtManager) Apply(pid int) (err error) { +func (m *Manager) Apply(pid int) (err error) { // If intelRdt is not specified in config, we do nothing if m.config.IntelRdt == nil { return nil @@ -557,7 +550,7 @@ func (m *intelRdtManager) Apply(pid int) (err error) { } // Destroys the Intel RDT container-specific 'container_id' group -func (m *intelRdtManager) Destroy() error { +func (m *Manager) Destroy() error { // Don't remove resctrl group if closid has been explicitly specified. The // group is likely externally managed, i.e. by some other entity than us. // There are probably other containers/tasks sharing the same group. @@ -574,7 +567,7 @@ func (m *intelRdtManager) Destroy() error { // Returns Intel RDT path to save in a state file and to be able to // restore the object later -func (m *intelRdtManager) GetPath() string { +func (m *Manager) GetPath() string { if m.path == "" { m.path, _ = m.getIntelRdtPath() } @@ -582,7 +575,7 @@ func (m *intelRdtManager) GetPath() string { } // Returns statistics for Intel RDT -func (m *intelRdtManager) GetStats() (*Stats, error) { +func (m *Manager) GetStats() (*Stats, error) { // If intelRdt is not specified in config if m.config.IntelRdt == nil { return nil, nil @@ -668,7 +661,7 @@ func (m *intelRdtManager) GetStats() (*Stats, error) { } // Set Intel RDT "resource control" filesystem as configured. -func (m *intelRdtManager) Set(container *configs.Config) error { +func (m *Manager) Set(container *configs.Config) error { // About L3 cache schema: // It has allocation bitmasks/values for L3 cache on each socket, // which contains L3 cache id and capacity bitmask (CBM). diff --git a/libcontainer/intelrdt/intelrdt_test.go b/libcontainer/intelrdt/intelrdt_test.go index 2184a1468df..c127cd8f7c6 100644 --- a/libcontainer/intelrdt/intelrdt_test.go +++ b/libcontainer/intelrdt/intelrdt_test.go @@ -20,7 +20,7 @@ func TestIntelRdtSetL3CacheSchema(t *testing.T) { }) helper.config.IntelRdt.L3CacheSchema = l3CacheSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -50,7 +50,7 @@ func TestIntelRdtSetMemBwSchema(t *testing.T) { }) helper.config.IntelRdt.MemBwSchema = memBwSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -80,7 +80,7 @@ func TestIntelRdtSetMemBwScSchema(t *testing.T) { }) helper.config.IntelRdt.MemBwSchema = memBwScSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -103,7 +103,7 @@ func TestApply(t *testing.T) { const closID = "test-clos" helper.config.IntelRdt.ClosID = closID - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Apply(1234); err == nil { t.Fatal("unexpected success when applying pid") } @@ -112,7 +112,7 @@ func TestApply(t *testing.T) { } // Dir should be created if some schema has been specified - intelrdt.(*intelRdtManager).config.IntelRdt.L3CacheSchema = "L3:0=f" + intelrdt.config.IntelRdt.L3CacheSchema = "L3:0=f" if err := intelrdt.Apply(1235); err != nil { t.Fatalf("Apply() failed: %v", err) } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 446649af843..0d9ceb9c98c 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -299,7 +299,7 @@ type initProcess struct { logFilePair filePair config *initConfig manager cgroups.Manager - intelRdtManager intelrdt.Manager + intelRdtManager *intelrdt.Manager container *linuxContainer fds []string process *Process diff --git a/utils_linux.go b/utils_linux.go index a9badf20f8b..60d534e86b1 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -32,8 +32,6 @@ func loadFactory(context *cli.Context) (libcontainer.Factory, error) { return nil, err } - intelRdtManager := libcontainer.IntelRdtFs - // We resolve the paths for {newuidmap,newgidmap} from the context of runc, // to avoid doing a path lookup in the nsexec context. TODO: The binary // names are not currently configurable. @@ -46,7 +44,7 @@ func loadFactory(context *cli.Context) (libcontainer.Factory, error) { newgidmap = "" } - return libcontainer.New(abs, intelRdtManager, + return libcontainer.New(abs, libcontainer.CriuPath(context.GlobalString("criu")), libcontainer.NewuidmapPath(newuidmap), libcontainer.NewgidmapPath(newgidmap))