Repository navigation
test(e2e): capture host-netns address attribution and a healthy-pod reference for the dataplane collector - #3617
Conversation
github-actions
Bot
added
area/testing
|
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 dataplane capture script selects healthy same-node reference pods, records host addresses and routes, and reuses reference captures. Tests cover pod selection, hostNetwork exclusion, affected-pod exclusion, cache reuse, transient lookup failures, and unknown node pod-list results. ChangesDataplane reference capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AffectedPodLoop
participant capture_pod_reference
participant pod_first_ready
participant capture_pod_dataplane
AffectedPodLoop->>capture_pod_reference: request same-node reference
capture_pod_reference->>pod_first_ready: select eligible pod
pod_first_ready-->>capture_pod_reference: return healthy pod or lookup result
capture_pod_reference->>capture_pod_dataplane: capture reference diagnostics
capture_pod_dataplane-->>AffectedPodLoop: append labeled reference capture
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-capture-dataplane.sh`:
- Around line 525-536: Update the reference selection flow around _cpr_ref and
pod_first_ready to run the kubectl get pods query through timeout while
capturing stderr and preserving its exit status before awk/filtering.
Distinguish query failures from successful queries with no eligible pod, and
write a “reference selection failed” artifact section containing the captured
error only for command failures; retain the existing “none found” section only
when the query succeeds without an eligible pod.
- Around line 408-414: Update the host netns route-capture block to run an
additional IPv6 query after the existing IPv4 command: invoke kubectl exec with
`ip -6 route show table local`, preserving the same namespace, container,
timeout, stderr handling, and non-failing behavior so both address families are
captured.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9935c4a-dfdd-482b-80ec-d92126416757
📒 Files selected for processing (2)
hack/capture-dataplane.batshack/e2e-capture-dataplane.sh
| echo | ||
| echo "=== host netns: ip route show table local (address attribution) ===" | ||
| # A podIP resolving to `local <ip> dev lo table local` here is the | ||
| # fingerprint from the header block: it wins over the ovn0 route in | ||
| # `main` because table local is consulted first (see `ip rule` above). | ||
| timeout 15 kubectl exec -n "$KUBEOVN_NS" "$cni" -c cni-server -- \ | ||
| ip route show table local 2>&1 || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)hack/e2e-capture-dataplane\.sh$' || true
echo "== relevant section =="
if [ -f hack/e2e-capture-dataplane.sh ]; then
nl -ba hack/e2e-capture-dataplane.sh | sed -n '370,430p'
fi
echo "== surrounding ip route / host netns context =="
rg -n "route show table local|ip -6 route|table local|podIP|dual|address attribution" hack/e2e-capture-dataplane.sh hack -S --glob '!vendor/**' --glob '!node_modules/**' | head -200
echo "== ip help/default family probe in container =="
command -v ip >/tmp/ip-paths.txt && cat /tmp/ip-paths.txt
if command -v ip >/dev/null 2>&1; then
ip help 2>&1 | sed -n '1,120p' | rg -n "ip route|family|inet|inet6|Default family" || true
fiRepository: cozystack/cozystack
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== available ip command and family help probe =="
command -v ip >/tmp/ip-paths.txt && cat /tmp/ip-paths.txt
if command -v ip >/dev/null 2>&1; then
ip help 2>&1 | sed -n '1,140p' | rg -n "route|family|inet|inet6|Default|Default family" || true
fiRepository: cozystack/cozystack
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant section =="
awk 'NR>=370 && NR<=435 { printf "%6d\t%s\n", NR, $0 }' hack/e2e-capture-dataplane.sh
echo "== address-attribution/ip route context =="
awk '/(route show table local|table local|address attribution|podIP|dual|inet6)/ { printf "%6d\t%s\n", NR, $0 }' hack/e2e-capture-dataplane.sh
echo "== ip help probe =="
if command -v ip >/tmp/ip-paths.txt; then
echo "ip path: $(cat /tmp/ip-paths.txt)"
ip help 2>&1 | sed -n '1,160p' | grep -nE 'route|family|inet|inet6|Default|usage: ip route' || true
else
echo "ip command not found in sandbox"
fiRepository: cozystack/cozystack
Length of output: 6184
🌐 Web query:
ip-route man page: default family inet inet6 ip route show table local
💡 Result:
In the iproute2 suite, the ip command uses a hierarchical structure where the address family (if specified) precedes the object being managed [1][2]. The syntax you referenced, ip route show table local, is used to inspect the special "local" routing table [3][4]. Understanding the Syntax Components: 1. Family (inet vs. inet6): The family parameter specifies the protocol family [1][2]. - -4 or -family inet (default): Limits operations to IPv4 [5][6]. - -6 or -family inet6: Limits operations to IPv6 [5][6]. If you do not specify a family, the command defaults to IPv4 [6]. For example, to view the local IPv6 routing table, you would use: ip -6 route show table local 2. ip route show: This command displays the contents of the routing tables [1][2]. While show is the default action if omitted, it is often included for clarity [1][2]. 3. table local: This directs the command to a specific routing table [3]. - The local table (ID 255) is a special-purpose, system-maintained table containing high-priority routes for local and broadcast addresses [2][7][8]. - The kernel automatically populates and maintains this table; administrators generally should not modify it [2][8]. - It is separate from the main table (ID 254), which is where most user-configured and dynamic routes are stored [2][3]. By combining these, you can inspect the IPv4 or IPv6 local routing tables specifically, which is useful for troubleshooting host-local traffic, loopback addresses, and broadcast routes [3][4][9].
Citations:
- 1: https://man7.org/linux/man-pages/man8/ip-route.8.html
- 2: https://manpages.debian.org/bookworm/iproute2/ip-route.8.en.html
- 3: https://www.simplified.guide/iproute2/route-show-table
- 4: https://mulatinho.net/posts/how-linux-routing-tables-works/
- 5: https://opsgrep.com/linux-show-routes-cli-command-reference/
- 6: https://kb.axx.no/ip-route-cheatsheet/
- 7: https://manpages.ubuntu.com/manpages/xenial/man8/ip-route.8.html
- 8: https://serverfault.com/questions/851221/what-is-the-local-routing-table-used-for
- 9: http://linux-ip.net/linux-ip/tools-ip-route.html
🌐 Web query:
iproute2 ip route show table local default family inet inet6
💡 Result:
The command ip route show table local is used to display the contents of the Linux kernel's local routing table [1][2]. ### Overview The local table (often identified by ID 255) is a special-purpose routing table maintained automatically by the Linux kernel [3][4][5]. It contains high-priority routes required for the system to function, specifically those related to local interface addresses, broadcast addresses, and loopback traffic [3][6][5]. ### Command Breakdown * ip route: The primary utility for managing and displaying the Linux kernel routing table [2]. * show: Lists the routes within the specified routing table [3][7]. * table local: Targets the built-in local routing table [1]. While the system typically defaults to the main table (ID 254) for standard routing commands, specifying local allows you to inspect the protected kernel-maintained entries [3][8][9]. ### Contextual Notes * Kernel Maintenance: The kernel manages this table dynamically; it automatically adds or removes entries as interface addresses are configured or removed [6][5]. Administrators generally do not need to modify this table manually [3][4][5]. * Visibility: While route (from the older net-tools package) cannot display this table, ip route show table local provides full visibility into it [6]. * Additional Flags: The family inet and family inet6 flags can be appended to the command to filter the output specifically for IPv4 (inet) or IPv6 (inet6) addresses, respectively [7]. Regarding your query for ip route show table local default family inet inet6, please note: * default is a separate built-in routing table (ID 253) [10]. If you intended to view both the local and default tables, you would need to run separate commands or use table all to list all tables [3][2]. * The family flag syntax is generally ip -4 route show table local or ip -6 route show table local rather than family inet [7]. Summary of table IDs: * local: 255 [3][10] * main: 254 [3][10] * default: 253 [10]
Citations:
- 1: https://www.simplified.guide/iproute2/route-show-table
- 2: https://oneuptime.com/blog/post/2026-03-20-display-routing-table-ip-route-show/view
- 3: https://man.archlinux.org/man/ip-route.8
- 4: https://dyn.manpages.debian.org/trixie/iproute2/ip-route.8.en.html
- 5: http://linux-ip.net/linux-ip/routing-tables.html
- 6: http://linux-ip.net/linux-ip/tools-ip-route.html
- 7: https://man7.org/linux/man-pages/man8/ip-route.8.html
- 8: https://manpages.debian.org/bullseye-backports/iproute2/ip-route.8.en.html
- 9: https://github.com/cilium/iproute2/blob/866e1d10/man/man8/ip-route.8.in
- 10: https://kernel.org/doc/Documentation/networking/policy-routing.txt
Capture both IPv4 and IPv6 local routes.
ip route show table local defaults to IPv4 when the command has no address-family selector. A dual-stack failure with an IPv6 pod IP can miss the local <ip> dev lo table local entry, so add ip -6 route show table local after the current command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e-capture-dataplane.sh` around lines 408 - 414, Update the host netns
route-capture block to run an additional IPv6 query after the existing IPv4
command: invoke kubectl exec with `ip -6 route show table local`, preserving the
same namespace, container, timeout, stderr handling, and non-failing behavior so
both address families are captured.
dc5928c to
af76088
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-capture-dataplane.sh`:
- Around line 351-357: Align the MAX_PODS default with the per-exec capture
budget so the total worst-case work for affected pods fits within the Chainsaw
caller’s 300-second timeout. Reduce MAX_PODS from 12 to an appropriate bound
based on the documented 135-second per-pod maximum, or explicitly document the
resulting shorter effective capture envelope if retaining the current value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96f61684-b02a-45de-a57c-00d33af488bd
📒 Files selected for processing (2)
hack/capture-dataplane.batshack/e2e-capture-dataplane.sh
af76088 to
4033ef1
Compare
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-capture-dataplane.sh`:
- Around line 817-823: Update the per-node cached reference handling around
_cpr_ref so the cached reference identity is compared with the current affected
pod before reuse. When the cached reference names the current pod, invalidate or
bypass the cache and recompute the baseline using the current pod’s
namespace/name exclusion; otherwise preserve the existing cached capture
behavior.
- Around line 825-840: Update the `_cpr_ref`-empty handling to write the
“unknown” record to `_cpr_of` instead of `_cpr_sink` when `_cpr_rc` is non-zero,
and do not populate or reuse the per-node cache for failed pod-list reads.
Preserve the existing cache behavior for successful reads and the current output
for cases where no reference pod is found.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88988fbc-8b95-424c-a447-dcd093767d62
📒 Files selected for processing (2)
hack/capture-dataplane.batshack/e2e-capture-dataplane.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/capture-dataplane.bats
| # The affected pod is excluded by name as well as by state. `pod_first_ready` | ||
| # already rejects it for not being Ready, but the two lists are read at | ||
| # different moments: a pod that recovered between the cluster-wide list and | ||
| # this one would otherwise be offered as the baseline for its own failure. | ||
| _cpr_ref=$(printf '%s\n' "$_cpr_raw" \ | ||
| | awk -F'|' -v sns="$_cpr_ns" -v spod="$_cpr_pod" '$1 == sns && $2 == spod { next } { print }' \ | ||
| | pod_first_ready) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The name-based exclusion applies to the first affected pod only.
The comment at Lines 817-820 states that the affected pod is excluded by name because a pod that recovered between the two list reads could otherwise become its own baseline. The cache defeats that guard for later pods: the reference is chosen once per node, using the first affected pod's identity, and every later affected pod on the node receives the same cached capture. If a later affected pod recovered before its own reference selection would have run, the cached baseline can name that same pod.
Compare the cached reference identity against the current pod, and recompute when they match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e-capture-dataplane.sh` around lines 817 - 823, Update the per-node
cached reference handling around _cpr_ref so the cached reference identity is
compared with the current affected pod before reuse. When the cached reference
names the current pod, invalidate or bypass the cache and recompute the baseline
using the current pod’s namespace/name exclusion; otherwise preserve the
existing cached capture behavior.
4033ef1 to
9b695f2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/capture-dataplane.bats (1)
926-928: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the baseline body for the first pod too.
Line 928 checks
POD tenant/healthyonly inwedged-b.txt. The first pod writes the capture into the cache and then copies it into its own file, so that copy is the step most likely to regress. Add the same assertion forwedged-a.txt.♻️ Proposed change
grep -q 'REFERENCE (Ready)' "$d/out/pod-tenant-test-wedged-a.txt" grep -q 'REFERENCE (Ready)' "$d/out/pod-tenant-test-wedged-b.txt" + grep -q 'POD tenant/healthy' "$d/out/pod-tenant-test-wedged-a.txt" grep -q 'POD tenant/healthy' "$d/out/pod-tenant-test-wedged-b.txt"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/capture-dataplane.bats` around lines 926 - 928, Add the missing baseline-body assertion alongside the existing checks in the capture test, verifying that pod/healthy appears in pod-tenant-test-wedged-a.txt as well as wedged-b.txt. Keep the existing reference-readiness assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hack/capture-dataplane.bats`:
- Around line 926-928: Add the missing baseline-body assertion alongside the
existing checks in the capture test, verifying that pod/healthy appears in
pod-tenant-test-wedged-a.txt as well as wedged-b.txt. Keep the existing
reference-readiness assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 414169fd-ff55-4bd5-a2b1-431dc9930164
📒 Files selected for processing (2)
hack/capture-dataplane.batshack/e2e-capture-dataplane.sh
e6f8d6b to
1cb53c3
Compare
github-actions
Bot
removed
the
size/XL
github-actions
Bot
added
the
size/XXL
1cb53c3 to
e09d22d
Compare
…eference The collector characterises the host->local-pod route for an affected pod through ovn0 alone, so when a pod IP turns out to be a local address of the host netns instead, nothing records which interface actually holds it. The host netns capture now also takes `ip -o addr show` across every interface and `ip route show table local`, so the owner can be named on the next occurrence rather than only ruled out on ovn0. The collector also inspects NotReady pods only, so a run has no same-window baseline for what a normal route and conntrack state look like on the same node. The per-pod capture becomes a reusable function and runs a second time against one Ready, Running, non-hostNetwork pod on that node, into the same file, so the comparison is in front of whoever opens it. hostNetwork is excluded because such a pod's podIP IS the node's own address and would show the same local-route fingerprint as the fault for an entirely ordinary reason. That baseline depends on the node and nothing else -- the candidates are the rows of the one snapshot the affected set was computed from, and a pod reporting Ready there is by definition not one this run is investigating -- so it is captured once per node and folded into each affected pod's file from a cache. Several affected pods on one node is what a wedged host->local-pod datapath looks like rather than an edge case, and capturing the same healthy pod once per affected pod would spend the caller's envelope re-asking a question already answered -- the cost the node-global dedup and the lookup memo beside it exist to avoid. This adds no read of its own. The cluster-wide pod list gains one column instead of being asked a second time, and that single read's status is what lets the artifact separate a node with no eligible pod from a list that never answered: one line for both would make an unchecked baseline read as an established absence. The two host-netns additions are best-effort execs, like the captures they sit beside. The new row filter carries the field count its jsonpath emits, so a list cut mid-record cannot hand over a fragment as the pod every reading of the affected one is compared against. The unit suite pins these on what they are rather than on what they leave behind. Both host-netns captures are asserted through the kubectl argv the collector issues, container included, because the section headers are separate echoes that outlive the exec being removed. The baseline is pinned from both sides: several affected pods on one node share a single capture, and an affected pod is served the healthy pod from its own node rather than from whichever node the walk reached first. The pod list's column order is asserted against the positions its readers index, because every runtime test answers that list from a stub written by hand and none of them can observe the jsonpath at all. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
e09d22d to
18be4ee
Compare
1 participant
What this PR does
Extends
hack/e2e-capture-dataplane.sh, the diagnostic collector that runs after an e2e test failure, with the instrumentation issue #3615 asks for to close its address-attribution gap.Issue #3615 found that a NotReady pod's IP can turn out to be a local address of the node's host netns instead of reachable through the ovn0 route, which makes kubelet's probes connect to the node's own loopback and fail forever. The collector already captured
ip addr show ovn0for the affected pod's node, but nothing that could name which interface actually holds a stolen address, since it only ever looked at ovn0.This adds
ip -o addr show(every interface) andip route show table localto the same host netns capture, so the interface actually holding a local address can be identified on the next occurrence.The collector also only ever inspected NotReady pods, so a run had no same-window baseline for what a normal route and conntrack state look like on the same node. This extracts the existing per-pod capture into a reusable function and runs it a second time against one Ready, Running pod on the same node as an in-run healthy-pod control, picked while explicitly excluding hostNetwork pods (their podIP is the node's own address, so one would show the exact same local-route fingerprint as the bug for an unrelated reason). When no eligible Ready pod exists on the node, that absence is recorded in the artifact instead of left silent.
This is diagnostic tooling only: it changes what gets captured after a failure, not any test's pass/fail outcome.
Known limitations
The baseline runs at a reduced scope: the host route and the kernel conntrack for the healthy pod's address, and nothing else. What it drops -- cilium CT, the OVN port binding, the OVS
ovn-installedflag, and the pod's own Ready conditions and events -- describes how a pod was programmed, and a pod picked for being Ready and Running is programmed correctly by definition. That scope is what holds the pod path's worst case at 579s against the 600s thecozytest.shtrap allows; the remaining 21 seconds are what this path leaves for the LoadBalancer section that runs after it, not headroom the run has. The other caller's envelope is 300s, and the worst case does not fit it -- nor did it at the merge base, where the same walk bounds at 519s. That overrun is tracked in #3671, and the term-by-term arithmetic is in theMAX_PODSnote in the script.The 579s above assumes the lookup memo exists. When
mktempfails the memo is empty, every per-node pod lookup runs live again, and the same single-pod case bounds at 667s -- past the 600s envelope as well as the 300s one. At the merge base that memo-less case bounds at 585s, inside 600s, so this change is what moves it out. The degradation is visible rather than silent: the caller's backstop truncates the capture and the run reportsdata-plane capture INCOMPLETE (exit 124).A baseline can be present and still carry nothing. Every capture at that scope goes through the node's
kube-ovn-cnipod, so when that lookup does not resolve, the reference section is its banner and the scope note with nothing beneath them -- and, unlikenode-<node>.txtnext to it, no line saying why. Two different standards of honesty inside one artifact. The affected-pod path has the same shape today, so this extends an existing gap rather than opening one, and it is left alone here.The per-node baseline is cached under
TMPDIRfor the run. A SIGKILL -- which is what a caller's wall-clock backstop delivers -- skips the cleanup at the end of the script and leaves that directory behind. The lookup memo this script already used has the same exposure.Nodes are visited in the order their affected pods appear in the cluster-wide list, which the API chooses. Once
MAX_PODScuts the walk, which pods and therefore which nodes got captured is a function of that ordering and not of anything a reader would pick.Screenshots
Not applicable, no UI change.
Downstream repositories
Release note
Summary by CodeRabbit
New Features
Tests