Add a test case for our internal reboot command #1426
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new internal reboot command and an integration test to verify its functionality. The changes to the Rust code are straightforward and look correct. My review focuses on the new shell scripts, where I've identified opportunities to improve robustness and portability. Overall, this is a valuable addition for ensuring the reliability of the reboot mechanism.
| tmpd=$(mktemp -d) | ||
| log() { | ||
| echo "$@" | ||
| "$@" | ||
| } | ||
| log timeout -f 120 podman run --rm -ti --systemd=always --privileged -v /sys:/sys:ro --label bootc.test=reboot --net=none -v $(pwd):/src:ro -v $tmpd:/run/bootc-test-reboot $image /bin/sh -c 'cp /src/*.service /etc/systemd/system && systemctl enable bootc-test-reboot && exec /sbin/init' || true | ||
| ls -al $tmpd | ||
| if test '!' -f $tmpd/success; then | ||
| echo "reboot failed" 1>&2 | ||
| rm -rf "$tmpd" | ||
| exit 1 | ||
| fi | ||
| rm -rf "$tmpd" | ||
| echo "ok reboot" |
There was a problem hiding this comment.
Using a trap ensures the temporary directory created by mktemp is cleaned up reliably, even if the script is interrupted. This also simplifies the script by removing duplicated rm commands.
tmpd=$(mktemp -d)
trap 'rm -rf -- "$tmpd"' EXIT
log() {
echo "$@"
"$@"
}
log timeout 120 podman run --rm -ti --systemd=always --privileged -v /sys:/sys:ro --label bootc.test=reboot --net=none -v "$(pwd)":/src:ro -v "$tmpd":/run/bootc-test-reboot "$image" /bin/sh -c 'cp /src/*.service /etc/systemd/system && systemctl enable bootc-test-reboot && exec /sbin/init' || true
ls -al "$tmpd"
if [ ! -f "$tmpd/success" ]; then
echo "reboot failed" >&2
exit 1
fi
echo "ok reboot"
tests/container/reboot/run
Outdated
| echo "$@" | ||
| "$@" | ||
| } | ||
| log timeout -f 120 podman run --rm -ti --systemd=always --privileged -v /sys:/sys:ro --label bootc.test=reboot --net=none -v $(pwd):/src:ro -v $tmpd:/run/bootc-test-reboot $image /bin/sh -c 'cp /src/*.service /etc/systemd/system && systemctl enable bootc-test-reboot && exec /sbin/init' || true |
There was a problem hiding this comment.
Several variables ($(pwd), $tmpd, $image) are unquoted. They should be quoted to prevent issues if their values contain spaces or special characters.
log timeout -f 120 podman run --rm -ti --systemd=always --privileged -v /sys:/sys:ro --label bootc.test=reboot --net=none -v "$(pwd)":/src:ro -v "$tmpd":/run/bootc-test-reboot "$image" /bin/sh -c 'cp /src/*.service /etc/systemd/system && systemctl enable bootc-test-reboot && exec /sbin/init' || true
| for case in $tests; do | ||
| if test $case = .; then continue; fi | ||
| echo "Running: $case" | ||
| cd $case | ||
| ./run $image | ||
| cd - | ||
| echo "ok $case" | ||
| done |
There was a problem hiding this comment.
The for case in $tests loop is subject to word splitting, which will cause issues with directory names that contain spaces or other special characters. Using a while read loop with find -print0 is the standard, robust way to iterate over find's output.
find . -maxdepth 1 -type d -print0 | while IFS= read -r -d '' case; do
if [ "$case" = "." ]; then continue; fi
echo "Running: $case"
cd $case
./run $image
cd -
echo "ok $case"
done
| cd $(dirname $0) | ||
|
|
||
| tests=$(find . -maxdepth 1 -type d) | ||
| for case in $tests; do | ||
| if test $case = .; then continue; fi | ||
| echo "Running: $case" | ||
| cd $case | ||
| ./run $image | ||
| cd - | ||
| echo "ok $case" | ||
| done |
There was a problem hiding this comment.
Path variables like $(dirname "$0") and $case should be quoted.
cd "$(dirname "$0")"
find . -maxdepth 1 -type d -print0 | while IFS= read -r -d '' case; do
if [ "$case" = "." ]; then continue; fi
echo "Running: $case"
cd "$case"
./run $image
cd -
echo "ok $case"
done
| cd $case | ||
| ./run $image | ||
| cd - |
There was a problem hiding this comment.
Using a subshell (...) to run commands in a different directory is often cleaner and safer than cd ...; cd -.
find . -maxdepth 1 -type d -print0 | while IFS= read -r -d '' case; do
if [ "$case" = "." ]; then continue; fi
echo "Running: $case"
(
cd -- "$case" && ./run "$image"
)
echo "ok $case"
done
This is intended to aid unit testing outside of `upgrade --apply`. Signed-off-by: Colin Walters <[email protected]>
And add a single test which verifies that our internal `reboot` code actually does what it should (via systemd-run etc.) This took me way, way too long to do...there were so many missteps and confusion. First of all, I kept trying to use `systemd.extra-unit` from https://www.freedesktop.org/software/systemd/man/latest/systemd-debug-generator.html# but that doesn't exist in stream9. I spent way too long trying to debug the fact that switching from `podman run <image> /sbin/init` to `podman run <image> /bin/sh -c '<stuff> && exec /sbin/init` fails because in the latter case podman's auto-detection fails and we need to explicitly say `--systemd=always`. In retrospect obvious...but oh well. On the positive side, I think with some cleanup we could extend this model and generalize it for "test running in a container with systemd" (with a lot of cleanup really) Signed-off-by: Colin Walters <[email protected]>
2 participants
Followup to bd6b372