I played with shfmt options (-bn, -ci, -sr) a bit trying to minimize
the patch generated (and also because I don't have a strong preference
on these matters), and it appears to be that the patch size is about the
same nevertheless, so I chose no options.
This commit is brought to you by
shfmt -ln bash -w man/*.sh script/*.sh tests/*.sh tests/integration/*.bash
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Various bats tests use various types of indentation (about half
is tabs, the rest is 2 spaces, 4 spaces, etc.).
Let's bring it to one style (tabs) using recently added
shfmt support for bats files (see [1]).
This commit is brought to you by
shfmt -ln bats -w tests/integration/*.bats
[1] https://github.com/mvdan/sh/issues/600
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When we call terminate(), we kill the process, and wait
returns the error indicating the process was killed.
This is exactly what we expect here, so there is no reason
to treat it as an error.
Before this patch, when a container with invalid cgroup parameters is
started:
> WARN[0000] unable to terminate initProcess error="signal: killed"
> ERRO[0000] container_linux.go:366: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "555": open /sys/fs/cgroup/blkio/user.slice/xx33/blkio.weight: permission denied
After:
> ERRO[0000] container_linux.go:366: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write "555": open /sys/fs/cgroup/blkio/user.slice/xx33/blkio.weight: permission denied
I.e. the useless warning is gone.
NOTE this breaks a couple of integration test cases, since they were
expecting a particular message in the second line, and now due to
"signal: killed" removed it's in the first line. Fix those, too.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of whatever is happening in the test cases is already available in
setup_hello and teardown_hello. Use these.
Rewrite the validation test to be more compact and not dependent
on `pwd` value. Also, pinning xeipuuv/gojsonschema to a particular
version is no longer required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The purpose of the spec validation test is to make sure
that config.json generated by runc spec is valid. Therefore,
it does not depend on ROOTLESS_FEATURES.
Taking that into account, and given the fact that this test
involves cloning repos and building some code, it makes sense
to not run it 4 times for various rootless features.
Time it takes to execute spec.bats in rootless mode has improved.
Before:
real 0m21.286s
user 0m37.837s
sys 0m5.745s
After:
real 0m13.162s
user 0m30.814s
sys 0m4.050s
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit d78ae51a2 adds this line, but the file is never created.
This is probably a leftover from copy-pasting update.bats code.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
With some grep|awk help, found a few places where the containers
supposed to be removed in teardown() weren't.
To find container names:
grep -E '^[[:space:]]*\<_*_*runc\>.*\<(create|run|restore)\>'
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For test cases where we used pipes for container's stdin/stdout/stderr,
stderr was redirected to the same pipe as stdout, which practically
means it is lost.
These redirects to fd is needed not because we check that container is
working by writing to its stdin and reading from stdout (see
check_pipes), but also because bats redirects test stdout/stderr to a
file, which makes c/r impossible (as the file is outside of container).
This is why we can't just do something like `2>stderr.log`, and have
to do what is done in this commit.
Introduce and use another pipe for stdout, to be used for both runc run
and runc restore, so it will be shown in case of errors.
Since its handling is somewhat complicated and is used from 4 places
(2 for run, 2 for restore), separate it into a helper functions.
NOTE the code assumes that runc exits with non-zero exit code in case
there is anything that needs to be shown to a user from runc's stderr.
While at it, add error checking to runc run calls.
Hopefully, this will help debug those rare checkpoint failures in CI.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 417f5ff40 added a code to close fds and kill processes, which
should have helped in an event of test case failure.
In fact, each test case is executed in a subshell, so
- any variables set there can't reach teardown();
- all the fds are closed (as the process is gone).
Now, I am not sure about the processes, but the code being removed
has never worked anyway, so it does not make sense to keep it.
Normally, those are waited for. In case of a test case failure,
well, the subsequent cases might fail, too.
Fixes: 417f5ff40d
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the following issue with the test:
not ok 12 checkpoint --lazy-pages and restore
# (in test file tests/integration/checkpoint.bats, line 202)
# `[ $ret -eq 0 ]' failed
...
# grep: ./work-dir/restore.log: No such file or directory
One might think that `--work-path ./image-dir` is a mistake, but it's
not, since `criu lazy-pages -D ./image-dir` creates a socket in
./image-dir, and then `criu restore --lazy-pages` expects a socket in
the workdir.
Fixes: e232a71a3d
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These tests expect group name to be "nogroup", while recent busybox
changed that to "nobody".
Use numeric uids/gids to fix.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test started to fail, as it expected the output from
`id` to be
uid=1000 gid=1000 groups=100(users),65534(nogroup)
while the actual output is now
uid=1000 gid=1000 groups=100(users),65534(nobody)
Apparently, busybox image changed group name.
As we're only interested in ids, not names, and to fix the test,
let's use `id -G`.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Note that stdout/stderr are already redirected by bats'
`run` command, so the only way to get a controlling terminal
is to open /dev/tty (which fails if there isn't one).
Here's how I tested the failure to open /dev/tty:
> [root@kir-rhat ~]# ssh -T root@localhost cat ./runme
> cd /home/kir/go/src/github.com/opencontainers/runc
> ./runc run -b tst xxx-$$
> echo $?
>
> [root@kir-rhat ~]# ssh -T root@localhost ./runme
> time="2020-07-31T16:35:47-07:00" level=error msg="chdir tst: no such file or directory"
> 1
If anyone knows how to obtain an tty-less environment without using
ssh -T, please raise your hand.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 335f0806c added a test case doing
```bash
for i in $(seq 1); do
...
done
```
and it does not make any sense to have it since we're only performing
a single iteration.
Remove the code.
I have not touched the indentation, for the sake of cleaner review,
also because already have different intentation in different tests;
this should be addressed separately.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Cesar Talledo (2):
Remove runc default devices that overlap with spec devices.
Skip redundant setup for /dev/ptmx when specified explicitly in the OCI spec.
LGTMs: @AkihiroSuda @cyphar
Closes#2522
Ignore the shellcheck warnings like this one:
> In tty.bats line 32:
> update_config '(.. | select(.[]? == "sh")) += ["-c", "stat -c %u:%g $(tty) | tr : \\\\n"]'
> ^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.
While at it, fix some minor whitespace issues in tty.bats.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Ignore warnings like this:
> In checkpoint.bats line 169:
> PIDS_TO_KILL=($cpt_pid)
> ^------^ SC2206: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
Since in all the cases we deal with either pids or fds, and they don't
have spaces.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fix or ignore warnings like this one:
> In cgroups.bats line 107:
> if [ $(id -u) = "0" ]; then
> ^------^ SC2046: Quote this to prevent word splitting.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those are pretty simple to allow shellcheck to fix these, so
this commit is courtesy of
> shellcheck -i SC2086 -i SC2006 -f diff *.bats > fix.diff
> patch -p1 < fix.diff
repeated 3 times ;)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the following warning, and implements a suggestion:
> In update.bats line 426:
> IFS='/' read -r -a dirs <<< $(echo ${CGROUP_CPU} | sed -e s@^${CGROUP_CPU_BASE_PATH}/@@)
> ^-- SC2046: Quote this to prevent word splitting.
> ^-- SC2001: See if you can use ${variable//search/replace} instead.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fixes the following warning:
> In update.bats line 422:
> local root_period=$(cat "${CGROUP_CPU_BASE_PATH}/cpu.rt_period_us")
> ^---------^ SC2155: Declare and assign separately to avoid masking return values.
>
>
> In update.bats line 423:
> local root_runtime=$(cat "${CGROUP_CPU_BASE_PATH}/cpu.rt_runtime_us")
> ^----------^ SC2155: Declare and assign separately to avoid masking return values.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fixes the following warning:
> In cgroups.bats line 58:
> if [ "$KERNEL_MAJOR" -lt 4 ] || [ "$KERNEL_MAJOR" -eq 4 -a "$KERNEL_MINOR" -le 5 ]; then
> ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. cd is useless as all the paths are absolute
2. run is redundant, does not make sense to use it
3. use mkdir -p to save a line of code
This also eliminates shellcheck warnings like this one:
> In spec.bats line 8:
> cd "$INTEGRATION_ROOT"
> ^--------------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It's not a regex but a substring, so use a substring match.
Fixes the following warning by shellcheck:
> In mounts.bats line 20:
> [[ "${lines[0]}" =~ '/tmp/bind/config.json' ]]
> ^---------------------^ SC2076: Don't quote right-hand side of =~, it'll match literally rather than as a regex.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The ending EOF should be
- all by itself (i.e. no extra characters on the same line);
- with no whitespace before it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TL;DR: this allows to show logs from failed runc restore.
Bats scripts are run with `set -e`. This is well known and obvious,
and yet there are a few errors with respect to that, including a few
"gems" by yours truly.
1. bats scripts are run with `set -e`, meaning that `[ $? -eq 0 ]` is
useless since the execution won't ever reach this line in case of
non-zero exit code from a preceding command. So, remove all such
checks, they are useless and misleading.
2. bats scripts are run with `set -e`, meaning that `ret=$?` is useless
since the execution won't ever reach this line in case of non-zero
exit code from a preceding command.
In particular, the code that calls runc restore needs to save the exit
code, show the errors in the log, and only when check the exit code and
fail if it's non-zero. It can not use `run` (or `runc` which uses `run`)
because of shell redirection that we need to set up.
The solution, implemented in this patch, is to use code like this:
```bash
ret=0
__runc ... || ret=$?
show_logs
[ $ret -eq 0 ]
```
In case __runc exits with non-zero exit code, `ret=$?` is executed, and
it always succeeds, so we won't fail just yet and have a chance to show
logs before checking the value of $ret.
In case __runc succeeds, `ret=$?` is never executed, so $ret will still
be zero (this is the reason why it needs to be set explicitly).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Per the OCI spec, /dev/ptmx is always a symlink to /dev/pts/ptmx. As such, if
the OCI spec has an explicit entry for /dev/ptmx, runc shall ignore it.
This change ensures this is the case. A integration test was also added
(in tests/integration/dev.bats).
Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>
Runc has a set of default devices that it includes in Linux containers
(e.g., /dev/null, /dev/random, /dev/tty, etc.)
However if the container's OCI spec includes all or a subset of those same devices,
runc is currently not detecting the redundancy, causing it to create a lib
container config that has redundant device configurations.
This causes a failure in rootless mode, in particular when the /dev/tty device
has a redundant config:
container_linux.go:370: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:70: creating device nodes caused: open /tmp/busyboxtest/rootfs/dev/tty: no such device or address"
The reason this fails in rootless mode only is that in this case runc sets up
/dev/tty not by doing mknod (it's not allowed within a user-ns) but rather by
creating a regular file under /dev/tty and bind-mounting the host's /dev/tty to
the container's /dev/tty. When this operation is done redundantly, it fails the
second time.
This change fixes this problem by ensuring runc checks for redundant devices
between the OCI spec it receives and the default devices it configures. If
a redundant device is detected, the OCI spec takes priority.
The change adds both a unit test and an integration test to verify the
behavior. Without this fix, this new integration test fails as shown above.
Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>
Akihiro Suda