Fix flaky network-join tests: wait for an assigned address, not just link existence
The tap+relay intern-ping test and the DNS hostname-ping test both
flaked intermittently (roughly 1 in 13-20 runs), always in a "found_success
== false" shape with no assertion-level clue as to why.
Root cause, confirmed via a temporary production-code diagnostic (since
reverted) and cross-referenced against network_join.cpp's own code: the
readiness poll only waited for eth0 to *exist* (`ip link show eth0`), but
an interface can become visible before join_one_network()'s own later
`ip addr add`/`ip link set ... up` steps for it have actually run. A
script that used the interface as soon as it merely existed could ping,
get "Network unreachable" (no address yet), and exit almost immediately
-- and since a session's own sandboxed process is the sole occupant of
its pid/net namespace (--unshare-pid/--unshare-net), its exit destroys
that namespace outright. That, in turn, made whichever other nsenter
call was still in flight against the same namespace -- join_one_network()'s
own remaining steps, or the entirely separate per-session DNS resolver
(network_dns.cpp's start_dns_resolver(), which enters every networked
session's namespace regardless of whether --hostname was given) -- fail
with "No such file or directory" against a namespace that had already
collapsed underneath it.
This is the same general class of race network_join.{h,cpp}'s own
CLAUDE.md entry already documents (a very-short-lived sandboxed command
can outrun its own concurrent network setup), just one step further than
the eth0-existence race already fixed earlier in this file -- a test-code
issue, not a production bug. Fixed by polling for an actually assigned
address on eth0 instead of mere existence, in both wait_for_eth0_then()
and BackgroundPeer's own inline readiness script.
Verified with the diagnostic in place that the DNS resolver's own
namespace lookup was never itself stale, isolating the cause to the
script's own premature exit. Re-verified extensively after the fix:
8/8 isolated repeats of the previously-flaky tap+relay test, and 6
consecutive full [integration][root][net] suite runs (78 test-case
executions total) with no failures.
This commit is contained in:
@@ -137,7 +137,9 @@ private:
|
||||
// *second* container can be started concurrently to interact with it (e.g.
|
||||
// ping it) while it's still running. Waits for a "READY" marker on the
|
||||
// child's own stdout before returning, printed only once the child's own
|
||||
// script has confirmed its eth<N> actually exists -- join_networks()
|
||||
// script has confirmed its eth<N> actually has an assigned address (not
|
||||
// merely exists -- see wait_for_eth0_then()'s own doc comment below for why
|
||||
// that distinction matters) -- join_networks()
|
||||
// (network_join.cpp) runs *concurrently with*, not before, the sandboxed
|
||||
// command starting (bwrap execs straight into it, with no hook point in
|
||||
// between), so a container that used its interface immediately could
|
||||
@@ -163,8 +165,8 @@ public:
|
||||
args.network_specs = networks;
|
||||
args.hostname_flag = hostname;
|
||||
args.command = {"sh", "-c",
|
||||
fmt::format("for i in $(seq 1 20); do ip link show eth0 >/dev/null 2>&1 && break; "
|
||||
"sleep 0.5; done; echo READY; sleep {}",
|
||||
fmt::format("for i in $(seq 1 20); do ip -4 addr show eth0 2>/dev/null | "
|
||||
"grep -q 'inet ' && break; sleep 0.5; done; echo READY; sleep {}",
|
||||
lifetime_seconds)};
|
||||
AppConfig config = reload_config();
|
||||
dispatch_command(args, "/nonexistent/unused-config.yaml", config);
|
||||
@@ -224,25 +226,51 @@ private:
|
||||
bool ready_ = false;
|
||||
};
|
||||
|
||||
// Prefixes `command_after_eth0` with the same "wait for eth0 to actually
|
||||
// exist" poll BackgroundPeer's own script above uses, wrapped between the
|
||||
// Prefixes `command_after_eth0` with the same "wait for eth0 to actually be
|
||||
// usable" poll BackgroundPeer's own script above uses, wrapped between the
|
||||
// usual BEGIN/END-TEST-OUTPUT markers. Needed for *any* sandboxed command
|
||||
// that uses its network interface at all, not just BackgroundPeer's own --
|
||||
// join_networks() (network_join.cpp) runs concurrently with, not before,
|
||||
// the sandboxed command starting (bwrap execs straight into it, no hook
|
||||
// point in between), so a command that used eth0 immediately could
|
||||
// otherwise race its own join. Confirmed by testing, not assumed: an
|
||||
// earlier version of this file's own tests pinged immediately, and the
|
||||
// container's own near-instant "ping, fail, exit" (no eth0 yet) sometimes
|
||||
// raced ahead of join_one_network()'s own veth-move step, which then failed
|
||||
// outright trying to move a veth into an already-exited container's pid
|
||||
// ("Invalid netns value") -- the exact documented limitation
|
||||
// network_join.{h,cpp}'s own CLAUDE.md entry already describes for a
|
||||
// very-short-lived sandboxed command.
|
||||
// otherwise race its own join. Confirmed by testing, not assumed, in two
|
||||
// stages:
|
||||
//
|
||||
// 1. An earlier version of this file's own tests pinged immediately, and
|
||||
// the container's own near-instant "ping, fail, exit" (no eth0 yet)
|
||||
// sometimes raced ahead of join_one_network()'s own veth-move step,
|
||||
// which then failed outright trying to move a veth into an
|
||||
// already-exited container's pid ("Invalid netns value") -- the exact
|
||||
// documented limitation network_join.{h,cpp}'s own CLAUDE.md entry
|
||||
// already describes for a very-short-lived sandboxed command. Fixed by
|
||||
// polling for the interface's own *existence* first (`ip link show
|
||||
// eth0`).
|
||||
//
|
||||
// 2. That alone still wasn't enough: an interface can become visible (the
|
||||
// device exists, already moved/created) *before* join_one_network()'s
|
||||
// own later `ip addr add`/`ip link set ... up` steps for it have run.
|
||||
// A script that only waited for existence could still start pinging
|
||||
// (getting "Network unreachable", no address yet) and exit almost
|
||||
// immediately -- and since this sandboxed process is the pid/net
|
||||
// namespace's own sole occupant (`--unshare-pid`/`--unshare-net`), its
|
||||
// exit destroys that namespace outright, which then made join_one_
|
||||
// network()'s own *remaining* steps for that same network -- or, in one
|
||||
// observed case, the entirely separate per-session DNS resolver's own
|
||||
// nsenter call (network_dns.cpp's start_dns_resolver(), also entering
|
||||
// this same namespace) -- fail with "No such file or directory" against
|
||||
// a namespace that had already collapsed underneath them. Confirmed via
|
||||
// a temporary production-code diagnostic that the DNS resolver's own
|
||||
// namespace lookup was never itself stale (proc_exists was always true
|
||||
// right up to its own nsenter call), narrowing the cause to the
|
||||
// sandboxed script's own premature exit, not a namespace-resolution bug.
|
||||
// Fixed by polling for an actually *assigned address* on eth0 (`ip -4
|
||||
// addr show eth0 | grep -q 'inet '`) instead of mere existence -- this
|
||||
// only becomes true once join_one_network()'s full sequence for that
|
||||
// interface has completed, so the script no longer outruns its own join.
|
||||
std::string wait_for_eth0_then(const std::string& command_after_eth0) {
|
||||
return fmt::format(
|
||||
"echo BEGIN-TEST-OUTPUT; for i in $(seq 1 20); do ip link show eth0 >/dev/null 2>&1 && break; "
|
||||
"sleep 0.5; done; {}; echo END-TEST-OUTPUT",
|
||||
"echo BEGIN-TEST-OUTPUT; for i in $(seq 1 20); do ip -4 addr show eth0 2>/dev/null | "
|
||||
"grep -q 'inet ' && break; sleep 0.5; done; {}; echo END-TEST-OUTPUT",
|
||||
command_after_eth0);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user