Repository navigation
chore: address review followups from #2509 and #2553 - #2577
myasnikovdaniil wants to merge 6 commits into
Conversation
time.ParseDuration accepts "1ms" as a valid positive value, but Flux HelmRelease fields below ~15s thrash the controller without usefully changing behaviour. Add a min-duration parameter to parsePositiveDuration and pass helmReleaseMinDuration (15s) at every HR-flag call site so a misconfigured flag fails fast at startup rather than producing a HR that never reconciles cleanly. Signed-off-by: Myasnikov Daniil <[email protected]>
Kubernetes resource names are DNS-label-validated and so cannot contain spaces in practice, but consistent quoting hardens the script against future changes that pipe in less-trusted strings (event message bodies, custom labels) and makes the intent unambiguous to readers. Signed-off-by: Myasnikov Daniil <[email protected]>
cozyreport.sh is best-effort diagnostic collection: a single failed kubectl (missing CRD, RBAC denial, transient timeout) must not abort the rest of the report. Make that explicit at the top of the script so the next maintainer does not "fix" it by adding set -e and silently shrinking the diagnostic surface for E2E debugging. Signed-off-by: Myasnikov Daniil <[email protected]>
Pipelines like `kubectl get hr -A | awk ...` swallow the kubectl exit code without pipefail, so a failed kubectl call would silently produce an empty section in summary.txt rather than a visible error. The call site already wraps this script in `|| true`, so a hard pipeline failure still produces a (truncated) summary, but it now surfaces as missing sections instead of empty ones. Guard with `2>/dev/null || true` since POSIX sh does not require `set -o pipefail`. Signed-off-by: Myasnikov Daniil <[email protected]>
The existing `kubectl get events --field-selector reason=OOMKilling`
section only catches kernel-OOM events emitted by kubelet at the node
level. Container-level cgroup kills that exit 137 without escalating to
a node OOMKilling event (no kernel memory-pressure broadcast) are
invisible there but visible in the pod's containerStatuses.lastState.
Add a "Containers with OOMKilled lastState" section that scans every
container and initContainer for a terminated.reason=="OOMKilled". Use
go-template instead of jsonpath because jsonpath's inner `{range}`
cannot reach the parent pod's metadata, while go-template's `$pod :=`
captures the outer scope.
Signed-off-by: Myasnikov Daniil <[email protected]>
…pefail Verified that the e2e-sandbox image's /bin/sh is Ubuntu's dash (0.5.11 on 22.04, 0.5.12 on 24.04) and neither build implements `set -o pipefail`. Rather than guarding the option with `|| true` (which silently drops the very behaviour we want) or rebasing the sandbox onto a different /bin/sh (high-cost image rebuild + ghcr push for one script), point the shebang at /bin/bash, which is already installed in the image and supports pipefail unconditionally. cozyreport.sh stays /bin/sh — it does not need pipefail. Signed-off-by: Myasnikov Daniil <[email protected]>
github-actions
Bot
added
size/L
📝 WalkthroughWalkthroughThis PR introduces duration validation for HelmRelease flags in the operator, and enhances two reporting scripts with Bash compatibility, improved path quoting for safety, and new diagnostic sections for OOMKilled containers and Talos node logs. ChangesHelmRelease Duration Validation
Reporting Script Enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 addresses feedback from previous issues by tightening configuration validation in the operator and improving the robustness and diagnostic capability of the 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
|
![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 introduces a minimum duration enforcement of 15 seconds for HelmRelease flags within the operator to prevent controller thrashing, accompanied by updated unit tests. Additionally, it enhances the diagnostic tools by adding OOMKilled container status reporting to cozyreport-summary.sh and improving shell script reliability in cozyreport.sh through consistent variable quoting and explicit error handling documentation. Feedback was provided regarding a missing release-note block in the PR description, which is required by the repository style guide.
|
|
||
| parseFlag := func(flagName, raw string) time.Duration { | ||
| d, err := parsePositiveDuration(flagName, raw) | ||
| d, err := parsePositiveDuration(flagName, raw, helmReleaseMinDuration) |
There was a problem hiding this comment.
The pull request description is missing a release-note block, which is required by the repository style guide (line 51). Please add a block to the PR description following this format:
```release-note
chore(operator): address review followups for HelmRelease knobs and cozyreport
<details>
<summary>References</summary>
1. PR body must contain a release note block. <sup>([link](https://github.com/cozystack/cozystack/blob/main/.gemini/styleguide.md))</sup>
</details>
![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/cozyreport-summary.sh`:
- Line 11: The script uses three instances of head -40 which, under set -o
pipefail, cause SIGPIPE failures; locate each occurrence of head -40 in the
script and replace them with a sed invocation that reads and prints the first 40
lines without closing the input early (e.g., sed -n '1,40p') so the producer
won't receive SIGPIPE and the pipeline won't abort.
In `@hack/cozyreport.sh`:
- Around line 6-7: Update the top-of-file comment that describes the strict-mode
behavior to match the actual invocation in hack/cozyreport.sh: change the
wording that currently states the companion script is "set -eu" to reflect "set
-euo pipefail" so the comment accurately documents the script's flags; locate
the existing comment near the script header and adjust its text to mention "set
-euo pipefail" (reference: the comment block describing fail-fast policy in
hack/cozyreport.sh).
🪄 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: 5ce02112-9a37-4b0d-a78e-0671357f43fc
📒 Files selected for processing (4)
cmd/cozystack-operator/main.gocmd/cozystack-operator/main_test.gohack/cozyreport-summary.shhack/cozyreport.sh
| # silently-empty ones, and the e2e-sandbox image's /bin/sh (Ubuntu dash) | ||
| # does not implement pipefail. The call site wraps this script in `|| true` | ||
| # so a hard pipeline failure still produces a (truncated) summary.txt. | ||
| set -euo pipefail |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pipefail behavior with early-closing consumer vs non-early-closing limiter.
set -u
bash -lc 'set -o pipefail; yes | head -n 40 >/dev/null; echo "head pipeline exit=$?"'
bash -lc 'set -o pipefail; yes | sed -n "1,40p" >/dev/null; echo "sed pipeline exit=$?"'Repository: cozystack/cozystack
Length of output: 398
🏁 Script executed:
cat hack/cozyreport-summary.shRepository: cozystack/cozystack
Length of output: 4423
🏁 Script executed:
#!/bin/bash
# Test pipefail behavior with head vs sed using seq (available in sandbox)
echo "=== Test 1: pipefail with head (should fail if output > 40 lines) ==="
bash -c 'set -o pipefail; seq 1 100 | head -n 40 >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 2: pipefail with sed (should succeed) ==="
bash -c 'set -o pipefail; seq 1 100 | sed -n "1,40p" >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 3: pipefail with head (small output, < 40 lines) ==="
bash -c 'set -o pipefail; seq 1 10 | head -n 40 >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 4: WITHOUT pipefail, head with large output (should succeed) ==="
bash -c 'seq 1 100 | head -n 40 >/dev/null; echo "Exit code: $?"'Repository: cozystack/cozystack
Length of output: 547
🏁 Script executed:
#!/bin/bash
# Test pipefail behavior using bash built-ins
echo "=== Test 1: pipefail with head (100 lines of output, limit to 40) ==="
bash -c 'set -o pipefail; (for i in {1..100}; do echo "line $i"; done) | head -n 40 >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 2: pipefail with sed (100 lines of output, limit to 40) ==="
bash -c 'set -o pipefail; (for i in {1..100}; do echo "line $i"; done) | sed -n "1,40p" >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 3: pipefail with head (10 lines of output, ask for 40) ==="
bash -c 'set -o pipefail; (for i in {1..10}; do echo "line $i"; done) | head -n 40 >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 4: WITHOUT pipefail, head with 100 lines ==="
bash -c '(for i in {1..100}; do echo "line $i"; done) | head -n 40 >/dev/null; echo "Exit code: $?"'
echo
echo "=== Test 5: Verbose - watch head with 100 lines under pipefail ==="
bash -c 'set -o pipefail; (for i in {1..100}; do echo "line $i"; done) | head -n 40; echo "Exit code: $?"' | tail -5Repository: cozystack/cozystack
Length of output: 492
🌐 Web query:
pipefail head SIGPIPE bash pipeline failure exit code
💡 Result:
In Bash pipelines like cmd1 | head, when head exits after reading its required input (e.g., first 10 lines), it closes the pipe. If cmd1 attempts to write more data, the kernel sends it SIGPIPE (signal 13), causing cmd1 to terminate with exit code 141 (128 + 13).[1][2][3] Without set -o pipefail, the pipeline's overall exit status is that of the last command (head), which is 0 (success).[4][5] With set -o pipefail enabled, the pipeline's exit status is the rightmost non-zero exit status from any command in the pipeline. Thus, it becomes 141 from the upstream command killed by SIGPIPE, causing the pipeline to "fail" (non-zero exit).[4][3][2][6] This is expected behavior, not a bug—SIGPIPE indicates normal early termination by the consumer (head), but pipefail propagates the non-zero status for strict error checking.[7][8] Official Bash docs confirm pipefail uses the last non-zero exit status without special SIGPIPE handling.[4][9] To handle/ignore SIGPIPE failures under pipefail (common with head, grep -q, etc.): - Check ${PIPESTATUS[0]} == 141 and treat as success: cmd1 | head; [[ ${PIPESTATUS[0]} -eq 141 || ${PIPESTATUS[0]} -eq 0 ]][6][2] - Use subshell or || true: (cmd1 | head) || [ $? -eq 141 ][6] - Process substitution: head < <(cmd1) (avoids pipe).[10] - Temporarily disable pipefail around the pipeline.[2] Example: set -o pipefail yes | head -n1 echo $? # 141 (fails) set -o pipefail yes | head -n1 echo ${PIPESTATUS[@]} # 141 0 # Check: [[ ${PIPESTATUS[0]} -ne 141 ]] || echo "OK" [3][2][11]
Citations:
- 1: https://www.gnu.org/s/bash/manual/html_node/Exit-Status.html
- 2: https://oneuptime.com/blog/post/2026-01-24-bash-broken-pipe/view
- 3: https://business-service.2software.net/linux-fu-failing-pipelines/
- 4: https://www.gnu.org/software/bash/manual/html_node/Pipelines
- 5: https://www.gnu.org/software/bash/manual/bash.html
- 6: https://stackoverflow.com/questions/22464786/ignoring-bash-pipefail-for-error-code-141
- 7: http://www.pixelbeat.org/programming/sigpipe_handling.html
- 8: https://unix.stackexchange.com/questions/774908/sigpipe-and-bash-pipefail
- 9: https://www.unix.com/man-page/linux/1/bash/
- 10: https://stackoverflow.com/questions/41516177/bash-zcat-head-causes-pipefail
- 11: https://stackoverflow.com/questions/63128067/cant-successfully-call-any-external-shell-script-in-pipe-with-set-eo-pipefail
Replace head with sed to avoid SIGPIPE failures under pipefail.
With set -o pipefail on line 11, pipelines ending in head -40 (lines 22, 30, 58) trigger SIGPIPE when the producer writes more data than head consumes. This causes the entire pipeline to fail with exit code 141, aborting summary generation even without actual kubectl errors.
Replace all three instances with sed -n '1,40p', which reads all input without closing the pipe early:
Suggested patch
- | head -40
+ | sed -n '1,40p'
@@
- | head -40
+ | sed -n '1,40p'
@@
- | head -40
+ | sed -n '1,40p'🤖 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/cozyreport-summary.sh` at line 11, The script uses three instances of
head -40 which, under set -o pipefail, cause SIGPIPE failures; locate each
occurrence of head -40 in the script and replace them with a sed invocation that
reads and prints the first 40 lines without closing the input early (e.g., sed
-n '1,40p') so the producer won't receive SIGPIPE and the pipeline won't abort.
| # is fail-fast (`set -eu`) because it is wrapped in `|| true` at the call | ||
| # site below — any change to either policy should be made deliberately. |
There was a problem hiding this comment.
Keep strict-mode comment aligned with actual summary script.
Line [6] says the companion script is set -eu, but it is now set -euo pipefail. Updating this avoids confusion during incident triage.
Suggested patch
-# is fail-fast (`set -eu`) because it is wrapped in `|| true` at the call
+# is fail-fast (`set -euo pipefail`) because it is wrapped in `|| true` at the call📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # is fail-fast (`set -eu`) because it is wrapped in `|| true` at the call | |
| # site below — any change to either policy should be made deliberately. | |
| # is fail-fast (`set -euo pipefail`) because it is wrapped in `|| true` at the call | |
| # site below — any change to either policy should be made deliberately. |
🤖 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/cozyreport.sh` around lines 6 - 7, Update the top-of-file comment that
describes the strict-mode behavior to match the actual invocation in
hack/cozyreport.sh: change the wording that currently states the companion
script is "set -eu" to reflect "set -euo pipefail" so the comment accurately
documents the script's flags; locate the existing comment near the script header
and adjust its text to mention "set -euo pipefail" (reference: the comment block
describing fail-fast policy in hack/cozyreport.sh).
|
I'd push back on this PR title. It describes the workflow that produced the PR ("address review followups from #2509 and #2553") rather than what the change actually does. Why this matters The PR title is the most-read piece of text this change will ever have. It shows up in:
Three problems with a title that points at other PRs:
Either would stand on its own. Bundled under "followups", they're invisible until someone opens the diff. Suggestion The two changes are mechanically unrelated (operator Go code vs. diagnostic shell scripts), so the cleanest fix is to split into two PRs with descriptive titles, e.g.:
If splitting isn't worth the churn, at minimum retitle to something content-bearing — e.g. |
Aleksei Sviridkin (lexfrei)
added
area/platform
2 participants
Summary
Address inline review followups left from #2509 (HelmRelease knobs) and #2553 (cozyreport).
Operator (#2509 followup):
cmd/cozystack-operator/main.go: extendparsePositiveDurationwith aminparameter and passhelmReleaseMinDuration(15s) at every HR-flag call site, so--helmrelease-install-timeout=1ms(and similar) fail fast at startup instead of producing a HelmRelease that thrashes the controller. Tests cover at-min, above-min, and below-min (including sub-second).cozyreport (#2553 followups):
hack/cozyreport.sh: consistent variable quoting across all loops; one-line comment at the top documenting the deliberate absence ofset -e(best-effort diagnostic collection — a single failed kubectl must not abort the rest of the report).hack/cozyreport-summary.sh: enableset -o pipefailso failed kubectl calls inkubectl ... | awk ...chains surface as missing sections instead of silently-empty ones. Switched shebang to/bin/bashbecause the e2e-sandbox image's/bin/sh(Ubuntu dash, both 22.04 and 24.04) does not implement pipefail; bash is already installed in the image.hack/cozyreport-summary.sh: surface container-stateOOMKilled(vialastState.terminated.reason) using a go-template — catches cgroup-only kills that exit 137 without escalating to a node-levelOOMKillingevent.Test plan
go test ./cmd/cozystack-operator/...passes (TestParsePositiveDuration covers at-min, above-min, below-min, sub-second)--helmrelease-install-timeout=1msand confirm it exits at startup withmust be >= 15sbash -n hack/cozyreport-summary.shandsh -n hack/cozyreport.shsyntax-cleanhack/cozyreport.shagainst an e2e cluster; confirmsummary.txtcontains the new## Containers with OOMKilled lastStatesectionsummary.txtis truncated at the failure instead of producing a complete-but-empty sectionSummary by CodeRabbit
Tests
Chores