From f09a3e1b8db33768d23916b9b1661300f1e40088 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 19:28:24 -0700 Subject: [PATCH 1/7] tests/int: don't set/use CGROUP_XXX variables Helper function init_cgroup_paths sets two sets of cgroup path variables for cgroup v1 case (below XXX is cgroup controller name, e.g. MEMORY): 1. CGROUP_XXX_BASE_PATH -- path to XXX controller mount point (e.g. CGROUP_MEMORY_BASE_PATH=/sys/fs/cgroup/memory); 2. CGROUP_XXX -- path to the particular container XXX controller cgroup (e.g. CGROUP_MEMORY=/sys/fs/cgroup/memory/runc-cgroups-integration-test/test-cgroup). The second set of variables is mostly used by check_cgroup_value(), with only two exceptions: - CGROUP_CPU in @test "update rt period and runtime"; - few CGROUP_XXX in @test "runc delete --force in cgroupv1 with subcgroups". Remove these variables, as their values are not used much and are easy to get (as can be seen in modified test cases). While at it, mark some variables as local. Signed-off-by: Kir Kolyshkin --- tests/integration/delete.bats | 4 ++-- tests/integration/helpers.bash | 12 +++++++----- tests/integration/update.bats | 10 ++++++---- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/integration/delete.bats b/tests/integration/delete.bats index ccb9108c9..9a6d7e184 100644 --- a/tests/integration/delete.bats +++ b/tests/integration/delete.bats @@ -85,8 +85,8 @@ EOF [[ "$output" =~ [0-9]+ ]] for s in ${subsystems}; do - name=CGROUP_${s^^} - eval path=\$"${name}"/foo + name=CGROUP_${s^^}_BASE_PATH + eval path=\$"${name}${REL_CGROUPS_PATH}/foo" # shellcheck disable=SC2154 [ -d "${path}" ] || fail "test failed to create memory sub-cgroup ($path not found)" done diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index a35ec62d7..6fcb122b3 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -149,11 +149,11 @@ function init_cgroup_paths() { else CGROUP_UNIFIED=no CGROUP_SUBSYSTEMS=$(awk '!/^#/ {print $1}' /proc/cgroups) + local g base_path for g in ${CGROUP_SUBSYSTEMS}; do base_path=$(gawk '$(NF-2) == "cgroup" && $NF ~ /\<'${g}'\>/ { print $5; exit }' /proc/self/mountinfo) test -z "$base_path" && continue eval CGROUP_${g^^}_BASE_PATH="${base_path}" - eval CGROUP_${g^^}="${base_path}${REL_CGROUPS_PATH}" done fi } @@ -167,14 +167,16 @@ function set_cgroups_path() { # Helper to check a value in cgroups. function check_cgroup_value() { - source=$1 - expected=$2 + local source=$1 + local expected=$2 + local cgroup var current if [ "x$CGROUP_UNIFIED" = "xyes" ]; then cgroup=$CGROUP_PATH else - ctrl=${source%%.*} - eval cgroup=\$CGROUP_${ctrl^^} + var=${source%%.*} # controller name (e.g. memory) + var=CGROUP_${var^^}_BASE_PATH # variable name (e.g. CGROUP_MEMORY_BASE_PATH) + eval cgroup=\$${var}${REL_CGROUPS_PATH} fi current=$(cat $cgroup/$source) diff --git a/tests/integration/update.bats b/tests/integration/update.bats index bea67312d..149da63cf 100644 --- a/tests/integration/update.bats +++ b/tests/integration/update.bats @@ -532,10 +532,12 @@ EOF [[ "$ROOTLESS" -ne 0 ]] && requires rootless_cgroup requires cgroups_v1 cgroups_rt no_systemd - # By default, "${CGROUP_CPU}/cpu.rt_runtime_us" is set to 0, which inhibits + local cgroup_cpu="${CGROUP_CPU_BASE_PATH}/${REL_CGROUPS_PATH}" + + # By default, "${cgroup_cpu}/cpu.rt_runtime_us" is set to 0, which inhibits # setting the container's realtimeRuntime. (#2046) # - # When $CGROUP_CPU is "/sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/test-cgroup", + # When ${cgroup_cpu} is "/sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/test-cgroup", # we write the values of /sys/fs/cgroup/cpu,cpuacct/cpu.rt_{period,runtime}_us to: # - sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/cpu.rt_{period,runtime}_us # - sys/fs/cgroup/cpu,cpuacct/runc-cgroups-integration-test/test-cgroup/cpu.rt_{period,runtime}_us @@ -543,12 +545,12 @@ EOF # Typically period=1000000 runtime=950000 . # # TODO: support systemd - mkdir -p "$CGROUP_CPU" + mkdir -p "$cgroup_cpu" local root_period root_runtime root_period=$(cat "${CGROUP_CPU_BASE_PATH}/cpu.rt_period_us") root_runtime=$(cat "${CGROUP_CPU_BASE_PATH}/cpu.rt_runtime_us") # the following IFS magic sets dirs=("runc-cgroups-integration-test" "test-cgroup") - IFS='/' read -r -a dirs <<<"${CGROUP_CPU//${CGROUP_CPU_BASE_PATH}/}" + IFS='/' read -r -a dirs <<<"$REL_CGROUPS_PATH" for ((i = 0; i < ${#dirs[@]}; i++)); do local target="$CGROUP_CPU_BASE_PATH" for ((j = 0; j <= i; j++)); do From 6e4c5b6e85e6b7898205451760cba8f40a2c5130 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 1 Apr 2021 11:43:56 -0700 Subject: [PATCH 2/7] tests/int/cgroups: don't use BUSYBOX_BUNDLE Commit 41670e21f removed BUSYBOX_BUNDLE env var, but c3ffd2ef81aa was developed before 41670e21f was merged. Everything still works because now BUSYBOX_BUNDLE has no value. Nevertheless, let's remove it to avoid confusion. 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 320a3588a..a2d935629 100644 --- a/tests/integration/cgroups.bats +++ b/tests/integration/cgroups.bats @@ -189,8 +189,8 @@ function setup() { @test "runc run (blkio weight)" { requires root cgroups_v2 - set_cgroups_path "$BUSYBOX_BUNDLE" - update_config '.linux.resources.blockIO |= {"weight": 750}' "${BUSYBOX_BUNDLE}" + set_cgroups_path + update_config '.linux.resources.blockIO |= {"weight": 750}' runc run -d --console-socket "$CONSOLE_SOCKET" test_cgroups_unified [ "$status" -eq 0 ] From e63df1e6f06450200e811596330cad435078d6a5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Mar 2021 14:16:16 -0700 Subject: [PATCH 3/7] tests/int: really randomize cgroup/unit names Commit 41670e21f0 added some randomization to cgroup paths and (if systemd cgroup driver is used) systemd unit names, but the randomization was per bats instance, not per test. Fix this by refactoring init_cgroups_path/set_cgroups_path (moving variable/random part to set_cgroups_path). NOTE though that the randomization is only performed for those tests that explicitly call set_cgroups_path. Signed-off-by: Kir Kolyshkin --- tests/integration/helpers.bash | 44 +++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/tests/integration/helpers.bash b/tests/integration/helpers.bash index 6fcb122b3..e5bf4f349 100644 --- a/tests/integration/helpers.bash +++ b/tests/integration/helpers.bash @@ -112,22 +112,6 @@ function init_cgroup_paths() { # init once test -n "$CGROUP_UNIFIED" && return - local rnd="$RANDOM" - if [ -n "${RUNC_USE_SYSTEMD}" ]; then - SD_UNIT_NAME="runc-cgroups-integration-test-${rnd}.scope" - if [ $(id -u) = "0" ]; then - REL_CGROUPS_PATH="/machine.slice/$SD_UNIT_NAME" - OCI_CGROUPS_PATH="machine.slice:runc-cgroups:integration-test-${rnd}" - else - 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-${rnd}" - fi - else - REL_CGROUPS_PATH="/runc-cgroups-integration-test/test-cgroup-${rnd}" - OCI_CGROUPS_PATH=$REL_CGROUPS_PATH - fi - if stat -f -c %t /sys/fs/cgroup | grep -qFw 63677270; then CGROUP_UNIFIED=yes # "pseudo" controllers do not appear in /sys/fs/cgroup/cgroup.controllers. @@ -140,7 +124,6 @@ function init_cgroup_paths() { echo devices ) CGROUP_BASE_PATH=/sys/fs/cgroup - CGROUP_PATH=${CGROUP_BASE_PATH}${REL_CGROUPS_PATH} # Find any cgroup.freeze files... if [ -n "$(find "$CGROUP_BASE_PATH" -type f -name "cgroup.freeze" -print -quit)" ]; then @@ -158,11 +141,34 @@ function init_cgroup_paths() { fi } -# Helper function to set cgroupsPath to the value of $OCI_CGROUPS_PATH +# Randomize cgroup path(s), and update cgroupsPath in config.json. +# This function sets a few cgroup-related variables. function set_cgroups_path() { bundle="${1:-.}" init_cgroup_paths - update_config '.linux.cgroupsPath |= "'"${OCI_CGROUPS_PATH}"'"' $bundle + + local rnd="$RANDOM" + if [ -n "${RUNC_USE_SYSTEMD}" ]; then + SD_UNIT_NAME="runc-cgroups-integration-test-${rnd}.scope" + if [ "$(id -u)" = "0" ]; then + REL_CGROUPS_PATH="/machine.slice/$SD_UNIT_NAME" + OCI_CGROUPS_PATH="machine.slice:runc-cgroups:integration-test-${rnd}" + else + 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-${rnd}" + fi + else + REL_CGROUPS_PATH="/runc-cgroups-integration-test/test-cgroup-${rnd}" + OCI_CGROUPS_PATH=$REL_CGROUPS_PATH + fi + + # Absolute path to container's cgroup v2. + if [ "$CGROUP_UNIFIED" == "yes" ]; then + CGROUP_PATH=${CGROUP_BASE_PATH}${REL_CGROUPS_PATH} + fi + + update_config '.linux.cgroupsPath |= "'"${OCI_CGROUPS_PATH}"'"' "$bundle" } # Helper to check a value in cgroups. From b09030a5f4e739b3962311702e96094c1c7b086d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Mar 2021 14:26:34 -0700 Subject: [PATCH 4/7] tests/int/checkpoint: close fds in check_pipes In check_pipes, make sure we - close all fds we opened in setup_pipes; - check that runc stderr is empty (and fail if it's not). Signed-off-by: Kir Kolyshkin --- tests/integration/checkpoint.bats | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/integration/checkpoint.bats b/tests/integration/checkpoint.bats index 1a58c2b64..60f46a3a3 100644 --- a/tests/integration/checkpoint.bats +++ b/tests/integration/checkpoint.bats @@ -38,11 +38,23 @@ function setup_pipes() { } function check_pipes() { + local output stderr + echo Ping >&${in_w} exec {in_w}>&- exec {out_w}>&- + exec {err_w}>&- + + exec {in_r}>&- output=$(cat <&${out_r}) + exec {out_r}>&- + stderr=$(cat <&${err_r}) + exec {err_r}>&- + [[ "${output}" == *"ponG Ping"* ]] + if [ -n "$stderr" ]; then + fail "runc stderr: $stderr" + fi } # Usage: runc_run_with_pipes container-name From 0e0890027c5486c4cc98cb6112aa72b49e15e907 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Mar 2021 14:28:30 -0700 Subject: [PATCH 5/7] tests/int/checkpoint: close lazy_r fd Signed-off-by: Kir Kolyshkin --- tests/integration/checkpoint.bats | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/checkpoint.bats b/tests/integration/checkpoint.bats index 60f46a3a3..d5b5c9fb7 100644 --- a/tests/integration/checkpoint.bats +++ b/tests/integration/checkpoint.bats @@ -194,6 +194,7 @@ function simple_cr() { # wait for lazy page server to be ready out=$(timeout 2 dd if=/proc/self/fd/${lazy_r} bs=1 count=1 2>/dev/null | od) + exec {lazy_r}>&- exec {lazy_w}>&- # shellcheck disable=SC2116,SC2086 out=$(echo $out) # rm newlines From 2dd62b3d428747f7b0bb813dd10a501ab9381480 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Mar 2021 19:13:01 -0700 Subject: [PATCH 6/7] libct/checkCriuFeatures: rm excessive debug 1. Remove printing criu args as now they are *always swrk 3. 2. Remove duplicated "feature check says" debug. Before: > DEBU[0000] Using CRIU with following args: [swrk 3] > DEBU[0000] Using CRIU in FEATURE_CHECK mode > DEBU[0000] Feature check says: type:FEATURE_CHECK success:true features: > DEBU[0000] Feature check says: mem_track:false lazy_pages:true After: > DEBU[0000] Using CRIU in FEATURE_CHECK mode > DEBU[0000] Feature check says: mem_track:false lazy_pages:true Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 7ce505ea0..5edd745fe 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -717,7 +717,6 @@ func (c *linuxContainer) checkCriuFeatures(criuOpts *CriuOpts, rpcOpts *criurpc. return errors.New("CRIU feature check failed") } - logrus.Debugf("Feature check says: %s", criuFeatures) missingFeatures := false // The outer if checks if the fields actually exist @@ -1518,7 +1517,6 @@ func (c *linuxContainer) criuSwrk(process *Process, req *criurpc.CriuReq, opts * // the initial CRIU run to detect the version. Skip it. logrus.Debugf("Using CRIU %d at: %s", c.criuVersion, c.criuPath) } - logrus.Debugf("Using CRIU with following args: %s", args) cmd := exec.Command(c.criuPath, args...) if process != nil { cmd.Stdin = process.Stdin From 36fe3cc28c35d7157dc8b02b01e2c15bf73c75d8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 31 Mar 2021 18:12:07 -0700 Subject: [PATCH 7/7] tests/int/cpt: fix lazy-pages flakiness "checkpoint --lazy-pages and restore" test sometimes fails on restore in our CI on Fedora 33 when systemd cgroup driver is used: > (00.076104) Error (compel/src/lib/infect.c:1513): Task 48521 is in unexpected state: f7f > (00.076122) Error (compel/src/lib/infect.c:1520): Task stopped with 15: Terminated > ... > (00.078246) Error (criu/cr-restore.c:2483): Restoring FAILED. I think what happens is 1. The test runs runc checkpoint in lazy-pages mode in background. 2. The test runs criu lazy-pages in background. 3. The test runs runc restore. Now, all three are working in together: criu restore restores, criu lazy-pages listens for page faults on a uffd and fetch missing pages from runc checkpoint, who serves those pages. At some point criu lazy-pages decides to fetch the rest of the pages, and once it's done it exits, and runc checkpoint, as there are no more pages to serve, exits too. At the end of runc checkpoint the container is removed (see "defer destroy(container)" in checkpoint.go. This involves a call to cgroupManager.Destroy, which, in case systemd manager is used, calls stopUnit, which makes systemd to not just remove the unit, but also send SIGTERM to its processes, if there are any. As the container is being restored into the same systemd unit, sometimes this results in sending SIGTERM to a process which criu restores, and thus restoring fails. The remedy here is to change the name of systemd unit to which the container is restored. Signed-off-by: Kir Kolyshkin --- tests/integration/checkpoint.bats | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/checkpoint.bats b/tests/integration/checkpoint.bats index d5b5c9fb7..f028010c8 100644 --- a/tests/integration/checkpoint.bats +++ b/tests/integration/checkpoint.bats @@ -211,11 +211,13 @@ function simple_cr() { lp_pid=$! # Restore lazily from checkpoint. - # The restored container needs a different name as the checkpointed + # The restored container needs a different name (as well as systemd + # unit name, in case systemd cgroup driver is used) as the checkpointed # container is not yet destroyed. It is only destroyed at that point # in time when the last page is lazily transferred to the destination. # Killing the CRIU on the checkpoint side will let the container # continue to run if the migration failed at some point. + [ -n "$RUNC_USE_SYSTEMD" ] && set_cgroups_path runc_restore_with_pipes ./image-dir test_busybox_restore --lazy-pages wait $cpt_pid