fix(nvca): preserve MiniService spec when saving workload config - #626
Open
sbaum1994 wants to merge 3 commits into
Open
fix(nvca): preserve MiniService spec when saving workload config#626sbaum1994 wants to merge 3 commits into
sbaum1994 wants to merge 3 commits into
Conversation
Serialize workload config through the MiniService compatibility wire type and persist it with a merge patch so unrelated spec fields retain their values and field ownership. Closes #625 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
📝 WalkthroughWalkthroughThe change adds ChangesWorkload configuration persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant KubernetesAPI
participant MiniService
Reconciler->>KubernetesAPI: Apply payload containing spec.workloadConfig
KubernetesAPI->>MiniService: Persist targeted field with forced ownership
KubernetesAPI-->>Reconciler: Return applied object and resource version
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
sbaum1994
marked this pull request as ready for review
August 3, 2026 22:37
kristinapathak
approved these changes
Aug 3, 2026
Apply workload config through a serializer-independent unstructured SSA payload so miniservice-controller owns only spec.workloadConfig. Assert that the exact payload omits unrelated spec fields, status, and resourceVersion. Refs #625 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go`:
- Around line 481-487: Add outbound tracing around the r.Client.Patch call in
the reconciliation flow, using the existing context and preserving propagation
through the patch operation. Ensure the span is ended and records an error
status when Patch returns an error, while retaining the current error-handling
behavior.
🪄 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: 381a9a6e-f364-47e3-8d00-ac595c1eda39
📒 Files selected for processing (4)
src/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/miniservice/revision.gosrc/compute-plane-services/nvca/internal/miniservice/transport_tls.go
Comment on lines
+481
to
+487
| if err := r.Client.Patch( | ||
| ctx, | ||
| applyPatch, | ||
| client.Apply, | ||
| client.FieldOwner(managedByValue), | ||
| client.ForceOwnership, | ||
| ); err != nil { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local tracing patterns and determine whether Reconciler.Client is already instrumented.
rg -n -C 3 'saveWorkloadConfig|r\.Client\.Patch|otel\.Tracer|tracer\.Start|span\.RecordError' \
src/compute-plane-services/nvca/internal/miniservice
rg -n -C 3 'Reconciler\{|Client:' src/compute-plane-services/nvca --glob '*.go'Repository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- miniservice tracing and patch call ---'
rg -n -C 12 'func .*saveWorkloadConfig|r\.Client\.Patch|tracer\.Start|span\.RecordError|span\.SetStatus' \
src/compute-plane-services/nvca/internal/miniservice/reconcile.go
printf '%s\n' '--- client instrumentation definition and uses ---'
rg -n -C 12 'type InstrumentedCRClient|func NewInstrumentedCRClient|InstrumentedCRClient|otelhttp|otel\.Tracer|tracer\.Start|RecordError|SetStatus' \
src/compute-plane-services/nvca/internal/metrics src/compute-plane-services/nvca/internal/miniservice \
--glob '*.go' | head -n 400
printf '%s\n' '--- client and tracer fields ---'
rg -n -C 8 'type Reconciler struct|tracer[[:space:]]+|Client[[:space:]]+client\.Client|NewInstrumentedCRClient' \
src/compute-plane-services/nvca/internal/miniservice --glob '*.go'Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete instrumented client implementation ---'
cat -n src/compute-plane-services/nvca/internal/metrics/instrumented_crclient.go | sed -n '1,260p'
printf '%s\n' '--- all Kubernetes client wrappers and tracing setup ---'
rg -n -C 8 'GetClient\(\)|NewInstrumentedCRClient|otel.*(client|kube|k8s)|kube.*otel|otel.*kube|tracing.*client|WithTracer|TracerProvider|oteltrace' \
src/compute-plane-services/nvca --glob '*.go' | head -n 500
printf '%s\n' '--- dependency versions ---'
rg -n 'controller-runtime|opentelemetry|otel' go.mod src/compute-plane-services/nvca/go.mod 2>/dev/null || trueRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- saveWorkloadConfig call sites and surrounding spans ---'
rg -n -C 20 'saveWorkloadConfig\(' src/compute-plane-services/nvca/internal/miniservice --glob '*.go'
printf '%s\n' '--- Kubernetes REST transport tracing configuration ---'
rg -n -C 8 'otelhttp\.NewTransport|WrapTransport|Transport:[[:space:]]*|HTTPClient|NewInstrumentedCRClient|mgr\.GetConfig\(\)' \
src/compute-plane-services/nvca --glob '*.go' --glob '!vendor/**' | head -n 400
printf '%s\n' '--- controller-runtime client tracing references in vendored code ---'
rg -n 'opentelemetry|otelhttp|Tracer|trace\.Span|SpanFromContext' \
src/compute-plane-services/nvca/vendor/sigs.k8s.io/controller-runtime/pkg/client \
src/compute-plane-services/nvca/vendor/k8s.io/client-go \
--glob '*.go' | head -n 200Repository: NVIDIA/nvcf
Length of output: 50367
Add an outbound span for the workload-config patch.
metrics.NewInstrumentedCRClient records metrics only. Add a span around r.Client.Patch, propagate ctx, and record an error status when the patch fails.
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go` around
lines 481 - 487, Add outbound tracing around the r.Client.Patch call in the
reconciliation flow, using the existing context and preserving propagation
through the patch operation. Ensure the span is ended and records an error
status when Patch returns an error, while retaining the current error-handling
behavior.
Sources: Coding guidelines, Path instructions
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
NVCA now reads and saves per-workload configuration without removing required MiniService fields, allowing affected Helm workloads to continue installation.
TL;DR
Persist
spec.workloadConfigthrough the MiniService compatibility serializer and apply only that field through a serializer-independent server-side apply payload owned byminiservice-controller.Additional Details
MiniServiceSpechas custom JSON compatibility logic for canonical and legacy ICMS request names. The newerWorkloadConfigfield was missing from that wire representation, so NVCA could neither serialize nor deserialize it.saveWorkloadConfigalso constructed a partial typed MiniService and force-applied it with the controller's existing field manager. The compatibility serializer emitted zero values for required fields such asspec.namespace,spec.icmsRequestName, andspec.helmChartConfig, while omitting the intended workload config. This left reconciliation terminally stuck.This change:
WorkloadConfigto the compatibility JSON representation in both directions.apiVersion,kind,metadata.name, andspec.workloadConfig.client.Apply,FieldOwner("miniservice-controller"), and deliberate force ownership while omittingresourceVersion, so unrelated concurrent spec updates are outside the patch and do not cause whole-object conflicts.No dependencies or generated API types changed.
Why serializer-independent targeted SSA
A typed partial
MiniServiceis not a targeted payload in this codebase: its customMiniServiceSpec.MarshalJSONmethod serializes zero-valued namespace, request-name, and Helm fields. Merely addingWorkloadConfigto that serializer would persist the flag but would still send those unrelated zero values.A JSON merge patch could safely update only
spec.workloadConfig, but it would replace the original declarative ownership model. Building an unstructured apply object preserves the intended Kubernetes server-side apply contract:miniservice-controllerdeclares ownership only ofspec.workloadConfig, force ownership resolves conflicts on that controller-owned field, and unrelated spec fields remain managed independently. The payload intentionally omitsresourceVersionso field-level SSA semantics, rather than whole-object optimistic locking, govern concurrency. See Kubernetes Server-Side Apply.Observed test sequence
The AWS BYOC test exercised this sequence:
nvcfWorkloadConfig.featureFlags.StatusByWorkerReadiness=true.nvcf-workload-configcontrol ConfigMap plus the workload objects.MiniService.spec.workloadConfigbefore applying the remaining workload objects.ACTIVE, then establish a successful inference baseline.The observed failure occurred at step 4, before workload application, baseline inference, or cordon/drain. The workload-config patch emptied
spec.namespaceandspec.icmsRequestNamewhile failing to persistspec.workloadConfig. The next reconcile returnedminiservice <name> has no namespace, the MiniService remainedInstalling, and the function remainedDEPLOYINGuntil the test timeout.For the Reviewer
Please focus on:
pkg/apis/nvca/v1alpha1/miniservice_json.gofor compatibility serialization completeness.internal/miniservice/reconcile.gofor the serializer-independent targeted SSA payload and retained field ownership.TestSaveWorkloadConfigUsesTargetedSSAfor the exact request assertion. The test proves thatnamespace,icmsRequestName,helmChartConfig,status, andresourceVersionare absent because it compares the complete decoded patch against the expected target-only object.For QA
Local verification completed:
The full affected package run used Kubernetes 1.34.1 envtest assets and passed. Changed-code lint reported
0 issues. The unfiltered full-tree lint currently reports 36 pre-existing findings onorigin/main; none are on changed lines.Runtime QA is still needed before this PR is ready to merge. Build and deploy the patched NVCA, install a Helm workload that renders
nvcf-workload-configwithStatusByWorkerReadiness: true, and verify:spec.workloadConfigcontains the flag.miniservice-controllerownsspec.workloadConfigwithout owning unrelated spec fields inmetadata.managedFields.Issues
Fixes #625
Checklist