Repository navigation
fix(container-cache): run the ownership init container from the certificates image - #2938
Conversation
…ficates image The init container that fixes cache volume ownership was the only image in the chart not taken from values: a public CUDA base image. Once the pod carried an org-scoped pull secret, containerd presented that secret to the public org and the pull failed with insufficient_scope, so a recreated cache pod sat in Init:ImagePullBackOff and the StatefulSet rollout stalled at 2/3. The container only needs bash, chown, chmod and find. It now runs from images.init, which defaults to images.certificates: the same registry org and pull secret as every other image in the pod, so no mirror or extra image is needed. PATH is pinned so the distribution coreutils win over busybox symlinks that image carries under /usr/local/bin. The exact rendered command was run inside that image against volumes containing a .snapshot directory: ownership set, .snapshot skipped. tests/chart-render/verify-init-image.sh checks that no image points at a public registry org, that the init container defaults to the certificates image, that it is overridable, and that the PATH pin is in the rendered command. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Configure and verify the init image deploy/helm/container-cache/deploy/values.yaml, deploy/helm/container-cache/deploy/templates/statefulset.yaml, deploy/helm/container-cache/README.md, deploy/helm/container-cache/tests/chart-render/verify-init-image.sh |
The chart adds images.init, defaulting to the certificates image when unset. The ownership init container uses that value and prepends /usr/bin:/bin to PATH. The README documents the setting, and the render checks verify the default and custom image output and init command. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: sbaum1994
Merge Risk: 
92edd
-
No actionable merge-blocking defect is established; proceed with the reported normal render, lint, and rollout validation.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the fix type with the required container-cache scope and follows the Conventional Commits grammar. The diff switches the init-chown-data container from a hard-coded public CUDA image to… Linked Issues check Issue #2937requires a chart-valued init image, a default to an image already used by the pod, and removal of the public CUDA image from rendered output.values.yamldefinesimages.initwith an em…Out of Scope Changes check The changes stay within issue #2937. The PATH pin protects the required ownership commands when the certificates image contains busybox symlinks. The values comment, README entry, and render test docu…Full details: Title check
-
Explanation
The title uses the fix type with the required container-cache scope and follows the Conventional Commits grammar. The diff switches the init-chown-data container from a hard-coded public CUDA image to a configurable images.init value that falls back to images.certificates, and it pins PATH. That is a bug fix for the public-image pull failure (insufficient_scope) that stalls StatefulSet rollouts, so the fix type reflects the primary nature of the change. The title's wording about running from the certificates image matches the default fallback, though it omits the new configurable value and PATH pin.
Full details: Linked Issues check
-
Explanation
Issue
#2937requires a chart-valued init image, a default to an image already used by the pod, and removal of the public CUDA image from rendered output.values.yamldefinesimages.initwith an empty default.statefulset.yamlusesimages.init | default images.certificatesforinit-chown-data. The same template pins the command to distribution coreutils.verify-init-image.shchecks the default image, the override, the PATH prefix, and the absence ofnvcr.io/nvidia/in rendered output.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#2937. The PATH pin protects the required ownership commands when the certificates image contains busybox symlinks. The values comment, README entry, and render test document and verify the new image value and its fallback. No unrelated product behavior is changed.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- 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.
|
🎉 This PR is included in deploy/helm/container-cache/v0.30.8 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
TL;DR
The
init-chown-datainit container was the only image in the chart not taken from values: a hard-coded public CUDA base image. With an org-scoped pull secret on the pod, that pull fails withinsufficient_scopeand a recreated cache pod sits inInit:ImagePullBackOff, stalling the StatefulSet rollout. The init container now runs fromimages.init, defaulting toimages.certificates, so the whole pod comes from one registry org with one pull secret and no extra image or mirror is needed.Additional Details
/usr/local/binahead of coreutils; the init command now pinsPATH=/usr/bin:/bin:$PATHso the distribution coreutils are used. The exact rendered command was run inside the image against volumes containing a.snapshotdirectory: root 755 and owner 1000:1000 set,.snapshotskipped, GNU chown confirmed.images.initstays overridable for anyone who prefers a dedicated mirrored base image.tests/chart-render/verify-init-image.sh: no rendered image points at a public registry org, the init container defaults to the certificates image, the override works, and the PATH pin is present.For the Reviewer
deploy/templates/statefulset.yaml: image line and theexport PATHprefix are the whole functional change.For QA
helm lintpasses.Issues
Fixes #2937
Checklist
Summary by CodeRabbit
images.initsetting to configure the image used by the cache initialization container. When left empty, it uses the certificates image.chown,chmod, andfind.