Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

cli: add systemd-cgroup option #747

Merged
bergwolf merged 1 commit into from
Sep 20, 2018
Merged

cli: add systemd-cgroup option #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions cli/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ var createCLICommand = cli.Command{
console,
context.String("pid-file"),
true,
context.Bool("systemd-cgroup"),
runtimeConfig,
)
},
Expand All @@ -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

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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()

Expand All @@ -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
}
Expand Down
33 changes: 17 additions & 16 deletions cli/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -893,7 +894,7 @@ func TestCreateSandboxConfigFail(t *testing.T) {
Quota: &quota,
}

_, 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))
}
Expand Down Expand Up @@ -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))
}
Expand Down
4 changes: 4 additions & 0 deletions cli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-)
Expand Down
5 changes: 3 additions & 2 deletions cli/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
}

Expand Down
51 changes: 26 additions & 25 deletions cli/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
}
23 changes: 21 additions & 2 deletions virtcontainers/kata_agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Copy link
Copy Markdown
Member

@bergwolf bergwolf Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can do the conversion in the cli package instead?

Copy link
Copy Markdown
Author

@devimc devimc Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same, the problem is that systemd cgroup should be applied in the host (if systemd is the init process and where docker is running), so I prefer to modify the grpcSpec before sending it to the agent and don't modify the original ociSpec to let virtcontainer apply it in the host

Copy link
Copy Markdown
Member

@WeiZhang555 WeiZhang555 Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @devimc

Copy link
Copy Markdown
Member

@bergwolf bergwolf Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, sound good to me. Thanks @devimc .

// 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.
Expand Down Expand Up @@ -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)

Expand Down
7 changes: 6 additions & 1 deletion virtcontainers/kata_agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down
Loading