From b23088a927203d15cb3498fef614f268a9468b53 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Fri, 2 Oct 2026 02:56:24 +0300 Subject: [PATCH 1/2] tests: pin the DIMT tombstone refusal by its dead ifindex; harden dplane_tid() (BLO-38883) Ally's suggestions on #128: - Assert the whole refusal, ifindex included. The tombstone keeps the dead link's index until main processes the RTM_DELLINK, so its FAIL_INSTALL / REMOVE_FAIL carry it. Had the hold lapsed and the owner's ADD replayed, a live entry would refuse a non-owner with the same codes but with 0 (still creating) or the new link's index; before, only the trailing window proof told those apart. With the index asserted that proof is redundant, so it goes, which also takes a vtysh round trip out of the 30 s hold. Measured at load ~60: 28.3-29.0 s of the hold is left after the non-owner block. - _held_delete() and _direct_answer() each carried the same popen / communicate(15) / kill / communicate(5) block; both now use _run_client(). Found while verifying them: dplane_tid() let stderr into its result. router.run() returns stderr with stdout, and a zebra thread that exits between the task glob and its comm read made `cat` print an error after the TID. _dplane_in_fd() then spliced "\ncat: ..." into its shell loop, which died as "syntax error near unexpected token `cat:'" -- a harness failure before any hold was armed, hit once on devbox under load and reproduced with a task dir whose comm is gone. Each read now drops its own stderr, and anything but one numeric TID is "not found", which every caller already reports. Signed-off-by: Omar Ramadan Co-Authored-By: Claude Opus 5.5 (1M context) --- .../test_zebra_dimt_tunnel.py | 88 +++++++++++-------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py b/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py index 0ac8203d49..36cf9e6df9 100644 --- a/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py +++ b/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py @@ -863,12 +863,24 @@ def assert_injection_fired(trace_output, what, tag="(INJECTED)"): def dplane_tid(router): - zebra_pid = router.run("cat /var/run/frr/zebra.pid").strip() - return router.run( + """The zebra_dplane pthread's TID, or "" when it cannot be found. + + router.run() returns stderr with stdout, so each read here discards its + own. A zebra thread that exits between the task glob and its comm read + used to make `cat` print its error into the result, and _dplane_in_fd() + then spliced "\\ncat: ..." into its shell loop, which died as a bash + syntax error near `cat:' instead of naming the problem. Anything but one + numeric TID is "not found", which every caller reports. + """ + zebra_pid = router.run("cat /var/run/frr/zebra.pid 2>/dev/null").strip() + if not zebra_pid.isdigit(): + return "" + tids = router.run( f"for task in /proc/{zebra_pid}/task/*; do " - '[ "$(cat $task/comm)" = zebra_dplane ] && basename "$task"; ' + '[ "$(cat $task/comm 2>/dev/null)" = zebra_dplane ] && basename "$task"; ' "done" - ).strip() + ).split() + return tids[0] if len(tids) == 1 and tids[0].isdigit() else "" def gre_in_fou_supported(router): @@ -1512,23 +1524,29 @@ def _prove_dellink_unread(router, name, ifindex, trace_file, when): _trace_text(router, trace_file))) -def _held_delete(router, tunnel_id): - """DEL `tunnel_id`; (parsed reply or None, returncode, stderr text).""" - delete = router.popen( - client_argv("del", tunnel_id), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) +def _run_client(router, argv): + """Run one dimt_zapi_client.py; (stdout text, stderr text, returncode). + + A client still waiting after 15s is killed, so a reply that never comes + cannot hold the test past the hold that is waiting on it. + """ + proc = router.popen(argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE) try: - stdout, stderr = delete.communicate(timeout=15) + stdout, stderr = proc.communicate(timeout=15) except subprocess.TimeoutExpired: - delete.kill() - stdout, stderr = delete.communicate(timeout=5) + proc.kill() + stdout, stderr = proc.communicate(timeout=5) + return _text(stdout), _text(stderr).strip(), proc.returncode + + +def _held_delete(router, tunnel_id): + """DEL `tunnel_id`; (parsed reply or None, returncode, stderr text).""" + stdout, stderr, rc = _run_client(router, client_argv("del", tunnel_id)) try: - removed = json.loads(_text(stdout)) + removed = json.loads(stdout) except ValueError: removed = None - return removed, delete.returncode, _text(stderr).strip() + return removed, rc, stderr def _direct_answer(router, action, tunnel_id, *extra): @@ -1539,21 +1557,14 @@ def _direct_answer(router, action, tunnel_id, *extra): --barrier) -- and `rest` is everything printed from the barrier on, so a caller can tell a refused request from one that went unanswered. """ - proc = router.popen( - client_argv(action, tunnel_id, "--barrier", BARRIER_TUNNEL_ID, - *extra), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + stdout, stderr, rc = _run_client( + router, + client_argv(action, tunnel_id, "--barrier", BARRIER_TUNNEL_ID, *extra), ) - try: - stdout, stderr = proc.communicate(timeout=15) - except subprocess.TimeoutExpired: - proc.kill() - stdout, stderr = proc.communicate(timeout=5) - lines = [json.loads(line) for line in _text(stdout).splitlines() if line] + lines = [json.loads(line) for line in stdout.splitlines() if line] cut = next((i for i, line in enumerate(lines) if line == {"barrier": True}), len(lines)) - return lines[:cut], lines[cut:], proc.returncode, _text(stderr).strip() + return lines[:cut], lines[cut:], rc, stderr def _held_before_delete_result(router, name, trace_file, removed): @@ -1728,20 +1739,25 @@ def test_add_after_removed_before_dellink_is_parked(): # The tombstone answers only its owner (BLO-38883). The owner # key is proto + instance and every request is pinned to PIM, # so the stranger is PIM instance 1. + # + # A refusal must carry the dead link's index: the tombstone + # keeps it until main processes the RTM_DELLINK. Had the hold + # lapsed and the owner's ADD replayed, a live entry would + # refuse with the same codes but with 0 (still creating) or the + # new link's index, so the index is what proves these came + # from the tombstone -- without another vtysh round trip + # inside the hold. for action, refused in (("add", 1), ("del", 3)): answers, rest, rc, stderr = _direct_answer( router, action, tunnel_id, "--instance", 1 ) assert rc == 0 and rest == [{"barrier": True}], ( action, answers, rest, rc, stderr) - assert [(a["tunnel_id"], a["result"]) for a in answers] == [ - (tunnel_id, refused) - ], ("a non-owner {} against the tombstone was not refused " - "before its barrier: {}".format(action, answers)) - _prove_dellink_unread( - router, name, ifindex, trace_file, - "after the non-owner's requests", - ) + assert answers == [{ + "tunnel_id": tunnel_id, "ifindex": ifindex, + "result": refused, + }], ("a non-owner {} against the tombstone was not refused " + "before its barrier: {}".format(action, answers)) finally: if tracer is not None: stop_tracer(tracer) From 8a915ce9c9b79fdcd7bd2addc2fbda4a13ca7802 Mon Sep 17 00:00:00 2001 From: Omar Ramadan Date: Fri, 2 Oct 2026 03:10:41 +0000 Subject: [PATCH 2/2] tests: resolve the dplane TID once per hold (BLO-38883) hold_dplane_sendmsg() and hold_dplane_in_recvmsg() each resolved the zebra_dplane TID to aim trace-fds, then _hold_dplane_syscalls() resolved it again for -p. A first resolve that missed while the second succeeded armed the hold with trace_fds=None and no warning, the unnarrowed BLO-28405 mis-aim. The callers now pass the FD resolver, and _hold_dplane_syscalls() applies it to the one TID it resolves and pytest.fails on. Every dplane_tid() caller now reports the empty sentinel, which makes the docstring's claim true. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../zebra_dimt_tunnel/test_zebra_dimt_tunnel.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py b/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py index 36cf9e6df9..a38bab8071 100644 --- a/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py +++ b/tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py @@ -615,12 +615,11 @@ def hold_dplane_sendmsg(router, delay_usecs): decoded sendmsg, so callers synchronize positively with dellink_state() instead of guessing with sleeps. """ - worker = dplane_tid(router) trace_file = "/tmp/dimt-sendmsg-trace-{}.log".format(os.getpid()) router.run("rm -f {}".format(trace_file)) tracer = _hold_dplane_syscalls( router, "sendmsg", "delay_enter", delay_usecs, trace_file, - trace_fds=_route_netlink_fds(router, worker) if worker else None, + trace_fds_for=_route_netlink_fds, ) return tracer, trace_file @@ -730,12 +729,11 @@ def hold_dplane_in_recvmsg(router, delay_usecs): them; callers prove the rest from zebra's and the kernel's own state. Returns (tracer, trace_file). """ - worker = dplane_tid(router) trace_file = "/tmp/dimt-recvmsg-trace-{}.log".format(os.getpid()) router.run("rm -f {}".format(trace_file)) tracer = _hold_dplane_syscalls( router, "recvmsg", "delay_enter", delay_usecs, trace_file, - trace_fds=_dplane_in_fd(router, worker) if worker else None, + trace_fds_for=_dplane_in_fd, ) return tracer, trace_file @@ -752,13 +750,18 @@ def held_recvmsg_lines(router, trace_file): def _hold_dplane_syscalls(router, syscalls, inject_kind, delay_usecs, - trace_file=None, trace_fds=None): + trace_file=None, trace_fds_for=None): + # trace_fds_for(router, worker) aims the hold at the worker's FDs. It is + # applied to the one TID resolved here, so the narrowing and the -p target + # cannot come from two different resolves: a caller-side lookup that missed + # while this one succeeded used to arm the hold unnarrowed (BLO-28405). require_strace(router) worker = dplane_tid(router) if not worker: pytest.fail( "zebra_dplane worker not found -- cannot hold the dplane worker" ) + trace_fds = trace_fds_for(router, worker) if trace_fds_for else None cmd = [ "strace", "-qq",