fix(nvca-operator): restore ngc-managed default and fix nil pointer on 3.0.x upgrade - #706
Open
apartha-nv wants to merge 1 commit into
Open
fix(nvca-operator): restore ngc-managed default and fix nil pointer on 3.0.x upgrade#706apartha-nv wants to merge 1 commit into
apartha-nv wants to merge 1 commit into
Conversation
…n 3.0.x upgrade
Two bugs in the nvca-operator 3.1.0 chart prevented successful installation
and upgrade:
1. The vendoring script changed ngcConfig.clusterSource from "ngc-managed" to
"self-managed". NGC UI-generated install commands do not pass this flag
explicitly, so NGC-managed clusters installed with 3.1.0 never registered
their NVCA agent. Restore "ngc-managed" as the chart default and add an
explicit "self-managed" override in the self-managed stack helmfile so that
self-hosted deployments continue to work.
2. self-managed-nvcfbackend-cm.yaml evaluates selfManaged.imageCredHelper and
selfManaged.sharedStorage in the ConfigMap metadata block, which runs for
all cluster types before the clusterSource guard. These sub-keys were
introduced in 3.1.0, so upgrading from 3.0.x with --reuse-values leaves
their parent maps nil. Accessing .imageRepository on a nil interface
panics with "nil pointer evaluating interface {}.imageRepository". Use
| default dict on the parent before accessing the field.
Closes #704
📝 WalkthroughWalkthroughThe NVCA Operator chart now defaults to ChangesNVCA Operator configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy/helm/nvca-operator/nvca-operator/values.yaml (1)
241-251: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd render assertions for
ngc-managedandself-managed.Both chart defaults are
ngc-managed, but the current test checks version overrides only. AssertNVCA_CLUSTER_SOURCEand the selected configuration resources for both modes.🤖 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 `@deploy/helm/nvca-operator/nvca-operator/values.yaml` around lines 241 - 251, Add Helm render assertions covering both ngc-managed and self-managed clusterSource values, including the rendered NVCA_CLUSTER_SOURCE and the configuration resources selected for each mode. Extend the existing version-override test coverage without changing the chart defaults or unrelated behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@deploy/helm/nvca-operator/nvca-operator/values.yaml`:
- Around line 241-251: Add Helm render assertions covering both ngc-managed and
self-managed clusterSource values, including the rendered NVCA_CLUSTER_SOURCE
and the configuration resources selected for each mode. Extend the existing
version-override test coverage without changing the chart defaults or unrelated
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea708f2f-4cdd-4236-9221-b49946d43a1e
📒 Files selected for processing (5)
deploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yamldeploy/helm/nvca-operator/nvca-operator/values.yamldeploy/helm/nvca-operator/scripts/ci_vendor_nvca_operator_chartdeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmplsrc/compute-plane-services/nvca/deployments/nvca-operator/values.yaml
vrv3814
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two bugs in the nvca-operator 3.1.0 chart prevented successful NGC-managed installation and upgrade from 3.0.x:
Bug 1 - Wrong clusterSource default (blocking NGC-managed installs)
The 3.1.0 vendoring script changed
ngcConfig.clusterSourcefromngc-managedtoself-managed. NGC UI-generated install commands do not include--set ngcConfig.clusterSource=..., so any cluster installed using the NGC UI with the 3.1.0 chart ends up in self-managed mode and the NVCA agent never registers.Bug 2 - Nil pointer crash on upgrade from 3.0.x (all cluster types)
self-managed-nvcfbackend-cm.yamlevaluates.Values.selfManaged.imageCredHelper.imageRepositoryand.Values.selfManaged.sharedStorage.imageRepositoryin the ConfigMap metadata block. This block runs unconditionally for every cluster type, before theclusterSourceguard on line 37. BothimageCredHelperandsharedStoragewere added as sub-keys in 3.1.0. When upgrading from 3.0.x with--reuse-values, these keys are absent from stored values, making their parent maps nil. Accessing.imageRepositoryon a nil interface panics:What changed
deploy/helm/nvca-operator/scripts/ci_vendor_nvca_operator_chart: restorengcConfig.clusterSource = "ngc-managed"(reverts accidental change to"self-managed")deploy/helm/nvca-operator/nvca-operator/values.yaml: restoreclusterSource: ngc-managedand update comment to list all three valid valuessrc/compute-plane-services/nvca/deployments/nvca-operator/values.yaml: same source chart updatedeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl: add explicitngcConfig.clusterSource: self-managedso self-hosted stack installs continue to work with the restored defaultdeploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml: use| default dictbefore accessing.imageRepositoryonselfManaged.imageCredHelperandselfManaged.sharedStorageto guard against nil parents on upgradeCustomer Release Notes
Fixes helm upgrade from nvca-operator 3.0.x to 3.1.0 crashing with "nil pointer evaluating interface {}.imageRepository" on all cluster types. Fixes NGC-managed clusters failing to register the NVCA agent after installing nvca-operator 3.1.0 via NGC UI.
Plan Summary
Not applicable.
Usage
No operator change required. After this fix,
helm upgradefrom 3.0.x to 3.1.0 and NGC UI install commands work without additional flags.Testing
helm lintpasses with nilselfManaged.imageCredHelperandselfManaged.sharedStoragehelm templatewith--set selfManaged.imageCredHelper=null --set selfManaged.sharedStorage=nullrenders without errorclusterSource: ngc-managedis the new default in both the vendored and source chartQA should validate:
clusterSourceflag)make installfromdeploy/stacks/nvcf-compute-plane)Notes
The self-managed stack helmfile now explicitly sets
ngcConfig.clusterSource: self-managed, making the install independent of the chart default.References
Closes #704
Related Pull Requests
None.
Dependencies
None.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation