Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run: Add cgroup-parent flag #1782

Merged
merged 1 commit into from
Jan 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ Resource flags:
- :whale: `--blkio-weight`: Block IO (relative weight), between 10 and 1000, or 0 to disable (default 0)
- :whale: `--cgroupns=(host|private)`: Cgroup namespace to use
- Default: "private" on cgroup v2 hosts, "host" on cgroup v1 hosts
- :whale: `--cgroup-parent`: Optional parent cgroup for the container
- :whale: `--device`: Add a host device to the container

Intel RDT flags:
Expand Down Expand Up @@ -575,7 +576,7 @@ Verify flags:
- :nerd_face: `--cosign-key`: Path to the public key file, KMS, URI or Kubernetes Secret for `--verify=cosign`

Unimplemented `docker run` flags:
`--attach`, `--blkio-weight-device`, `--cgroup-parent`, `--cpu-rt-*`, `--detach-keys`, `--device-*`,
`--attach`, `--blkio-weight-device`, `--cpu-rt-*`, `--detach-keys`, `--device-*`,
`--disable-content-trust`, `--domainname`, `--expose`, `--health-*`, `--ip6`, `--isolation`, `--no-healthcheck`,
`--link*`, `--mac-address`, `--publish-all`, `--sig-proxy`, `--storage-opt`,
`--userns`, `--volume-driver`, `--volumes-from`
Expand Down
1 change: 1 addition & 0 deletions cmd/nerdctl/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ func setCreateFlags(cmd *cobra.Command) {
cmd.Flags().StringSlice("cgroup-conf", nil, "Configure cgroup v2 (key=value)")
cmd.Flags().Uint16("blkio-weight", 0, "Block IO (relative weight), between 10 and 1000, or 0 to disable (default 0)")
cmd.Flags().String("cgroupns", defaults.CgroupnsMode(), `Cgroup namespace to use, the default depends on the cgroup version ("host"|"private")`)
cmd.Flags().String("cgroup-parent", "", "Optional parent cgroup for the container")
cmd.RegisterFlagCompletionFunc("cgroupns", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return []string{"host", "private"}, cobra.ShellCompDirectiveNoFileComp
})
Expand Down
70 changes: 58 additions & 12 deletions cmd/nerdctl/run_cgroup_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,28 +86,35 @@ func generateCgroupOpts(cmd *cobra.Command, id string) ([]oci.SpecOpts, error) {
if err != nil {
return nil, err
}

parent, err := cmd.Flags().GetString("cgroup-parent")
if err != nil {
return nil, err
}

if cgroupManager == "none" {
if !rootlessutil.IsRootless() {
return nil, errors.New("cgroup-manager \"none\" is only supported for rootless")
return nil, errors.New(`cgroup-manager "none" is only supported for rootless`)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel like raw string literals fit nicer than manually escaping quotes and other things. Also a habit from windows land where you'd commonly have to escape backslashes, so raw string literals came in handy there.

}

if cpus > 0.0 || memStr != "" || memSwap != "" || pidsLimit > 0 {
logrus.Warn("cgroup manager is set to \"none\", discarding resource limit requests. " +
logrus.Warn(`cgroup manager is set to "none", discarding resource limit requests. ` +
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this change?

"(Hint: enable cgroup v2 with systemd: https://rootlesscontaine.rs/getting-started/common/cgroup2/)")
}
dcantah marked this conversation as resolved.
Show resolved Hide resolved
if parent != "" {
logrus.Warnf(`cgroup manager is set to "none", ignoring cgroup parent %q`+
"(Hint: enable cgroup v2 with systemd: https://rootlesscontaine.rs/getting-started/common/cgroup2/)", parent)
}
return []oci.SpecOpts{oci.WithCgroup("")}, nil
}

var opts []oci.SpecOpts // nolint: prealloc

if cgroupManager == "systemd" {
slice := "system.slice"
if rootlessutil.IsRootlessChild() {
slice = "user.slice"
}
// "slice:prefix:name"
cg := slice + ":nerdctl:" + id
opts = append(opts, oci.WithCgroup(cg))
path, err := generateCgroupPath(cmd, cgroupManager, parent, id)
if err != nil {
return nil, err
}
if path != "" {
opts = append(opts, oci.WithCgroup(path))
}

// cpus: from https://github.com/containerd/containerd/blob/v1.4.3/cmd/ctr/commands/run/run_unix.go#L187-L193
Expand Down Expand Up @@ -155,15 +162,16 @@ func generateCgroupOpts(cmd *cobra.Command, id string) ([]oci.SpecOpts, error) {
if cpusetMems != "" {
opts = append(opts, oci.WithCPUsMems(cpusetMems))
}

var mem64 int64
if memStr != "" {
mem64, err = units.RAMInBytes(memStr)
if err != nil {
return nil, fmt.Errorf("failed to parse memory bytes %q: %w", memStr, err)
}
opts = append(opts, oci.WithMemoryLimit(uint64(mem64)))

}

var memReserve64 int64
if memReserve != "" {
memReserve64, err = units.RAMInBytes(memReserve)
Expand Down Expand Up @@ -280,6 +288,44 @@ func generateCgroupOpts(cmd *cobra.Command, id string) ([]oci.SpecOpts, error) {
return opts, nil
}

func generateCgroupPath(cmd *cobra.Command, cgroupManager, parent, id string) (string, error) {
var (
path string
usingSystemd = cgroupManager == "systemd"
slice = "system.slice"
scopePrefix = ":nerdctl:"
)
if rootlessutil.IsRootlessChild() {
slice = "user.slice"
}

if parent == "" {
if usingSystemd {
// "slice:prefix:name"
path = slice + scopePrefix + id
}
// Nothing to do for the non-systemd case if a parent wasn't supplied,
// containerd already sets a default cgroup path as /<namespace>/<containerID>
return path, nil
}

// If the user asked for a cgroup parent and we're using systemd,
// Docker uses the following:
// parent + prefix (in our case, nerdctl) + containerID.
//
// In the non systemd case, it's just /parent/containerID
if usingSystemd {
if len(parent) <= 6 || !strings.HasSuffix(parent, ".slice") {
return "", errors.New(`cgroup-parent for systemd cgroup should be a valid slice named as "xxx.slice"`)
}
path = parent + scopePrefix + id
} else {
path = filepath.Join(parent, id)
}

return path, nil
}

func parseDevice(s string) (hostDevPath string, mode string, err error) {
mode = "rwm"
split := strings.Split(s, ":")
Expand Down
47 changes: 46 additions & 1 deletion cmd/nerdctl/run_cgroup_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"fmt"
"os"
"path/filepath"
"testing"

"github.com/containerd/cgroups"
Expand Down Expand Up @@ -261,7 +262,6 @@ func TestParseDevice(t *testing.T) {
assert.ErrorContains(t, err, tc.err)
}
}

}

func TestRunCgroupConf(t *testing.T) {
Expand All @@ -283,6 +283,51 @@ func TestRunCgroupConf(t *testing.T) {
"cat", "memory.high").AssertOutExactly("33554432\n")
}

func TestRunCgroupParent(t *testing.T) {
t.Parallel()
base := testutil.NewBase(t)
info := base.Info()
containerName := testutil.Identifier(t)
defer base.Cmd("rm", "-f", containerName).Run()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be AssertOK; since bugs were found in the past of containers leaked out.

Copy link
Member Author

@dcantah dcantah Jan 1, 2023

Choose a reason for hiding this comment

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

Are you referring to the deferred cleanup? I was just following the standard set in most of the other tests in this file, but seems it's actually the lesser used of the two when taking into account all of the other tests; only run_cgroup_linux_test and run_mount_linux_test just defer Run and call it a day. I'll open up a PR to swap the cgroup tests to using AssertOK instead (and use AssertOK here)

Copy link
Member Author

Choose a reason for hiding this comment

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


switch info.CgroupDriver {
case "none", "":
t.Skip("test requires cgroup driver")
}

t.Logf("Using %q cgroup driver", info.CgroupDriver)

parent := "/foobarbaz"
if info.CgroupDriver == "systemd" {
dcantah marked this conversation as resolved.
Show resolved Hide resolved
// Path separators aren't allowed in systemd path. runc
// explicitly checks for this.
// https://github.com/opencontainers/runc/blob/016a0d29d1750180b2a619fc70d6fe0d80111be0/libcontainer/cgroups/systemd/common.go#L65-L68
parent = "foobarbaz.slice"
}

// cgroup2 without host cgroup ns will just output 0::/ which doesn't help much to verify
// we got our expected path. This approach should work for both cgroup1 and 2, there will
// just be many more entries for cgroup1 as there'll be an entry per controller.
base.Cmd(
"run",
"-d",
"--name",
containerName,
"--cgroupns=host",
"--cgroup-parent", parent,
testutil.AlpineImage,
"sleep",
"infinity",
).AssertOK()

id := base.InspectContainer(containerName).ID
expected := filepath.Join(parent, id)
if info.CgroupDriver == "systemd" {
expected = filepath.Join(parent, fmt.Sprintf("nerdctl-%s", id))
}
base.Cmd("exec", containerName, "cat", "/proc/self/cgroup").AssertOutContains(expected)
}

func TestRunBlkioWeightCgroupV2(t *testing.T) {
t.Parallel()
if cgroups.Mode() != cgroups.Unified {
Expand Down