A couple of test cases in delete.bats check that a particular cgroup
exists (or doesn't exist) using find. This is now resulting in errors
like these:
find: ‘/sys/fs/cgroup/blkio/azsec’: Permission denied
find: ‘/sys/fs/cgroup/blkio/azsec_clamav’: Permission denied
find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec’: Permission denied
find: ‘/sys/fs/cgroup/cpu,cpuacct/azsec_clamav’: Permission denied
find: ‘/sys/fs/cgroup/memory/azsec’: Permission denied
find: ‘/sys/fs/cgroup/memory/azsec_clamav’: Permission denied
leading to test case failures.
Apparently, GHA runs something else on a test box, so we get this.
To fix, ignore non-zero exit code from find, and redirect its stderr
to /dev/null.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When doing a lazy checkpoint/restore, we should not restore into the
same cgroup, otherwise there is a race which result in occasional
killing of the restored container (GH #2760, #2924).
The fix is to use --manage-cgroup-mode=ignore, which allows to restore
into a different cgroup.
Note that since cgroupsPath is not set in config.json, the cgroup is
derived from the container name, so calling set_cgroups_path is not
needed.
For the previous (unsuccessful) attempt to fix this, as well as detailed
(and apparently correct) analysis, see commit 36fe3cc28c.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test checks that the container is restored into a different cgroup.
To do so, a user should
- use --manage-cgroups-mode ignore on both checkpoint and restore;
- change the cgroupsPath value in config.json before restoring.
The test does some checks to ensure that its logic is correct, and that
after the restore the old (original) cgroup does not exist, the new one
exists, and the container's init is in that new cgroup.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test (initially added by commit 58ea21daef and later amended in
commit 26dc55ef1a) currently has two major deficiencies:
1. All possible flag combinations, and their respective numeric values,
have to be explicitly listed. Currently we support 3 flags, so
there is only 2^3 - 1 = 7 combinations, but adding more flags will
become increasingly difficult (for example, 5 flags will result in
31 combinations).
2. The test requires kernel 4.17 (for SECCOMP_FILTER_FLAG_SPEC_ALLOW),
and not doing any tests when running on an older kernel. This, too,
will make it more difficult to add extra flags in the future.
Both issues can be solved by using runc features which now prints all
known and supported runc flags. We still have to hardcode the numeric
values of all flags, but most of the other work is coded now.
In particular:
* The test only uses supported flags, meaning it can be used with
older kernels, removing the limitation (2) above.
* The test calculates the powerset (all possible combinations) of
flags and their numeric values. This makes it easier to add more
flags, removing the limitation (1) above.
* The test will fail (in flags_value) if any new flags will be added
to runc but the test itself is not amended.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If no seccomps flags are set in OCI runtime spec (not even the empty
set), set SPEC_ALLOW as the default (if it's supported).
Otherwise, use the flags as they are set (that includes no flags for
empty seccomp.Flags array).
This mimics the crun behavior, and makes runc seccomp performance on par
with crun.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is aimed at solving the problem of cgroup v2 memory controller
behavior which is not compatible with that of cgroup v1.
In cgroup v1, if the new memory limit being set is lower than the
current usage, setting the new limit fails.
In cgroup v2, same operation succeeds, and the container is OOM killed.
Introduce a new setting, memory.checkBeforeUpdate, and use it to mimic
cgroup v1 behavior.
Note that this is not 100% reliable because of TOCTOU, but this is the
best we can do.
Add some test cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
On ARM, mkdirat(2) is used instead of mkdir(2), thus the seccomp rules
needs to be amended accordingly.
This is a change similar to one in commit e119db7a23, but but it
evaded the test case added in commit 58ea21dae as it took a long time to
merge, and we don't have ARM CI.
Fixes: 58ea21dae ("seccomp: add support for flags")
Reported-by: Ryan Phillips <rphillips@redhat.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test case for an issue fixed by the previous commit.
The env should has more than 8 core CPU to meet the test requirement.
Signed-off-by: Chengen, Du <chengen.du@canonical.com>
We use awk in other 9 or so places, and here it's gawk.
Since this is on Linux, most probably awk is gawk.
So s/gawk/awk/.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The situation when /sys/fs/cgroup/unified is not present normal and
should not result in anything on stderr. Suppress it.
Fixes: cc15b887a0
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a debug print of seccomp flags value, so the test can check
those (without using something like strace, that is).
Amend the flags setting test with the numeric values expected, and the
logic to check those.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A regression reported for runc v1.1.3 says that "runc exec -t" fails
after doing "systemctl daemon-reload":
> exec failed: unable to start container process: open /dev/pts/0: operation not permitted: unknown
Apparently, with commit 7219387eb7 we are no longer adding
"DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns
ENOENT).
The bug can only be seen after "systemctl daemon-reload" because runc
also applies the same rules manually (by writing to devices.allow for
cgroup v1), and apparently reloading systemd leads to re-applying the
rules that systemd has (thus removing the char-pts access).
The fix is to do os.Stat only for "/dev" paths.
Also, emit a warning that the path was skipped. Since the original idea
was to emit less warnings, demote the level to debug.
Note this also fixes the issue of not adding "m" permission for block-*
and char-* devices.
A test case is added, which reliably fails before the fix
on both cgroup v1 and v2.
Fixes: https://github.com/opencontainers/runc/issues/3551
Fixes: 7219387eb7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Recently released codespell 2.2 adds some more false positives,
such as:
./Makefile:78: ro ==> to, row, rob, rod, roe, rot
./Makefile:88: ro ==> to, row, rob, rod, roe, rot
./notify_socket.go:51: ro ==> to, row, rob, rod, roe, rot
./LICENSE:128: complies ==> compiles
./go.sum:59: BU ==> BY
./types/features/features.go:17: ro ==> to, row, rob, rod, roe, rot
./libcontainer/rootfs_linux.go:52: ro ==> to, row, rob, rod, roe, rot
./libcontainer/rootfs_linux.go:166: ro ==> to, row, rob, rod, roe, rot
....
./tests/integration/cgroup_delegation.bats:38: inh ==> in
...
To fix:
- exclude go.sum;
- add ro and complies to the list of ignored words;
- s/inh/inherit in cgroup_delegation.bats.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently only amd64 and arm64v8 tarball have been checked in testdata,
while busybox bundle is downloaded on fly, and supports multiple architectures.
To enable integration tests for more architectures, the hello world
bundle is replaced by busybox one.
Signed-off-by: Shengjing Zhu <zhsj@debian.org>
Due to a bug in commit 9c444070ec, when the user and mount namespaces
are used, and the bind mount is followed by the cgroup mount in the
spec, the cgroup is mounted using the bind mount's mount fd.
This can be reproduced with podman 4.1 (when configured to use runc):
$ podman run --uidmap 0:100:10000 quay.io/libpod/testimage:20210610 mount
Error: /home/kir/git/runc/runc: runc create failed: unable to start container process: error during container init: error mounting "cgroup" to rootfs at "/sys/fs/cgroup": mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted: OCI permission denied
or manually with the spec mounts containing something like this:
{
"destination": "/etc/resolv.conf",
"type": "bind",
"source": "/userdata/resolv.conf",
"options": [
"bind"
]
},
{
"destination": "/sys/fs/cgroup",
"type": "cgroup",
"source": "cgroup",
"options": [
"rprivate",
"nosuid",
"noexec",
"nodev",
"relatime",
"ro"
]
}
The issue was not found earlier since it requires using userns, and even then
mount fd is ignored by mountToRootfs, except for bind mounts, and all the bind
mounts have mountfd set, except for the case of cgroup v1's /sys/fs/cgroup
which is internally transformed into a bunch of bind mounts.
This is a minimal fix for the issue, suitable for backporting.
A test case is added which reproduces the issue without the fix applied.
Fixes: 9c444070ec ("Open bind mount sources from the host userns")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The variable $ROOTLESS, as set by helpers.bash and used in many places,
provides the same value as $EUID which is always set by bash. Since we
are using bash, we can rely on $EUID being omnipresent.
Modify all uses accordingly, and since the value is known to be a
number, omit the quoting.
Similarly, replace all uses of $(id -u) to $EUID.
Do some trivial cleanups along the way, such as
- simplify some if A; then B; to A && B;
- do not use [[ instead of [ where not necessary.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test requires both rootless and root, which does not make sense.
Remove the rootless part.
Fixes: d41a273da
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes it work similar to all the other variables we use as binary
flags.
The new 'shellcheck disable' is due to a bug in shellcheck (basically,
it does not track the scope of variables or execution order, assuming
everything is executed as soon as it is seen).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Strictly speaking, == is for [[ only, not for [ / test,
and, unlike =, the right side is a pattern.
To avoid confusion, use =. In cases where we compare with empty string,
use -z instead.
Keep using [[ in some cases since it does not require quoting the left
and right side of comparison (I trust shellcheck on that one).
This should have no effect (other than the code being a tad more
strict).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
While doing the previous fix, I went over all the tests in this file and
made sure they were named correctly. This patch just adds a small
sentence to clarify the intent, and does some minor improvements to some
other test names.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
There was a typo and instead of "empty" we should have used "non-empty".
Let's add a small sentence explaining the intent (like other tests in
this file) and let's highlight what we expect to happen in this test (to
ignore the listenerPath).
Fixes: #3415
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
The following failure was observed in CI (on centos-stream-8 in
integration-cgroup suite):
not ok 42 runc delete
(from function `fail' in file tests/integration/helpers.bash, line 338,
in test file tests/integration/delete.bats, line 30)
`[ "$output" = "" ] || fail "cgroup not cleaned up correctly: $output"' failed
....
cgroup not cleaned up correctly: /sys/fs/cgroup/pids/system.slice/tmp-bats\x2drun\x2d68012-runc.IPOypI-state-testbusyboxdelete-runc.zriC8C.mount
/sys/fs/cgroup/cpu,cpuacct/system.slice/tmp-bats\x2drun\x2d68012-runc.IPOypI-state-testbusyboxdelete-runc.zriC8C.mount
...
Apparently, this is a cgroup systemd creates for a mount unit which
appears then runc does internal /proc/self/exe bind-mount. The test
case should not take it into account.
The second problem with this test is it does not check that cgroup
actually exists when the container is running (so checking that it
was removed after makes less sense). For example, in rootless mode
the cgroup might not have been created.
Fix the find arguments to look for a specific cgroup name, and add
a check that these arguments are correct (i.e. the cgroup is found
when the container is running).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a way to prevent the code doing something really bad when a
variable it uses is not set. Good to have since it helps to catch some
logical errors etc.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Audit all checks for non-empty variables (i.e. ' -z ', ' -n ',
' != ""' and '= ""'), and fix those cases where a variable might be
unset. Those variables (that might not be set) are
- RUNC_USE_SYSTEMD
- BATS_RUN_TMPDIR
- AUX_UID
- AUX_DIR
- SD_PARENT_NAME
- REL_PARENT_PATH
- ROOT
- HAVE_CRIU
- ROOTLESS_FEATURES
- and a few test-specific or file-specific variables
This should allow us to enable set -u.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add "unset ALT_ROOT" since it should not be used after teardown is
called.
2. Remove "rm -rf $ALT_ROOT". It is not needed, because ALT_ROOT is a
subdirectory of ROOT, which is removed in teardown_bundle.
3. Checking for ALT_ROOT being non-empty is a leftover from the era when
teardown() was called as the first step from setup(). Since commit
41670e21f0 this is no longer the case, so the condition
is no longer needed (plus, the `set -u` which is about to be added
should catch any possible use of unset ALT_ROOT).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Some tests (those in help.bats and version.bats) do not use setup_bundle
(as they do not need to start any containers), and thus they do not set
$ROOT. As a consequence, these tests now call "runc --root /state" which
is not nice.
Make adding --root conditional (only if $ROOT is set).
Amazingly, this change breaks help.bats tests under rootless, because
"sudo rootless" does not change the value of XDG_RUNTIME_DIR which still
points to root-owned directory, and as a result we have this:
> runc foo -h (status=1):
> the path in $XDG_RUNTIME_DIR must be writable by the user
> time="2022-02-08T07:04:57Z" level=error msg="mkdir /run/user/0/runc: permission denied"
This could be fixed by adding proper $ROOT, but it's easier just to skip
those tests under non-root.
NOTE that version.bats is not broken because -v is handled by urfave/cli
very early, so app.Before function is not run.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Older bash versions treats variable as unset if nothing has been
assigned to it. Here is an example from CentOS 7 system:
[kir@localhost ~]$ bash -u -c 'x() { local args=(); echo "${args[@]}"; }; x'
bash: args[@]: unbound variable
[kir@localhost ~]$ echo $BASH_VERSION
4.2.46(2)-release
Rewrite to work around this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. This valid warning is reported by shellcheck v0.8.0:
In tests/integration/helpers.bash line 38:
KERNEL_MINOR="${KERNEL_VERSION#$KERNEL_MAJOR.}"
^-----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Did you mean:
KERNEL_MINOR="${KERNEL_VERSION#"$KERNEL_MAJOR".}"
Fix this.
2. These (invalid) warnings are also reported by the new version:
In tests/integration/events.bats line 13:
@test "events --stats" {
^-- SC2030 (info): Modification of status is local (to subshell caused by @bats test).
In tests/integration/events.bats line 41:
[ "$status" -eq 0 ]
^-----^ SC2031 (info): status was modified in a subshell. That change might be lost.
Basically, this is happening because shellcheck do not really track
the call tree and/or local variables. This is a known (and reported)
deficiency, and the alternative to disabling these warnings is moving
the code around, which is worse due to more changes in git history.
So we have to silence/disable these.
3. Update shellcheck to 0.8.0.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.
The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.
Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.
Add a warning that --criu is ignored and will be removed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using "$@" instead of $1 in update_config() allows us to use it from
hooks.bats, where jq is used with more options than usual.
We need to disable SC2016 as otherwise shellcheck sees $something inside
single quotes and think we are losing the shell expansion (we are not).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This can be used to specify a different runc binary, for example:
sudo -E RUNC=$PWD/runc.mine tests/integration/cwd.bats
A different (but compatible enough) runtime also works:
sudo -E RUNC=/usr/local/bin/crun tests/integration/cwd.bats
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new mount option "rro" makes the mount point recursively read-only,
by calling `mount_setattr(2)` with `MOUNT_ATTR_RDONLY` and `AT_RECURSIVE`.
https://man7.org/linux/man-pages/man2/mount_setattr.2.html
Requires kernel >= 5.12.
The "rro" option string conforms to the proposal in util-linux/util-linux Issue 1501.
Fix issue 2823
Similary, this commit also adds the following mount options:
- rrw
- r[no]{suid,dev,exec,relatime,atime,strictatime,diratime,symfollow}
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Delegating cgroups to the container enables more complex workloads,
including systemd-based workloads. The OCI runtime-spec was
recently updated to explicitly admit such delegation, through
specification of cgroup ownership semantics:
https://github.com/opencontainers/runtime-spec/pull/1123
Pursuant to the updated OCI runtime-spec, change the ownership of
the container's cgroup directory and particular files therein, when
using cgroups v2 and when the cgroupfs is to be mounted read/write.
As a result of this change, systemd workloads can run in isolated
user namespaces on OpenShift when the sandbox's cgroupfs is mounted
read/write.
It might be possible to implement this feature in other cgroup
managers, but that work is deferred.
Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
Kir Kolyshkin