From 9949daf4dcba2d39cd0de6b4dc425570b2648cc1 Mon Sep 17 00:00:00 2001 From: Julio Montes Date: Wed, 11 Dec 2019 16:34:10 +0000 Subject: [PATCH] virtcontainers: move validCgroupPath move `validCgroupPath` to `cgroups.go` since it's cgroups specific. Now `validCgroupPath` supports systemd cgroup path and returns a cgroup path ready to use, calls to `renameCgroupPath` are no longer needed. Signed-off-by: Julio Montes --- virtcontainers/api_test.go | 9 ++--- virtcontainers/cgroups.go | 26 ++++++++++++ virtcontainers/cgroups_test.go | 64 ++++++++++++++++++++++++++++++ virtcontainers/container.go | 5 +-- virtcontainers/sandbox.go | 1 - virtcontainers/utils/utils.go | 18 --------- virtcontainers/utils/utils_test.go | 15 ------- 7 files changed, 96 insertions(+), 42 deletions(-) diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index c570b4ad8..42cff7328 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -22,7 +22,6 @@ import ( "github.com/kata-containers/runtime/virtcontainers/pkg/mock" vcTypes "github.com/kata-containers/runtime/virtcontainers/pkg/types" "github.com/kata-containers/runtime/virtcontainers/types" - "github.com/kata-containers/runtime/virtcontainers/utils" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" ) @@ -510,7 +509,7 @@ func TestStatusSandboxSuccessfulStateReady(t *testing.T) { assert := assert.New(t) config := newTestSandboxConfigNoop() - cgroupPath, err := renameCgroupPath(utils.DefaultCgroupPath) + cgroupPath, err := renameCgroupPath(defaultCgroupPath) assert.NoError(err) hypervisorConfig := HypervisorConfig{ @@ -569,7 +568,7 @@ func TestStatusSandboxSuccessfulStateRunning(t *testing.T) { assert := assert.New(t) config := newTestSandboxConfigNoop() - cgroupPath, err := renameCgroupPath(utils.DefaultCgroupPath) + cgroupPath, err := renameCgroupPath(defaultCgroupPath) assert.NoError(err) hypervisorConfig := HypervisorConfig{ @@ -1136,7 +1135,7 @@ func TestStatusContainerStateReady(t *testing.T) { contID := "101" config := newTestSandboxConfigNoop() - cgroupPath, err := renameCgroupPath(utils.DefaultCgroupPath) + cgroupPath, err := renameCgroupPath(defaultCgroupPath) assert.NoError(err) ctx := context.Background() @@ -1195,7 +1194,7 @@ func TestStatusContainerStateRunning(t *testing.T) { contID := "101" config := newTestSandboxConfigNoop() - cgroupPath, err := renameCgroupPath(utils.DefaultCgroupPath) + cgroupPath, err := renameCgroupPath(defaultCgroupPath) assert.NoError(err) ctx := context.Background() diff --git a/virtcontainers/cgroups.go b/virtcontainers/cgroups.go index e8a81e927..3aa71db9d 100644 --- a/virtcontainers/cgroups.go +++ b/virtcontainers/cgroups.go @@ -40,6 +40,9 @@ const cgroupKataPath = "/kata/" // from grabbing the stats data. const cgroupKataPrefix = "kata" +// DefaultCgroupPath runtime-determined location in the cgroups hierarchy. +const defaultCgroupPath = "/vc" + var cgroupsLoadFunc = cgroups.Load var cgroupsNewFunc = cgroups.New @@ -190,6 +193,29 @@ func renameCgroupPath(path string) (string, error) { } +// validCgroupPath returns a valid cgroup path. +// see https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#cgroups-path +func validCgroupPath(path string, systemdCgroup bool) (string, error) { + if isSystemdCgroup(path) { + return path, nil + } + + if systemdCgroup { + return "", fmt.Errorf("malformed systemd path '%v': expected to be of form 'slice:prefix:name'", path) + } + + // In the case of an absolute path (starting with /), the runtime MUST + // take the path to be relative to the cgroups mount point. + if filepath.IsAbs(path) { + return renameCgroupPath(filepath.Clean(path)) + } + + // In the case of a relative path (not starting with /), the runtime MAY + // interpret the path relative to a runtime-determined location in the cgroups hierarchy. + // clean up path and return a new path relative to defaultCgroupPath + return renameCgroupPath(filepath.Join(defaultCgroupPath, filepath.Clean("/"+path))) +} + func isSystemdCgroup(cgroupPath string) bool { // systemd cgroup path: slice:prefix:name re := regexp.MustCompile(`([[:alnum:]]|\.)+:([[:alnum:]]|\.)+:([[:alnum:]]|\.)+`) diff --git a/virtcontainers/cgroups_test.go b/virtcontainers/cgroups_test.go index 2f744ffbc..a1cca5f3b 100644 --- a/virtcontainers/cgroups_test.go +++ b/virtcontainers/cgroups_test.go @@ -10,6 +10,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/containerd/cgroups" @@ -224,3 +225,66 @@ func TestIsSystemdCgroup(t *testing.T) { assert.Equal(t.expected, isSystemdCgroup(t.path), "invalid systemd cgroup path: %v", t.path) } } + +func TestValidCgroupPath(t *testing.T) { + assert := assert.New(t) + + for _, t := range []struct { + path string + systemdCgroup bool + error bool + }{ + // empty paths + {"../../../", false, false}, + {"../", false, false}, + {".", false, false}, + {"../../../", false, false}, + {"./../", false, false}, + + // valid no-systemd paths + {"../../../foo", false, false}, + {"/../hi", false, false}, + {"/../hi/foo", false, false}, + {"o / m /../ g", false, false}, + + // invalid systemd paths + {"o / m /../ g", true, true}, + {"slice:kata", true, true}, + {"/kata/afhts2e5d4g5s", true, true}, + {"a:b:c:d", true, true}, + {":::", true, true}, + {"", true, true}, + {":", true, true}, + {"::", true, true}, + {":::", true, true}, + {"a:b", true, true}, + {"a:b:", true, true}, + {":a:b", true, true}, + {"@:@:@", true, true}, + + // valid system paths + {"slice:kata:55555", true, false}, + {"slice.system:kata:afhts2e5d4g5s", true, false}, + } { + path, err := validCgroupPath(t.path, t.systemdCgroup) + if t.error { + assert.Error(err) + continue + } else { + assert.NoError(err) + } + + if filepath.IsAbs(t.path) { + cleanPath := filepath.Dir(filepath.Clean(t.path)) + assert.True(strings.HasPrefix(path, cleanPath), + "%v should have prefix %v", cleanPath) + } else if t.systemdCgroup { + assert.Equal(t.path, path) + } else { + assert.True(strings.HasPrefix(path, "/"+cgroupKataPrefix) || + strings.HasPrefix(path, defaultCgroupPath), + "%v should have prefix /%v or %v", path, cgroupKataPrefix, defaultCgroupPath) + } + } + +} diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 6c3d759ed..f75ac79f0 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -1398,10 +1398,9 @@ func (c *Container) cgroupsCreate() (err error) { resources.CPU = validCPUResources(spec.Linux.Resources.CPU) } - cgroupPath := utils.ValidCgroupPath(spec.Linux.CgroupsPath) - c.state.CgroupPath, err = renameCgroupPath(cgroupPath) + c.state.CgroupPath, err = validCgroupPath(spec.Linux.CgroupsPath, c.sandbox.config.SystemdCgroup) if err != nil { - return err + return fmt.Errorf("Invalid cgroup path: %v", err) } cgroup, err := cgroupsNewFunc(cgroups.V1, diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 4fd50bb55..5e67c7990 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -12,7 +12,6 @@ import ( "math" "net" "os" - "path/filepath" "strings" "sync" "syscall" diff --git a/virtcontainers/utils/utils.go b/virtcontainers/utils/utils.go index 02bb50b2e..6be68f428 100644 --- a/virtcontainers/utils/utils.go +++ b/virtcontainers/utils/utils.go @@ -14,9 +14,6 @@ import ( "path/filepath" ) -// DefaultCgroupPath runtime-determined location in the cgroups hierarchy. -const DefaultCgroupPath = "/vc" - const cpBinaryName = "cp" const fileMode0755 = os.FileMode(0755) @@ -238,21 +235,6 @@ func SupportsVsocks() bool { return true } -// ValidCgroupPath returns a valid cgroup path. -// see https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#cgroups-path -func ValidCgroupPath(path string) string { - // In the case of an absolute path (starting with /), the runtime MUST - // take the path to be relative to the cgroups mount point. - if filepath.IsAbs(path) { - return filepath.Clean(path) - } - - // In the case of a relative path (not starting with /), the runtime MAY - // interpret the path relative to a runtime-determined location in the cgroups hierarchy. - // clean up path and return a new path relative to defaultCgroupPath - return filepath.Join(DefaultCgroupPath, filepath.Clean("/"+path)) -} - // StartCmd pointer to a function to start a command. // Defined this way to allow mock testing. var StartCmd = func(c *exec.Cmd) error { diff --git a/virtcontainers/utils/utils_test.go b/virtcontainers/utils/utils_test.go index 1b3011664..63a8f2e98 100644 --- a/virtcontainers/utils/utils_test.go +++ b/virtcontainers/utils/utils_test.go @@ -272,18 +272,3 @@ func TestSupportsVsocks(t *testing.T) { assert.True(SupportsVsocks()) } - -func TestValidCgroupPath(t *testing.T) { - assert := assert.New(t) - - assert.Equal(DefaultCgroupPath, ValidCgroupPath("../../../")) - assert.Equal(filepath.Join(DefaultCgroupPath, "foo"), ValidCgroupPath("../../../foo")) - assert.Equal("/hi", ValidCgroupPath("/../hi")) - assert.Equal("/hi/foo", ValidCgroupPath("/../hi/foo")) - assert.Equal(DefaultCgroupPath, ValidCgroupPath("")) - assert.Equal(DefaultCgroupPath, ValidCgroupPath("")) - assert.Equal(DefaultCgroupPath, ValidCgroupPath("../")) - assert.Equal(DefaultCgroupPath, ValidCgroupPath(".")) - assert.Equal(DefaultCgroupPath, ValidCgroupPath("./../")) - assert.Equal(filepath.Join(DefaultCgroupPath, "o / g"), ValidCgroupPath("o / m /../ g")) -}