From 0e68c00ef03ea10a92ec48fcb88e31146788dfed Mon Sep 17 00:00:00 2001 From: Eric Ernsteernst Date: Wed, 8 Jul 2020 13:38:12 -0700 Subject: [PATCH] cgroups: Add systemd detection when creating cgroup manager Look at the provided cgroup path to determine whether systemd is being used to manage the cgroups. With this, systemd cgroups are being detected and created appropriately for the sandbox. Fixes: #2818 Signed-off-by: Eric Ernsteernst (cherry picked from commit ad5484ba2efdcd96ccdf64e4218d31ebee48d9a3) --- virtcontainers/pkg/cgroups/manager.go | 16 +++++++-- virtcontainers/pkg/cgroups/manager_test.go | 41 +++++++--------------- virtcontainers/pkg/cgroups/utils.go | 21 +++++++---- virtcontainers/pkg/cgroups/utils_test.go | 10 +++--- virtcontainers/sandbox.go | 2 +- 5 files changed, 46 insertions(+), 44 deletions(-) diff --git a/virtcontainers/pkg/cgroups/manager.go b/virtcontainers/pkg/cgroups/manager.go index ac20ec298e..f7cd017a30 100644 --- a/virtcontainers/pkg/cgroups/manager.go +++ b/virtcontainers/pkg/cgroups/manager.go @@ -107,7 +107,6 @@ func hypervisorDevices() []specs.LinuxDeviceCgroup { // New creates a new CgroupManager func New(config *Config) (*Manager, error) { var err error - useSystemdCgroup := UseSystemdCgroup() devices := config.Resources.Devices devices = append(devices, hypervisorDevices()...) @@ -125,6 +124,9 @@ func New(config *Config) (*Manager, error) { cgroups := config.Cgroups cgroupPaths := config.CgroupPaths + // determine if we are utilizing systemd managed cgroups based on the path provided + useSystemdCgroup := IsSystemdCgroup(config.CgroupPath) + // Create a new cgroup if the current one is nil // this cgroups must be saved later if cgroups == nil { @@ -220,7 +222,14 @@ func (m *Manager) moveToParent() error { m.Lock() defer m.Unlock() for _, cgroupPath := range m.mgr.GetPaths() { + pids, err := readPids(cgroupPath) + // possible that the cgroupPath doesn't exist. If so, skip: + if os.IsNotExist(err) { + // The cgroup is not present on the filesystem: no pids to move. The systemd cgroup + // manager lists all of the subsystems, including those that are not actually being managed. + continue + } if err != nil { return err } @@ -286,7 +295,10 @@ func (m *Manager) GetPaths() map[string]string { func (m *Manager) Destroy() error { // cgroup can't be destroyed if it contains running processes if err := m.moveToParent(); err != nil { - return fmt.Errorf("Could not move processes into parent cgroup: %v", err) + // If the process migration to the parent cgroup fails, then + // we expect the Destroy to fail as well. Let's log an error here + // and attempt to execute the Destroy still to help cleanup the hosts' FS. + m.logger().WithError(err).Error("Could not move processes into parent cgroup") } m.Lock() diff --git a/virtcontainers/pkg/cgroups/manager_test.go b/virtcontainers/pkg/cgroups/manager_test.go index 0bf1f0c800..1e868cf692 100644 --- a/virtcontainers/pkg/cgroups/manager_test.go +++ b/virtcontainers/pkg/cgroups/manager_test.go @@ -11,34 +11,11 @@ import ( "github.com/stretchr/testify/assert" ) -func TestEnableSystemdCgroup(t *testing.T) { - assert := assert.New(t) - - orgSystemdCgroup := systemdCgroup - defer func() { - systemdCgroup = orgSystemdCgroup - }() - - useSystemdCgroup := UseSystemdCgroup() - if systemdCgroup != nil { - assert.Equal(*systemdCgroup, useSystemdCgroup) - } else { - assert.False(useSystemdCgroup) - } - - EnableSystemdCgroup() - assert.True(UseSystemdCgroup()) -} - +//very very basic test; should be expanded func TestNew(t *testing.T) { assert := assert.New(t) - useSystemdCgroup := false - orgSystemdCgroup := systemdCgroup - defer func() { - systemdCgroup = orgSystemdCgroup - }() - systemdCgroup = &useSystemdCgroup + // create a cgroupfs cgroup manager c := &Config{ Cgroups: nil, CgroupPath: "", @@ -48,8 +25,14 @@ func TestNew(t *testing.T) { assert.NoError(err) assert.NotNil(mgr.mgr) - useSystemdCgroup = true - mgr, err = New(c) - assert.Error(err) - assert.Nil(mgr) + // create a systemd cgroup manager + s := &Config{ + Cgroups: nil, + CgroupPath: "system.slice:kubepod:container", + } + + mgr, err = New(s) + assert.NoError(err) + assert.NotNil(mgr.mgr) + } diff --git a/virtcontainers/pkg/cgroups/utils.go b/virtcontainers/pkg/cgroups/utils.go index 78f6aba5d1..0086be6272 100644 --- a/virtcontainers/pkg/cgroups/utils.go +++ b/virtcontainers/pkg/cgroups/utils.go @@ -9,7 +9,7 @@ import ( "fmt" "os" "path/filepath" - "regexp" + "strings" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runtime-spec/specs-go" @@ -60,13 +60,20 @@ func ValidCgroupPath(path string, systemdCgroup bool) (string, error) { } func IsSystemdCgroup(cgroupPath string) bool { - // systemd cgroup path: slice:prefix:name - re := regexp.MustCompile(`([[:alnum:]]|\.)+:([[:alnum:]]|\.)+:([[:alnum:]]|\.)+`) - found := re.FindStringIndex(cgroupPath) - // if found string is equal to cgroupPath then - // it's a correct systemd cgroup path. - return found != nil && cgroupPath[found[0]:found[1]] == cgroupPath + // If we are utilizing systemd to manage cgroups, we expect to receive a path + // in the format slice:scopeprefix:name. A typical example would be: + // + // system.slice:docker:6b4c4a4d0cc2a12c529dcb13a2b8e438dfb3b2a6af34d548d7d + // + // Based on this, let's split by the ':' delimiter and verify that the first + // section has .slice as a suffix. + parts := strings.Split(cgroupPath, ":") + if len(parts) == 3 && strings.HasSuffix(parts[0], ".slice") { + return true + } + + return false } func DeviceToCgroupDevice(device string) (*configs.Device, error) { diff --git a/virtcontainers/pkg/cgroups/utils_test.go b/virtcontainers/pkg/cgroups/utils_test.go index fff34ad1c1..4203085b82 100644 --- a/virtcontainers/pkg/cgroups/utils_test.go +++ b/virtcontainers/pkg/cgroups/utils_test.go @@ -22,8 +22,8 @@ func TestIsSystemdCgroup(t *testing.T) { path string expected bool }{ - {"slice:kata:afhts2e5d4g5s", true}, - {"slice.system:kata:afhts2e5d4g5s", true}, + {"foo.slice:kata:afhts2e5d4g5s", true}, + {"system.slice:kata:afhts2e5d4g5s", true}, {"/kata/afhts2e5d4g5s", false}, {"a:b:c:d", false}, {":::", false}, @@ -78,9 +78,9 @@ func TestValidCgroupPath(t *testing.T) { {":a:b", true, true}, {"@:@:@", true, true}, - // valid system paths - {"slice:kata:55555", true, false}, - {"slice.system:kata:afhts2e5d4g5s", true, false}, + // valid systemd paths + {"x.slice:kata:55555", true, false}, + {"system.slice:kata:afhts2e5d4g5s", true, false}, } { path, err := ValidCgroupPath(t.path, t.systemdCgroup) if t.error { diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index f3809cb97e..f34a116af9 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -2269,7 +2269,7 @@ func (s *Sandbox) getSandboxCPUSet() (string, error) { if ctr.Resources.CPU != nil { currSet, err := cpuset.Parse(ctr.Resources.CPU.Cpus) if err != nil { - return "", fmt.Errorf("unable to parse CPUset for container %s", ctr.ID) + return "", fmt.Errorf("unable to parse CPUset for container %s: %v", ctr.ID, err) } result = result.Union(currSet) }