Merge pull request #2817 from egernst/fixup-systemd-cgroup-handling

Fixup systemd cgroup handling

virtcontainers/pkg/cgroups/manager.go

@@ -53,9 +53,6 @@ const (
 )
 
 var (
-	// If set to true, expects cgroupsPath to be of form "slice:prefix:name", otherwise cgroups creation will fail
-	systemdCgroup *bool
-
 	cgroupsLogger = logrus.WithField("source", "virtcontainers/pkg/cgroups")
 )
 
@@ -66,18 +63,6 @@ func SetLogger(logger *logrus.Entry) {
 	cgroupsLogger = logger.WithFields(fields)
 }
 
-func EnableSystemdCgroup() {
-	systemd := true
-	systemdCgroup = &systemd
-}
-
-func UseSystemdCgroup() bool {
-	if systemdCgroup != nil {
-		return *systemdCgroup
-	}
-	return false
-}
-
 // returns the list of devices that a hypervisor may need
 func hypervisorDevices() []specs.LinuxDeviceCgroup {
 	devices := []specs.LinuxDeviceCgroup{}
@@ -107,7 +92,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 +109,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 +207,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 +280,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)
 		}