From 4fc9fa05dad959fc260b4b5f98c202878ea354c0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 19 May 2020 19:19:59 -0700 Subject: [PATCH 1/6] tests/int: simplify check_systemd_value use ...so it will be easier to write more tests Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 21 ++++++++++----------- tests/integration/update.bats | 22 +++++----------------- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index edbe8fb0..298d22df 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -110,11 +110,12 @@ function init_cgroup_paths() { test -n "$CGROUP_UNIFIED" && return if [ -n "${RUNC_USE_SYSTEMD}" ] ; then + SD_UNIT_NAME="runc-cgroups-integration-test.scope" if [ $(id -u) = "0" ]; then - REL_CGROUPS_PATH="/machine.slice/runc-cgroups-integration-test.scope" + REL_CGROUPS_PATH="/machine.slice/$SD_UNIT_NAME" OCI_CGROUPS_PATH="machine.slice:runc-cgroups:integration-test" else - REL_CGROUPS_PATH="/user.slice/user-$(id -u).slice/user@$(id -u).service/machine.slice/runc-cgroups-integration-test.scope" + REL_CGROUPS_PATH="/user.slice/user-$(id -u).slice/user@$(id -u).service/machine.slice/$SD_UNIT_NAME" # OCI path doesn't contain "/user.slice/user-$(id -u).slice/user@$(id -u).service/" prefix OCI_CGROUPS_PATH="machine.slice:runc-cgroups:integration-test" fi @@ -178,16 +179,14 @@ function check_cgroup_value() { # Helper to check a value in systemd. function check_systemd_value() { - unitname=$1 - source=$2 - expected=$3 + [ -z "${RUNC_USE_SYSTEMD}" ] && return + source=$1 + expected="$2" + user="" + [ $(id -u) != "0" ] && user="--user" - if [ $(id -u) = "0" ]; then - current=$(systemctl show $unitname | grep $source) - else - current=$(systemctl --user show $unitname | grep $source) - fi - echo "current" $current "!?" "$expected" + current=$(systemctl show $user --property $source $SD_UNIT_NAME | awk -F= '{print $2}') + echo "systemd $source: current $current !? $expected" [ "$current" = "$expected" ] } diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 0bb6597e..062cc61f 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -63,6 +63,7 @@ EOF case $CGROUP_UNIFIED in no) MEM_LIMIT="memory.limit_in_bytes" + SD_MEM_LIMIT="MemoryLimit" MEM_RESERVE="memory.soft_limit_in_bytes" MEM_SWAP="memory.memsw.limit_in_bytes" SYSTEM_MEM=$(cat "${CGROUP_MEMORY_BASE_PATH}/${MEM_LIMIT}") @@ -70,6 +71,7 @@ EOF ;; yes) MEM_LIMIT="memory.max" + SD_MEM_LIMIT="MemoryMax" MEM_RESERVE="memory.low" MEM_SWAP="memory.swap.max" SYSTEM_MEM="max" @@ -101,24 +103,12 @@ EOF runc update test_update --memory 67108864 [ "$status" -eq 0 ] check_cgroup_value $MEM_LIMIT 67108864 - if [[ -n "${RUNC_USE_SYSTEMD}" ]] ; then - if [ "$CGROUP_UNIFIED" != "yes" ]; then - check_systemd_value "runc-cgroups-integration-test.scope" "MemoryLimit=" "MemoryLimit=67108864" - else - check_systemd_value "runc-cgroups-integration-test.scope" "MemoryMax=" "MemoryMax=67108864" - fi - fi + check_systemd_value $SD_MEM_LIMIT 67108864 runc update test_update --memory 50M [ "$status" -eq 0 ] check_cgroup_value $MEM_LIMIT 52428800 - if [[ -n "${RUNC_USE_SYSTEMD}" ]] ; then - if [ "$CGROUP_UNIFIED" != "yes" ]; then - check_systemd_value "runc-cgroups-integration-test.scope" "MemoryLimit=" "MemoryLimit=52428800" - else - check_systemd_value "runc-cgroups-integration-test.scope" "MemoryMax=" "MemoryMax=52428800" - fi - fi + check_systemd_value $SD_MEM_LIMIT 52428800 # update memory soft limit runc update test_update --memory-reservation 33554432 @@ -154,9 +144,7 @@ EOF runc update test_update --pids-limit 10 [ "$status" -eq 0 ] check_cgroup_value "pids.max" 10 - if [[ -n "${RUNC_USE_SYSTEMD}" ]] ; then - check_systemd_value "runc-cgroups-integration-test.scope" "TasksMax=" "TasksMax=10" - fi + check_systemd_value "TasksMax" 10 # Revert to the test initial value via json on stdin runc update -r - test_update < Date: Wed, 20 May 2020 00:55:11 -0700 Subject: [PATCH 2/6] cgroupv2+systemd: set MemoryLow For some reason, this was not set before. Test case is added by the next commit. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/v2.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index 59d7d899..b929e880 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -53,6 +53,10 @@ func genV2ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) properties = append(properties, newProp("MemoryMax", uint64(c.Resources.Memory))) } + if c.Resources.MemoryReservation != 0 { + properties = append(properties, + newProp("MemoryLow", uint64(c.Resources.MemoryReservation))) + } // swap is set if c.Resources.MemorySwap != 0 { swap, err := cgroups.ConvertMemorySwapToCgroupV2Value(c.Resources.MemorySwap, c.Resources.Memory) From 7abd93d1565a0db942249c44ddc65916e9d2e74e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 19 May 2020 19:45:25 -0700 Subject: [PATCH 3/6] tests/integration/update.bats: more systemd checks 1. add missing checks for systemd's MemoryMax / MemoryLimit. 2. add checks for systemd's MemoryLow and MemorySwapMax. Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 1 + tests/integration/update.bats | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index 298d22df..c712b2d0 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -181,6 +181,7 @@ function check_cgroup_value() { function check_systemd_value() { [ -z "${RUNC_USE_SYSTEMD}" ] && return source=$1 + [ "$source" = "unsupported" ] && return expected="$2" user="" [ $(id -u) != "0" ] && user="--user" diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 062cc61f..fab94abb 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -65,7 +65,9 @@ EOF MEM_LIMIT="memory.limit_in_bytes" SD_MEM_LIMIT="MemoryLimit" MEM_RESERVE="memory.soft_limit_in_bytes" + SD_MEM_RESERVE="unsupported" MEM_SWAP="memory.memsw.limit_in_bytes" + SD_MEM_SWAP="unsupported" SYSTEM_MEM=$(cat "${CGROUP_MEMORY_BASE_PATH}/${MEM_LIMIT}") SYSTEM_MEM_SWAP=$(cat "${CGROUP_MEMORY_BASE_PATH}/$MEM_SWAP") ;; @@ -73,13 +75,16 @@ EOF MEM_LIMIT="memory.max" SD_MEM_LIMIT="MemoryMax" MEM_RESERVE="memory.low" + SD_MEM_RESERVE="MemoryLow" MEM_SWAP="memory.swap.max" + SD_MEM_SWAP="MemorySwapMax" SYSTEM_MEM="max" SYSTEM_MEM_SWAP="max" # checking swap is currently disabled for v2 #CGROUP_MEMORY=$CGROUP_PATH ;; esac + SD_UNLIMITED="infinity" # check that initial values were properly set check_cgroup_value "cpuset.cpus" 0 @@ -88,8 +93,13 @@ EOF skip "memory controller not available" fi check_cgroup_value $MEM_LIMIT 33554432 + check_systemd_value $SD_MEM_LIMIT 33554432 + check_cgroup_value $MEM_RESERVE 25165824 + check_systemd_value $SD_MEM_RESERVE 25165824 + check_cgroup_value "pids.max" 20 + check_systemd_value "TasksMax" 20 # update cpuset if supported (i.e. we're running on a multicore cpu) cpu_count=$(grep -c '^processor' /proc/cpuinfo) @@ -114,6 +124,7 @@ EOF runc update test_update --memory-reservation 33554432 [ "$status" -eq 0 ] check_cgroup_value "$MEM_RESERVE" 33554432 + check_systemd_value "$SD_MEM_RESERVE" 33554432 # Run swap memory tests if swap is available if [ -f "$CGROUP_MEMORY/$MEM_SWAP" ]; then @@ -121,11 +132,13 @@ EOF runc update test_update --memory-swap -1 [ "$status" -eq 0 ] check_cgroup_value "$MEM_SWAP" $SYSTEM_MEM_SWAP + check_systemd_value "$SD_MEM_SWAP" $SD_UNLIMITED # update memory swap runc update test_update --memory-swap 96468992 [ "$status" -eq 0 ] check_cgroup_value "$MEM_SWAP" 96468992 + check_systemd_value "$SD_MEM_SWAP" 96468992 fi # try to remove memory limit @@ -134,10 +147,12 @@ EOF # check memory limit is gone check_cgroup_value $MEM_LIMIT $SYSTEM_MEM + check_systemd_value $SD_MEM_LIMIT $SD_UNLIMITED # check swap memory limited is gone if [ -f "$CGROUP_MEMORY/$MEM_SWAP" ]; then check_cgroup_value $MEM_SWAP $SYSTEM_MEM + check_systemd_value "$SD_MEM_SWAP" $SD_UNLIMITED fi # update pids limit @@ -166,9 +181,15 @@ EOF EOF [ "$status" -eq 0 ] check_cgroup_value "cpuset.cpus" 0 + check_cgroup_value $MEM_LIMIT 33554432 + check_systemd_value $SD_MEM_LIMIT 33554432 + check_cgroup_value $MEM_RESERVE 25165824 + check_systemd_value $SD_MEM_RESERVE 25165824 + check_cgroup_value "pids.max" 20 + check_systemd_value "TasksMax" 20 # redo all the changes at once runc update test_update \ @@ -177,8 +198,13 @@ EOF --pids-limit 10 [ "$status" -eq 0 ] check_cgroup_value $MEM_LIMIT 67108864 + check_systemd_value $SD_MEM_LIMIT 67108864 + check_cgroup_value $MEM_RESERVE 33554432 + check_systemd_value $SD_MEM_RESERVE 33554432 + check_cgroup_value "pids.max" 10 + check_systemd_value "TasksMax" 10 # reset to initial test value via json file cat << EOF > $BATS_TMPDIR/runc-cgroups-integration-test.json @@ -202,9 +228,15 @@ EOF runc update -r $BATS_TMPDIR/runc-cgroups-integration-test.json test_update [ "$status" -eq 0 ] check_cgroup_value "cpuset.cpus" 0 + check_cgroup_value $MEM_LIMIT 33554432 + check_systemd_value $SD_MEM_LIMIT 33554432 + check_cgroup_value $MEM_RESERVE 25165824 + check_systemd_value $SD_MEM_RESERVE 25165824 + check_cgroup_value "pids.max" 20 + check_systemd_value "TasksMax" 20 } @test "update cgroup v1 cpu limits" { From 06d7c1d2616052974ebb39a403a31e4ff95b9a20 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 20 May 2020 01:37:03 -0700 Subject: [PATCH 4/6] systemd+cgroupv1: fix updating CPUQuotaPerSecUSec 1. do not allow to set quota without period or period without quota, as we won't be able to calculate new value for CPUQuotaPerSecUSec otherwise. 2. do not ignore setting quota to -1 when a period is not set. 3. update the test case accordingly. Note that systemd value checks will be added in the next commit. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/v1.go | 14 +++++++++++++- tests/integration/update.bats | 25 +++++++++++++++++-------- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index d5bc1713..a805f72c 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -4,6 +4,7 @@ package systemd import ( "errors" + "fmt" "io/ioutil" "math" "os" @@ -89,7 +90,18 @@ func genV1ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) } // cpu.cfs_quota_us and cpu.cfs_period_us are controlled by systemd. - if c.Resources.CpuQuota != 0 && c.Resources.CpuPeriod != 0 { + if c.Resources.CpuQuota != 0 || c.Resources.CpuPeriod != 0 { + if c.Resources.CpuQuota < -1 { + return nil, fmt.Errorf("Invalid CPU quota value: %d", c.Resources.CpuQuota) + } + if c.Resources.CpuQuota != -1 { + if c.Resources.CpuQuota == 0 || c.Resources.CpuPeriod == 0 { + return nil, errors.New("CPU quota and period should both be set") + } + if c.Resources.CpuPeriod < 0 { + return nil, fmt.Errorf("Invalid CPU period value: %d", c.Resources.CpuPeriod) + } + } // corresponds to USEC_INFINITY in systemd // if USEC_INFINITY is provided, CPUQuota is left unbound by systemd // always setting a property value ensures we can apply a quota and remove it later diff --git a/tests/integration/update.bats b/tests/integration/update.bats index fab94abb..26594174 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -252,15 +252,24 @@ EOF check_cgroup_value "cpu.cfs_quota_us" 500000 check_cgroup_value "cpu.shares" 100 - # update cpu-period - runc update test_update --cpu-period 900000 - [ "$status" -eq 0 ] - check_cgroup_value "cpu.cfs_period_us" 900000 + # systemd driver does not allow to update quota and period separately + if [ -z "$RUNC_USE_SYSTEMD" ]; then + # update cpu period + runc update test_update --cpu-period 900000 + [ "$status" -eq 0 ] + check_cgroup_value "cpu.cfs_period_us" 900000 - # update cpu-quota - runc update test_update --cpu-quota 600000 - [ "$status" -eq 0 ] - check_cgroup_value "cpu.cfs_quota_us" 600000 + # update cpu quota + runc update test_update --cpu-quota 600000 + [ "$status" -eq 0 ] + check_cgroup_value "cpu.cfs_quota_us" 600000 + else + # update cpu quota and period together + runc update test_update --cpu-period 900000 --cpu-quota 600000 + [ "$status" -eq 0 ] + check_cgroup_value "cpu.cfs_period_us" 900000 + check_cgroup_value "cpu.cfs_quota_us" 600000 + fi # update cpu-shares runc update test_update --cpu-share 200 From 95413ecdb09488595103bd61779a839c3e399c46 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 19 May 2020 21:41:41 -0700 Subject: [PATCH 5/6] tests/int/update: add cgroupv1 systemd CPU checks Signed-off-by: Kir Kolyshkin --- tests/integration/update.bats | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/integration/update.bats b/tests/integration/update.bats index 26594174..ca47f850 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -250,7 +250,10 @@ EOF # check that initial values were properly set check_cgroup_value "cpu.cfs_period_us" 1000000 check_cgroup_value "cpu.cfs_quota_us" 500000 + check_systemd_value "CPUQuotaPerSecUSec" 500ms + check_cgroup_value "cpu.shares" 100 + check_systemd_value "CPUShares" 100 # systemd driver does not allow to update quota and period separately if [ -z "$RUNC_USE_SYSTEMD" ]; then @@ -269,12 +272,14 @@ EOF [ "$status" -eq 0 ] check_cgroup_value "cpu.cfs_period_us" 900000 check_cgroup_value "cpu.cfs_quota_us" 600000 + check_systemd_value "CPUQuotaPerSecUSec" 670ms fi # update cpu-shares runc update test_update --cpu-share 200 [ "$status" -eq 0 ] check_cgroup_value "cpu.shares" 200 + check_systemd_value "CPUShares" 200 # Revert to the test initial value via json on stding runc update -r - test_update < $BATS_TMPDIR/runc-cgroups-integration-test.json @@ -314,7 +325,10 @@ EOF [ "$status" -eq 0 ] check_cgroup_value "cpu.cfs_period_us" 1000000 check_cgroup_value "cpu.cfs_quota_us" 500000 + check_systemd_value "CPUQuotaPerSecUSec" 500ms + check_cgroup_value "cpu.shares" 100 + check_systemd_value "CPUShares" 100 } @test "update rt period and runtime" { From 59897367c4685b58b80e02ced0f66d4de8f7e9ea Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 4 May 2020 19:19:46 -0700 Subject: [PATCH 6/6] cgroups/systemd: allow to set -1 as pids.limit Currently, both systemd cgroup drivers (v1 and v2) only set "TasksMax" unit property if the value > 0, so there is no way to update the limit to -1 / unlimited / infinity / max. Since systemd driver is backed by fs driver, and both fs and fs2 set the limit of -1 properly, it works, but systemd still has the old value: # runc --systemd-cgroup update $CT --pids-limit 42 # systemctl show runc-$CT.scope | grep TasksMax TasksMax=42 # cat /sys/fs/cgroup/system.slice/runc-$CT.scope/pids.max 42 # ./runc --systemd-cgroup update $CT --pids-limit -1 # systemctl show runc-$CT.scope | grep TasksMax= TasksMax=42 # cat /sys/fs/cgroup/system.slice/runc-xx77.scope/pids.max max Fix by changing the condition to allow -1 as a valid value. NOTE other negative values are still being ignored by systemd drivers (as it was done before). I am not sure whether this is correct, or should we return an error. A test case is added. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/systemd/v1.go | 2 +- libcontainer/cgroups/systemd/v2.go | 2 +- tests/integration/update.bats | 6 ++++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index a805f72c..5d324133 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -125,7 +125,7 @@ func genV1ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) newProp("BlockIOWeight", uint64(c.Resources.BlkioWeight))) } - if c.Resources.PidsLimit > 0 { + if c.Resources.PidsLimit > 0 || c.Resources.PidsLimit == -1 { properties = append(properties, newProp("TasksAccounting", true), newProp("TasksMax", uint64(c.Resources.PidsLimit))) diff --git a/libcontainer/cgroups/systemd/v2.go b/libcontainer/cgroups/systemd/v2.go index b929e880..2fcf312a 100644 --- a/libcontainer/cgroups/systemd/v2.go +++ b/libcontainer/cgroups/systemd/v2.go @@ -92,7 +92,7 @@ func genV2ResourcesProperties(c *configs.Cgroup) ([]systemdDbus.Property, error) newProp("CPUQuotaPerSecUSec", cpuQuotaPerSecUSec)) } - if c.Resources.PidsLimit > 0 { + if c.Resources.PidsLimit > 0 || c.Resources.PidsLimit == -1 { properties = append(properties, newProp("TasksAccounting", true), newProp("TasksMax", uint64(c.Resources.PidsLimit))) diff --git a/tests/integration/update.bats b/tests/integration/update.bats index ca47f850..76d931f5 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -161,6 +161,12 @@ EOF check_cgroup_value "pids.max" 10 check_systemd_value "TasksMax" 10 + # unlimited + runc update test_update --pids-limit -1 + [ "$status" -eq 0 ] + check_cgroup_value "pids.max" max + check_systemd_value "TasksMax" $SD_UNLIMITED + # Revert to the test initial value via json on stdin runc update -r - test_update <