Repository navigation
docs(api): separate the healthCheckExprs failure modes - #4387
Conversation
The field doc filed every mistake under one outcome, a HelmRelease that hangs until its timeout. helm-controller treats them differently. An expression that does not parse is rejected before any Helm action and stalls the release at once, with a named reason. One that parses but errors on the object keeps the wait polling with the CEL error attached, and a current expression that evaluates cleanly but never turns true runs the wait out with no error at all. A read of status errors until the CR's controller writes one, even inside has(), but the wait survives it. A wrong group or kind silently falls back to the built-in readiness rules, and a CR whose CRD is missing fails the Helm action before the wait starts. The text ships in the CRDs and is what kubectl explain shows, so a reader who cannot check it against the controller source was sent to the wrong place for the parse error and the missing CRD. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe ChangesHealth-check expression documentation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Fixed issue severity: <fixed_issue_severity>Low</fixed_issue_severity> Merge Risk: ⚪ Minimal · up to This documentation-only change is ready to merge; the reviewed API comments and generated CRDs consistently describe the relevant health-check caveats. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The documentation distinguishes syntax failures from evaluation errors as
✨ 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/api
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Every claim in the new list holds against the helm-controller v1.5.0 this repo installs, and both CRDs regenerate byte-identical. The paragraph left standing underneath the list now contradicts its first bullet.
Findings
- [MINOR]
api/v1alpha1/applicationdefinitions_types.go:123, the retained disable-wait paragraph re-merges the modes the list separated
- [MINOR]
api/v1alpha1/applicationdefinitions_types.go:102, the shipped text names no version for the behaviour it asserts
- [MINOR]
api/v1alpha1/applicationdefinitions_types.go:118, the over-match direction is missing
Caveats
The six claims were run rather than read, against fluxcd/pkg runtime v0.100.2. A parse error, an empty current, and two entries differing only in apiVersion all fail at construction. A misspelled field, a non-boolean result, and has(status.x) against an object with no status at all each raise an eval error that cli-utils statusreaders/common.go:181-186 turns into Unknown carrying that error. A present status: {} makes has() come out false cleanly, and a clean-but-false current gives InProgress. ssa v0.67.1 manager_wait.go:133-148 and :172-181 confirm that neither state is terminal, that FailFast fires only on Failed, and that the error rides the timeout message only when there is one.
controller-gen v0.16.4 over ./api/... reproduced all 25 CRD files byte-identical to what is committed, so neither manifest is hand-edited and none is missing. Only that step ran: the full hack/update-codegen.sh does not execute in a review checkout, so its client-gen half is unverified here.
With every description stripped, base and head are identical on both CRDs, so no field, enum or validation rule moved. The chart renders and kubeconform -strict accepts both manifests.
One scoping note the text doesn't carry. waitStrategy: legacy routes to helm's own waiter (internal/action/client.go:68-69), which never sees the CEL readers, so bullets 2 through 4 describe the poller path only. The sibling WaitStrategy doc says as much and cozystack defaults to poller when expressions are set, but kubectl explain on this field alone does not.
| // kstatus rules instead. | ||
| // A CR whose CRD is not installed fails the Helm action before any wait, | ||
| // whatever these expressions say. | ||
| // Upstream evaluates these only when the Helm action itself has wait |
There was a problem hiding this comment.
[MINOR] the retained disable-wait paragraph re-merges the modes the list separated
"makes them a silent no-op whatever waitStrategy says ... with no error or warning" is not true of the bullet-1 class. In helm-controller v1.5.0 cel.NewStatusReader runs at internal/controller/helmrelease_controller.go:202, above reconcileChartTemplate (213) and reconcileRelease (217), while DisableWait is read later in the action config at internal/action/wait.go:51, reached from internal/action/install.go:92. With the annotation set, a non-parsing expression, an empty current or a duplicate group+kind still stalls the release at Ready=False / Stalled=True with reason InvalidCELExpression and a Warning Event, and helm never applies the CR. Scope the sentence to evaluation and note the first bullet still applies, since that check runs before any Helm action.
| // that the CEL compiles or that the referenced apiVersion/kind CRD is | ||
| // installed; a bad expression or a not-yet-installed CRD makes the | ||
| // HelmRelease hang until its timeout (the backstop). | ||
| // as soon as helm applies it. OpenAPI validates only the struct shape, so |
There was a problem hiding this comment.
[MINOR] the shipped text names no version for the behaviour it asserts
The PR body pins the claims to helm-controller v1.5.0, fluxcd/pkg runtime v0.100.2 and Helm v4.1.1. The comment that ships into the CRD and into kubectl explain names none, so a flux bump turns it into a description of a version nobody runs, with nothing to flag it. That is not hypothetical here: v1.5.0 exists only in the vendored render (packages/core/flux-aio/manifests/fluxcd.yaml:8097), while the bundle it comes from asks for version: "latest" (packages/core/flux-aio/flux-aio.cue:8), so a regeneration can move it without touching this file. The repo already stamps this kind of claim, at packages/apps/postgres/values.yaml:170 and packages/system/postgres-rd/cozyrds/postgres.yaml:7; one clause on the "mistakes surface in helm-controller" line would carry it. Same for api/v1alpha1/packagesource_types.go:158.
| // such as a comparison against the wrong value, leaves the CR | ||
| // InProgress, and the Helm action waits out its timeout with no CEL | ||
| // error to point at the expression. | ||
| // - A group or kind that does not match the CR the chart renders leaves |
There was a problem hiding this comment.
[MINOR] the over-match direction is missing
The list covers a group/kind matching nothing, not one matching too much. Spec-defined readers are prepended ahead of the built-ins (helmrelease_controller.go:1055-1057, [email protected] polling/polling.go:29 before :32) and statusReaderForGroupKind returns the first reader supporting the kind (polling/engine/engine.go:242-249), so an entry for apps/v1 Deployment replaces kstatus's Deployment rules for every Deployment in the release.
2 participants
What this PR does
The
healthCheckExprsfield doc said any bad expression makes the HelmRelease hang until its timeout. That is true for some mistakes and wrong for others, and the text ships in the CRDs, sokubectl explainsends people to the wrong place.The doc now lists the cases separately, checked against the controller versions this repo installs (helm-controller v1.5.0, fluxcd/pkg runtime v0.100.2, Helm v4.1.1). An expression that does not parse, an empty
current, or a duplicate group and kind stall the release right away with reasonInvalidCELExpression, and no Helm action runs. An expression that errors on the object keeps the wait polling until the timeout with the CEL error in the message. Acurrentexpression that evaluates but never turns true also runs the timeout out, with no error to point at it. A wrong group or kind leaves the expressions unused.Two details go against the issue text. Reading
statusbefore the CR's controller writes one errors even insidehas(), as the issue says, but the object is only Unknown on that poll, and the wait still succeeds once a status shows up. A CRD that is not installed does not hang the wait: Helm fails to build the manifest and the action fails before any wait starts.Doc comments and the regenerated CRD YAML only, no behaviour change.
Closes #4154
Screenshots
No UI change.
Downstream repositories
Release note
Summary by CodeRabbit