From a3f91b9864b2aaa3c5f5081cc7dd16176fc9c36d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Sep 2020 12:50:20 -0700 Subject: [PATCH 1/5] vendor: bump runtime-spec ... to include unified resources support. Signed-off-by: Kir Kolyshkin --- go.mod | 2 +- go.sum | 4 ++-- .../opencontainers/runtime-spec/specs-go/config.go | 10 +++++++--- vendor/modules.txt | 2 +- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 9c83901f0..fce8e9612 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/golang/protobuf v1.4.2 github.com/moby/sys/mountinfo v0.1.3 github.com/mrunalp/fileutils v0.5.0 - github.com/opencontainers/runtime-spec v1.0.3-0.20200728170252-4d89ac9fbff6 + github.com/opencontainers/runtime-spec v1.0.3-0.20200817204227-f9c09b4ea1df github.com/opencontainers/selinux v1.6.0 github.com/pkg/errors v0.9.1 github.com/seccomp/libseccomp-golang v0.9.1 diff --git a/go.sum b/go.sum index 1778432ce..bac892b0f 100644 --- a/go.sum +++ b/go.sum @@ -36,8 +36,8 @@ github.com/moby/sys/mountinfo v0.1.3 h1:KIrhRO14+AkwKvG/g2yIpNMOUVZ02xNhOw8KY1Ws github.com/moby/sys/mountinfo v0.1.3/go.mod h1:w2t2Avltqx8vE7gX5l+QiBKxODu2TX0+Syr3h52Tw4o= github.com/mrunalp/fileutils v0.5.0 h1:NKzVxiH7eSk+OQ4M+ZYW1K6h27RUV3MI6NUTsHhU6Z4= github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ= -github.com/opencontainers/runtime-spec v1.0.3-0.20200728170252-4d89ac9fbff6 h1:NhsM2gc769rVWDqJvapK37r+7+CBXI8xHhnfnt8uQsg= -github.com/opencontainers/runtime-spec v1.0.3-0.20200728170252-4d89ac9fbff6/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= +github.com/opencontainers/runtime-spec v1.0.3-0.20200817204227-f9c09b4ea1df h1:5AW5dMFSXVH4Mg3WYe4z7ui64bK8n66IoWK8i6T4QZ8= +github.com/opencontainers/runtime-spec v1.0.3-0.20200817204227-f9c09b4ea1df/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= github.com/opencontainers/selinux v1.6.0 h1:+bIAS/Za3q5FTwWym4fTB0vObnfCf3G/NC7K6Jx62mY= github.com/opencontainers/selinux v1.6.0/go.mod h1:VVGKuOLlE7v4PJyT6h7mNWvq1rzqiriPsEqVhc+svHE= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= diff --git a/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go b/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go index 3dc9efd23..5fceeb635 100644 --- a/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go +++ b/vendor/github.com/opencontainers/runtime-spec/specs-go/config.go @@ -60,7 +60,7 @@ type Process struct { SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"` } -// LinuxCapabilities specifies the whitelist of capabilities that are kept for a process. +// LinuxCapabilities specifies the list of allowed capabilities that are kept for a process. // http://man7.org/linux/man-pages/man7/capabilities.7.html type LinuxCapabilities struct { // Bounding is the set of capabilities checked by the kernel. @@ -354,7 +354,7 @@ type LinuxRdma struct { // LinuxResources has container runtime resource constraints type LinuxResources struct { - // Devices configures the device whitelist. + // Devices configures the device allowlist. Devices []LinuxDeviceCgroup `json:"devices,omitempty"` // Memory restriction configuration Memory *LinuxMemory `json:"memory,omitempty"` @@ -372,6 +372,8 @@ type LinuxResources struct { // Limits are a set of key value pairs that define RDMA resource limits, // where the key is device name and value is resource limits. Rdma map[string]LinuxRdma `json:"rdma,omitempty"` + // Unified resources. + Unified map[string]string `json:"unified,omitempty"` } // LinuxDevice represents the mknod information for a Linux special device file @@ -392,7 +394,8 @@ type LinuxDevice struct { GID *uint32 `json:"gid,omitempty"` } -// LinuxDeviceCgroup represents a device rule for the whitelist controller +// LinuxDeviceCgroup represents a device rule for the devices specified to +// the device controller type LinuxDeviceCgroup struct { // Allow or deny Allow bool `json:"allow"` @@ -628,6 +631,7 @@ const ( ArchS390X Arch = "SCMP_ARCH_S390X" ArchPARISC Arch = "SCMP_ARCH_PARISC" ArchPARISC64 Arch = "SCMP_ARCH_PARISC64" + ArchRISCV64 Arch = "SCMP_ARCH_RISCV64" ) // LinuxSeccompAction taken upon Seccomp rule match diff --git a/vendor/modules.txt b/vendor/modules.txt index 5e18a8e63..6b35185c0 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -38,7 +38,7 @@ github.com/moby/sys/mountinfo # github.com/mrunalp/fileutils v0.5.0 ## explicit github.com/mrunalp/fileutils -# github.com/opencontainers/runtime-spec v1.0.3-0.20200728170252-4d89ac9fbff6 +# github.com/opencontainers/runtime-spec v1.0.3-0.20200817204227-f9c09b4ea1df ## explicit github.com/opencontainers/runtime-spec/specs-go # github.com/opencontainers/selinux v1.6.0 From 74b57fea547af67a176916c8c6d4c7cfe33a9587 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Sep 2020 16:41:59 -0700 Subject: [PATCH 2/5] fscommon.WriteFile: simplify error message As the underlying error message from iotuils.WriteFile already contains file name, there's no need to put it, otherwise we end up with something like: failed to write "val" to "/sys/fs/cgroup/.../file": open /sys/fs/cgroup/.../file: permission denied With this patch, the error will be failed to write "val": open /sys/fs/cgroup/.../file: permission denied Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/fscommon.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/cgroups/fscommon/fscommon.go b/libcontainer/cgroups/fscommon/fscommon.go index 46e06dc62..446b9a1f0 100644 --- a/libcontainer/cgroups/fscommon/fscommon.go +++ b/libcontainer/cgroups/fscommon/fscommon.go @@ -21,7 +21,7 @@ func WriteFile(dir, file, data string) error { return err } if err := retryingWriteFile(path, []byte(data), 0700); err != nil { - return errors.Wrapf(err, "failed to write %q to %q", data, path) + return errors.Wrapf(err, "failed to write %q", data) } return nil } From b006f4a18047f5815bb24db3a18b276018f9d0a1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Sep 2020 18:10:30 -0700 Subject: [PATCH 3/5] libct/cgroups: support Cgroups.Resources.Unified Add support for unified resource map (as per [1]), and add some test cases for the new functionality. [1] https://github.com/opencontainers/runtime-spec/pull/1040 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/fs.go | 8 +- libcontainer/cgroups/fs2/fs2.go | 33 ++++++ libcontainer/cgroups/systemd/v1.go | 7 ++ libcontainer/cgroups/v1_utils.go | 3 +- libcontainer/configs/cgroup_linux.go | 3 + libcontainer/integration/exec_test.go | 149 ++++++++++++++++++++++++++ libcontainer/specconv/spec_linux.go | 7 ++ 7 files changed, 208 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 98e1e3426..29690dd62 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -218,7 +218,10 @@ func (m *manager) Apply(pid int) (err error) { m.mu.Lock() defer m.mu.Unlock() - var c = m.cgroups + c := m.cgroups + if c.Resources.Unified != nil { + return cgroups.ErrV1NoUnified + } m.paths = make(map[string]string) if c.Paths != nil { @@ -309,6 +312,9 @@ func (m *manager) Set(container *configs.Config) error { if m.cgroups != nil && m.cgroups.Paths != nil { return nil } + if container.Cgroups.Resources.Unified != nil { + return cgroups.ErrV1NoUnified + } m.mu.Lock() defer m.mu.Unlock() diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 0975064f2..268a630e4 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -3,11 +3,14 @@ package fs2 import ( + "fmt" "io/ioutil" + "os" "path/filepath" "strings" "github.com/opencontainers/runc/libcontainer/cgroups" + "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" "github.com/pkg/errors" ) @@ -206,10 +209,40 @@ func (m *manager) Set(container *configs.Config) error { if err := setFreezer(m.dirPath, container.Cgroups.Freezer); err != nil { return err } + if err := m.setUnified(container.Cgroups.Unified); err != nil { + return err + } m.config = container.Cgroups return nil } +func (m *manager) setUnified(res map[string]string) error { + for k, v := range res { + if strings.Contains(k, "/") { + return fmt.Errorf("unified resource %q must be a file name (no slashes)", k) + } + if err := fscommon.WriteFile(m.dirPath, k, v); err != nil { + errC := errors.Cause(err) + // Check for both EPERM and ENOENT since O_CREAT is used by WriteFile. + if errors.Is(errC, os.ErrPermission) || errors.Is(errC, os.ErrNotExist) { + // Check if a controller is available, + // to give more specific error if not. + sk := strings.SplitN(k, ".", 2) + if len(sk) != 2 { + return fmt.Errorf("unified resource %q must be in the form CONTROLLER.PARAMETER", k) + } + c := sk[0] + if _, ok := m.controllers[c]; !ok && c != "cgroup" { + return fmt.Errorf("unified resource %q can't be set: controller %q not available", k, c) + } + } + return errors.Wrapf(err, "can't set unified resource %q", k) + } + } + + return nil +} + func (m *manager) GetPaths() map[string]string { paths := make(map[string]string, 1) paths[""] = m.dirPath diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index cc2d19cd2..eeda18a90 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -101,6 +101,10 @@ func (m *legacyManager) Apply(pid int) error { properties []systemdDbus.Property ) + if c.Resources.Unified != nil { + return cgroups.ErrV1NoUnified + } + m.mu.Lock() defer m.mu.Unlock() if c.Paths != nil { @@ -342,6 +346,9 @@ func (m *legacyManager) Set(container *configs.Config) error { if m.cgroups.Paths != nil { return nil } + if container.Cgroups.Resources.Unified != nil { + return cgroups.ErrV1NoUnified + } dbusConnection, err := getDbusConnection(false) if err != nil { return err diff --git a/libcontainer/cgroups/v1_utils.go b/libcontainer/cgroups/v1_utils.go index a94f20861..8b9275fb9 100644 --- a/libcontainer/cgroups/v1_utils.go +++ b/libcontainer/cgroups/v1_utils.go @@ -23,7 +23,8 @@ const ( ) var ( - errUnified = errors.New("not implemented for cgroup v2 unified hierarchy") + errUnified = errors.New("not implemented for cgroup v2 unified hierarchy") + ErrV1NoUnified = errors.New("invalid configuration: cannot use unified on cgroup v1") ) type NotFoundError struct { diff --git a/libcontainer/configs/cgroup_linux.go b/libcontainer/configs/cgroup_linux.go index 6e90ae16b..dcc29c619 100644 --- a/libcontainer/configs/cgroup_linux.go +++ b/libcontainer/configs/cgroup_linux.go @@ -127,6 +127,9 @@ type Resources struct { // CpuWeight sets a proportional bandwidth limit. CpuWeight uint64 `json:"cpu_weight"` + // Unified is cgroupv2-only key-value map. + Unified map[string]string `json:"unified"` + // SkipDevices allows to skip configuring device permissions. // Used by e.g. kubelet while creating a parent cgroup (kubepods) // common for many containers. diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 923c5797c..3fdf04be8 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -705,6 +705,155 @@ func testRunWithKernelMemory(t *testing.T, systemd bool) { } } +func TestCgroupResourcesUnifiedErrorOnV1(t *testing.T) { + testCgroupResourcesUnifiedErrorOnV1(t, false) +} + +func TestCgroupResourcesUnifiedErrorOnV1Systemd(t *testing.T) { + if !systemd.IsRunningSystemd() { + t.Skip("Systemd is unsupported") + } + testCgroupResourcesUnifiedErrorOnV1(t, true) +} + +func testCgroupResourcesUnifiedErrorOnV1(t *testing.T, systemd bool) { + if testing.Short() { + return + } + if cgroups.IsCgroup2UnifiedMode() { + t.Skip("requires cgroup v1") + } + rootfs, err := newRootfs() + ok(t, err) + defer remove(rootfs) + + config := newTemplateConfig(rootfs) + if systemd { + config.Cgroups.Parent = "system.slice" + } + config.Cgroups.Resources.Unified = map[string]string{ + "memory.min": "10240", + } + _, _, err = runContainer(config, "", "true") + if !strings.Contains(err.Error(), cgroups.ErrV1NoUnified.Error()) { + t.Fatalf("expected error to contain %v, got %v", cgroups.ErrV1NoUnified, err) + } +} + +func TestCgroupResourcesUnified(t *testing.T) { + testCgroupResourcesUnified(t, false) +} + +func TestCgroupResourcesUnifiedSystemd(t *testing.T) { + if !systemd.IsRunningSystemd() { + t.Skip("Systemd is unsupported") + } + testCgroupResourcesUnified(t, true) +} + +func testCgroupResourcesUnified(t *testing.T, systemd bool) { + if testing.Short() { + return + } + if !cgroups.IsCgroup2UnifiedMode() { + t.Skip("requires cgroup v2") + } + rootfs, err := newRootfs() + ok(t, err) + defer remove(rootfs) + + config := newTemplateConfig(rootfs) + config.Cgroups.Resources.Memory = 536870912 // 512M + config.Cgroups.Resources.MemorySwap = 536870912 // 512M, i.e. no swap + config.Namespaces.Add(configs.NEWCGROUP, "") + config.Mounts = append(config.Mounts, &configs.Mount{ + Destination: "/sys/fs/cgroup", + Device: "cgroup", + Flags: defaultMountFlags | unix.MS_RDONLY, + }) + if systemd { + config.Cgroups.Parent = "system.slice" + } + + testCases := []struct { + name string + cfg map[string]string + expError string + cmd []string + exp string + }{ + { + name: "dummy", + cmd: []string{"true"}, + exp: "", + }, + { + name: "set memory.min", + cfg: map[string]string{"memory.min": "131072"}, + cmd: []string{"cat", "/sys/fs/cgroup/memory.min"}, + exp: "131072\n", + }, + { + name: "check memory.max", + cmd: []string{"cat", "/sys/fs/cgroup/memory.max"}, + exp: strconv.Itoa(int(config.Cgroups.Resources.Memory)) + "\n", + }, + + { + name: "overwrite memory.max", + cfg: map[string]string{"memory.max": "268435456"}, + cmd: []string{"cat", "/sys/fs/cgroup/memory.max"}, + exp: "268435456\n", + }, + { + name: "no such controller error", + cfg: map[string]string{"privet.vsem": "vam"}, + expError: "controller \"privet\" not available", + }, + { + name: "slash in key error", + cfg: map[string]string{"bad/key": "val"}, + expError: "must be a file name (no slashes)", + }, + { + name: "no dot in key error", + cfg: map[string]string{"badkey": "val"}, + expError: "must be in the form CONTROLLER.PARAMETER", + }, + { + name: "read-only parameter", + cfg: map[string]string{"pids.current": "42"}, + expError: "failed to write", + }, + } + + for _, tc := range testCases { + config.Cgroups.Resources.Unified = tc.cfg + buffers, ret, err := runContainer(config, "", tc.cmd...) + if tc.expError != "" { + if err == nil { + t.Errorf("case %q failed: expected error, got nil", tc.name) + continue + } + if !strings.Contains(err.Error(), tc.expError) { + t.Errorf("case %q failed: expected error to contain %q, got %q", tc.name, tc.expError, err) + } + continue + } + if err != nil { + t.Errorf("case %q failed: expected no error, got %v (command: %v, status: %d, stderr: %q)", + tc.name, err, tc.cmd, ret, buffers.Stderr.String()) + continue + } + if tc.exp != "" { + out := buffers.Stdout.String() + if out != tc.exp { + t.Errorf("expected %q, got %q", tc.exp, out) + } + } + } +} + func TestContainerState(t *testing.T) { if testing.Short() { return diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 11235acc4..ce74bcf2d 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -619,6 +619,13 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*configs.Device) (*confi }) } } + if len(r.Unified) > 0 { + // copy the map + c.Resources.Unified = make(map[string]string, len(r.Unified)) + for k, v := range r.Unified { + c.Resources.Unified[k] = v + } + } } } From 6e2159be743c662ff722b38f111b6b2c03cba6fd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 16 Sep 2020 11:55:50 -0700 Subject: [PATCH 4/5] tests/int/cgroups: make sure to rm containers Container test_cgroups_group was missing from the teardown. Fixes: c91fe9aebac83bab6b. Signed-off-by: Kir Kolyshkin --- tests/integration/cgroups.bats | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats index 676655ede..de4cf257b 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -6,6 +6,7 @@ function teardown() { rm -f "$BATS_TMPDIR"/runc-cgroups-integration-test.json teardown_running_container test_cgroups_kmem teardown_running_container test_cgroups_permissions + teardown_running_container test_cgroups_group teardown_busybox } From 4929c05ad18ab99686040d8b40a969c3f884717c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 16 Sep 2020 11:52:49 -0700 Subject: [PATCH 5/5] tests/int: add cgroupv2 unified resources tests Note that various invalid keys are covered by test cases in libcontainer/integration/exec_test.go Signed-off-by: Kir Kolyshkin --- tests/integration/cgroups.bats | 62 ++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats index de4cf257b..4c2bfb8e1 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -7,6 +7,7 @@ function teardown() { teardown_running_container test_cgroups_kmem teardown_running_container test_cgroups_permissions teardown_running_container test_cgroups_group + teardown_running_container test_cgroups_unified teardown_busybox } @@ -177,3 +178,64 @@ function setup() { [ "$status" -eq 0 ] # } + +@test "runc run (cgroup v1 + unified resources should fail)" { + requires root cgroups_v1 + + set_cgroups_path "$BUSYBOX_BUNDLE" + set_resources_limit "$BUSYBOX_BUNDLE" + update_config '.linux.resources.unified |= {"memory.min": "131072"}' "$BUSYBOX_BUNDLE" + + runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_unified + [ "$status" -ne 0 ] + [[ "$output" == *'invalid configuration'* ]] +} + +@test "runc run (cgroup v2 resources.unified only)" { + requires root cgroups_v2 + + set_cgroups_path "$BUSYBOX_BUNDLE" + update_config ' .linux.resources.unified |= { + "memory.min": "131072", + "memory.low": "524288", + "memory.high": "5242880", + "memory.max": "10485760", + "pids.max": "99", + "cpu.max": "10000 100000" + }' "$BUSYBOX_BUNDLE" + + runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_unified + [ "$status" -eq 0 ] + + runc exec test_cgroups_unified sh -c 'cd /sys/fs/cgroup && grep . *.min *.max *.low *.high' + [ "$status" -eq 0 ] + echo "$output" + + echo "$output" | grep -q '^memory.min:131072$' + echo "$output" | grep -q '^memory.low:524288$' + echo "$output" | grep -q '^memory.high:5242880$' + echo "$output" | grep -q '^memory.max:10485760$' + echo "$output" | grep -q '^pids.max:99$' + echo "$output" | grep -q '^cpu.max:10000 100000$' +} +@test "runc run (cgroup v2 resources.unified override)" { + requires root cgroups_v2 + + set_cgroups_path "$BUSYBOX_BUNDLE" + update_config ' .linux.resources.memory |= {"limit": 33554432} + | .linux.resources.memorySwap |= {"limit": 33554432} + | .linux.resources.unified |= + {"memory.min": "131072", "memory.max": "10485760" }' \ + "$BUSYBOX_BUNDLE" + + runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_unified + [ "$status" -eq 0 ] + + runc exec test_cgroups_unified cat /sys/fs/cgroup/memory.min + [ "$status" -eq 0 ] + [ "$output" = '131072' ] + + runc exec test_cgroups_unified cat /sys/fs/cgroup/memory.max + [ "$status" -eq 0 ] + [ "$output" = '10485760' ] +}