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 <julio.montes@intel.com>

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

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:]]|\.)+`)

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

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,

virtcontainers/sandbox.go

@@ -12,7 +12,6 @@ import (
 	"math"
 	"net"
 	"os"
-	"path/filepath"
 	"strings"
 	"sync"
 	"syscall"

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 {

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