fix(compute-plane): update observability defaults and coverage - #648
Open
sbaum1994 wants to merge 6 commits into
Open
fix(compute-plane): update observability defaults and coverage#648sbaum1994 wants to merge 6 commits into
sbaum1994 wants to merge 6 commits into
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:
📝 WalkthroughWalkthroughThe change updates NVCA collector defaults, adds compute-plane image and version override handling, introduces context-aware ChangesCompute observability
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Godog
participant ControlPlane
participant Observability
participant ComputePlane
participant NVCA
Godog->>ControlPlane: Configure split-cluster fixtures
Godog->>ControlPlane: Register compute cluster
Godog->>Observability: Install shared observability
Godog->>ComputePlane: Install NVCA compute plane
Godog->>NVCA: Inject credential and restart NVCA
Godog->>ComputePlane: Verify releases and monitoring configuration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/bdd/godog_test.go (1)
663-670: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert manifest applies for both selected contexts.
Line 663 rejects only the ambient-context form. It does not confirm that control-plane manifests use the control context and compute-plane manifests use the compute context.
A regression that sends all manifest applies to one explicit context passes this test. Count the generated apply commands and assert 16 control-plane applies and 4 compute-plane applies.
🤖 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 `@tests/bdd/godog_test.go` around lines 663 - 670, Update the command-validation loop in the relevant Godog test to classify generated manifest apply commands by explicit control-plane and compute-plane context, then assert exactly 16 control-plane applies and 4 compute-plane applies. Retain the existing rejection for ambient-context applies and API-key leakage, using the test’s existing command/context symbols to distinguish the two apply groups.
🤖 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/stacks/nvcf-compute-plane/tests/observability-profile.sh`:
- Around line 78-79: Update the regression test inputs and assertions in
observability-profile.sh to use distinct sentinel values for
global.nvcaOperator.imageTag and global.nvcaOperator.selfManaged.nvcaVersion.
Scope each check specifically to the corresponding image.tag and
selfManaged.nvcaVersion fields, so swapped mappings or values written to
unrelated matching keys fail.
In `@tests/bdd/godog_test.go`:
- Around line 640-653: The wiring test should stop requiring exact matches for
every recorded command. In the test around fakeRunner.runs, retain the existing
status == 0 assertion and replace the loop over registryLoginCommand and related
commands with a single commandRanThatContains assertion targeting the
destructive pod-delete command.
---
Nitpick comments:
In `@tests/bdd/godog_test.go`:
- Around line 663-670: Update the command-validation loop in the relevant Godog
test to classify generated manifest apply commands by explicit control-plane and
compute-plane context, then assert exactly 16 control-plane applies and 4
compute-plane applies. Retain the existing rejection for ambient-context applies
and API-key leakage, using the test’s existing command/context symbols to
distinguish the two apply groups.
🪄 Autofix (Beta)
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: e52e56b0-ac5d-4560-a1e6-12bc233ca614
📒 Files selected for processing (8)
deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpldeploy/stacks/nvcf-compute-plane/tests/observability-profile.shtests/bdd/dsl/kubectl.gotests/bdd/dsl/kubectl_test.gotests/bdd/features/observability-compute.featuretests/bdd/godog_test.gotests/bdd/steps/infra_steps.gotests/bdd/steps/steps_test.go
sbaum1994
commented
Aug 4, 2026
sbaum1994
force-pushed
the
test/bdd-observability-compute-pr3
branch
from
August 4, 2026 06:55
ffc7151 to
5156232
Compare
sbaum1994
changed the title
Add explicit kube-context secret setup and optional NVCA image overrides so local split-cluster installs exercise the compute profile with published multi-arch images. Relates to #522 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
Make the compute-plane stack own the operator and backend versions instead of overriding them in the live BDD. Remove the redundant Stargate and collector tag overrides. Relates to #522 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
Use the latest published multi-architecture NVCF OTel collector. Keep the source and release charts, schema, tests, and compute-plane compatibility default aligned. Relates-to: #522 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
sbaum1994
force-pushed
the
test/bdd-observability-compute-pr3
branch
from
August 5, 2026 05:19
5156232 to
109cc6d
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. |
sbaum1994
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/stacks/nvcf-compute-plane/README.md`:
- Around line 71-73: Update the README’s chart-defaults description near the
compute-plane image-tag documentation to identify chart defaults as fallback
values, not the source of tested image tags. Keep the existing statement that
compute-plane base values in 02-nvca.yaml.gotmpl provide the tested NVCA, NVCA
operator, and OTel Collector tags.
In `@tests/bdd/godog_test.go`:
- Around line 654-662: Update the command assertions in the test around the
profile-install target loop to also require the register-cluster command and an
explicit KUBECONFIG_FILE on the nvcf-compute-plane install target. Ensure every
compute operation is matched with its expected kubeconfig path so ambient
Kubernetes context cannot satisfy the test.
🪄 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: 6ff523c5-df24-4d29-b5c6-fce01da344ce
⛔ Files ignored due to path filters (3)
deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/deployment.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/pre-delete-cleanup-job.yamlis excluded by!**/testdata/**deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-8e61e521-nvca-operator/helm-nvca-operator/templates/self-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**
📒 Files selected for processing (20)
deploy/helm/nvca-operator/nvca-operator/README.mddeploy/helm/nvca-operator/nvca-operator/templates/deployment.yamldeploy/helm/nvca-operator/nvca-operator/values.schema.jsondeploy/helm/nvca-operator/nvca-operator/values.yamldeploy/helm/nvca-operator/tests/release_image_manifest_test.shdeploy/helm/nvca-operator/tests/self_managed_nvca_image_reference_test.shdeploy/stacks/nvcf-compute-plane/README.mddeploy/stacks/nvcf-compute-plane/environments/base.yamldeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpldeploy/stacks/nvcf-compute-plane/tests/observability-profile.shsrc/compute-plane-services/nvca/deployments/nvca-operator/README.mdsrc/compute-plane-services/nvca/deployments/nvca-operator/templates/deployment.yamlsrc/compute-plane-services/nvca/deployments/nvca-operator/values.schema.jsonsrc/compute-plane-services/nvca/deployments/nvca-operator/values.yamltests/bdd/dsl/kubectl.gotests/bdd/dsl/kubectl_test.gotests/bdd/features/observability-compute.featuretests/bdd/godog_test.gotests/bdd/steps/infra_steps.gotests/bdd/steps/steps_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/bdd/steps/steps_test.go
- tests/bdd/dsl/kubectl.go
- deploy/stacks/nvcf-compute-plane/tests/observability-profile.sh
- tests/bdd/features/observability-compute.feature
- deploy/stacks/nvcf-compute-plane/environments/base.yaml
- tests/bdd/dsl/kubectl_test.go
- deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl
- tests/bdd/steps/infra_steps.go
sbaum1994
commented
Aug 5, 2026
| "${manifest}" | ||
| )" | ||
|
|
||
| if [[ "${otel_collector_tag}" != "0.157.9" ]]; then |
Contributor
Author
There was a problem hiding this comment.
this test doesn't make sense since the version will change in future
sbaum1994
commented
Aug 5, 2026
| exit 1 | ||
| fi | ||
|
|
||
| if [[ "${backend_otel_collector_tag}" != "0.157.9" ]]; then |
Contributor
Author
There was a problem hiding this comment.
same comment as above this is a brittle test
sbaum1994
commented
Aug 5, 2026
sbaum1994
commented
Aug 5, 2026
sbaum1994
commented
Aug 5, 2026
Compare rendered collector values to their configured sources and use sentinel overrides for version routing. Keep the BDD wiring assertion focused on the destructive NVCA restart command. Relates-to: #522 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
Keep compute-profile coverage focused on collector enablement and the installed monitor resources. Relates-to: #522 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
| nvcaOperator: | ||
| imageTag: "3.1.0" | ||
| selfManaged: | ||
| nvcaVersion: "3.1.0" |
Contributor
There was a problem hiding this comment.
we're working on fixing publishing now, but nvca is already on 3.1.6
kristinapathak
approved these changes
Aug 5, 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.
Customer Summary
Self-managed compute observability now uses published NVCA and collector versions.
TL;DR
Adds live split-cluster BDD coverage for
observability.profile: compute, updates NVCA defaults to3.1.0, and replaces the unavailable collector default with multi-architecture0.157.9.Additional Details
ncp-local-cp, then observability and NVCA onncp-local-compute-1.0.157.9.Related work: #509, #523, and #564.
For the Reviewer
Focus on
observability-compute.feature, compute-plane defaults, and the NVCA operator chart default change.For QA
TestObservabilityCompute: 58/58 steps passed in 15m9s after destructive recreation of the splitncp-localtopology.No third-party dependency, NOTICE, or architecture diagram changes.
Issues
Relates to #522
Checklist
Summary by CodeRabbit
Improvements
0.157.9across compute-plane and NVCA configurations.3.1.0.Documentation
Tests