fix(nvca-operator): restore ngc-managed default and fix nil pointer on 3.0.x upgrade - #706
Open
apartha-nv wants to merge 3 commits into
Open
fix(nvca-operator): restore ngc-managed default and fix nil pointer on 3.0.x upgrade#706apartha-nv wants to merge 3 commits into
apartha-nv wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe NVCA Operator chart now defaults to ChangesNVCA Operator configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
kristinapathak
approved these changes
Aug 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@deploy/helm/nvca-operator/tests/cluster_source_defaults_test.sh`:
- Around line 36-44: Extend the self-managed rendering test around the existing
manifest_self_managed flow to use a legacy values fixture where
selfManaged.imageCredHelper and selfManaged.sharedStorage are omitted or null.
Assert helm template succeeds for that fixture, then verify
nvcfbackend-self-managed contains the supplied ICMS, revalidation, and NATS
service URLs rather than relying only on a nonzero data-key count.
🪄 Autofix
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: Enterprise
Run ID: ee3298f7-5da0-4120-aa4c-ee4565b571ed
📒 Files selected for processing (1)
deploy/helm/nvca-operator/tests/cluster_source_defaults_test.sh
apartha-nv
enabled auto-merge
August 7, 2026 08:37
…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
…d self-managed Add a chart render test that verifies: - default render produces NVCA_CLUSTER_SOURCE=ngc-managed with empty nvcfbackend config maps - self-managed render produces NVCA_CLUSTER_SOURCE=self-managed with populated nvcfbackend-self-managed and empty nvcfbackend-helm-managed Closes #704
Use compute plane instead of control plane in the self-managed mode description, matching the correct NVCF terminology.
apartha-nv
force-pushed
the
fix/nvca-operator-cluster-source-defaults
branch
from
August 7, 2026 08:38
b999026 to
4879050
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
3 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