test(e2e): capture per-node network diagnostics in cozyreport - #3213

Code Review

This pull request introduces a new section in hack/cozyreport.sh to collect per-node network diagnostics (such as iptables, conntrack, OVS flows, and Hubble/Cilium endpoint states) to help debug transient network probe failures. The review feedback suggests several shell scripting improvements: handling potential syntax errors in line-count checks, using standard POSIX flags for grep portability, replacing subshells running basename with POSIX shell parameter expansions for efficiency, and simplifying the directory cleanup logic by directly executing rmdir.

Actionable comments posted: 1

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/cozyreport.sh`:
- Around line 411-413: The kube-ovn report loops still call kubectl get without
a hang guard, so they can block the whole script on a slow API server. Update
the pod and node lookup logic in cozyreport.sh, including the kube-ovn CNI loop
that uses kubectl get pods and kubectl get ... -o jsonpath, to run those kubectl
get calls through timeout just like the kubectl exec captures. Keep the existing
fallback behavior for node names so the report continues even when a lookup
times out.

---

Nitpick comments:
In `@hack/cozyreport.sh`:
- Around line 417-432: The repeated `kubectl exec` plus `_keep_if_useful`
pattern should be refactored into a helper in `hack/cozyreport.sh` so the
repeated `command -v BIN ... && CMD || echo ...` logic is centralized. Update
the helper to distinguish “binary not present” from “command failed” instead of
using a single fallback message, and make the caller pass the command name and a
context-specific failure message. Apply this to the `iptables-save`,
`conntrack`, and other repeated artifact collection blocks so the generated
files report accurate root-cause details.

NOT LGTM — the plumbing is correct, but the section does not deliver two of the three diagnostics it promises, deletes genuine capture-failure evidence, and forks a divergent copy of a sibling capture whose output already lands in the same tarball.

Business context: cozyreport is blind to host-side networking state, so a recurrent probe-refused transient (pod bound and Ready, kubelet still gets connection refused) cannot be attributed after the fact from the uploaded artefact.

Credit first — every selector and container name is right, and I verified each against the shipped charts: app=kube-ovn-cni/cni-server (ovncni-ds.yaml:16,90), app=ovs/openvswitch (ovsovn-ds.yaml:22,85, correctly not matching app=ovs-dpdk), app.kubernetes.io/name=cilium-agent/cilium-agent (the label is on the pod template, daemonset.yaml:66,92). timeout 30 does match the sibling's heaviest capture (e2e-capture-dataplane.sh:266). What blocks is below the plumbing.

Blockers

B1: The Hubble capture can never produce output on the shipped platform

File: hack/cozyreport.sh:458-461
Issue: packages/system/cilium/values.yaml:13-14 sets hubble.enabled: false, overriding the upstream chart default of true (charts/cilium/values.yaml:1393). It is the only hubble key in the cozystack cilium config — nothing re-enables it.
Evidence: cilium-configmap.yaml:1036-1039 emits hubble-socket-path only under {{- if .Values.hubble.enabled }}, so with the flag off cilium-agent never creates /var/run/cilium/hubble.sock. The [ -S /var/run/cilium/hubble.sock ] guard therefore always fails, the echo "hubble not enabled or socket missing…" stub is written, and _keep_if_useful deletes it (single line, matches not (present|enabled|found)). Net result: two extra kubectl exec per node per report, zero bytes captured, on every cluster.
Impact: the block comment at :361-362 names hypothesis (b) — "blocked by a hostFirewall or Cilium policy drop that only Hubble sees" — as one of three the section exists to disambiguate. hostFirewall.enabled: true (values.yaml:11-12), so policy drops are a live hypothesis, and this ships the one instrument that cannot observe them. The comment at :367-369 also asserts every source has "the required binary preinstalled", which is not true for the hubble path.
Fix: capture drops with cilium-dbg monitor --type drop (needs no Hubble; the sibling already runs it, e2e-capture-dataplane.sh:306-311) and/or cilium-dbg bpf ct list global (sibling :414-416). Drop the hubble capture and the (b) claim, or enable Hubble.

B2: _keep_if_useful deletes real capture-failure evidence

File: hack/cozyreport.sh:399-409
Issue: the regex at :406 includes error: and Error from server. Since every capture redirects 2>&1, a failed kubectl exec writes its one-line error into the file — and the file is then silently removed.
Evidence: I ran the helper verbatim against real kubectl failure text. Error from server (BadRequest): pod … does not have a host assigned → deleted. error: unable to upgrade connection: container not found ("cilium-agent") → deleted. A legitimate header-only capture is correctly kept, so the deletions are specifically the exec failures.
Impact: a missing file now means "node healthy", "binary absent", or "the capture broke" — three different conclusions the artefact can no longer distinguish. An agent that is CrashLooping or Terminating (precisely the interesting case) leaves no trace at all. docs/agents/e2e-testing.md §8 is explicit that failure must be self-explanatory and diagnostics are first-class. The pruning is also inconsistent: a one-line OCI runtime error is deleted, but a multi-line exec error is kept and reads as a real capture.
Fix: keep the stubs, or fold them into one net-diag/capture-errors.txt index. Do not delete a record of a failed capture.

B3: conntrack drops the /proc/net/nf_conntrack fallback the sibling deliberately carries

File: hack/cozyreport.sh:428-430
Issue: command -v conntrack … || echo "conntrack not present" with no fallback.
Evidence: e2e-capture-dataplane.sh:429-433 does the same capture and explicitly comments "Prefer the conntrack CLI; fall back to /proc/net/nf_conntrack when it is absent from the image" — in-repo evidence that the cni-server image may not ship the CLI. If it doesn't, this emits a one-line "not present" stub, which B2 then deletes.
Impact: conntrack is hypothesis (a) in the block's own comment (:360-361, stale conntrack from a prior pod on the same IP) — the headline one. In the absent-CLI case the report carries zero conntrack evidence and zero indication of why.
Fix: port the /proc/net/nf_conntrack fallback.

B4: No aggregate timeout bound — the worst case can cost the entire artefact

File: hack/cozyreport.sh:388
Issue: each exec is bounded at 30 s, but the section has none in aggregate: 5 execs × N nodes. On a 3-node cluster with a wedged API server that is ~7.5 minutes appended to report collection, and it scales linearly with node count.
Evidence: the repo already recognised this hazard for this exact class of capture — hack/cozytest.sh:123 wraps the sibling in timeout -k 30 600. The new block gets no equivalent, and neither does its call site: packages/core/testing/Makefile:45 invokes cozyreport.sh bare, and the CI step (.github/workflows/pull-requests.yaml:674-678) is if: always() with no timeout-minutes, inside a job capped at 180 minutes (:495).
Impact: on a long full-suite run already near the cap, the added exec time can push the job past the timeout. The job is cancelled, Upload cozyreport.tgz (:680-685) never runs, and the whole diagnostic artefact is lost — in exactly the failure case this change is built for.
Fix: wrap the section in an outer timeout -k.

B5: No tests, in an area that has them

File: hack/cozyreport.sh:399-409
Issue: _keep_if_useful is a pure, self-contained, file-deleting helper whose semantics are a 7-alternative regex plus an arbitrary ≤ 1 line threshold. A regression in that regex silently eats real captures — and per B2 it already does.
Evidence: hack/capture-dataplane.bats exists for precisely this purpose, carries 15 @test cases pinning the sibling's pure helpers against mock input via an E2E_CAPTURE_DATAPLANE_LIB sourcing guard (:40-42), and states in its own header that only the pure logic is unit-testable. There are 18 hack/*.bats files following that pattern.
Fix: minimum coverage — empty file → deleted; one-line Error from server stub → kept (per B2); real multi-line capture → kept; a legitimate one-line capture containing the word error: → kept. The sourcing-guard pattern is already established.

B6: Four of the five captures already exist in the sibling, whose output lands in the same tarball

File: hack/cozyreport.sh:411-470
Issue: cilium-dbg endpoint list (:466 vs sibling :301), ovs-ofctl dump-flows br-int (:441 vs :324), hubble observe (:458 vs :314), conntrack -L (:428 vs :432, with the fallback). Only iptables-save is genuinely new.
Evidence: hack/cozytest.sh:123 writes the sibling's output to $_snap/dataplane, and hack/cozyreport.sh:485-489 copies $SNAP_DIR into $REPORT_DIR/snapshots. So on a failing run the tarball carries both snapshots/<test>/dataplane/node-<node>.txt and net-diag/<node>-*.txt with overlapping content captured at different times — double the exec load, and confusing to read.
Impact: the intent is legitimate and worth keeping — the sibling only fires on a failed test with NotReady pods, so a baseline per-node capture on every report genuinely covers a self-healing transient the sibling misses. But this forks a divergent copy that already disagrees with the original on day one: no /proc fallback, no monitor --type drop, a different cilium selector, and namespaces hardcoded here (cozy-kubeovn/cozy-cilium) versus env-overridable there (COZY_KUBEOVN_NS/COZY_CILIUM_NS, :211-212). Two copies of the same CNI topology knowledge now have to be kept in sync.
Fix: factor the per-node capture into one helper both callers share, or give e2e-capture-dataplane.sh an all-nodes/baseline mode that cozyreport.sh invokes — it already has capture_node() (:393) and pod_on_node() (:274). Add iptables-save there. Same value, far less surface, and it inherits the existing tests.

Non-blocking follow-ups

1. hack/cozyreport.sh:385-387 — the command -v timeout guard is dead code. The only environment that runs this script is the e2e sandbox, and packages/core/testing/images/e2e-sandbox/Dockerfile:1 is FROM ubuntu:24.04, so coreutils timeout is always present. It is also inconsistent with e2e-capture-dataplane.sh and cozytest.sh:123, which both use timeout unguarded. Harmless today, but if it ever fired it would silently skip the entire section on one line of output.
2. hack/cozyreport.sh:447 — the cilium selector diverges from the sibling (app.kubernetes.io/name=cilium-agent here, k8s-app=cilium at e2e-capture-dataplane.sh:292). Both are correct; two scripts in the same pipeline should agree.
3. hack/cozyreport.sh:388-477 — the else body is not indented, which makes the closing fi at :478 hard to pair. An early-skip guard would read better.
4. There is no shellcheck gate in CI, so none of the above is caught mechanically — worth considering separately.

On the existing bot comments: the wc -l "syntax error" concern does not hold — the [ ! -s "$_f" ] early return guarantees the file exists and is non-empty before wc runs. The long-flag portability concern likewise does not apply (Ubuntu sandbox, GNU grep; the script already uses free -m, ps auxww, base64 -d). Wrapping the kubectl get calls in timeout is not the right scope either: kubectl get only blocks when the API server is unreachable, at which point this script has already hung ~60 unguarded calls earlier — the calls that can hang against a healthy API server are the execs, and those are bounded. B4 is the version of that concern that actually bites.

LGTM.

Diagnostic-only e2e tooling — a single file, no product or chart change. Every load-bearing selector / namespace / container / socket path checks out against the in-tree charts: cozy-kubeovn + app=kube-ovn-cni, app=ovs (correctly distinct from app=ovs-dpdk), cozy-cilium + app.kubernetes.io/name=cilium-agent, and the hubble socket /var/run/cilium/hubble.sock. Fail-safe throughout — every exec is timeout 30-bounded and || true, the loops self-guard on empty pod lists, the empty parent dir is cleaned up, and the whole block is skipped when timeout(1) is absent. One cosmetic nit inline.

Thanks—the shared --baseline implementation resolves the previous Hubble, conntrack-fallback, aggregate-timeout, duplication, and testability blockers. Three correctness issues remain inline. This head is also CONFLICTING/DIRTY against current main after #3421; please rebase and reconcile the overlapping diagnostics changes.

Validation: focused Bats passed, sh -n passed, and git diff --check passed. Full pre-commit run --all-files was run; it fails only because this stale head still uses .git/pre-commit.lock, while .git is a file in a linked worktree. #3421 fixed that on main, so please rerun pre-commit after rebasing. The current E2E failure is in the mariadb suite and appears unrelated, but CI should be rerun after the update.

IvanHunters NOT LGTM. The rework fixes what I asked for, but the branch conflicts with main, a capture that times out still disappears without a trace, and the commit history and PR body describe code that is no longer there.

Where the earlier blockers stand (4693353604, on 4a9663570)

• B1 (Hubble): closed. The hubble capture is gone and cilium-dbg monitor --type drop replaced it.
• B2 (pruning deletes failures): half closed. A kubectl error line is now kept, since _diag_prunable only drops empty files and lone DIAG-SKIP: markers. An exec that produced nothing is still deleted, though. That is B2 below.
• B3 (conntrack fallback): closed. The /proc/net/nf_conntrack fallback is in.
• B4 (aggregate bound): closed. timeout -k 30 300 wraps the call at hack/cozyreport.sh:370. On the head's E2E run the baseline took 28s (20:48:10Z to 20:48:38Z).
• B5 (tests): closed. hack/capture-dataplane.bats gains 8 cases for _diag_prunable, both kept and dropped paths. All 23 pass locally.
• B6 (duplicate copy): closed in structure, since cozyreport now calls e2e-capture-dataplane.sh --baseline. The rebase decides whether that stays true (B1 below).

Is it still needed

Partly. Since July main has added a lot to the failure path: hack/e2e-chainsaw/.chainsaw.yaml now runs e2e-capture-dataplane.sh from the global error.catch, and that collector now reads a same-window healthy pod on each affected node as a reference (route and conntrack), next to the node-wide cilium-dbg endpoint list, monitor --type drop and ovs-ofctl dump-flows br-int. What main still lacks and this PR adds: iptables-save (no script in hack/ runs it), a whole-table conntrack dump (main only greps for one IP), and a capture on runs, or nodes, where no pod is NotReady. Those three are worth keeping. The drop monitor is weaker in baseline mode, see the follow-ups.

Blockers

B1: Rebase onto main

The branch conflicts with current main (b113cc77d) in hack/e2e-capture-dataplane.sh and hack/capture-dataplane.bats. hack/cozyreport.sh merges cleanly. This is more than a textual conflict. On main, pod_on_node is memoised and bounded, and the collector's reads go through dp_read_outcome and write the reason it came back empty to capture-notes.txt. The PR moves the July version of pod_on_node up the file. After the rebase capture_baseline has to use main's helpers, or it becomes the second copy of the topology knowledge that B6 asked to remove.

B2: A capture that times out or fails quietly is deleted

File: hack/e2e-capture-dataplane.sh:301-354

The capture leaves no record when an exec produces nothing. Each capture is timeout 25 kubectl exec ... > file 2>&1 || true followed by _diag_prune file. When timeout kills the exec before it writes anything, the file is empty and gets removed. I sourced the script and ran timeout 1 sh -c "sleep 5" > f; _diag_prunable f, which returns rc=124 and then PRUNED. The conntrack capture also runs conntrack -L 2>/dev/null | head, so a CLI that fails leaves an empty file too. kubectl get nodes (:301) has no ceiling of its own, and a failure there looks the same as a cluster with no nodes: the loop does nothing, rmdir removes net-diag/, and the report looks exactly like one from a cluster without kube-ovn or cilium. The same goes for the outer timeout -k 30 300 firing: nothing records it.

This is the silent loss that docs/agents/e2e-testing.md §8 rules out for this collector: "why a read produced nothing goes to capture-notes.txt beside the capture rather than only to the job log, because an empty result and a read that never answered are the same bytes on disk". Fix: record the exec's exit status (a 124 in particular) and every read that came back empty in a net-diag/capture-notes.txt before pruning, and only prune a file when a read that answered produced nothing.

B3: The docs name two callers

docs/agents/e2e-testing.md §8 says "Its two callers bound it differently, 600s from cozytest.sh and 300s from the Chainsaw catch", and the script header on main says it captures state "for NotReady pods". With this PR there is a third caller, cozyreport at 300s, and a mode that walks every node. Please update both in the same change.

B4: The commit history describes the wrong code

The repository merges with merge commits, so both commit bodies land in main.

• 8fb1e11f describes <node>-hubble-flows.txt, hubble observe --last 2000, and pruning that the second commit removes. It also says the conntrack head cap "preserves the failure-window entries on the first-in-first-out flush semantics of the kernel table". conntrack -L dumps in hash-table order, not by age, so head -n 5000 keeps an arbitrary 5000 entries.
• 1f6bc1be opens with "Address the review on the per-node network diagnostics". That is review-cycle wording, not a reason.
• hack/capture-dataplane.bats:239 puts "(PR review blocker B2)" into a code comment.

Please squash into one commit whose message describes the final design, and drop the review reference from the bats comment.

B5: The PR body describes the first version

The body becomes the merge commit message. It still says "hack/cozyreport.sh only, +129 lines under a new net-diag module block", lists hubble observe, says _keep_if_useful drops "error:" stubs, and says a timeout(1) guard skips the section. None of that is in the head. Please rewrite it for the --baseline design.

Non-blocking follow-ups

1. I suggested cilium-dbg monitor --type drop in the last round, but in baseline mode it only watches the 8 seconds when the report runs, which is usually long after any transient. So it cannot answer "was the refused probe a policy drop". The failure-path collector already runs it at the moment of failure. Dropping it here saves 12s per node, and the state captures (iptables, conntrack, flows, endpoints) are the part that is actually worth taking at report time.
2. iptables-save and ovs-ofctl dump-flows br-int have no size cap, unlike the 5000-line conntrack dump. Neither is likely to be large on the sandbox, but a head -n bound would make the report size predictable.
3. The cozyreport comment says the 300s wrapper matches "the wrapper cozytest.sh puts around the same script". cozytest uses timeout -k 30 600, and the Chainsaw catch uses 300.
4. The bats cases pin the predicate. Nothing pins the fact that --baseline actually issues the iptables-save exec, the only capture that is new. Main's suite already stubs kubectl and asserts argv for the other captures, so one case there would cover it.

CI on 1f6bc1be (2026-07-22T18:40Z, attempt 1): pre-commit green. E2E Tests red on chainsaw/mariadb. That failure came before the report step and has nothing to do with this diff. The run is two months old and will re-run after the rebase.