From 00751754a9746cc093f3fdbb3fefdfc55403c532 Mon Sep 17 00:00:00 2001 From: Julio Montes Date: Tue, 18 Sep 2018 14:50:09 -0500 Subject: [PATCH] cli: add systemd-cgroup option Add support for cgroup driver systemd. systemd cgroup is not applied in the VM since in some cases like initrd images there is no systemd running and nobody can update a systemd cgroup using systemctl. fixes #596 Signed-off-by: Julio Montes --- cli/create.go | 9 ++--- cli/create_test.go | 33 +++++++++--------- cli/main.go | 4 +++ cli/run.go | 5 +-- cli/run_test.go | 51 ++++++++++++++-------------- virtcontainers/kata_agent.go | 23 +++++++++++-- virtcontainers/kata_agent_test.go | 7 +++- virtcontainers/pkg/oci/utils.go | 4 ++- virtcontainers/pkg/oci/utils_test.go | 4 ++- virtcontainers/sandbox.go | 3 ++ 10 files changed, 91 insertions(+), 52 deletions(-) diff --git a/cli/create.go b/cli/create.go index 9a09448591..dbbf2e4455 100644 --- a/cli/create.go +++ b/cli/create.go @@ -83,6 +83,7 @@ var createCLICommand = cli.Command{ console, context.String("pid-file"), true, + context.Bool("systemd-cgroup"), runtimeConfig, ) }, @@ -91,7 +92,7 @@ var createCLICommand = cli.Command{ // Use a variable to allow tests to modify its value var getKernelParamsFunc = getKernelParams -func create(ctx context.Context, containerID, bundlePath, console, pidFilePath string, detach bool, +func create(ctx context.Context, containerID, bundlePath, console, pidFilePath string, detach, systemdCgroup bool, runtimeConfig oci.RuntimeConfig) error { var err error @@ -146,7 +147,7 @@ func create(ctx context.Context, containerID, bundlePath, console, pidFilePath s var process vc.Process switch containerType { case vc.PodSandbox: - process, err = createSandbox(ctx, ociSpec, runtimeConfig, containerID, bundlePath, console, disableOutput) + process, err = createSandbox(ctx, ociSpec, runtimeConfig, containerID, bundlePath, console, disableOutput, systemdCgroup) if err != nil { return err } @@ -252,7 +253,7 @@ func setKernelParams(containerID string, runtimeConfig *oci.RuntimeConfig) error } func createSandbox(ctx context.Context, ociSpec oci.CompatOCISpec, runtimeConfig oci.RuntimeConfig, - containerID, bundlePath, console string, disableOutput bool) (vc.Process, error) { + containerID, bundlePath, console string, disableOutput, systemdCgroup bool) (vc.Process, error) { span, ctx := trace(ctx, "createSandbox") defer span.Finish() @@ -261,7 +262,7 @@ func createSandbox(ctx context.Context, ociSpec oci.CompatOCISpec, runtimeConfig return vc.Process{}, err } - sandboxConfig, err := oci.SandboxConfig(ociSpec, runtimeConfig, bundlePath, containerID, console, disableOutput) + sandboxConfig, err := oci.SandboxConfig(ociSpec, runtimeConfig, bundlePath, containerID, console, disableOutput, systemdCgroup) if err != nil { return vc.Process{}, err } diff --git a/cli/create_test.go b/cli/create_test.go index d09ac4f5ef..930f6832f0 100644 --- a/cli/create_test.go +++ b/cli/create_test.go @@ -326,19 +326,20 @@ func TestCreateInvalidArgs(t *testing.T) { console string pidFilePath string detach bool + systemdCgroup bool runtimeConfig oci.RuntimeConfig } data := []testData{ - {"", "", "", "", false, oci.RuntimeConfig{}}, - {"", "", "", "", true, oci.RuntimeConfig{}}, - {"foo", "", "", "", true, oci.RuntimeConfig{}}, - {testContainerID, bundlePath, testConsole, pidFilePath, false, runtimeConfig}, - {testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig}, + {"", "", "", "", false, false, oci.RuntimeConfig{}}, + {"", "", "", "", true, true, oci.RuntimeConfig{}}, + {"foo", "", "", "", true, false, oci.RuntimeConfig{}}, + {testContainerID, bundlePath, testConsole, pidFilePath, false, false, runtimeConfig}, + {testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig}, } for i, d := range data { - err := create(context.Background(), d.containerID, d.bundlePath, d.console, d.pidFilePath, d.detach, d.runtimeConfig) + err := create(context.Background(), d.containerID, d.bundlePath, d.console, d.pidFilePath, d.detach, d.systemdCgroup, d.runtimeConfig) assert.Errorf(err, "test %d (%+v)", i, d) } } @@ -377,7 +378,7 @@ func TestCreateInvalidConfigJSON(t *testing.T) { f.Close() for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -421,7 +422,7 @@ func TestCreateInvalidContainerType(t *testing.T) { assert.NoError(err) for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -466,7 +467,7 @@ func TestCreateContainerInvalid(t *testing.T) { assert.NoError(err) for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -561,7 +562,7 @@ func TestCreateProcessCgroupsPathSuccessful(t *testing.T) { assert.NoError(err) for _, detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, detach, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, detach, true, runtimeConfig) assert.NoError(err, "detached: %+v", detach) os.RemoveAll(path) } @@ -645,7 +646,7 @@ func TestCreateCreateCgroupsFilesFail(t *testing.T) { assert.NoError(err) for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -721,7 +722,7 @@ func TestCreateCreateCreatePidFileFail(t *testing.T) { assert.NoError(err) for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -791,7 +792,7 @@ func TestCreate(t *testing.T) { assert.NoError(err) for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.NoError(err, "%+v", detach) os.RemoveAll(path) } @@ -848,7 +849,7 @@ func TestCreateInvalidKernelParams(t *testing.T) { } for detach := range []bool{true, false} { - err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, runtimeConfig) + err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, true, true, runtimeConfig) assert.Errorf(err, "%+v", detach) assert.False(vcmock.IsMockError(err)) os.RemoveAll(path) @@ -893,7 +894,7 @@ func TestCreateSandboxConfigFail(t *testing.T) { Quota: "a, } - _, err = createSandbox(context.Background(), spec, runtimeConfig, testContainerID, bundlePath, testConsole, true) + _, err = createSandbox(context.Background(), spec, runtimeConfig, testContainerID, bundlePath, testConsole, true, true) assert.Error(err) assert.False(vcmock.IsMockError(err)) } @@ -928,7 +929,7 @@ func TestCreateCreateSandboxFail(t *testing.T) { spec, err := readOCIConfigFile(ociConfigFile) assert.NoError(err) - _, err = createSandbox(context.Background(), spec, runtimeConfig, testContainerID, bundlePath, testConsole, true) + _, err = createSandbox(context.Background(), spec, runtimeConfig, testContainerID, bundlePath, testConsole, true, true) assert.Error(err) assert.True(vcmock.IsMockError(err)) } diff --git a/cli/main.go b/cli/main.go index 546062b955..588137427a 100644 --- a/cli/main.go +++ b/cli/main.go @@ -105,6 +105,10 @@ var runtimeFlags = []cli.Flag{ Name: showConfigPathsOption, Usage: "show config file paths that will be checked for (in order)", }, + cli.BoolFlag{ + Name: "systemd-cgroup", + Usage: "enable systemd cgroup support, expects cgroupsPath to be of form \"slice:prefix:name\" for e.g. \"system.slice:runc:434234\"", + }, } // runtimeCommands is the list of supported command-line (sub-) diff --git a/cli/run.go b/cli/run.go index ac0198d0f2..13a08620ca 100644 --- a/cli/run.go +++ b/cli/run.go @@ -75,11 +75,12 @@ var runCLICommand = cli.Command{ context.String("console-socket"), context.String("pid-file"), context.Bool("detach"), + context.Bool("systemd-cgroup"), runtimeConfig) }, } -func run(ctx context.Context, containerID, bundle, console, consoleSocket, pidFile string, detach bool, +func run(ctx context.Context, containerID, bundle, console, consoleSocket, pidFile string, detach, systemdCgroup bool, runtimeConfig oci.RuntimeConfig) error { span, ctx := trace(ctx, "run") defer span.Finish() @@ -89,7 +90,7 @@ func run(ctx context.Context, containerID, bundle, console, consoleSocket, pidFi return err } - if err := create(ctx, containerID, bundle, consolePath, pidFile, detach, runtimeConfig); err != nil { + if err := create(ctx, containerID, bundle, consolePath, pidFile, detach, systemdCgroup, runtimeConfig); err != nil { return err } diff --git a/cli/run_test.go b/cli/run_test.go index f77c27453e..362e563897 100644 --- a/cli/run_test.go +++ b/cli/run_test.go @@ -118,32 +118,33 @@ func TestRunInvalidArgs(t *testing.T) { consoleSocket string pidFile string detach bool + systemdCgroup bool runtimeConfig oci.RuntimeConfig } args := []testArgs{ - {"", "", "", "", "", true, oci.RuntimeConfig{}}, - {"", "", "", "", "", false, oci.RuntimeConfig{}}, - {"", "", "", "", "", true, runtimeConfig}, - {"", "", "", "", "", false, runtimeConfig}, - {"", "", "", "", pidFilePath, false, runtimeConfig}, - {"", "", "", "", inexistentPath, false, runtimeConfig}, - {"", "", "", "", pidFilePath, false, runtimeConfig}, - {"", "", "", inexistentPath, pidFilePath, false, runtimeConfig}, - {"", "", inexistentPath, inexistentPath, pidFilePath, false, runtimeConfig}, - {"", "", inexistentPath, "", pidFilePath, false, runtimeConfig}, - {"", "", consolePath, "", pidFilePath, false, runtimeConfig}, - {"", bundlePath, consolePath, "", pidFilePath, false, runtimeConfig}, - {testContainerID, inexistentPath, consolePath, "", pidFilePath, false, oci.RuntimeConfig{}}, - {testContainerID, inexistentPath, consolePath, "", inexistentPath, false, oci.RuntimeConfig{}}, - {testContainerID, bundlePath, consolePath, "", pidFilePath, false, oci.RuntimeConfig{}}, - {testContainerID, inexistentPath, consolePath, "", pidFilePath, false, runtimeConfig}, - {testContainerID, inexistentPath, consolePath, "", inexistentPath, false, runtimeConfig}, - {testContainerID, bundlePath, consolePath, "", pidFilePath, false, runtimeConfig}, + {"", "", "", "", "", true, true, oci.RuntimeConfig{}}, + {"", "", "", "", "", false, false, oci.RuntimeConfig{}}, + {"", "", "", "", "", true, false, runtimeConfig}, + {"", "", "", "", "", false, true, runtimeConfig}, + {"", "", "", "", pidFilePath, false, false, runtimeConfig}, + {"", "", "", "", inexistentPath, false, false, runtimeConfig}, + {"", "", "", "", pidFilePath, false, true, runtimeConfig}, + {"", "", "", inexistentPath, pidFilePath, false, true, runtimeConfig}, + {"", "", inexistentPath, inexistentPath, pidFilePath, false, false, runtimeConfig}, + {"", "", inexistentPath, "", pidFilePath, false, false, runtimeConfig}, + {"", "", consolePath, "", pidFilePath, false, true, runtimeConfig}, + {"", bundlePath, consolePath, "", pidFilePath, false, true, runtimeConfig}, + {testContainerID, inexistentPath, consolePath, "", pidFilePath, false, true, oci.RuntimeConfig{}}, + {testContainerID, inexistentPath, consolePath, "", inexistentPath, false, true, oci.RuntimeConfig{}}, + {testContainerID, bundlePath, consolePath, "", pidFilePath, false, false, oci.RuntimeConfig{}}, + {testContainerID, inexistentPath, consolePath, "", pidFilePath, false, false, runtimeConfig}, + {testContainerID, inexistentPath, consolePath, "", inexistentPath, false, true, runtimeConfig}, + {testContainerID, bundlePath, consolePath, "", pidFilePath, false, true, runtimeConfig}, } for i, a := range args { - err := run(context.Background(), a.containerID, a.bundle, a.console, a.consoleSocket, a.pidFile, a.detach, a.runtimeConfig) + err := run(context.Background(), a.containerID, a.bundle, a.console, a.consoleSocket, a.pidFile, a.detach, a.systemdCgroup, a.runtimeConfig) assert.Errorf(err, "test %d (%+v)", i, a) } } @@ -289,7 +290,7 @@ func TestRunContainerSuccessful(t *testing.T) { testingImpl.DeleteContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) // should return ExitError with the message and exit code e, ok := err.(*cli.ExitError) @@ -367,7 +368,7 @@ func TestRunContainerDetachSuccessful(t *testing.T) { testingImpl.DeleteContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, true, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, true, true, d.runtimeConfig) // should not return ExitError assert.NoError(err) @@ -440,7 +441,7 @@ func TestRunContainerDeleteFail(t *testing.T) { testingImpl.DeleteContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) // should not return ExitError err, ok := err.(*cli.ExitError) @@ -517,7 +518,7 @@ func TestRunContainerWaitFail(t *testing.T) { testingImpl.DeleteContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) // should not return ExitError err, ok := err.(*cli.ExitError) @@ -575,7 +576,7 @@ func TestRunContainerStartFail(t *testing.T) { testingImpl.StatusContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) // should not return ExitError err, ok := err.(*cli.ExitError) @@ -630,7 +631,7 @@ func TestRunContainerStartFailExistingContainer(t *testing.T) { testingImpl.StartSandboxFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, d.runtimeConfig) + err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) assert.Error(err) assert.False(vcmock.IsMockError(err)) } diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index e5c03fdda7..3f0a2916c7 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -11,6 +11,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strconv" "strings" "sync" @@ -714,7 +715,7 @@ func (k *kataAgent) replaceOCIMountsForStorages(spec *specs.Spec, volumeStorages return nil } -func constraintGRPCSpec(grpcSpec *grpc.Spec) { +func constraintGRPCSpec(grpcSpec *grpc.Spec, systemdCgroup bool) { // Disable Hooks since they have been handled on the host and there is // no reason to send them to the agent. It would make no sense to try // to apply them on the guest. @@ -734,6 +735,24 @@ func constraintGRPCSpec(grpcSpec *grpc.Spec) { grpcSpec.Linux.Resources.HugepageLimits = nil grpcSpec.Linux.Resources.Network = nil + // There are three main reasons to do not apply systemd cgroups in the VM + // - Initrd image doesn't have systemd. + // - Nobody will be able to modify the resources of a specific container by using systemctl set-property. + // - docker is not running in the VM. + if systemdCgroup { + // Convert systemd cgroup to cgroupfs + // systemd cgroup path: slice:prefix:name + re := regexp.MustCompile(`([[:alnum:]]|.)+:([[:alnum:]]|.)+:([[:alnum:]]|.)+`) + systemdCgroupPath := re.FindString(grpcSpec.Linux.CgroupsPath) + if systemdCgroupPath != "" { + slice := strings.Split(systemdCgroupPath, ":") + // 0 - slice: system.slice + // 1 - prefix: docker + // 2 - name: abc123 + grpcSpec.Linux.CgroupsPath = filepath.Join("/", slice[1], slice[2]) + } + } + // Disable network namespace since it is already handled on the host by // virtcontainers. The network is a complex part which cannot be simply // passed to the agent. @@ -964,7 +983,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, // We need to constraint the spec to make sure we're not passing // irrelevant information to the agent. - constraintGRPCSpec(grpcSpec) + constraintGRPCSpec(grpcSpec, sandbox.config.SystemdCgroup) k.handleShm(grpcSpec, sandbox) diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 7c8c762e09..d62d916c7f 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -421,6 +421,7 @@ func TestAppendDevices(t *testing.T) { func TestConstraintGRPCSpec(t *testing.T) { assert := assert.New(t) + expectedCgroupPath := "/foo/bar" g := &pb.Spec{ Hooks: &pb.Hooks{}, @@ -448,10 +449,11 @@ func TestConstraintGRPCSpec(t *testing.T) { HugepageLimits: []pb.LinuxHugepageLimit{}, Network: &pb.LinuxNetwork{}, }, + CgroupsPath: "system.slice:foo:bar", }, } - constraintGRPCSpec(g) + constraintGRPCSpec(g, true) // check nil fields assert.Nil(g.Hooks) @@ -470,6 +472,9 @@ func TestConstraintGRPCSpec(t *testing.T) { // check mounts assert.Len(g.Mounts, 1) + + // check cgroup path + assert.Equal(expectedCgroupPath, g.Linux.CgroupsPath) } func TestHandleShm(t *testing.T) { diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index f9f97fb172..7978be9cd3 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -453,7 +453,7 @@ func addAssetAnnotations(ocispec CompatOCISpec, config *vc.SandboxConfig) { // SandboxConfig converts an OCI compatible runtime configuration file // to a virtcontainers sandbox configuration structure. -func SandboxConfig(ocispec CompatOCISpec, runtime RuntimeConfig, bundlePath, cid, console string, detach bool) (vc.SandboxConfig, error) { +func SandboxConfig(ocispec CompatOCISpec, runtime RuntimeConfig, bundlePath, cid, console string, detach, systemdCgroup bool) (vc.SandboxConfig, error) { containerConfig, err := ContainerConfig(ocispec, bundlePath, cid, console, detach) if err != nil { return vc.SandboxConfig{}, err @@ -507,6 +507,8 @@ func SandboxConfig(ocispec CompatOCISpec, runtime RuntimeConfig, bundlePath, cid }, ShmSize: shmSize, + + SystemdCgroup: systemdCgroup, } addAssetAnnotations(ocispec, &sandboxConfig) diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index 349c119b83..042a02f49e 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -231,6 +231,8 @@ func TestMinimalSandboxConfig(t *testing.T) { vcAnnotations.ConfigJSONKey: string(ociSpecJSON), vcAnnotations.BundlePathKey: tempBundlePath, }, + + SystemdCgroup: true, } ociSpec, err := ParseConfigJSON(tempBundlePath) @@ -238,7 +240,7 @@ func TestMinimalSandboxConfig(t *testing.T) { t.Fatalf("Could not parse config.json: %v", err) } - sandboxConfig, err := SandboxConfig(ociSpec, runtimeConfig, tempBundlePath, containerID, consolePath, false) + sandboxConfig, err := SandboxConfig(ociSpec, runtimeConfig, tempBundlePath, containerID, consolePath, false, true) if err != nil { t.Fatalf("Could not create Sandbox configuration %v", err) } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 073ece5272..4913bcd943 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -352,6 +352,9 @@ type SandboxConfig struct { // Stateful keeps sandbox resources in memory across APIs. Users will be responsible // for calling Release() to release the memory resources. Stateful bool + + // SystemdCgroup enables systemd cgroup support + SystemdCgroup bool } func (s *Sandbox) trace(name string) (opentracing.Span, context.Context) {