From f659871f557c78803105e7c2ee35895434951107 Mon Sep 17 00:00:00 2001 From: Eric Ernsteernst Date: Wed, 8 Jul 2020 13:46:58 -0700 Subject: [PATCH 1/2] cgroups: remove unused SystemdCgroup variable and accessor/mutators Since we are now detecting, no longer to keep this state. Signed-off-by: Eric Ernsteernst (forward port of https://github.com/kata-containers/runtime/pull/2817) Signed-off-by: Francesco Giudici --- src/runtime/virtcontainers/pkg/cgroups/manager.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/cgroups/manager.go b/src/runtime/virtcontainers/pkg/cgroups/manager.go index 23ca1d8279..1748f1aaa9 100644 --- a/src/runtime/virtcontainers/pkg/cgroups/manager.go +++ b/src/runtime/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{} From d7cb3df0d211dbbb9adabf3d32c51e770dbc4e81 Mon Sep 17 00:00:00 2001 From: Eric Ernsteernst Date: Wed, 8 Jul 2020 13:38:12 -0700 Subject: [PATCH 2/2] 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: #599 Signed-off-by: Eric Ernsteernst (forward port of https://github.com/kata-containers/runtime/pull/2817) Signed-off-by: Francesco Giudici --- .../virtcontainers/pkg/cgroups/manager.go | 16 +++++++- .../pkg/cgroups/manager_test.go | 41 ++++++------------- .../virtcontainers/pkg/cgroups/utils.go | 21 ++++++---- .../virtcontainers/pkg/cgroups/utils_test.go | 10 ++--- 4 files changed, 45 insertions(+), 43 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/cgroups/manager.go b/src/runtime/virtcontainers/pkg/cgroups/manager.go index 1748f1aaa9..b752b64594 100644 --- a/src/runtime/virtcontainers/pkg/cgroups/manager.go +++ b/src/runtime/virtcontainers/pkg/cgroups/manager.go @@ -92,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()...) @@ -110,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 { @@ -205,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 } @@ -271,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() diff --git a/src/runtime/virtcontainers/pkg/cgroups/manager_test.go b/src/runtime/virtcontainers/pkg/cgroups/manager_test.go index 0bf1f0c800..1e868cf692 100644 --- a/src/runtime/virtcontainers/pkg/cgroups/manager_test.go +++ b/src/runtime/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/src/runtime/virtcontainers/pkg/cgroups/utils.go b/src/runtime/virtcontainers/pkg/cgroups/utils.go index 78f6aba5d1..0086be6272 100644 --- a/src/runtime/virtcontainers/pkg/cgroups/utils.go +++ b/src/runtime/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/src/runtime/virtcontainers/pkg/cgroups/utils_test.go b/src/runtime/virtcontainers/pkg/cgroups/utils_test.go index fff34ad1c1..4203085b82 100644 --- a/src/runtime/virtcontainers/pkg/cgroups/utils_test.go +++ b/src/runtime/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 {