Repository navigation
test(e2e): make crust-gather failure snapshots reliable and complete - #2952
myasnikovdaniil wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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 enhances the reliability and completeness of e2e failure snapshots in the crust-gather process. By addressing issues with silent collection failures, unreachable tenant APIs, and missing Helm release data, the changes ensure that debugging information is consistently available when tests fail. The scope is strictly limited to the e2e test harness, improving diagnostic capabilities without impacting production code. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesFailure Snapshot Diagnostics
Sequence Diagram(s)sequenceDiagram
participant on_exit as _cozy_on_exit
participant collect as _cozy_collect
participant gather as crust-gather collect
participant disk as snapshot artifacts
on_exit->>collect: collect host non-Secret resources
collect->>gather: run with timeout and filters
gather->>disk: write tree and dest.collect.log
on_exit->>collect: collect host Helm release Secrets
collect->>gather: run with Helm filters
gather->>disk: write tree and dest.collect.log
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
area/testing
dosubot
Bot
added
the
area/ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the E2E test runner script hack/cozytest.sh by improving the collection of diagnostic snapshots when tests fail. It introduces helper functions _cozy_collect and _cozy_collect_helm to capture standard logs and Helm release secrets, and adds logic to automatically re-establish dead port-forwards to tenant clusters during cleanup. The review feedback suggests declaring several variables as local within these new functions and the exit trap handler to prevent global variable pollution and dynamic scoping issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _cozy_collect() { | ||
| _label=$1; _dest=$2; shift 2 | ||
| if timeout 5m crust-gather collect -f "$_dest" "$@" >"$_dest.collect.log" 2>&1; then |
There was a problem hiding this comment.
The variables _label and _dest are currently assigned globally. Since _cozy_collect_helm also uses these same variable names and calls _cozy_collect, this creates a global variable shadowing/overwriting risk. Declaring them as local prevents any dynamic scoping issues or accidental overwrites.
| _cozy_collect() { | |
| _label=$1; _dest=$2; shift 2 | |
| if timeout 5m crust-gather collect -f "$_dest" "$@" >"$_dest.collect.log" 2>&1; then | |
| _cozy_collect() { | |
| local _label=$1 _dest=$2; shift 2 | |
| if timeout 5m crust-gather collect -f "$_dest" "$@" >"$_dest.collect.log" 2>&1; then |
| _cozy_collect_helm() { | ||
| _label=$1; _dest=$2; shift 2 | ||
| _cozy_collect "$_label" "$_dest" \ |
There was a problem hiding this comment.
Similarly, _label and _dest in _cozy_collect_helm should be declared as local to avoid polluting the global scope and prevent dynamic scoping bugs.
| _cozy_collect_helm() { | |
| _label=$1; _dest=$2; shift 2 | |
| _cozy_collect "$_label" "$_dest" \ | |
| _cozy_collect_helm() { | |
| local _label=$1 _dest=$2; shift 2 | |
| _cozy_collect "$_label" "$_dest" \\ |
| _cozy_on_exit() { | ||
| _rc=$? | ||
| if [ "$_rc" -ne 0 ] && command -v crust-gather >/dev/null 2>&1; then |
There was a problem hiding this comment.
In _cozy_on_exit, several variables (_snap, _kc, _name, _srv, _port, _pf_pid) are used globally. Since _cozy_on_exit is registered as an EXIT trap handler, it is highly recommended to declare these variables as local to avoid polluting the global environment or accidentally overwriting variables of the same name in the main script or BATS tests.
| _cozy_on_exit() { | |
| _rc=$? | |
| if [ "$_rc" -ne 0 ] && command -v crust-gather >/dev/null 2>&1; then | |
| _cozy_on_exit() { | |
| _rc=$? | |
| local _snap _kc _name _srv _port _pf_pid | |
| if [ "$_rc" -ne 0 ] && command -v crust-gather >/dev/null 2>&1; then |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/cozytest.sh`:
- Around line 150-151: Between the pkill command that terminates the existing
port-forward process and the kubectl port-forward command that starts a new one,
add a brief sleep delay to allow the operating system to fully release the TCP
port. This prevents the intermittent "address already in use" binding failure
that occurs when port-forward attempts to bind immediately after pkill returns,
as pkill does not wait for the process to fully release system resources.
🪄 Autofix (Beta)
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
Run ID: 514583ab-17bc-4dd2-8646-21cb411d2108
📒 Files selected for processing (1)
hack/cozytest.sh
b47b66a to
00ed99d
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM (APPROVE)
Business context: improves the on-failure crust-gather snapshot in the e2e runner so a failed collect leaves a legible breadcrumb instead of vanishing into /dev/null, bounds each collect with a timeout, and captures the Helm release Secrets that the blanket --exclude-kind Secret otherwise drops.
I verified the shell correctness on the head and cross-checked it with a second model. Findings:
- The
else-branch status capture is correct:_crc=$?is the first statement afterelse, so it still holds the failedtimeout/crust-gatherstatus rather than theif-compound's 0. Evidence:hack/cozytest.sh:115-118. - The collect helpers always return 0 (a trailing
echois the last command in every branch), so underset -ethe EXIT trap proceeds tocozy_cleanupexactly as before — no regression in exit-status handling, and no risk of the trap aborting early on a failed collect. Evidence:hack/cozytest.sh:110-125, trap registration athack/cozytest.sh:163. - The dedicated Helm-storage collect relies on
crust-gathercomposing--include-kindand--include-nameas a single AND-evaluatedFilterList, so it keeps onlysh.helm.release.*Secrets. Evidence:hack/cozytest.sh:134-138. - No new retry loop and no new trap — the existing runner-level EXIT trap is modified in place, and the only added control flow is
timeout-bounding. This is consistent with the e2e conventions (fail-fast, no deterministic-step retries, diagnostics-first). Evidence:hack/cozytest.sh:140-156;docs/agents/e2e-testing.mdsections 1, 3, 8.
No blockers.
Non-blocking:
- The description overpromises relative to the delivered diff. The body describes a tenant
/readyzprobe, port-forward re-establishment from the kubeconfig, and a per-tenant Helm collect ("host + each tenant"), but the head only enhances the host snapshot: a host collect plus ahost-helmcollect. There is no tenant probing, no port-forward recovery, and no per-tenant capture in this change, so the opening framing ("the tenant-cluster snapshot — the one that matters most — silently went missing") is not addressed here. Please align the body with the actual host-only scope, or call out the tenant work as a follow-up. Evidence:hack/cozytest.sh:140-156contains only thehostandhost-helmcollects;git diff origin/main...HEADtoucheshack/cozytest.shonly, with no port-forward or tenant logic.
cozytest.sh's parent-shell EXIT trap snapshots the HOST cluster on a failed test (nested tenant clusters are captured separately, by run-kubernetes.sh's in-subshell trap, while their port-forward is still alive). That host snapshot had three gaps: the collect's output went to /dev/null so a failed collect left no clue why; Helm release storage (kind Secret) was dropped along with every other Secret, losing the revision history that explains a HelmRelease stuck mid-install; and the node host-log scanner burned the collect's --duration deadline on debug pods PodSecurity can never admit, forcing a non-zero exit even though the API tree was already on disk. - factor the collect into a _cozy_collect helper that captures stdout/stderr/rc to a sibling .collect.log and prints a per-target status, so a failed collect leaves evidence in the artifact instead of vanishing; bound it with `timeout -k 30 5m` so a degraded cluster can't stall the job - classify a non-zero collect that still wrote a resource tree as PARTIAL rather than FAILED, so a usable snapshot isn't discarded over the label (capture $? as the first statement in the else — after `fi` it would be the if-compound's 0, not the collect's exit code) - add a dedicated include-only collect (_cozy_collect_helm) for Helm release secrets (sh.helm.release.*), so the artifact carries each release's revision history and status - pass --disable-additional-logs so the host-log scanner doesn't burn the deadline on debug pods that can never be admitted; container/pod logs come from a separate path and are unaffected, and Talos host logs aren't reachable that way anyway crust-gather puts all CLI filters in one FilterList evaluated with .all(), so `--include-kind Secret --include-name <re>` keeps a Secret only when both match — verified from source. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
bfaac1e
00ed99d to
bfaac1e
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Approve — the code is correct and safe on the failure path.
Verified: _crc=$? captures the failed collect status, both helpers end in an echo (return 0) so they can't abort the trap under set -eu, every collect is bounded by timeout -k 30 5m, and the snapshot step never alters the exit code or masks a real failure. shellcheck clean.
Non-blocking: the PR body describes tenant /readyz probing, port-forward re-establishment, and per-tenant helm capture that are not in this diff — the change is host-only, and the tenant collect in run-kubernetes.sh is a separate, untouched trap. Worth trimming the body to the actual scope or marking the rest as follow-up. The bot suggestions don't apply here: local is non-portable in this POSIX sh file (the file uses the _-prefixed global convention), and the port-forward sleep targets code that no longer exists.
3 participants
What this PR does
Follow-up to the crust-gather failure-snapshot logic on
ci-next(#2948). On a failed e2e test, the EXIT trap inhack/cozytest.shproduced an incomplete, opaque dump — the tenant-cluster snapshot (the one that matters most, e.g. for a tenant HelmRelease stuck mid-install) silently went missing. Three causes:/dev/null, so a failed collect left no trace of why;https://localhost:<port>served by akubectl port-forwardbackgrounded minutes earlier — usually dead by the time the trap runs, so crust-gather couldn't reach the tenant API;Secret) was dropped along with every other Secret, so the dump couldn't show a release's revision history / status.This makes the gather reliable and complete:
<target>.collect.log(stdout + stderr + rc) and prints a per-target✓/✗status, so a failed collect leaves a breadcrumb in the artifact instead of vanishing./readyz; if unreachable, re-establish the port-forward from the kubeconfig (name + server port) before collecting.timeout 5mso a wedged forward can't stall the job.sh.helm.release.*secrets. Verified from crust-gather v0.15.2 source: all CLI filters land in oneFilterListevaluated with.all(), so--include-kind Secret --include-name <re>keeps a Secret only when both match.Scope is limited to
hack/cozytest.sh, which runs only in the e2e harness — no production code paths are touched.Release note
Summary by CodeRabbit