From 0ed1f8022d99b70309d60870360df9a070bbdfbb Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 16:35:53 -0700 Subject: [PATCH 1/8] tests/int/helpers: rm old code This was added by commit ca4f427af, together with set_resources_limit, and was needed for the latter, since sed was used to update resources. Now when we switched to jq in commit 79fe41d3c16b, this kludge is no longer needed. Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index d265f667ab6..c5148f34815 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -68,11 +68,6 @@ function runc_spec() { if [[ "$ROOTLESS" -ne 0 ]] && [[ "$ROOTLESS_FEATURES" == *"idmap"* ]]; then runc_rootless_idmap "$bundle" fi - - # Ensure config.json contains linux.resources - if [[ "$ROOTLESS" -ne 0 ]] && [[ "$ROOTLESS_FEATURES" == *"cgroup"* ]]; then - runc_rootless_cgroup "$bundle" - fi } # Helper function to reformat config.json file. Input uses jq syntax. @@ -92,12 +87,6 @@ function runc_rootless_idmap() { | .linux.gidMappings += [{"hostID": '"$(($ROOTLESS_GIDMAP_START + 100))"', "containerID": 1000, "size": '"$(($ROOTLESS_GIDMAP_LENGTH - 1000))"'}]' $bundle } -# Shortcut to add empty resources as part of a rootless configuration. -function runc_rootless_cgroup() { - bundle="${1:-.}" - update_config '.linux.resources += {"memory":{},"cpu":{},"blockio":{},"pids":{}}' $bundle -} - # Returns systemd version as a number (-1 if systemd is not enabled/supported). function systemd_version() { if [ -n "${RUNC_USE_SYSTEMD}" ]; then From 4f8ccc5ff5828ec5e68eebc4ce747742f572e73c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 15:45:36 -0700 Subject: [PATCH 2/8] libct/cg/sd/v2: call initPath from Path Sometimes Path() is called before m.path is initialized (in particular, this happens from (*linuxContainer).newInitConfig), so we do need to make sure to call initPath. This fixes the following integration tests (for cgroup v2 + systemd case, currently not enabled -- to be enabled by further commits): * runc run (blkio weight) * runc run (cgroupv2 mount inside container) Fixes: ff692f289b6 ("Fix cgroup2 mount for rootless case") Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/v2.go | 1 + 1 file changed, 1 insertion(+) diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 99f3e450e93..8abb0feb748 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -317,6 +317,7 @@ func (m *unifiedManager) Destroy() error { } func (m *unifiedManager) Path(_ string) string { + _ = m.initPath() return m.path } From 353f2ad193ec36f831f921e69168519d8079a03a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 18:18:41 -0700 Subject: [PATCH 3/8] tests/int/update.bats: don't set cpuset in setup Setting cpuset.cpus requires cpuset controller, which might not be available in case of cgroup v2 + systemd < 244. Rather than require cpuset support from every test case, do not set the initial cpuset value from setup(), and do not check it. Signed-off-by: Kir Kolyshkin --- tests/integration/update.bats | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 149da63cff9..9e4d702f06d 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -14,7 +14,7 @@ function setup() { # Set some initial known values update_config ' .linux.resources.memory |= {"limit": 33554432, "reservation": 25165824} - | .linux.resources.cpu |= {"shares": 100, "quota": 500000, "period": 1000000, "cpus": "0"} + | .linux.resources.cpu |= {"shares": 100, "quota": 500000, "period": 1000000} | .linux.resources.pids |= {"limit": 20}' } @@ -70,7 +70,6 @@ function setup() { fi # check that initial values were properly set - check_cgroup_value "cpuset.cpus" 0 if [[ "$CGROUP_UNIFIED" = "yes" ]] && ! grep -qw memory "$CGROUP_PATH/cgroup.controllers"; then # This happen on containerized environment because "echo +memory > /sys/fs/cgroup/cgroup.subtree_control" fails with EINVAL skip "memory controller not available" From 44fcbfd68f4064d453bb08c86caba19f6b2e8ad0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 17:32:27 -0700 Subject: [PATCH 4/8] tests/int/helpers: generalize require cgroups_freezer With this, something like cgroups_pids can be used (to be added by the next commit). Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index c5148f34815..d099acae227 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -304,12 +304,6 @@ function requires() { skip_me=1 fi ;; - cgroups_freezer) - init_cgroup_paths - if [[ "$CGROUP_SUBSYSTEMS" != *"freezer"* ]]; then - skip_me=1 - fi - ;; cgroups_rt) init_cgroup_paths if [ ! -e "${CGROUP_CPU_BASE_PATH}/cpu.rt_period_us" ]; then @@ -339,6 +333,13 @@ function requires() { skip_me=1 fi ;; + cgroups_*) + init_cgroup_paths + var=${var#cgroups_} + if [[ "$CGROUP_SUBSYSTEMS" != *"$var"* ]]; then + skip_me=1 + fi + ;; smp) local cpu_count=$(grep -c '^processor' /proc/cpuinfo) if [ "$cpu_count" -lt 2 ]; then From 40b97919163494223ef72c7e7e161ef8e69e6094 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 18:21:57 -0700 Subject: [PATCH 5/8] tests/int: enable/use requires cgroups_ 1. In case of cgroup v2 + systemd + rootless, get the list of controllers from the own cgroup, rather than from the root one (as systemd cgroup delegation is required in this case, and it might not be set or fully working). 2. Use "requires cgroups_" in tests that need those controllers. Tested on Fedora 31 (which does not support delegation of cpuset controller). Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 10 +++++++++- tests/integration/update.bats | 20 ++++---------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index d099acae227..236f8652836 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -103,13 +103,21 @@ function init_cgroup_paths() { if stat -f -c %t /sys/fs/cgroup | grep -qFw 63677270; then CGROUP_UNIFIED=yes + local controllers="/sys/fs/cgroup/cgroup.controllers" + # For rootless + systemd case, controllers delegation is required, + # so check the controllers that the current user has, not the top one. + # NOTE: delegation of cpuset requires systemd >= 244 (Fedora >= 32, Ubuntu >= 20.04). + if [[ "$ROOTLESS" -ne 0 && -n "$RUNC_USE_SYSTEMD" ]]; then + controllers="/sys/fs/cgroup/user.slice/user-$(id -u).slice/user@$(id -u).service/cgroup.controllers" + fi + # "pseudo" controllers do not appear in /sys/fs/cgroup/cgroup.controllers. # - devices (since kernel 4.15) we must assume to be supported because # it's quite hard to test. # - freezer (since kernel 5.2) we can auto-detect by looking for the # "cgroup.freeze" file a *non-root* cgroup. CGROUP_SUBSYSTEMS=$( - cat /sys/fs/cgroup/cgroup.controllers + cat "$controllers" echo devices ) CGROUP_BASE_PATH=/sys/fs/cgroup diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 9e4d702f06d..bc9005663d3 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -22,15 +22,7 @@ function setup() { # v1 and v2: memory/swap, pids, and cpuset. @test "update cgroup v1/v2 common limits" { [[ "$ROOTLESS" -ne 0 && -z "$RUNC_USE_SYSTEMD" ]] && requires rootless_cgroup - if [[ "$ROOTLESS" -ne 0 && -n "$RUNC_USE_SYSTEMD" ]]; then - file="/sys/fs/cgroup/user.slice/user-$(id -u).slice/user@$(id -u).service/cgroup.controllers" - # NOTE: delegation of cpuset requires systemd >= 244 (Fedora >= 32, Ubuntu >= 20.04). - for f in memory pids cpuset; do - if grep -qwv $f "$file"; then - skip "$f is not enabled in $file" - fi - done - fi + requires cgroups_memory cgroups_pids cgroups_cpuset init_cgroup_paths # run a few busyboxes detached @@ -70,10 +62,6 @@ function setup() { fi # check that initial values were properly set - if [[ "$CGROUP_UNIFIED" = "yes" ]] && ! grep -qw memory "$CGROUP_PATH/cgroup.controllers"; then - # This happen on containerized environment because "echo +memory > /sys/fs/cgroup/cgroup.subtree_control" fails with EINVAL - skip "memory controller not available" - fi check_cgroup_value $MEM_LIMIT 33554432 check_systemd_value $SD_MEM_LIMIT 33554432 @@ -83,7 +71,7 @@ function setup() { check_cgroup_value "pids.max" 20 check_systemd_value "TasksMax" 20 - # update cpuset if supported (i.e. we're running on a multicore cpu) + # update cpuset if possible (i.e. we're running on a multicore cpu) cpu_count=$(grep -c '^processor' /proc/cpuinfo) if [ "$cpu_count" -gt 1 ]; then runc update test_update --cpuset-cpus "1" @@ -426,7 +414,7 @@ EOF @test "update cpuset parameters via resources.CPU" { [[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup - requires smp + requires smp cgroups_cpuset local AllowedCPUs='AllowedCPUs' AllowedMemoryNodes='AllowedMemoryNodes' # these properties require systemd >= v244 @@ -482,7 +470,7 @@ EOF @test "update cpuset parameters via v2 unified map" { # This test assumes systemd >= v244 [[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup - requires cgroups_v2 smp + requires cgroups_v2 smp cgroups_cpuset update_config ' .linux.resources.unified |= { "cpuset.cpus": "0", From 601cf5825f6cf7cd4c360497ac609540a8fa5cf3 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 6 May 2021 19:55:02 -0700 Subject: [PATCH 6/8] tests/int/cgroups: don't check for hugetlb Systemd is not able to delegate hugetlb controller, and it is needed for cgroup v2 + systemd + rootless case (which is currently skipped because of "requires rootless_cgroup", but will be enabled by a later commit). The failure being fixed looks like this: > not ok 4 runc create (limits + cgrouppath + permission on the cgroup dir) succeeds > # (from function `check_cgroup_value' in file /vagrant/tests/integration/helpers.bash, line 188, > # in test file /vagrant/tests/integration/cgroups.bats, line 53) > # `check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers)"' failed > # <....> > # /sys/fs/cgroup/user.slice/user-2000.slice/user@2000.service/machine.slice/runc-cgroups-integration-test-20341.scope/cgroup.controllers > # current cpuset cpu io memory pids !? cpuset cpu io memory hugetlb pids Signed-off-by: Kir Kolyshkin --- tests/integration/cgroups.bats | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats index 50c4f9bb8ee..2bc94074d01 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -55,8 +55,8 @@ function setup() { if [ "$(id -u)" = "0" ]; then check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/machine.slice/cgroup.controllers)" else - # shellcheck disable=SC2046 - check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers)" + # Filter out hugetlb as systemd is unable to delegate it. + check_cgroup_value "cgroup.controllers" "$(sed 's/ hugetlb//' Date: Thu, 6 May 2021 13:18:09 -0700 Subject: [PATCH 7/8] tests/int: run rootless_cgroup tests for v2+systemd Before this commit, "require rootless_cgroup" feature check required "cgroup" to be present in ROOTLESS_FEATURES. The idea of the requirement, though, is to ensure that rootless runc can manage cgroups. In case of systemd + cgroup v2, rootless runc can manage cgroups, thanks to systemd delegation, so modify the feature check accordingly. Next, convert (simplify) some of the existing users to the modified check. Signed-off-by: Kir Kolyshkin --- tests/integration/cgroups.bats | 10 ++-------- tests/integration/helpers.bash | 9 +++++++-- tests/integration/update.bats | 2 +- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/integration/cgroups.bats b/tests/integration/cgroups.bats index 2bc94074d01..5875469c68c 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -16,10 +16,7 @@ function setup() { } @test "runc create (rootless + no limits + cgrouppath + no permission) fails with permission error" { - requires rootless - requires rootless_no_cgroup - # systemd controls the permission, so error does not happen - requires no_systemd + requires rootless rootless_no_cgroup set_cgroups_path @@ -29,10 +26,7 @@ function setup() { } @test "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" { - requires rootless - requires rootless_no_cgroup - # systemd controls the permission, so error does not happen - requires no_systemd + requires rootless rootless_no_cgroup set_resources_limit diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index 236f8652836..b92c4169a4f 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -271,6 +271,11 @@ function fail() { exit 1 } +# Check whether rootless runc can use cgroups. +function rootless_cgroup() { + [[ "$ROOTLESS_FEATURES" == *"cgroup"* || -n "$RUNC_USE_SYSTEMD" ]] +} + # Allows a test to specify what things it requires. If the environment can't # support it, the test is skipped with a message. function requires() { @@ -298,12 +303,12 @@ function requires() { fi ;; rootless_cgroup) - if [[ "$ROOTLESS_FEATURES" != *"cgroup"* ]]; then + if ! rootless_cgroup; then skip_me=1 fi ;; rootless_no_cgroup) - if [[ "$ROOTLESS_FEATURES" == *"cgroup"* ]]; then + if rootless_cgroup; then skip_me=1 fi ;; diff --git a/tests/integration/update.bats b/tests/integration/update.bats index bc9005663d3..e204ea49550 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -21,7 +21,7 @@ function setup() { # Tests whatever limits are (more or less) common between cgroup # v1 and v2: memory/swap, pids, and cpuset. @test "update cgroup v1/v2 common limits" { - [[ "$ROOTLESS" -ne 0 && -z "$RUNC_USE_SYSTEMD" ]] && requires rootless_cgroup + [[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup requires cgroups_memory cgroups_pids cgroups_cpuset init_cgroup_paths From 12e9cac902726c8deb03511488d81d7f2a6a8a06 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 7 May 2021 00:14:19 -0700 Subject: [PATCH 8/8] Vagrantfile.fedora: set Delegate=yes Instead of listing all individual controllers we want to be delegated, just say "yes" which means "all the controllers that are available". Signed-off-by: Kir Kolyshkin --- Vagrantfile.fedora34 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Vagrantfile.fedora34 b/Vagrantfile.fedora34 index 449d3b3f279..2c1f049a518 100644 --- a/Vagrantfile.fedora34 +++ b/Vagrantfile.fedora34 @@ -41,8 +41,8 @@ EOF cat > /etc/systemd/system/user@.service.d/delegate.conf << EOF [Service] # default: Delegate=pids memory -# NOTE: delegation of cpuset requires systemd >= 244 (Fedora >= 32, Ubuntu >= 20.04). cpuset is ignored on Fedora 31. -Delegate=cpu cpuset io memory pids +# NOTE: delegation of cpuset requires systemd >= 244 (Fedora >= 32, Ubuntu >= 20.04). +Delegate=yes EOF systemctl daemon-reload SHELL