smoke: namespace isolation and parallel execution - #508
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates CI and smoke-test orchestration. CI: check.sh switches to /bin/bash, wraps build/test/smoke invocations with timing and runs smoke-tests in parallel; check.yml flips Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smoke/_init.sh`:
- Around line 84-90: The stop_grout function currently unconditionally runs kill
%?grcli and wait %?grcli which will error when no grcli background job exists
(e.g., INTERACTIVE=false); wrap those two calls in a guard that checks for the
job before acting (for example: use jobs %?grcli >/dev/null 2>&1 or jobs -p
%?grcli to detect existence) and only run kill %?grcli and wait %?grcli if the
job check succeeds so cleanup won’t print errors or abort under set -e.
- Around line 6-17: The mount and namespace setup commands are unreachable
because `exec env _SMOKE_UNSHARED=1 unshare ... "$0" "$@"` replaces the process
before lines like `mount --make-rprivate /` run; move those mount/setup commands
so they execute in the re-entered (unshared) process when `_SMOKE_UNSHARED=1`.
Concretely: keep the `if [ "${_SMOKE_UNSHARED:-}" != 1 ]; then exec env
_SMOKE_UNSHARED=1 unshare ... "$0" "$@"; fi` guard as the initial re-exec, and
then place the `mount --make-rprivate /`, `mkdir -p /run/netns`, `mount -t tmpfs
tmpfs /run/netns`, `touch /run/netns/host`, `mount --bind /proc/1/ns/net
/run/netns/host`, and `mount -t proc proc /proc` after that `fi` so they run
when `_SMOKE_UNSHARED=1`; reference `_SMOKE_UNSHARED` and the `exec env ...
unshare` line to locate where to move the commands.
- Around line 97-124: The stop routine currently loses the grout exit code and
always treats GDB as enabled; capture the exit status immediately after wait
"$grout_pid" by moving/localizing ret right after that wait (before any
subsequent commands like the GDB tmux block) and change the GDB check from the
truthy string test if [ "${GDB:-true}" ] to an explicit comparison such as if [
"${GDB:-true}" = "true" ] or [[ "${GDB:-true}" == "true" ]] so tmux kill-window
-t gdb only runs when GDB is actually enabled; update references around kill
-TERM "$grout_pid", wait "$grout_pid", local ret="$?" and tmux kill-window -t
gdb accordingly.
rjarry
force-pushed
the
smoke-parallel
branch
2 times, most recently
from
February 17, 2026 15:09
f6b0192 to
aaf7650
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@smoke/_init.sh`:
- Around line 6-17: The mount commands are unreachable because exec replaces the
process; relocate the block that performs mount --make-rprivate /, mkdir -p
/run/netns, mount -t tmpfs tmpfs /run/netns, touch /run/netns/host, mount --bind
/proc/1/ns/net /run/netns/host, and mount -t proc proc /proc so they run after
the if/fi (on re-entry when _SMOKE_UNSHARED=1). Concretely, keep the exec
unshare invocation as-is, then move the sequence of mount/mkdir/touch/mount
--bind/mount -t proc lines out of the if-body and place them immediately after
the fi so they execute in the child namespace where _SMOKE_UNSHARED is set.
- Around line 84-90: The stop_grout function currently runs "kill %?grcli" and
"wait %?grcli" which will error if no background grcli job exists; modify
stop_grout to first check for the existence of the job before killing/waiting
(for example test jobs -p %?grcli or inspect "jobs" output) and only call
kill/wait when that check succeeds, or alternatively use a safe process lookup
(pgrep/pkill with the grcli command pattern) to conditionally terminate; update
references in stop_grout to perform this guard so cleanup does not emit errors
when grcli was never started.
- Around line 97-104: The current assignment local ret="$?" occurs after the
if/fi that may run tmux and thus captures that block's exit code instead of the
grout process exit from wait "$grout_pid"; move the capture so you save the exit
status immediately after wait "$grout_pid" (e.g., right after the wait command
capture into ret from "$?") and only then run the conditional tmux block so the
grout exit code (from wait) is preserved; reference the commands kill -TERM
"$grout_pid", wait "$grout_pid", the local ret assignment, and the tmux if/fi
when making this change.
rjarry
force-pushed
the
smoke-parallel
branch
4 times, most recently
from
February 17, 2026 15:59
81da3f8 to
c7c207f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/check.sh:
- Line 30: The echo line in check.sh prints a stray closing parenthesis; update
the echo command that currently uses "($(nproc)) parallel jobs)" to remove the
extra trailing ')' so the output becomes e.g. "smoke-tests (4) parallel jobs"
(for example change to echo "smoke-tests ($(nproc)) parallel jobs"). Locate the
echo statement in .github/workflows/check.sh and adjust the quoted string
accordingly.
In `@smoke/_init.sh`:
- Around line 299-301: The current calculation of cpu and affinity can divide by
zero when nproc returns 1; update the logic to first capture num_cpus="$(nproc)"
and branch: if num_cpus is <= 1 set cpu=0 and affinity="$cpu" (or "$cpu,$cpu"),
otherwise compute cpu="$((RANDOM % (num_cpus - 1)))" and
affinity="$cpu,$((cpu+1))". Change references to the variables cpu and affinity
accordingly so $RANDOM % (nproc - 1) is never evaluated with zero.
rjarry
force-pushed
the
smoke-parallel
branch
2 times, most recently
from
February 17, 2026 22:42
7d16915 to
7b601b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smoke/_init.sh`:
- Around line 100-121: The wait/ret error-check block incorrectly runs in GDB
mode because grout_pid is a non-child PID; update the shutdown sequence to skip
the wait and error/status handling when GDB mode is enabled: detect GDB via the
existing "${GDB:-false}" check and, if true, avoid calling wait "$grout_pid",
avoid setting status or interpreting ret, and skip the coredumpctl/debugger
steps (leaving the tmux gdb cleanup intact); ensure normal behavior remains
unchanged when GDB is not true and continue to use grout_pid, wait, ret, status,
and coredumpctl in that branch.
---
Duplicate comments:
In `@smoke/_init.sh`:
- Around line 283-285: The current computation cpu="$(($RANDOM % ($(nproc) -
1)))" will divide by zero when nproc returns 1; change the logic in this block
to first read total="$(nproc)" and handle the single-CPU case by setting cpu=0
(or cpu=0 and affinity="$cpu" or "$cpu,$cpu") when total <= 1, otherwise compute
cpu as RANDOM % (total - 1) and set affinity="$cpu,$((cpu+1))"; update the
variables cpu and affinity (the ones referenced in this snippet) accordingly to
avoid the modulo by zero.
Comment on lines
+100
to
+121
| kill -TERM "$grout_pid" | ||
|
|
||
| set +x | ||
| echo "Waiting for grout (PID $grout_pid) to terminate ..." | ||
| wait "$grout_pid" | ||
| local ret="$?" | ||
|
|
||
| if [ "${GDB:-false}" = true ]; then | ||
| tmux kill-window -t gdb | ||
| fi | ||
|
|
||
| if [ "$ret" -ne 0 ]; then | ||
| status="$ret" | ||
| if [ "$ret" -gt 128 ]; then | ||
| local sig=$((ret - 128)) | ||
| echo "fail: grout terminated by signal SIG$(kill -l $sig)" | ||
| else | ||
| echo "fail: grout exited with an error status $ret" | ||
| fi >&2 | ||
| coredumpctl debug --no-pager -q "$grout_pid" \ | ||
| --debugger-arguments="-batch -ex 'thread apply all bt'" | ||
| fi |
There was a problem hiding this comment.
wait "$grout_pid" in GDB mode operates on a non-child PID — returns 127, falsely marking every GDB run as failed.
In GDB mode (line 302 branch), grout_pid is obtained via pgrep -P $gdb_pid — grout is a grandchild of the current shell started through tmux new-window, not a direct child. In bash, wait <pid> on a non-child process returns 127 immediately. local ret="$?" on line 105 captures 127, the ret -ne 0 branch at line 111 triggers, status is overwritten with 127, and the test is reported as FAILED even when grout exited cleanly.
The fix is to skip the wait/error-check block entirely in GDB mode:
🐛 Proposed fix
kill -TERM "$grout_pid"
set +x
echo "Waiting for grout (PID $grout_pid) to terminate ..."
- wait "$grout_pid"
- local ret="$?"
if [ "${GDB:-false}" = true ]; then
+ wait "$grout_pid" 2>/dev/null || true
tmux kill-window -t gdb
- fi
-
- if [ "$ret" -ne 0 ]; then
- status="$ret"
- if [ "$ret" -gt 128 ]; then
- local sig=$((ret - 128))
- echo "fail: grout terminated by signal SIG$(kill -l $sig)"
- else
- echo "fail: grout exited with an error status $ret"
- fi >&2
- coredumpctl debug --no-pager -q "$grout_pid" \
- --debugger-arguments="-batch -ex 'thread apply all bt'"
+ else
+ wait "$grout_pid"
+ local ret="$?"
+ if [ "$ret" -ne 0 ]; then
+ status="$ret"
+ if [ "$ret" -gt 128 ]; then
+ local sig=$((ret - 128))
+ echo "fail: grout terminated by signal SIG$(kill -l $sig)"
+ else
+ echo "fail: grout exited with an error status $ret"
+ fi >&2
+ coredumpctl debug --no-pager -q "$grout_pid" \
+ --debugger-arguments="-batch -ex 'thread apply all bt'"
+ fi
fi
set -x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@smoke/_init.sh` around lines 100 - 121, The wait/ret error-check block
incorrectly runs in GDB mode because grout_pid is a non-child PID; update the
shutdown sequence to skip the wait and error/status handling when GDB mode is
enabled: detect GDB via the existing "${GDB:-false}" check and, if true, avoid
calling wait "$grout_pid", avoid setting status or interpreting ret, and skip
the coredumpctl/debugger steps (leaving the tmux gdb cleanup intact); ensure
normal behavior remains unchanged when GDB is not true and continue to use
grout_pid, wait, ret, status, and coredumpctl in that branch.
aharivel
suggested changes
Feb 18, 2026
aharivel
left a comment
aharivel
left a comment
Collaborator
There was a problem hiding this comment.
Unshare isolation is elegant. I love the idea !
That's a significant improvement.
Just tiny minors nit picking..
| fi | ||
| if [ -t 1 ]; then | ||
| # try to spread load on all CPUs | ||
| cpu="$(($RANDOM % ($(nproc) - 1)))" |
Collaborator
There was a problem hiding this comment.
Single proc CPU are not existing anymore for sure but the division by zero is possible here !
If you think it should never happen, then it's fine.
Otherwise:
n=$(nproc)
cpu=$(($RANDOM % n))
affinity="$cpu,$(( (cpu + 1) % n ))"
Collaborator
Author
There was a problem hiding this comment.
if it ever happens, we have a bigger problem I think :D
| for name in "${tmux_windows[@]}"; do | ||
| tmux kill-window -t "$name" | ||
| done | ||
| kill %?grcli |
Collaborator
There was a problem hiding this comment.
I would put a kill %?grcli || true in case grcli failed at start.. (socket path issue, etc..) this would make it sure the script doesn't fail..
Collaborator
Author
There was a problem hiding this comment.
this code is already executed with set +e, so it is fine. I just moved it in a separate function for readability.
Collaborator
|
Tested-by: Roman Safronov rsafrono@redhat.com |
grout-bot
force-pushed
the
smoke-parallel
branch
from
February 18, 2026 16:24
7b601b8 to
98d4bb2
Compare
aharivel
self-requested a review
February 18, 2026 17:42
In preparation for interactive tmux debugging and parallel test execution, refactor the monolithic cleanup() function. Move the pause-for-debug logic and the grout stop/crash-detection sequence into standalone pause_for_debug() and stop_grout() functions. Introduce smoke_setenv() to export environment variables, which will be extended to propagate variables to tmux sessions. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
When INTERACTIVE=true, start a tmux session and open dedicated windows for each component: a grcli shell, one shell per network namespace, and vtysh sessions for each FRR instance. On test completion (pass or fail), pause before cleanup so the user can inspect state in any window. Extend smoke_setenv() to propagate environment variables into the tmux server so that all windows share the same GROUT_SOCK_PATH and PATH. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
When GDB=true, imply INTERACTIVE=true and start grout inside gdb in a dedicated tmux window. Configure gdb to pass SIGTERM through to grout so that stop_grout() can request a graceful shutdown. Wait indefinitely for grout to open its control socket, giving the user time to set breakpoints before issuing "run" in the gdb window. Once grout is running, find its PID by querying the gdb tmux pane PID and looking up its child process. For non-gdb code paths, capture grout's PID via $! right after backgrounding instead of using pgrep. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
The FRR install script already creates etc/frr, var/log/frr and var/lib/frr under the install prefix. Add var/run/frr with mode 0700 so that FRR daemons can write pid and socket files without relying on runtime directory creation. Remove the duplicate var/lib/frr mkdir that was created without explicit permissions. The next commit will overlay these directories with per-test tmpfs mounts for parallel test isolation. The mount points need to exist before the tmpfs can be mounted on top. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Replace the named "grout" netns with unshare --mount --net --fork to give each test its own mount tree and network stack. This enables parallel execution without name collisions on network namespaces or shared FRR state. The previous commits made it possible to interact with the test topology via tmux windows and grcli without relying on "ip netns exec grout" from outside. The named netns is no longer needed and can be replaced with an anonymous one from unshare --net. Mount isolation: * Make all mounts private to prevent propagation. * Mount a tmpfs on /run/netns so each test starts with a clean namespace registry, free of stale host entries. Guard the mounts with _SMOKE_MOUNTS_DONE so they only run once, even when the script is re-executed by tmux. * Pass the host netns as fd 3 (opened on /proc/1/ns/net before unshare) and bind-mount it to /run/netns/host so hardware port tests can move devices between the host and test network namespaces using "ip -n host". * Overlay FRR state directories (etc/frr, var/log/frr, var/run/frr, var/lib/frr) with per-test tmpfs mounts so parallel FRR tests do not collide and no root-owned files are left in the build tree. Use mode=1777 since FRR daemons drop privileges to the user that compiled them (--enable-user). Network isolation: * Each test gets a fresh network namespace from --net, removing the need for the named "grout" netns. * Sub-namespaces are created with plain "ip netns add" since we are already isolated. * Hardware ports are moved in/out via the "host" netns reference. Replace the core_pattern sysctl with coredumpctl for crash analysis. The core_pattern sysctl is global (not per-namespace), so parallel tests would race on it. Instead, rely on systemd-coredump and use "coredumpctl debug" to get full thread backtraces on crash. Move "ulimit -c unlimited" to the top so it applies to all processes including those started in tmux windows. Add systemd-coredump to the CI packages. Drop the explicit "ip addr add 127.0.0.1/8 dev lo" and the grep check that guarded it. The kernel automatically assigns 127.0.0.1/8 and ::1/128 when the loopback device is brought up (net/ipv4/devinet.c, NETDEV_UP handler, since Linux 2.6.12). Only "ip link set lo up" is needed. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Now that each test runs in its own set of namespaces, they can be executed in parallel without interference. Add individual phony make targets for each test script alongside the existing run.sh runner. FRR tests are filtered out when the frr meson option is not enabled, using the same jq check as run.sh. Each target runs the test under sudo with output captured to a temporary log. On success, the log is discarded. On failure, it is dumped to the terminal. The test name is printed before starting so progress is visible during parallel runs. Parallel execution is controlled from the command line: make smoke-tests # sequential make smoke-tests -j4 # 4 parallel tests make smoke-tests -j$(nproc) -k # all CPUs, continue on failure Spread CPU affinity across all available cores using a random offset instead of always pinning to CPUs 0,1. This avoids contention when multiple tests run in parallel. Increase the socat connection retry count from 10 to 30 since grout may take longer to start under heavy parallel load. Update the CI script to use make -j$(nproc) -k instead of the old sudo make smoke-tests invocation. The sudo is now in the make recipe itself. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Swap the rebase flags between the two gcc-14 CI jobs so that intermediate commits are smoke tested with debugoptimized builds instead of debug+asan builds. Running smoke tests on every intermediate commit with address sanitizer is too slow. The optimized build catches most regressions while keeping CI times reasonable. The asan job still runs on the final merge result. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
Display duration of compilation, unit and smoke tests. Signed-off-by: Robin Jarry <rjarry@redhat.com> Tested-by: Roman Safronov <rsafrono@redhat.com> Reviewed-by: Anthony Harivel <aharivel@redhat.com>
grout-bot
force-pushed
the
smoke-parallel
branch
from
February 18, 2026 17:43
98d4bb2 to
f6b020f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run each smoke test in its own mount and network namespace via unshare. This allows parallel execution with
make smoke-tests -jN, significantly reducing CI time.Along the way, add interactive debugging support:
INTERACTIVE=truestarts a tmux session with dedicated windows for grcli, network namespaces and vtysh.GDB=trueadditionally runs grout under gdb.Summary by CodeRabbit
Tests
Chores