Repository navigation
fix(e2e): keep backup preflight failure evidence across attempts - #4242
Conversation
github-actions
Bot
added
area/testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe backup access preflight now tracks failed PUT, GET, compare, and DELETE stages across retries. It preserves retry logs and reports port-forward output on timeout. Bats tests cover first-attempt PUT failures and successive failures across all stages. ChangesBackup access preflight diagnostics
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The updated timeout diagnostics preserve the relevant logs and identify each failed S3 stage without changing preflight pass/fail behavior. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
the
size/L
The backup access preflight printed its port-forward capture only when the port-forward never came up, and truncated its mc log at the top of every attempt. On the path that fails in practice, a healthy port-forward and an S3 round trip that never completes, the output was the tail of the last attempt with nothing to say which stage it died at, which reads as a successful upload followed by a hung download. Print the port-forward capture alongside the mc log on the timeout path, append the mc log across attempts, and record which of PUT, GET, compare or DELETE each attempt reached. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
4a5d074 to
65c183e
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Pass/fail behaviour is identical to the merge base on all 16 outcome combinations, the new evidence lines are mutation-covered, and the red check on this head is not from this change.
Findings
- [MINOR]
hack/backup-access-preflight.bats:160, clock comment describes a stub discipline the stub does not have
Caveats
- The red
E2E (in-tree)check on65c183e31is pre-existing. ItsRun E2E testsstep ends in 7 chainsaw failures, every backup one beingBackupJob ... reached terminal 'Failed'after the preflight had already loggedpassed PUT, GET, compare, and DELETE on attempt 1. Unrelated PR #4237 fails the same set the same day, so the diff is not the cause and the changed path is not implicated. - The branch this PR rewrites has still never run in CI. All four preflight call sites passed on their first attempt in that job, so a cluster exercised only the success path; the timeout path is held by the bats suite alone.
- Dump volume is bounded on the mc side only. A 90s budget admits 19 attempts (
sleep 5per iteration) and a fail-fast run emits 40 stderr lines, measured locally. Thekubectl port-forwardhalf is a real client whose per-connection line count I could not measure without a cluster.
Recommended follow-ups
- The stage line says where an attempt stopped, not how it stopped. In the run cited by #4240 the one surviving line was
mc's arrow line with no error after it, which is the shape oftimeout 15killing it rather than S3 refusing the operation. Carrying the exit status (rc=124againstrc=1) onto the stage line would settle that without falling back to the port-forward block.
| esac | ||
| } | ||
| # A stubbed clock keeps the retry budget independent of how fast the | ||
| # stubs run: only a completed PUT advances it, so the loop ends after a |
There was a problem hiding this comment.
[MINOR] clock comment describes a stub discipline the stub does not have
The comment reads "only a completed PUT advances it", but the upload arm bumps $now by 10 before it reaches if [ "$count" -eq 3 ], so attempt 4's failing PUT advances the clock too, and that advance (130 to 140, past the 135 deadline) is what ends the loop. Reordering the stub to match the sentence leaves the suite green, so nothing is hidden by the mismatch, but the next person to tidy this arm is being told the wrong rule. Drop "completed", or move the two clock lines below the failure check.
2 participants
What this PR does
The S3 access preflight that runs before a backup roundtrip prints its port-forward capture only when the port-forward never comes up. On the failure that actually happens, a healthy port-forward and an S3 roundtrip that never completes, that capture is written and then thrown away. The mc log is truncated at the top of every attempt on top of that, so what reaches the job log is the tail of the last attempt with nothing in it to say which stage died. One real run left a single line of evidence, an upload progress line, which reads as an upload that finished and a download that hung. The upload was what failed.
The timeout path now prints both logs, the mc log accumulates across attempts, and every attempt records which of PUT, GET, compare or DELETE it reached. That is enough to tell a broken channel to S3 from S3 refusing the operation, and to see how many attempts ran and how each one ended.
The port-forward log goes in whole rather than filtered.
kubectl port-forwardwrites oneHandling connection for <port>line per TCP connection, and whether those lines are there at all is the answer to whether mc ever reached the forwarder, so dropping them to cut noise would delete the signal this change exists to produce.Diagnosability only. Nothing changes about what the preflight passes or fails.
Closes #4240
Screenshots
No UI change.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff. Nothing underhack/is moved or renamed, no make target changes behaviour, and the node prerequisites the Ansible collection and talm mirror are untouched.Release note
Summary by CodeRabbit
Bug Fixes
Tests