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 0c4bbe9a0a..92aa0c12c5 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -2268,7 +2268,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) }