refactor(operator): move rollout control into workload programs - #12121
refactor(operator): move rollout control into workload programs#12121julienmancuso wants to merge 3 commits into
Conversation
github-actions
Bot
added
refactor
deployment::k8s
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
I think this PR makes the important architectural move in the right direction: the selected program now owns the order and timing of rollout preparation, shared reconciliation steps, restart resolution, workload reconciliation, and result processing. The outer DGD reconciler selects one program and invokes it once rather than driving provider hooks through a common lifecycle. So this is not an objection to the composition-first direction. My concern is narrower: before this boundary becomes the contract between separately packaged programs, I would give the program invocation a clearer input/output shape. The current contract is: type workloadProgram interface {
Reconcile(context.Context, *graphReconcileState) error
}
This makes That is especially relevant to the design goal here. The program correctly owns the flow, but the shared mutable state makes part of that flow implicit again. Suggested program contractI would follow controller-runtime's type workloadProgram interface {
Reconcile(
context.Context,
Request,
) (Result, error)
}
type Request struct {
// DGD is the mutable primary object for this reconciliation.
//
// The program may mutate and directly persist non-status fields. Status
// persistence remains owned by the outer DGD reconciler.
DGD *nvidiacomv1beta1.DynamoGraphDeployment
// Facts contains immutable observations that were genuinely resolved
// independently of the selected program. This can initially be empty.
Facts ResolvedFacts
}
type Result struct {
// Standard controller-runtime requeue semantics.
ctrl.Result
// Status is the complete desired DGD status produced by the program.
// The outer reconciler persists it once. Nil means that the program did
// not produce a successful status result.
Status *nvidiacomv1beta1.DynamoGraphDeploymentStatus
}I do not think we need a generic Status is different because there should be exactly one status writer. The selected program returns the desired The current code already has this useful property: DGD conditions are accumulated in memory and there is one central DGD status update. The proposed result type makes that ownership part of the program contract rather than an implicit property of the enclosing reconciler. Conceptually: programResult, err := program.Reconcile(ctx, Request{
DGD: dgd,
})
if err != nil {
return ctrl.Result{}, err
}
if programResult.Status != nil {
dgd.Status = *programResult.Status
}
// Add controller-owned Ready/ObservedGeneration/common conditions in memory.
// The existing outer status path persists the complete status exactly once.
return programResult.Result, nilKeep derived state local and typedI would not move the current Instead, the concrete program should continue to decide when shared operations run, but those operations should return structured values rather than mutate a universal state blob: func (p *componentProgram) Reconcile(
ctx context.Context,
req Request,
) (Result, error) {
inputs, err := p.reconcileInputs(ctx, req.DGD, req.Facts)
if err != nil {
return Result{}, err
}
if inputs.HasMultinode && !p.lwsEnabled {
return Result{}, fmt.Errorf("no multinode orchestrator available")
}
restart := p.resolveRestart(req.DGD, inputs)
workloads, err := p.reconcileWorkloads(ctx, componentWorkloadRequest{
DGD: req.DGD,
Inputs: inputs,
Restart: restart.State,
Checkpoints: inputs.Checkpoints,
})
if err != nil {
return Result{}, err
}
status := req.DGD.DeepCopy().Status
applyComponentResult(&status, restart, workloads)
return Result{
Status: &status,
}, nil
}The exact intermediate types are less important than the direction: each value should state what has been established and what the next operation may rely on. Comparison
This should be a focused contract refactor, not an architectural rewrite. The implementation already lands on the composition-first side with respect to flow ownership. I would make this change before extracting Grove and the other pathways into separate packages, while the boundary is still package-private and inexpensive to change. Otherwise |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe controller now delegates reconciliation to typed workload programs, prepares shared program inputs, preserves status across failures, and threads deployment status through rolling-update helpers. Component and Grove paths share the new request/result flow, with updated tests covering selection, immutability, rollout, and state transitions. ChangesWorkload program reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![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 (3)
deploy/operator/internal/controller/dynamographdeployment_program.go (3)
338-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent failure reason for the same
desiredWorkerHashescall.Line 342 classifies a
desiredWorkerHashesfailure asreasonFailedToInitializeWorkerHash, while the identical call at line 377 reportsreasonRollingUpdateFailed. Same operation, same log message, two different reasons surfaced to users.♻️ Align the reason
hashes, err := r.desiredWorkerHashes(dgd) if err != nil { logger.Error(err, "Failed to compute worker hash for unsupported pathway") - return failWorkloadProgram(reasonRollingUpdateFailed, err) + return failWorkloadProgram(reasonFailedToInitializeWorkerHash, err) }🤖 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.go` around lines 338 - 381, Align the failure reason for the later desiredWorkerHashes call in the unsupported-pathway rolling-update flow with the earlier initialization call. Update the return after the second call to use reasonFailedToInitializeWorkerHash, keeping the existing logging and hash-update behavior unchanged.
165-174: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUnclassified failure reason for the missing-multinode-orchestrator path.
Every other terminal failure in this program is wrapped with
failWorkloadProgram(...), so the controller can surface a preciseReason. This one falls back to the genericfailed_to_reconcile_the_resources. If a dedicated reason existed before, this is a status regression; otherwise consider adding one for consistency.🤖 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.go` around lines 165 - 174, The missing-multinode-orchestrator branch in the reconciliation program currently returns a generic error without classifying the failure. Update this path to return through failWorkloadProgram(...) like the other terminal failures, using the existing dedicated reason for this condition if available, or add and use a specific reason for the unavailable multinode orchestrator. Preserve the current error context and return behavior.
495-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrove and component programs duplicate the shared tail sequence.
Lines 502-525 are a near-verbatim copy of
componentProgram.Reconcilelines 158-191 (inputs → checkpoint status → restart → workloads → result → apply), and the worker-hash migration at 495-498 repeatsreconcileWorkerRollout's first step. As the provider boundary grows, these will drift. A small shared helper taking afunc(ctx, workloadReconcileRequest) (ReconcileResult, error)would keep both pathways in lockstep while leaving rollout selection program-specific.🤖 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.go` around lines 495 - 526, Extract the duplicated reconciliation tail from the current program flow and componentProgram.Reconcile into a shared helper that accepts a workload reconciliation callback of type func(ctx, workloadReconcileRequest) (ReconcileResult, error). Have the helper handle reconcileProgramInputs, checkpoint status assignment, restart resolution, reconcileProgramResult, and applying the result, while preserving program-specific rollout selection. Also centralize the repeated worker-hash migration step used by reconcileWorkerRollout and this flow without changing existing error behavior.
🤖 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_program_test.go`:
- Around line 301-346: Refactor TestComponentProgram_ReconcileWorkerRollout into
a table-driven test covering single-node and multinode cases, parameterizing the
Multinode spec and expected RollingUpdate behavior. Add t.Log headings before
each setup, action, and assertion block so every test step is clearly
identified, while preserving the existing hash assertions and outcomes.
---
Nitpick comments:
In `@deploy/operator/internal/controller/dynamographdeployment_program.go`:
- Around line 338-381: Align the failure reason for the later
desiredWorkerHashes call in the unsupported-pathway rolling-update flow with the
earlier initialization call. Update the return after the second call to use
reasonFailedToInitializeWorkerHash, keeping the existing logging and hash-update
behavior unchanged.
- Around line 165-174: The missing-multinode-orchestrator branch in the
reconciliation program currently returns a generic error without classifying the
failure. Update this path to return through failWorkloadProgram(...) like the
other terminal failures, using the existing dedicated reason for this condition
if available, or add and use a specific reason for the unavailable multinode
orchestrator. Preserve the current error context and return behavior.
- Around line 495-526: Extract the duplicated reconciliation tail from the
current program flow and componentProgram.Reconcile into a shared helper that
accepts a workload reconciliation callback of type func(ctx,
workloadReconcileRequest) (ReconcileResult, error). Have the helper handle
reconcileProgramInputs, checkpoint status assignment, restart resolution,
reconcileProgramResult, and applying the result, while preserving
program-specific rollout selection. Also centralize the repeated worker-hash
migration step used by reconcileWorkerRollout and this flow without changing
existing error 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: 1d15ac70-bfd2-44f4-82a4-f640213237b5
📒 Files selected for processing (6)
deploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.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.go
|
thanks @sttts , that makes sense, |
sttts
left a comment
sttts
left a comment
There was a problem hiding this comment.
One request-contract simplification.
| // resolvedFacts contains immutable observations resolved independently of the | ||
| // selected workload program. It is intentionally empty until such a fact is | ||
| // demonstrated; program-derived values remain typed locals. | ||
| type resolvedFacts struct{} |
There was a problem hiding this comment.
Disclosure: This comment was written by Codex as its interpretation of a discussion with Stefan; it is not Stefan’s text.
I would remove both resolvedFacts and the Facts field from workloadProgramRequest for now.
The outer reconciler currently does not resolve any program-independent observations before selecting and invoking the program. All ephemeral computed state is intentionally derived inside the selected program and in sequence: programInputs, programRestart, the component rolling-update context, and Grove render inputs. There is therefore no concrete value that belongs in Facts today.
Keeping an empty placeholder anticipates an abstraction without a demonstrated contract and creates an obvious place for a generic state bag to grow back over time. The honest current boundary seems to be:
type workloadProgramRequest struct {
// DGD is the mutable primary object. Programs may persist non-status
// mutations directly; proposed status is returned through Result.
DGD *nvidiacomv1beta1.DynamoGraphDeployment
}If a genuinely program-independent observation is later computed outside the selected program, we can add it then with a domain-specific type and name. Until that exists, the minimal request makes the ownership boundary clearer.
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
7bfdae6 to
34b2418
Compare
9aefb4d to
22c8135
Compare
|
🔄 Datadog auto-retried 4 jobs - 3 passed on retry 🔗 Commit SHA: 34b2418 | Docs | Datadog PR Page | Give us feedback! |
2 participants
Summary
This MR moves workload rollout control out of the outer
DynamoGraphDeploymentreconciliation flow and into the selected workload program.After finalization, the outer DGD controller now:
Each concrete program composes its complete ordered reconciliation flow.
The component pathway now performs:
The Grove pathway performs the equivalent composition while retaining its existing unsupported managed-rollout behavior and temporary Grove workload adapter.
This MR is stacked on the preceding workload-program MR.
Motivation
The outer DGD controller previously selected the workload pathway in one place while independently deciding rollout behavior in another.
That left the common controller coupled to component-specific decisions such as:
It also meant the selected workload program did not yet own its complete graph-level state machine.
This change makes rollout behavior part of each concrete program’s composition while keeping shared resource operations reusable and explicit.
Design
Complete workload programs
The
workloadProgramcontract remains a singleReconcilemethod. No provider lifecycle callbacks such asPrepare,Render,Scale, orCleanupare added.componentProgram.ReconcileandgroveProgram.Reconcileexplicitly compose their operations in order. Earlier operations enrichgraphReconcileState; later operations consume those resolved values.Shared reconciliation steps
The former
reconcileResourcesflow is split into focused shared operations:reconcileProgramInputsreconciles common resources and records resolved checkpoint and multinode information.resolveProgramRestartStatecomputes restart state after pathway-specific validation.reconcileProgramResultapplies checkpoint-readiness policy and reconciles scaling adapters.These are concrete shared operations composed by each program, not hooks driven by a generic framework.
Rollout ownership
componentProgramnow decides whether a component deployment uses:groveProgramexplicitly composes the unsupported-path worker-hash behavior.The underlying rolling-update helper implementations remain on
DynamoGraphDeploymentReconcilertemporarily. This MR moves control-flow ownership without combining it with a large mechanical dependency/receiver migration.Failure semantics
Program-specific rollout failures retain their existing DGD failure reasons:
failed_to_migrate_worker_hashfailed_to_initialize_worker_hashrolling_update_failedOther program failures retain
failed_to_reconcile_the_resources.A program publishes
state.Resultonly after its entire reconciliation sequence succeeds. A rollout, shared-resource, workload, or post-processing error therefore cannot replace the last-known-good component or restart status with a partial result.Behavior and compatibility
This is intended to be a behavior-preserving refactor:
There are no CRD, rendered-resource, ownership, watch-registration, or persisted-state changes.
Tests
Added or updated coverage for:
The existing rolling-update characterization suite remains unchanged apart from transferring the managed-rollout support predicate to
componentProgram.Summary by CodeRabbit
Improvements
Tests