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 <[email protected]>

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()

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)
+
 }

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) {

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 {

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)
 		}