Repository navigation
fix(helm): apply manager.image.pullPolicy to the controller container - #531
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains the Helm pull-policy change, its default behavior, documentation updates, testing, and compatibility considerations. Title check The title clearly identifies the main change: applying manager.image.pullPolicy to the controller container. Linked Issues check Issue #511requirements are met.values.yamluses an empty default.deployment.yamlomitsimagePullPolicyfor an unset value and renders supported explicit values, includingAlways. Helm tests…Out of Scope Changes check The changes stay within issue #511. Template validation, render-test helpers, Helm cases, and deployment and setup documentation support the pull-policy behavior. No unrelated behavior change is shown…Full details: Linked Issues check
-
Explanation
Issue
#511requirements are met.values.yamluses an empty default.deployment.yamlomitsimagePullPolicyfor an unset value and renders supported explicit values, includingAlways. Helm tests cover omission, explicit policies, digest-pinned images, and invalid values. The deployment documentation describes the setting and the tag-based default behavior.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#511. Template validation, render-test helpers, Helm cases, and deployment and setup documentation support the pull-policy behavior. No unrelated behavior change is shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
This fixes #511 as proposed. The value now defaults to "", imagePullPolicy is rendered only when set, and the docs table and air-gap step are updated. With the value unset, the chart renders the same as main, which the air-gap guidance depends on. Defaulting to omission, with an opt-in override, is the right design: wiring up the old IfNotPresent default would have silently changed every :latest install. The test helper checks that the field is actually absent rather than empty, which is the right thing to check here.
A few follow-ups, most important first:
-
Upgrade note for
--reuse-values. You noted this in the description, but it should also be in the deployment guide. Upgrading with--reuse-valuesfrom an older release carries the oldIfNotPresentdefault forward, and it now renders. Only a:latestor untagged controller image behaves differently.--set manager.image.pullPolicy=once, or--reset-then-reuse-values(Helm 3.14+), avoids it. The paragraph neardeployment.md:311already covers both flags forrbac.*, so the note could go there. -
setup initkeepspullPolicybut works out the image afresh each run (ADR-098). Before this change a kept value did nothing. Now a keptNeverapplies to whatever image the next run picks. If that image isn't preloaded, the rollout stalls onErrImageNeverPull. A sentence indocs/cli-reference/setup.mdwould cover it. -
Validate the value.
--set manager.image.pullPolicy=alwayspasses the chart and only fails when the API server rejects the Deployment.setup initalso keeps the bad value, so every later run fails the same way. The chart already usesfailfor a badrbac.scope; a check againstAlways/IfNotPresent/Neverwould match that. -
Air-gap step wording. "override it with
manager.image.pullPolicywhen needed" could name the case it's for:IfNotPresentorNeverfor a preloaded controller image. -
Tests (nits):
managerImagePullPolicyrepeats the decode-and-find loop frommanagerImageandmanagerArgs. One helper that returns the unstructured manager container would serve all three and still tell a missing field from an empty one.- The "latest tag leaves pull policy to Kubernetes" case only checks that the field is omitted, which the default case already covers.
-
Rebase. The branch predates #528, so on GitHub the diff appears to remove the "MPI workers stuck installing sshd" troubleshooting section and revert the FAQ. It merges cleanly, but a rebase would make the diff readable.
|
Thanks for the detailed review. I'll address the follow-ups and rebase onto the latest main. |
Default the value to empty and render imagePullPolicy only when set, so the chart value takes effect and an unset value keeps the API server's tag-based default. Document the value and add test/helm cases. Closes NVIDIA#511 Signed-off-by: kang0225 <kjskkdkmj@gmail.com>
Reject unsupported pull policies during chart rendering so configuration errors surface before deployment. Keep imagePullPolicy omitted for empty or null values. Document upgrade behavior and the preloading requirement for a retained Never policy. Share manager container lookup across render test helpers and remove the redundant latest-tag omission case. Signed-off-by: Kang-minjong <kjskkdkmj@gmail.com>
2668e04 to
ef2eb2d
Compare
|
I've pushed the fixes and rebased onto main. I also reran the Helm render tests and |
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. All six follow-ups are addressed:
- The
--reuse-valuesnote is now under the upgrade step indeployment.mdand covers both--set-string manager.image.pullPolicy=and--reset-then-reuse-values. setup.mdnow says to preload each new controller image whenNeveris kept.- The chart rejects any value other than empty,
Always,IfNotPresentorNever, and names the bad value in the error. - The air-gap step names the preloaded-image case.
managerContainerFromRenderedis now shared by the args, image and pull-policy helpers, and the redundantlatestcase is gone.- After the rebase, the diff shows only the intended files.
I merged this onto current main and ran test/helm, which passes. With the value unset, helm template output is byte-identical to main. A null value, from a values file or from --set, omits the field, and false, " Always" and [] are all rejected with the new message. setup init reads helm get values without --all, so it keeps only values someone actually set and can't bring back the old default; the new note correctly scopes that to helm upgrade --reuse-values.
Two optional nits, neither blocking:
- The upgrade note says the old default "is now rendered". Once this ships, "now" will be ambiguous. Naming the release ("from vX.Y on…") would read better later. The note's manual line breaks also don't match the wrapping of the surrounding text.
- The two commits use different author names (
kang0225andKang-minjong) with the same email. Each sign-off matches its own commit, so DCO is fine, but squashing on merge would tidy it.
|
/ok to test eabab67 |
ndipebot
left a comment
ndipebot
left a comment
There was a problem hiding this comment.
The fix is right and the shape of it is right. imagePullPolicy only renders when the value is set, the default moves to "" so an unset release renders byte-identical to before, and the fail guard sits before the render guard so false and 0 get rejected instead of quietly falling through the truthiness check. The upgrade note about --reuse-values carrying the old IfNotPresent default is the part I would have forgotten, and it is already there.
Two small things, both optional.
-
The
manager.image.pullPolicyrow in the chart values table explains the empty case but does not nameAlways,IfNotPresentorNever. Therbac.scoperow a few lines down spells its allowed set out inline, so naming the values here would match what the table already does. I would also add a sentence to the upgrade note atdeployment.md:469saying a kept value outside the enum now fails the render, since that is the one way an existing release can break on upgrade, and point at--set-string manager.image.pullPolicy=(which clears it) ornvcrectl setup init --reset-values. -
TestHelmTemplateRejectsInvalidImagePullPolicypins thefailtext with a Go table and afmt.Sprintfof the expected string, while the chart's only otherfailsite is pinned with atestutil.TestCaseParsernegative golden attest/helm/testdata/render-rbac/invalid-scope/error. The two guards intemplates/are symmetric now but their tests are not, so whoever adds a third has two precedents to pick from. Worth moving the four rejected values torender-pull-policy/<case>/{input.yaml,error}? The positive cases can stay a table.
List Always, IfNotPresent and Never in the chart values table, since the chart now rejects anything else. In the --reuse-values upgrade note, name v0.8.2 instead of "now", say how to clear a kept value the chart rejects, and unwrap the paragraph to match the surrounding list items. Signed-off-by: Kang-minjong <kjskkdkmj@gmail.com>
|
Thanks! Updated the docs in a4643f2. Could you take another look and run /ok to test again? |
kaynetu
left a comment
kaynetu
left a comment
There was a problem hiding this comment.
LGTM. The values table now names the accepted policies, and the upgrade note is anchored to v0.8.2 and says how to clear a kept value the chart now rejects. If another release is cut before this merges, bump the version in that note.
|
/ok to test aa97c84 |
|
/ok to test 57277da |
|
/ok-to-test ef9117b |
Summary
manager.image.pullPolicyis declared invalues.yaml(defaultIfNotPresent), buttemplates/deployment.yamlnever rendersimagePullPolicy, so setting it has no effect.The value now defaults to
"", andimagePullPolicyis rendered only when it is set. With the value unset the chart renders exactly as before, so the API server still picks the policy from the tag, as the air-gap guidance expects. The value is also added to the key chart values table, and the air-gap step now mentions it.Related Issue
Closes #511
Type of Change
Component(s) Affected
Testing
ef2eb2d):go test ./test/helm/render_test.go -v -count=1(22 cases, none skipped),helm lint helm/cluster-readiness-engine, andgo vet ./test/helm/.go test ./test/helm/ -v -count=1fails during catalog initialization on Windows due to embedded-file path handling. The same initialization failure reproduces on upstreammain(765ff4a).main's chart withpullPolicy=Alwaysleft the Deployment atIfNotPresent. After upgrading to this branch it wasAlways, andIfNotPresentagain once the override was removed.main.helm upgrade --reuse-valuesfrom an older release carries the oldIfNotPresentdefault, which now renders; only alatesttag behaves differently.Checklist
git commit -s)make manifests generaterun (if*_types.gowas modified): not modified