test(e2e): capture previous container logs and stop killing the snapshot - #3346

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request improves the diagnostic capabilities of the E2E testing suite by addressing silent data loss during failure collection. It introduces a mechanism to capture logs from previous container instances, which are often critical for debugging crash-looping pods. Additionally, it refines the timeout and budget management for diagnostic tools like crust-gather to ensure that snapshots are not silently truncated or killed mid-execution, providing more reliable artifacts for investigating test failures.

Highlights

• Previous Container Log Capture: Added a new mechanism to capture logs from the previous instance of crash-looping containers using kubectl logs --previous, ensuring decisive failure evidence is preserved before kubelet garbage collection.
• Explicit Collection Budgets: Configured explicit timeouts and collection durations for crust-gather and catch scripts to prevent silent truncation and ensure that collection processes are not prematurely killed by outer orchestrator timeouts.
• Diagnostic Reliability: Improved failure reporting by logging diagnostic outcomes (complete, truncated, or failed) to files within snapshots, allowing for better post-mortem analysis of E2E failures.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant E2E as E2E failure handler
  participant Logs as e2e-capture-previous-logs.sh
  participant K8s as kubectl
  participant Gather as crust-gather
  participant Snapshot as snapshot artifacts
  E2E->>Logs: capture restarted container logs
  Logs->>K8s: list pods and fetch --previous logs
  Logs->>Snapshot: write container log artifacts
  E2E->>Gather: collect with bounded duration
  Gather->>Snapshot: write crust-gather.log
  E2E->>Snapshot: record completion or truncation status

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 help to get the list of available commands.}

I went looking for evidence to size the tradeoff this PR takes, and ended up auditing e2e diagnostics coverage more broadly — findings in #3368. Two of them bear directly on this PR, so rather than sit on them I want to put them here and let you decide the split.

The premise behind the ceiling increase does not hold. .chainsaw.yaml justifies 5m → 19m with "the report-upload steps are if: always() so a job timeout still yields the artifact". Measured against two real jobs that hit timeout-minutes: 180 (29557003429 / job 87812435947, 29568562337 / job 87848132058):

JOB 87848132058   E2E Tests   conclusion=cancelled   09:11:25 -> 12:16:26
  14 cancelled    Run E2E tests
  15 success      Collect chainsaw report
  16 success      Upload chainsaw report
  17 in_progress  Collect report            <- consumes the grace window
  18 pending      Upload cozyreport.tgz     <- never ran

GitHub's post-cancellation grace measured ~4m48s in both, and step 17 (a full crust-gather) consumes it, so the upload never starts. Complete job does not run either, so the job log itself returns BlobNotFound. Artifact listings confirm it: neither run has a cozyreport. One other run in the window did keep it, only because its tail happened to fit the grace — so the outcome is binary and luck-dependent rather than guaranteed.

This matters more than a stale comment because the same reasoning is being written into docs/agents/e2e-testing.md as guidance.

On the arithmetic, which I think survives but with less margin than it looks. Marginal cost is +14m per failed test, not +19m, since today's cap is 5m. Against a 95.3m median / 106.9m p90 baseline, three failing tests lands at ~152m / 164m — 16m from the cliff before cleanup (delete 5m + cleanup 5m per test), the failing tests' own timeout burn, cozyreport.sh, and upload. That moves break-even from ~4-5 failing tests to ~3. Observed maximum in 400 runs is 3, so it holds — but the ceiling is incidental rather than designed (a broken install short-circuits before chainsaw, 20 of 70 confirmed), and gateway/ alone holds 15 Tests.

What I would suggest, entirely your call on where it lands:

The single cheapest fix for both of the above is an internal deadline (~150m) around the chainsaw step, so the job always retains ~30m to collect and upload. That converts the cancelled case — maximum diagnostics collected, zero delivered — into an ordinary failure with full artifacts, and it removes the risk this PR's ceiling increase adds. It is also the thing that protects every other diagnostics investment.

So: would you rather fold the deadline (and the one-line comment correction) into this PR, or land this as-is and take them in a follow-up? Either works from my side. If you prefer to defer, I would still ask for the comment and the docs bullet to be corrected here, since the claim as written will outlive the PR.

Worth saying plainly: the rest of this PR is well-evidenced and I verified the core claims independently.

• The catch really was structurally doomed — timeouts.exec: 5m against 330s + 630s of inner budget. In the window, 17 jobs show signal: killed mid-crust-gather having collected nothing, and catch durations are sharply bimodal at 60-62s and exactly 300-301s. The timeout: override is the highest-value line in the diff.
• The 60s truncation is real: --duration was never passed at any call site, so every crust-gather budget in the repo was really 60s, not the 300s the timeout -k 30 300 implies. On elapse it exits 1, not 124 — which is worth knowing for the verdict case, since the most likely truncation currently lands in the *) FAILED arm. Mitigating: no --encoding is passed and Encoding::default() == Path, so the partial tree stays readable.
• chainsaw v0.2.15 genuinely has no previous on podLogs, and it is still absent on main upstream, so the local workaround is the right call rather than a version bump.
• The new script is properly failure-tolerant — no set -e/set -u, failures classified and continued, exit 0 — and its go-template holds up against Pending pods, pods with no status key, and init-container-only restarts. --tail is bounded (200 × max 12 containers), so there is no risk of pushing the real error out of the visible log window.

One genuinely useful thing the audit turned up in this PR's favour: crust-gather never collects init or ephemeral container logs at all (src/scanners/logs.rs:107 iterates pod.spec.containers only, and there are no init_container references anywhere in src/). So a crash-looping init container — our bootstrap-job failure class — is invisible in the snapshot on both clusters. That makes this script strictly additive rather than partly redundant with crust-gather, which is a stronger justification than the PR currently claims for itself.

The tenant-cluster gap (previous-logs is wired to the host cluster only, while run-kubernetes.sh got the --duration fix but no previous-logs against CURRENT_TENANT_KC) is in the issue as a follow-up rather than something I would hold this PR for.

Thanks for the measured review and for verifying the core claims independently. Landed the blocking item:

The if: always() / job-timeout comment in .chainsaw.yaml is corrected to your wording (1ff83f5). On the docs bullet: I checked docs/agents/e2e-testing.md at HEAD and that reasoning isn't present — the bullets this PR adds cover previous-instance capture, catch timeout: sizing, and crust-gather --duration, none of which claim a job timeout preserves the artifact. So the corrected comment is the only copy in the tree. If you'd like an explicit job-timeout caveat added to the standing guidance anyway, say the word and I'll add it.

Also corrected the --previous semantics across the helper, both failure handlers, and the doc (76f7267): it captures the immediately preceding terminated instance, not the first.

The explicitly-not-blocking items — the ~150m internal deadline in pull-requests.yaml, the tenant-cluster previous-logs gap, the verdict case labelling, and the two e2e-capture-dataplane.sh fold-ins (capture_lb_datapath early-exit and the podIP filter) — I'm taking as follow-ups to keep this PR tight, tracked against #3368. They're all real; I'd rather land them separately than widen this change.