refactor(operator): implement composition-first DGD reconciliation - #12283
Conversation
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
github-actions
Bot
added
refactor
deployment::k8s
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
sttts
left a comment
sttts
left a comment
There was a problem hiding this comment.
Disclosure: This review was written by Codex as its interpretation of a discussion with Stefan; it is not Stefan’s text.
This is an update to the architectural review on #12263 at fc2e40a, after reading the incremental b09d873 commit in this PR.
What this PR improves
This is a meaningful step in the proposed direction:
groveProgramnow owns concrete renderer, scaler, and status collaborators.- Grove watch setup is separated from the common controller.
- Read-only paths use
client.Readerinstead ofclient.Client. - Readiness is observed once from a coherent post-scaling snapshot.
- Grove topology status is accumulated in the returned program status; the outer reconciler remains the status writer.
- Kubernetes-backed retrieval and PodCliqueSet construction are now mechanically separated.
The new groveScaler, groveStatusResolver, and groveWatchSetup demonstrate the useful part of the approach: concrete responsibilities with narrower capabilities.
1. Keep the retrieval/rendering boundary, but avoid facts and request bags
The capability boundary is good. The resulting data abstraction is not.
GrovePodCliqueSetRenderFactsRequest, GrovePodCliqueSetRenderFacts, GrovePodCliqueSetRenderRequest, and groveRenderInputs divide one concrete operation into several structureless carrier types. They make the call graph more abstract without giving the values a stronger domain invariant.
checkpoint.ResolvedPodSpecRestore is different: it represents one cohesive, validated domain result and prevents callers from depending on checkpoint-storage internals. That is a useful resolved type. A generic collection of unrelated “facts” is not.
I would keep groveWorkloadRenderer as the concrete boundary, inject client.Reader, retrieve the required objects directly, keep temporary collections local, and pass concrete values to pure leaf functions:
type groveWorkloadRenderer struct {
reader client.Reader
config *configv1alpha1.OperatorConfiguration
runtimeConfig *commoncontroller.RuntimeConfig
dockerSecretRetriever dockerSecretRetriever
}
func (r *groveWorkloadRenderer) Render(
ctx context.Context,
dgd *nvidiacomv1beta1.DynamoGraphDeployment,
restart *dynamo.RestartState,
checkpoints map[string]*checkpoint.CheckpointInfo,
) (*grovev1alpha1.PodCliqueSet, error) {
existing := &grovev1alpha1.PodCliqueSet{}
key := types.NamespacedName{
Name: dynamo.PCSNameForDGD(dgd.Name, dgd.Spec.Components),
Namespace: dgd.Namespace,
}
if err := r.reader.Get(ctx, key, existing); err != nil {
if !apierrors.IsNotFound(err) {
return nil, fmt.Errorf("get PodCliqueSet %s: %w", key, err)
}
existing = nil
}
renderDGD := dgd.DeepCopy()
applyGroveCompatibility(renderDGD, existing)
// Resolve queue, topology, and checkpoint inputs here. Keep collections
// local unless an individual value has a cohesive domain contract.
// Then call pure rendering helpers with the actual inputs.
// ...
}The goal is a hard read/write capability boundary, not purity at the price of argument-bag abstractions. There is also no need for a custom one-method reader interface or a helper that merely renames reader.Get.
2. Continue replacing method-name composition with struct composition
The Grove-specific extractions are good examples, but the program still retains the complete DynamoGraphDeploymentReconciler and still composes large responsibilities through it:
p.reconciler.reconcileProgramInputs(...)
p.reconciler.resolveProgramRestartState(...)
p.reconciler.reconcileProgramResult(...)
p.reconcileStableResources(...)I would continue until the struct graph itself explains the composition:
type groveProgram struct {
rollout *dgdUnsupportedRolloutReconciler
sharedResources *dgdSharedResourcesReconciler
restart *dgdRestartReconciler
workloads *groveWorkloadReconciler
stableResources *dgdStableResourcesReconciler
topology *dgdGroveTopologyConditionReconciler
}Each should be independently constructible and testable with only its own fixtures. Use short standardized method names such as Reconcile, Resolve, and Render; let the concrete type identify the purpose. Separate files should use a consistent DGD prefix, for example dgd_checkpoints_reconciler.go and dgd_restart_reconciler.go.
In particular, reconcileProgramInputs still effectfully reconciles RBAC, PVCs, discovery, GMS claims, checkpoints, EPP resources, the wait-for-leader ConfigMap, and MPI SSH keys. These are the next concrete reconcilers to extract. The criterion is cohesive resource ownership and an independent fixture contract, not mechanical fragmentation.
3. Finish the Request/Result and status-ownership boundary
The empty resolvedFacts and unused Request.Facts still exist. Remove them until a real common input appears.
workloadProgramResult also still splits Status from ReadyReason and ReadyMessage, while the outer reconciler computes and overrides Ready based on rollout state. That leaves condition ownership shared.
The contract should remain:
type Request struct {
// Mutable primary object. Programs may persist non-status mutations.
DGD *nvidiacomv1beta1.DynamoGraphDeployment
}
type Result struct {
ctrl.Result
Status nvidiacomv1beta1.DynamoGraphDeploymentStatus
Events []Event
}The selected program should return the complete desired status, including the final Ready condition, rollout/restart state, and ObservedGeneration. The outer reconciler should persist that status unchanged.
This PR moves topology projection in that direction. The same ownership rule should now be applied to Ready instead of returning a reason/message that another layer later interprets and overrides.
4. Return status-transition events with the status
groveStatusResolver now writes into the returned status, but it also owns an EventRecorder and emits the topology warning before the outer Status().Update succeeds.
That event describes a DGD status transition. It should therefore be queued in Result.Events and emitted only after the corresponding status update succeeds and actually changes persisted status. Otherwise an update failure can produce an event for an edge that was not persisted, and retries can emit it again.
Direct event creation remains fine after a locally detected semantic resource mutation successfully completes. The distinction is where the edge becomes real:
- resource mutation edge: emit after the successful
Create,Update,Patch, orDelete, and only when something actually changed; - DGD status edge: return it with the status, persist the status, then emit it.
Direction
This PR materially improves Grove isolation and read-side dependency boundaries. It does not yet complete the program abstraction described in the earlier review.
The main remaining work is:
- remove the empty common facts container;
- avoid Grove-wide facts/request bags;
- make the returned status authoritative, including Ready;
- queue status-transition events and emit them after persistence;
- replace the remaining all-powerful reconciler calls with concrete nested reconcilers;
- decompose
reconcileProgramInputsinto cohesive DGD reconcilers.
DCD and Grove should remain separate workload programs by design. No renderer or provider lifecycle composition between them is being proposed. The shared abstraction is the small program Request/Result contract; each program continues to own its own order and timing.
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
…gram Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
…oller Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
…aborators Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
fc2e40a to
d7f86f1
Compare
b09d873 to
3856af4
Compare
This comment has been minimized.
This comment has been minimized.
julienmancuso
changed the title
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
|
/ok to test 6bf4030 |
6bf4030 to
6773306
Compare
WalkthroughChangesCheckpointed workload reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (19)
deploy/operator/internal/controller/dynamocomponentdeployment_controller.go (1)
184-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line story comment above the checkpoint-storage step.
The block creates the checkpoint storage reconciler and converges storage before workload rendering. The guidelines require a one-line story comment above such a block.
♻️ Proposed comment
+ // Converge checkpoint storage first so workload rendering stays read-only. checkpointStorageReconciler := newDCDCheckpointStorageReconciler(As per coding guidelines: "In Go code, put a one-line story comment above every multi-line block of logically connected code."
🤖 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/operator/internal/controller/dynamocomponentdeployment_controller.go` around lines 184 - 191, Add a concise one-line story comment immediately above the checkpointStorageReconciler creation and Reconcile block, describing that checkpoint storage is converged before workload rendering. Do not alter the reconciliation logic or error handling.Source: Coding guidelines
deploy/operator/internal/dynamo/graph_test.go (1)
1245-1320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
t.Logheadings for each test step.The guidelines require
t.Logheadings that explain the test story, with one heading before each block that implements a test step. This test has three steps: build the PVC-backed fixtures, generate the PodCliqueSet, and assert the injected checkpoint volume.As per coding guidelines: "In Go tests, use
t.Logto explain the test's story, with one heading before each block implementing a test step."🤖 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/operator/internal/dynamo/graph_test.go` around lines 1245 - 1320, Add t.Log headings in TestGenerateGrovePodCliqueSet_InjectsReadyCheckpointRestore before each test step: building the PVC-backed fixtures, generating the PodCliqueSet, and asserting the injected checkpoint volume. Keep the existing setup, generation, and assertions unchanged.Source: Coding guidelines
deploy/operator/internal/controller/dynamocomponentdeployment_renderer.go (2)
395-438: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
runtimeConfiglikereader.Line 400 checks
r == nilandr.reader == nil, but Line 413 dereferencesr.runtimeConfig.Gatewithout a check. A renderer built with a nilruntimeConfigpanics here instead of returning an error. Either drop the defensive nil checks at Line 400 or extend them tor.runtimeConfig.🤖 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/operator/internal/controller/dynamocomponentdeployment_renderer.go` around lines 395 - 438, The nil guard in hasExistingLegacyWorkerSelector is incomplete because the method dereferences runtimeConfig when evaluating the LWS feature gate. Extend the initial guard to include r.runtimeConfig, preserving the existing false, nil return for an unavailable renderer dependency.
81-105: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve the workload component type once per render.
getDCDWorkloadPodLabelscallsgetDCDWorkloadComponentType, andgeneratePodTemplateSpeccalls it again for each role.getDCDWorkloadComponentTypecan callhasExistingLegacyWorkerSelector, which issues up to threeGetcalls (Deployment, LeaderWorkerSet, Service). For a multinode render this repeats the same resolution three times, so up to nine reads occur per reconcile. Resolve the component type once inrenderMultinodePodTemplateSpecsand pass it into the per-role helpers. This also removes the risk that the pod labels and the pod spec disagree if a legacy object appears between calls.Also applies to: 149-159
🤖 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/operator/internal/controller/dynamocomponentdeployment_renderer.go` around lines 81 - 105, Update renderMultinodePodTemplateSpecs to resolve the workload component type once, then pass that resolved value through getDCDWorkloadPodLabels and the leader/worker pod-template generation helpers. Adjust generatePodTemplateSpec and related helper signatures to reuse the supplied type instead of calling getDCDWorkloadComponentType per role. Preserve existing label and pod-spec behavior while eliminating repeated legacy-object lookups and ensuring both paths use the same resolution.deploy/operator/internal/dynamo/graph.go (1)
2457-2472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the "needs resolved restore" predicate and add a story comment.
The resolve condition here and the
shouldUseAdmissionRestorecondition inbuildCliqueForRole(Lines 2158-2164) encode the same policy with inverted logic. The two sites must stay in agreement, otherwisebuildCliqueForRolefails withresolved checkpoint restore is required for role ...or silently skips restore. Extract one helper, for exampleneedsResolvedRestore(gate, checkpointInfo), and call it from both sites.Also add a one-line story comment above this block, as the guidelines require for multi-line blocks of logically connected Go code.
As per coding guidelines: "In Go code, put a one-line story comment above every multi-line block of logically connected code."
🤖 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/operator/internal/dynamo/graph.go` around lines 2457 - 2472, Extract the shared checkpoint policy into a helper such as needsResolvedRestore, and use it both in buildCliqueForRole’s shouldUseAdmissionRestore logic and this checkpoint.ResolvePodSpecRestore block so the conditions remain consistent. Add a one-line story comment immediately above this multi-line restore-resolution block describing its purpose.Source: Coding guidelines
deploy/operator/internal/checkpoint/podspec.go (1)
256-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
Readychecks in the read-free phase.
ResolvePodSpecRestorereturnsnilunless the checkpoint is enabled and ready. Thereforerestore.info.Readyis alwaystrueinsideInjectResolvedCheckpointIntoPodSpec. The parameter at Line 277 and the condition at Line 296 always evaluate totrue. Consider dropping these checks, or document thatResolvedPodSpecRestoreis only constructed for ready checkpoints, so a future caller does not assume the unready case is still handled here.🤖 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/operator/internal/checkpoint/podspec.go` around lines 256 - 308, Remove the redundant restore.info.Ready argument passed to PrepareRestorePodSpec and the restore.info.Ready condition guarding GPUMemoryService handling in InjectResolvedCheckpointIntoPodSpec, since ResolvedPodSpecRestore is only produced for ready checkpoints. Preserve the existing storage, seccompProfile, GPU mode, and error behavior.deploy/operator/internal/controller/dcd_checkpoint_storage_reconciler_test.go (1)
74-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd per-step
t.Logheadings and a disabled-checkpoint case.Two points:
- The repository guidelines require one
t.Logheading before each block that implements a test step. The subtest has three steps: build the fixtures, reconcile, and assert the PVC state. Only one heading exists.- Add a table case where the component declares
Checkpoint: &ComponentCheckpointConfig{Enabled: false}. That case pins the behavior for a declared-but-disabled checkpoint, which the current cases do not cover.As per coding guidelines: "Use one
t.Logheading before each block that implements a test step so test output tells the scenario's story."🤖 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/operator/internal/controller/dcd_checkpoint_storage_reconciler_test.go` around lines 74 - 101, Add separate t.Log headings before each test-step block in the subtest: fixture construction, reconciler execution, and PVC-state assertion. Extend the table-driven cases with a scenario where Experimental.Checkpoint is declared but Enabled is false, and assert the expected PVC behavior for that case alongside the existing enabled and absent configurations.Source: Coding guidelines
deploy/operator/internal/controller/dcd_checkpoint_storage_reconciler.go (1)
54-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line story comment above the validation block.
The repository guidelines require a one-line story comment above every multi-line block of logically connected code in Go. Lines 58-69 form one validation and short-circuit block without such a comment.
♻️ Proposed comment
) error { + // Skip storage convergence unless this DCD actually requests checkpointing. if dcd == nil { return fmt.Errorf("dynamo component deployment is required") }As per coding guidelines: "In Go code, put a one-line story comment above every multi-line block of logically connected code."
🤖 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/operator/internal/controller/dcd_checkpoint_storage_reconciler.go` around lines 54 - 72, Add a single one-line story comment immediately above the validation and early-return block in Reconcile, covering the nil deployment, nil feature gate, disabled Checkpoint gate, and missing checkpoint configuration checks. Leave the validation logic and EnsureStoragePVC call unchanged.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_controller.go (3)
141-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a
Reasonconstant for the finalizer failure.Line 143 passes the raw string
"failed_to_handle_the_finalizer". All other failure paths use theReasonconstants declared near line 72. Declare a constant and use it so the reason set stays discoverable in one place.♻️ Proposed change
- programResult.Fail(dynamoDeployment.Generation, "failed_to_handle_the_finalizer", err) + programResult.Fail(dynamoDeployment.Generation, reasonFailedToHandleFinalizer, err)Add the constant next to the other reasons:
reasonFailedToHandleFinalizer Reason = "failed_to_handle_the_finalizer"🤖 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/operator/internal/controller/dynamographdeployment_controller.go` around lines 141 - 149, Declare a reasonFailedToHandleFinalizer Reason constant alongside the existing Reason constants, then update the newWorkloadProgramResult failure call in the finalizer error path to use it instead of the raw string.
172-187: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the status write when the projected status is unchanged.
persistWorkloadProgramResultcallsStatus().Updateon every reconcile, including reconciles that produce an identical status. This adds one API write per reconcile for steady-state deployments. Compare the projected status with the stored status and write only on a difference. Keep the event flush after a successful write.♻️ Proposed change
) error { + if equality.Semantic.DeepEqual(dgd.Status, result.Status) { + return nil + } dgd.Status = result.Status if err := r.Status().Update(ctx, dgd); err != nil { return fmt.Errorf("update DynamoGraphDeployment status: %w", err) }Note that queued events must still flush when the status is already current, so move the event loop before the early return if that ordering is required.
🤖 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/operator/internal/controller/dynamographdeployment_controller.go` around lines 172 - 187, Update persistWorkloadProgramResult to compare the projected result.Status with dgd.Status before calling r.Status().Update, skipping the status write when unchanged. Ensure result.Events are still flushed through r.Recorder.Event in both unchanged and successfully updated cases, while preserving error propagation for failed status updates.
154-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd story comments above the new reconcile blocks.
Lines 154-157 select and run the workload program. Lines 158-166 persist the result and propagate errors. The coding guidelines require a one-line story comment above every multi-line block of logically connected Go code. Add one comment per block.
As per coding guidelines: "In Go code, put a one-line story comment above every multi-line block of logically connected code."
🤖 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/operator/internal/controller/dynamographdeployment_controller.go` around lines 154 - 169, Add one-line story comments immediately above the workload program selection/reconciliation block and above the result persistence/error-propagation block in the reconciliation flow surrounding selectWorkloadProgram, program.Reconcile, and persistWorkloadProgramResult. Keep each comment concise and describing the block’s purpose.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_controller_test.go (1)
455-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
t.Logheadings to the new tests.
TestReconcileScalingAdaptersEmitsDeleteEventOnlyAfterSuccessfulDeletehas not.Logheading before its setup and verification steps.TestGroveWatchSetup_MapPodCliqueToRequests(lines 4994-5025) andTestGroveChildEventPredicates(lines 5387-5399) have the same gap. Add one heading per test step so the test output tells the scenario story, as the neighboringTestDynamoGraphDeploymentReconciler_isGrovePathwayalready does.♻️ Proposed change for the adapter delete test
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Log("Seed a DGD with one orphaned scaling adapter") dgd := &v1beta1.DynamoGraphDeployment{+ t.Log("Verify the delete event is emitted only after a successful delete") require.NoError(t, reconciler.reconcileScalingAdapters(context.Background(), dgd))As per coding guidelines: "In Go tests, use
t.Logto explain the test's story, with one heading before each block implementing a test step".🤖 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/operator/internal/controller/dynamographdeployment_controller_test.go` around lines 455 - 529, Add t.Log headings for each setup and verification step in TestReconcileScalingAdaptersEmitsDeleteEventOnlyAfterSuccessfulDelete, TestGroveWatchSetup_MapPodCliqueToRequests, and TestGroveChildEventPredicates. Follow the existing heading style used by TestDynamoGraphDeploymentReconciler_isGrovePathway and ensure every test-step block has one descriptive log heading.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_grove_scaling.go (1)
57-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the checkpoint startup-gate predicate.
Lines 58-61 inline the rule "checkpoint enabled, policy WaitForCheckpoint, not ready".
componentProgram.applyCheckpointStartupPolicyapplies the same rule indynamographdeployment_program.goat lines 573-576. If the policy set changes, the two sites can diverge and Grove and component pathways would gate replicas differently. Extract one helper in the checkpoint package or in the controller package and call it from both sites.♻️ Proposed helper
// checkpointGatesStartup reports whether a component must stay at zero // replicas until its checkpoint is ready. func checkpointGatesStartup(info *checkpoint.CheckpointInfo) bool { return info != nil && info.Enabled && info.StartupPolicy == nvidiacomv1alpha1.CheckpointStartupPolicyWaitForCheckpoint && !info.Ready }- gated := info != nil && - info.Enabled && - info.StartupPolicy == nvidiacomv1alpha1.CheckpointStartupPolicyWaitForCheckpoint && - !info.Ready + gated := checkpointGatesStartup(info)🤖 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/operator/internal/controller/dynamographdeployment_grove_scaling.go` around lines 57 - 71, Extract the shared checkpoint startup-gating predicate currently in the Grove scaling logic into a reusable helper, such as checkpointGatesStartup, in an accessible checkpoint or controller package. Replace the inline condition in the Grove replica calculation and the equivalent condition in componentProgram.applyCheckpointStartupPolicy with this helper, preserving the existing zero-replica behavior.deploy/operator/internal/controller/dynamographdeployment_program_test.go (1)
466-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
t.Logheadings to both rollout subtests.Both subtests execute three steps: fixture construction,
reconcileWorkerRolloutinvocation, and verification. Neither subtest states its story in the test output. Add one heading before each step, as the other tests in this file do.♻️ Proposed headings for the single-node subtest
t.Run("single-node component workload starts a managed rollout", func(t *testing.T) { + t.Log("Build a single-node worker change against a stale hash annotation") dgd := createTestDGD("test-dgd", map[string]*nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{ @@ status := dgd.DeepCopy().Status + t.Log("Reconcile the worker rollout against the isolated status accumulator") require.NoError(t, program.reconcileWorkerRollout(context.Background(), dgd, &status)) + t.Log("Verify the managed rollout starts without mutating request status or the hash") require.NotNil(t, status.RollingUpdate)As per coding guidelines: "In Go tests, use
t.Logto explain the test's story, with one heading before each block implementing a test step".🤖 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/operator/internal/controller/dynamographdeployment_program_test.go` around lines 466 - 511, Add three t.Log headings to each rollout subtest—before fixture construction, before the reconcileWorkerRollout invocation, and before verification—using the existing test-output heading style from this file. Keep the test logic and assertions unchanged.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_grove_program.go (1)
205-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit the stable-resource loop and add story comments.
reconcileStableResourcesspans about 180 lines and mixes three concerns: component Service synchronization, frontend Ingress synchronization, and frontend VirtualService synchronization. Two improvements apply here:
- Extract one helper per resource kind, for example
reconcileComponentService,reconcileFrontendIngress, andreconcileFrontendVirtualService. Each helper returns([]Resource, error).- Add a one-line story comment above the annotation-reconciliation block at Lines 244-273 and above the frontend blocks at Lines 287 and 327. The Service block at Lines 210-216 already has one.
isK8sDiscoveryEnabledat Lines 212-215 is loop-invariant. Compute it once before the loop.As per coding guidelines: "In Go code, put a one-line story comment above every multi-line block of logically connected code."
🤖 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/operator/internal/controller/dynamographdeployment_grove_program.go` around lines 205 - 364, Refactor reconcileStableResources by computing isK8sDiscoveryEnabled once before the component loop and extracting component Service, frontend Ingress, and frontend VirtualService synchronization into separate helpers returning ([]Resource, error). Preserve existing behavior and error handling while composing each helper’s resources in the loop. Add one-line story comments above the Service annotation-reconciliation block and the frontend Ingress and VirtualService blocks.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_grove_program_test.go (2)
75-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the ordering assertion out of the interceptor closure.
The
Getinterceptor closure asserts that a scale update already happened. This hides bespoke verification logic inside a setup closure. Record the observation order in the closure, then assert it in the test body.For example, capture
scaleUpdatesAtFirstRead := len(scaleClient.updates)on the first PodClique read, and assertassert.Equal(t, 1, scaleUpdatesAtFirstRead)afterreconcileWorkloadsreturns.As per path instructions: "Keep bespoke test execution in the test body rather than hiding it in closures".
🤖 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/operator/internal/controller/dynamographdeployment_grove_program_test.go` around lines 75 - 89, Move the readiness ordering assertion out of the Get interceptor in the test setup. In the PodClique branch, record the scaleClient.updates length only on the first read using a variable such as scaleUpdatesAtFirstRead; after reconcileWorkloads returns, assert that recorded value equals 1 in the test body, while keeping the interceptor focused on observation and delegation.Source: Path instructions
40-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
t.Logstory headings in the new controller tests. The repository requires onet.Logheading before each block that implements a test step. Three new tests omit them, so the test output does not describe the scenario being verified.
deploy/operator/internal/controller/dynamographdeployment_grove_program_test.go#L40-L111: add headings for fixture construction, fake-client and interceptor setup,reconcileWorkloadsinvocation, and verification.deploy/operator/internal/controller/dynamographdeployment_grove_scaling_test.go#L102-L159: add headings for fixture construction, scaling reconciliation, and verification, and record whygraph-0-defaultedis absent from the expected updates.deploy/operator/internal/controller/dynamographdeployment_program_test.go#L466-L511: add headings inside botht.Runsubtests for fixture construction,reconcileWorkerRolloutinvocation, and verification.As per path instructions: "Use one
t.Logheading before each block that implements a test step so test output tells the scenario's story."🤖 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/operator/internal/controller/dynamographdeployment_grove_program_test.go` around lines 40 - 111, Add descriptive t.Log headings before each test-step block in deploy/operator/internal/controller/dynamographdeployment_grove_program_test.go:40-111 for fixture construction, fake-client/interceptor setup, reconcileWorkloads invocation, and verification. In deploy/operator/internal/controller/dynamographdeployment_grove_scaling_test.go:102-159, add headings for fixture construction, scaling reconciliation, and verification, documenting why graph-0-defaulted is absent from expected updates. In both t.Run subtests in deploy/operator/internal/controller/dynamographdeployment_program_test.go:466-511, add headings for fixture construction, reconcileWorkerRollout invocation, and verification.Sources: Coding guidelines, Path instructions
deploy/operator/internal/controller/dynamographdeployment_grove_renderer.go (1)
255-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
checkpointInfoByComponenta required parameter.The production caller passes one map. Only the generic test omits it; update that call to pass
nilor an empty map.🤖 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/operator/internal/controller/dynamographdeployment_grove_renderer.go` around lines 255 - 273, Update preserveGrovePodCliqueSetReplicas to accept checkpointInfoByComponent as a required map parameter rather than a variadic argument, and adjust its callers accordingly. In the generic test caller that currently omits the map, pass nil or an empty map while preserving the existing production caller’s behavior.deploy/operator/internal/controller/dynamographdeployment_grove_watches.go (1)
168-176: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCheck the
PodCliqueSetowner API version before enqueuing a DGD request. ComparepcsOwnerRef.APIVersionwithnvidiacomv1beta1.GroupVersion.String()in addition to checkingKind. Otherwise, a same-kind owner from another API group can produce an incorrect request.🤖 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/operator/internal/controller/dynamographdeployment_grove_watches.go` around lines 168 - 176, Update the owner-reference validation in the PodCliqueSet watch handler to require pcsOwnerRef.APIVersion to equal nvidiacomv1beta1.GroupVersion.String(), alongside the existing controller and Kind checks, before enqueuing a DynamoGraphDeployment request.
🤖 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/operator/internal/controller/dynamographdeployment_grove_renderer.go`:
- Around line 328-358: Update orderLikeExisting to verify that the reordered
result preserves every item in desired. After constructing ordered, return
desired unchanged if ordered has fewer items than desired; otherwise retain the
existing ordering behavior.
---
Nitpick comments:
In `@deploy/operator/internal/checkpoint/podspec.go`:
- Around line 256-308: Remove the redundant restore.info.Ready argument passed
to PrepareRestorePodSpec and the restore.info.Ready condition guarding
GPUMemoryService handling in InjectResolvedCheckpointIntoPodSpec, since
ResolvedPodSpecRestore is only produced for ready checkpoints. Preserve the
existing storage, seccompProfile, GPU mode, and error behavior.
In
`@deploy/operator/internal/controller/dcd_checkpoint_storage_reconciler_test.go`:
- Around line 74-101: Add separate t.Log headings before each test-step block in
the subtest: fixture construction, reconciler execution, and PVC-state
assertion. Extend the table-driven cases with a scenario where
Experimental.Checkpoint is declared but Enabled is false, and assert the
expected PVC behavior for that case alongside the existing enabled and absent
configurations.
In `@deploy/operator/internal/controller/dcd_checkpoint_storage_reconciler.go`:
- Around line 54-72: Add a single one-line story comment immediately above the
validation and early-return block in Reconcile, covering the nil deployment, nil
feature gate, disabled Checkpoint gate, and missing checkpoint configuration
checks. Leave the validation logic and EnsureStoragePVC call unchanged.
In `@deploy/operator/internal/controller/dynamocomponentdeployment_controller.go`:
- Around line 184-191: Add a concise one-line story comment immediately above
the checkpointStorageReconciler creation and Reconcile block, describing that
checkpoint storage is converged before workload rendering. Do not alter the
reconciliation logic or error handling.
In `@deploy/operator/internal/controller/dynamocomponentdeployment_renderer.go`:
- Around line 395-438: The nil guard in hasExistingLegacyWorkerSelector is
incomplete because the method dereferences runtimeConfig when evaluating the LWS
feature gate. Extend the initial guard to include r.runtimeConfig, preserving
the existing false, nil return for an unavailable renderer dependency.
- Around line 81-105: Update renderMultinodePodTemplateSpecs to resolve the
workload component type once, then pass that resolved value through
getDCDWorkloadPodLabels and the leader/worker pod-template generation helpers.
Adjust generatePodTemplateSpec and related helper signatures to reuse the
supplied type instead of calling getDCDWorkloadComponentType per role. Preserve
existing label and pod-spec behavior while eliminating repeated legacy-object
lookups and ensuring both paths use the same resolution.
In
`@deploy/operator/internal/controller/dynamographdeployment_controller_test.go`:
- Around line 455-529: Add t.Log headings for each setup and verification step
in TestReconcileScalingAdaptersEmitsDeleteEventOnlyAfterSuccessfulDelete,
TestGroveWatchSetup_MapPodCliqueToRequests, and TestGroveChildEventPredicates.
Follow the existing heading style used by
TestDynamoGraphDeploymentReconciler_isGrovePathway and ensure every test-step
block has one descriptive log heading.
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`:
- Around line 141-149: Declare a reasonFailedToHandleFinalizer Reason constant
alongside the existing Reason constants, then update the
newWorkloadProgramResult failure call in the finalizer error path to use it
instead of the raw string.
- Around line 172-187: Update persistWorkloadProgramResult to compare the
projected result.Status with dgd.Status before calling r.Status().Update,
skipping the status write when unchanged. Ensure result.Events are still flushed
through r.Recorder.Event in both unchanged and successfully updated cases, while
preserving error propagation for failed status updates.
- Around line 154-169: Add one-line story comments immediately above the
workload program selection/reconciliation block and above the result
persistence/error-propagation block in the reconciliation flow surrounding
selectWorkloadProgram, program.Reconcile, and persistWorkloadProgramResult. Keep
each comment concise and describing the block’s purpose.
In
`@deploy/operator/internal/controller/dynamographdeployment_grove_program_test.go`:
- Around line 75-89: Move the readiness ordering assertion out of the Get
interceptor in the test setup. In the PodClique branch, record the
scaleClient.updates length only on the first read using a variable such as
scaleUpdatesAtFirstRead; after reconcileWorkloads returns, assert that recorded
value equals 1 in the test body, while keeping the interceptor focused on
observation and delegation.
- Around line 40-111: Add descriptive t.Log headings before each test-step block
in
deploy/operator/internal/controller/dynamographdeployment_grove_program_test.go:40-111
for fixture construction, fake-client/interceptor setup, reconcileWorkloads
invocation, and verification. In
deploy/operator/internal/controller/dynamographdeployment_grove_scaling_test.go:102-159,
add headings for fixture construction, scaling reconciliation, and verification,
documenting why graph-0-defaulted is absent from expected updates. In both t.Run
subtests in
deploy/operator/internal/controller/dynamographdeployment_program_test.go:466-511,
add headings for fixture construction, reconcileWorkerRollout invocation, and
verification.
In `@deploy/operator/internal/controller/dynamographdeployment_grove_program.go`:
- Around line 205-364: Refactor reconcileStableResources by computing
isK8sDiscoveryEnabled once before the component loop and extracting component
Service, frontend Ingress, and frontend VirtualService synchronization into
separate helpers returning ([]Resource, error). Preserve existing behavior and
error handling while composing each helper’s resources in the loop. Add one-line
story comments above the Service annotation-reconciliation block and the
frontend Ingress and VirtualService blocks.
In `@deploy/operator/internal/controller/dynamographdeployment_grove_renderer.go`:
- Around line 255-273: Update preserveGrovePodCliqueSetReplicas to accept
checkpointInfoByComponent as a required map parameter rather than a variadic
argument, and adjust its callers accordingly. In the generic test caller that
currently omits the map, pass nil or an empty map while preserving the existing
production caller’s behavior.
In `@deploy/operator/internal/controller/dynamographdeployment_grove_scaling.go`:
- Around line 57-71: Extract the shared checkpoint startup-gating predicate
currently in the Grove scaling logic into a reusable helper, such as
checkpointGatesStartup, in an accessible checkpoint or controller package.
Replace the inline condition in the Grove replica calculation and the equivalent
condition in componentProgram.applyCheckpointStartupPolicy with this helper,
preserving the existing zero-replica behavior.
In `@deploy/operator/internal/controller/dynamographdeployment_grove_watches.go`:
- Around line 168-176: Update the owner-reference validation in the PodCliqueSet
watch handler to require pcsOwnerRef.APIVersion to equal
nvidiacomv1beta1.GroupVersion.String(), alongside the existing controller and
Kind checks, before enqueuing a DynamoGraphDeployment request.
In `@deploy/operator/internal/controller/dynamographdeployment_program_test.go`:
- Around line 466-511: Add three t.Log headings to each rollout subtest—before
fixture construction, before the reconcileWorkerRollout invocation, and before
verification—using the existing test-output heading style from this file. Keep
the test logic and assertions unchanged.
In `@deploy/operator/internal/dynamo/graph_test.go`:
- Around line 1245-1320: Add t.Log headings in
TestGenerateGrovePodCliqueSet_InjectsReadyCheckpointRestore before each test
step: building the PVC-backed fixtures, generating the PodCliqueSet, and
asserting the injected checkpoint volume. Keep the existing setup, generation,
and assertions unchanged.
In `@deploy/operator/internal/dynamo/graph.go`:
- Around line 2457-2472: Extract the shared checkpoint policy into a helper such
as needsResolvedRestore, and use it both in buildCliqueForRole’s
shouldUseAdmissionRestore logic and this checkpoint.ResolvePodSpecRestore block
so the conditions remain consistent. Add a one-line story comment immediately
above this multi-line restore-resolution block describing its purpose.
🪄 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: c5d24667-7ef0-45ac-bca1-debc83c00d5c
📒 Files selected for processing (26)
deploy/operator/internal/checkpoint/checkpoint_test.godeploy/operator/internal/checkpoint/podspec.godeploy/operator/internal/checkpoint/resolve.godeploy/operator/internal/checkpoint/resource.godeploy/operator/internal/controller/dcd_checkpoint_storage_reconciler.godeploy/operator/internal/controller/dcd_checkpoint_storage_reconciler_test.godeploy/operator/internal/controller/dynamocomponentdeployment_controller.godeploy/operator/internal/controller/dynamocomponentdeployment_controller_test.godeploy/operator/internal/controller/dynamocomponentdeployment_renderer.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.godeploy/operator/internal/controller/dynamographdeployment_grove_program.godeploy/operator/internal/controller/dynamographdeployment_grove_program_test.godeploy/operator/internal/controller/dynamographdeployment_grove_renderer.godeploy/operator/internal/controller/dynamographdeployment_grove_scaling.godeploy/operator/internal/controller/dynamographdeployment_grove_scaling_test.godeploy/operator/internal/controller/dynamographdeployment_grove_status.godeploy/operator/internal/controller/dynamographdeployment_grove_watches.godeploy/operator/internal/controller/dynamographdeployment_program.godeploy/operator/internal/controller/dynamographdeployment_program_test.godeploy/operator/internal/controller/dynamographdeployment_rollingupdate.godeploy/operator/internal/controller/dynamographdeployment_rollingupdate_test.godeploy/operator/internal/controller/upgrade_test.godeploy/operator/internal/dynamo/graph.godeploy/operator/internal/dynamo/graph_test.godeploy/operator/internal/dynamo/grove.go
github-actions
Bot
added
the
documentation
|
/ok to test 6773306 |
julienmancuso
changed the title
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (50 commits) docs(cli): correct removed vLLM prefill-worker flag reference (#12581) docs(operator): reserve webhook Ignore for emergencies (#12563) ci(docs): make previews and checks match what actually publishes (#12339) refactor(vllm): organize custom encoder modules (#12416) feat(llm): Select reasoning output field via env var (#11464) feat(runtime): add TLS support to TCP request plane (#10921) fix: convert conditional disagg sglang warning to httperror 400 (#12578) feat(operator): add runtime feature gates (#12421) refactor(runtime): extract PushRouter transport seam behind StreamingDispatch trait (#12447) feat(replay): add deterministic canonical offline reports (#12363) build: bump ModelExpress to 0.5.0(OPS-7978) (#12455) fix(mocker): use logical KV tokens for decode timing (#12583) fix(examples): update Triton example for CUDA 13 + fix libdcgm copy (DYN-3697) (#12577) refactor(operator): implement composition-first DGD reconciliation (#12283) feat(frontend): add basetenkenizer backend (#12376) fix(profiler): configure rapid mocker without planner (#12573) docs(vllm): correct worker-role flags and document --kv-transfer-config (#12568) ci: add Kubernetes deploy test to nightly (#12090) fix(container): reuse pinned protoc in runtime image (#12535) feat(self-host): flip DYN_SELF_HOST_METADATA default to ON (gh-8749) (#11417) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
3 participants
Summary
Complete the composition-first refactor of
DynamoGraphDeploymentreconciliation while preserving the existing workload pathways and Kubernetes behavior.This pull request consolidates the full series into one reviewable change. Component/DCD and Grove remain separate, complete workload programs, but both now compose focused reconcilers with explicit dependencies instead of delegating back to the all-capable
DynamoGraphDeploymentReconciler.The outer DGD reconciler is reduced to lifecycle ownership: load and finalize the DGD, select one workload program, invoke it once, persist its authoritative status, and emit queued status-transition events after persistence succeeds.
What changed
Establish a small program contract
ctrl.Result, complete desired status, and queued status-transition events in the program result.Compose complete, separate workload programs
The Component program owns DCD generation and synchronization, managed worker rollout, restart progress, checkpoint startup policy, readiness aggregation, and component-path cleanup.
The Grove program owns PodCliqueSet rendering and synchronization, Grove scaling, stable services and ingress resources, restart progress, coherent readiness observation, topology-condition projection, and Grove child watches.
Decompose shared DGD reconciliation
Replace the former monolithic program-input flow with focused reconcilers for:
The shared-resources reconciler preserves the existing execution order and returns focused, typed checkpoint observations for later workload steps.
Narrow rendering and provider boundaries
client.Readerdependencies.Make source and test ownership explicit
Status and event ownership
The selected program returns the complete desired status for the reconciliation attempt, including:
Readycondition;ObservedGeneration;The outer reconciler persists that status once. Status-transition events are queued by the program and emitted only after persistence succeeds. Events for child-resource mutations are emitted only when the corresponding create, update, patch, or delete succeeds and produces a semantic change.
Behavior and compatibility
This is an internal architecture refactor. It does not intentionally change:
Component and Grove remain separate complete workload programs. No cross-provider lifecycle or capability-driven rollout algorithm is introduced.
Review guidance
A useful review order is:
Review history
This pull request contains the cumulative work previously presented through:
GitHub marked #12502 merged after its head became fully contained by this pull request.
Validation
Combined head
6773306ee9passed:The ARM64 tester runs the complete operator
make testtarget, including formatting, vetting, generated-resource checks, envtest-backed tests, and coverage. Controller-package coverage is 73.9%.Summary by CodeRabbit
New Features
Bug Fixes
Tests