feat(operator): add DisaggregatedSet workload program - #12625
Open
panpan0000 wants to merge 29 commits into
Open
feat(operator): add DisaggregatedSet workload program#12625panpan0000 wants to merge 29 commits into
panpan0000 wants to merge 29 commits into
Conversation
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 06:19 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 06:19 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
👋 Hi panpan0000! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
feat
external-contribution
This comment has been minimized.
This comment has been minimized.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
WalkthroughAdds feature-gated DisaggregatedSet reconciliation for eligible workloads. The operator now detects DisaggregatedSet and LeaderWorkerSet APIs, routes opted-in deployments through a dedicated DisaggregatedSet reconciliation path, manages service ownership during transitions between pathways, cleans up legacy resources, and validates readiness through both DisaggregatedSet and child LeaderWorkerSet status. ChangesDisaggregatedSet reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
deploy/operator/internal/controller/dgd_workload_program_test.go (2)
119-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test-step heading for the DisaggregatedSet assertions.
Add
t.Log("Verify the DisaggregatedSet program dependencies")before Line 119.Proposed change
if grove, ok := got.(*groveProgram); ok { assert.NotNil(t, grove.sharedResources) assert.NotNil(t, grove.rollout) assert.NotNil(t, grove.restart) assert.NotNil(t, grove.restartProgress) assert.NotNil(t, grove.workloads) assert.NotNil(t, grove.scalingAdapters) assert.NotNil(t, grove.topology) } + t.Log("Verify the DisaggregatedSet program dependencies") if ds, ok := got.(*disaggregatedSetProgram); ok {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/dgd_workload_program_test.go` around lines 119 - 125, Add a test-step heading immediately before the DisaggregatedSet type assertion block in the relevant test, using t.Log with the message “Verify the DisaggregatedSet program dependencies”. Keep the existing dependency assertions unchanged.Source: Coding guidelines
74-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the Grove-first routing conflict.
The added case tests only DisaggregatedSet opt-in. Add a case with
groveEnabled: trueand the DisaggregatedSet annotation. ExpectgroveProgram. This protects the required Grove-first routing order.Proposed test case
{ name: "explicit DisaggregatedSet opt-in selects DS program", annotations: map[string]string{ commonconsts.KubeAnnotationEnableDisaggregatedSet: commonconsts.KubeLabelValueTrue, }, wantProgram: &disaggregatedSetProgram{}, }, + { + name: "Grove pathway takes precedence over DisaggregatedSet opt-in", + groveEnabled: true, + annotations: map[string]string{ + commonconsts.KubeAnnotationEnableDisaggregatedSet: commonconsts.KubeLabelValueTrue, + }, + wantProgram: &groveProgram{}, + },🤖 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/dgd_workload_program_test.go` around lines 74 - 80, Add a test case in the workload program selection tests alongside the explicit DisaggregatedSet opt-in case, setting groveEnabled to true and retaining the DisaggregatedSet annotation; assert that wantProgram is groveProgram to verify Grove takes precedence over DisaggregatedSet routing.deploy/operator/internal/controller/dgd_workload_program.go (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required story comments before the new semantic blocks.
deploy/operator/internal/controller/dgd_workload_program.go#L81-L83: add a story comment for DisaggregatedSet selection after Grove routing.deploy/operator/internal/controller/dgd_disaggregatedset_program.go#L62-L131: add story comments for failure handling, fallback, rollout, shared resources, restart, workloads, and final result handling.deploy/operator/internal/controller/dgd_disaggregatedset_program.go#L139-L143: add a story comment for worker-hash migration and unsupported-rollout handling.deploy/operator/internal/controller/dgd_component_program.go#L60-L63: add a story comment for legacy cleanup wiring.deploy/operator/internal/controller/dgd_component_program.go#L140-L144: add a story comment for cleanup after successful component reconciliation.deploy/operator/internal/controller/dgd_grove_program.go#L65-L68: add a story comment for legacy cleanup wiring.deploy/operator/internal/controller/dgd_grove_program.go#L136-L140: add a story comment for cleanup after successful Grove reconciliation.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/dgd_workload_program.go` around lines 81 - 83, Add one-line story comments above each specified multi-line semantic block: in deploy/operator/internal/controller/dgd_workload_program.go lines 81-83, document DisaggregatedSet selection after Grove routing; in deploy/operator/internal/controller/dgd_disaggregatedset_program.go lines 62-131, document failure handling, fallback, rollout, shared resources, restart, workloads, and final result handling, and lines 139-143, document worker-hash migration and unsupported-rollout handling; in deploy/operator/internal/controller/dgd_component_program.go lines 60-63 and 140-144, document legacy cleanup wiring and cleanup after successful component reconciliation; and in deploy/operator/internal/controller/dgd_grove_program.go lines 65-68 and 136-140, document legacy cleanup wiring and cleanup after successful Grove reconciliation.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_controller.go (1)
188-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a one-line story comment above each new predicate helper.
workloadRoutingAnnotationsChangedanddgdOwnedServiceEventPredicateeach contain a multi-line block of connected logic. The repository guidelines require a one-line story comment above such blocks. State why routing-annotation updates must bypass generation filtering, and why owned Service creation events are ignored.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 188 - 208, Add one-line story comments immediately above workloadRoutingAnnotationsChanged and dgdOwnedServiceEventPredicate, explaining that routing-annotation changes must bypass generation filtering and that owned Service creation events are ignored, respectively.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go (2)
965-977: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated owner-reference rewrite.
Lines 966-977 and lines 1027-1038 implement the same algorithm: drop every existing controller reference, drop any reference matching the new one, then append the new controller reference. Only the reference value differs. Extract one helper such as
replaceControllerOwnerReference(obj client.Object, ownerRef metav1.OwnerReference)and call it from both places. That keeps the adoption rule in one location.Also applies to: 1027-1038
🤖 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_disaggregatedset.go` around lines 965 - 977, Extract the duplicated owner-reference replacement algorithm into a shared helper such as replaceControllerOwnerReference(obj client.Object, ownerRef metav1.OwnerReference). Move the existing filtering and append logic from both call sites into that helper, then replace each inline block with a helper call while preserving the current adoption behavior.
1451-1480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive these delegations explicit collaborators instead of ad-hoc zero-value construction.
Each helper builds a
componentWorkloadsReconcilerat call time. Line 1479 builds one with no fields at all, which works only whileapplyCheckpointStartupPolicyreads no struct field. If that method later needssyncerorrollout, the call panics with a nil dereference, and nothing in the type signature warns about it. The repeated inline construction also re-creates the syncer and rollout collaborators on every reconcile.Hold one constructed collaborator on the DS pathway and pass it in, so the dependency is explicit and checked at construction time.
As per coding guidelines: "Use concrete nested reconcilers with explicit dependencies; programs must not retain or call an all-capable
DynamoGraphDeploymentReconciler."🤖 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_disaggregatedset.go` around lines 1451 - 1480, Refactor the DCD helper delegations, especially applyDCDCheckpointStartupPolicy and the methods on DynamoGraphDeploymentReconciler, to use one explicitly constructed componentWorkloadsReconciler with its syncer and rollout collaborators supplied at construction. Hold and reuse that collaborator on the DS pathway instead of rebuilding it per call or using the zero-value construction in applyDCDCheckpointStartupPolicy, and pass the concrete dependency through the relevant call sites.Source: Coding guidelines
deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.go (3)
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet the opt-in annotation without replacing the whole annotation map.
Line 149 replaces
current.Annotationswith a new single-entry map. The test then re-readscurrentafter each reconcile, so any annotation that the program or the API server adds to the DGD is silently dropped at this point. That makes the cutover step depend on the DGD carrying no other annotations. Mutate the existing map instead, and keep the nil case handled.♻️ Proposed change to set the annotation in place
- current.Annotations = map[string]string{consts.KubeAnnotationEnableDisaggregatedSet: consts.KubeLabelValueTrue} + if current.Annotations == nil { + current.Annotations = map[string]string{} + } + current.Annotations[consts.KubeAnnotationEnableDisaggregatedSet] = consts.KubeLabelValueTrue🤖 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_disaggregatedset_envtest_test.go` at line 149, Update the annotation assignment in the reconcile test to mutate current.Annotations in place rather than replacing the map, preserving existing annotations across re-reads. Initialize the map when it is nil, then set consts.KubeAnnotationEnableDisaggregatedSet to consts.KubeLabelValueTrue.
371-384: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueScope the DCD list to the test namespace.
ownedEnvtestCutoverDCDslistsDynamoComponentDeploymentobjects across all namespaces and then filters by controller reference. The filter keeps the result correct, but the list grows with every object that other suites leave behind. Addclient.InNamespace(dgd.Namespace)to keep the read scoped.♻️ Proposed change to scope the list
- Expect(k8sClient.List(ctx, list)).To(Succeed()) + Expect(k8sClient.List(ctx, list, client.InNamespace(dgd.Namespace))).To(Succeed())🤖 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_disaggregatedset_envtest_test.go` around lines 371 - 384, Update ownedEnvtestCutoverDCDs to scope the DynamoComponentDeploymentList call to dgd.Namespace by passing client.InNamespace(dgd.Namespace) to k8sClient.List, while preserving the existing controller-reference filtering and returned results.
359-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the component service name the same way in both helpers.
createComponentServicesreads the service atdcds[i].Name(line 365). The cutover assertion reads the same service atdynamo.NormalizeKubeResourceName(dcd.Name)(line 201). The two derivations agree only while every generated DCD name is already normalized. If a component name ever needs normalization, this helper fails with aNotFoundinstead of reporting the ownership behavior under test. Use the same normalization in both places.♻️ Proposed change to normalize the service name
service := &corev1.Service{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: dcds[i].Name, Namespace: dcds[i].Namespace}, service)).To(Succeed()) + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: dynamo.NormalizeKubeResourceName(dcds[i].Name), + Namespace: dcds[i].Namespace, + }, service)).To(Succeed()) serviceUIDs[service.Name] = service.UID🤖 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_disaggregatedset_envtest_test.go` around lines 359 - 366, Update the service lookup in createComponentServices to derive the name with dynamo.NormalizeKubeResourceName, matching the cutover assertion’s lookup. Use the normalized name consistently when retrieving the Service and recording its UID, while preserving the existing namespace and ownership assertions.deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.go (2)
146-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a status-only change in the predicate test.
disaggregatedSetStatusChangedandleaderWorkerSetStatusChangedgate reconciliation on status transitions. This test exercises only label changes and an owner-reference change. It never asserts that a change confined tostatusreturnstrue. A regression that drops status comparison would still pass. Add a case that mutates onlystatuson both object kinds.As per coding guidelines: "Preserve coverage for pathway selection, rollout and restart transitions, readiness, significant watches, ownership, cleanup, and generated-resource parity."
🤖 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_disaggregatedset_test.go` around lines 146 - 176, Add status-only mutation cases to TestDisaggregatedSetPredicatesObserveRoutingMetadata for both disaggregatedSetStatusChanged and leaderWorkerSetStatusChanged, changing only each object’s status while preserving labels, ownership, and other metadata; assert the predicates return true for those status transitions.Source: Coding guidelines
22-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a table test for the component selection cases.
The two subtests build almost the same
DynamoGraphDeploymentand differ only in theScalingAdapterfield and the expected outcome. A table test keeps the fixture in one place and makes future rejection reasons cheap to add.As per coding guidelines: "Prefer table tests over one-off Go tests and do not duplicate behavior already covered by a table."
🤖 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_disaggregatedset_test.go` around lines 22 - 78, Refactor TestSelectDisaggregatedSetComponents into a table-driven test, consolidating the shared DynamoGraphDeployment fixture and varying only fields such as ScalingAdapter and expected selection or rejection reason. Preserve the existing assertions for multinode role selection and scaling-adapter rejection, while making additional cases easy to add.Source: Coding guidelines
deploy/operator/internal/controller/dgd_disaggregatedset_workloads_reconciler.go (1)
73-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd story comments for restart-control blocks.
Add a one-line story comment before each multi-line semantic block. State the restart-state transition, such as preserving terminal state, superseding a restart, waiting for a component, or coalescing selected components.
As per coding guidelines, “In Go code, put a one-line story comment above every multi-line block of logically connected code.”
Also applies to: 103-145, 153-206, 214-231
🤖 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/dgd_disaggregatedset_workloads_reconciler.go` around lines 73 - 94, Add one-line story comments above each multi-line restart-control block in the restart-status logic, including the terminal-state preservation, already-processed handling, rolling-update supersession, parallel/sequential computation, and the additionally referenced blocks. Each comment should describe the resulting restart-state transition or coordination behavior without changing the implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/operator/internal/controller/dgd_disaggregatedset_program.go`:
- Line 52: The workload programs retain excessive controller coupling and invoke
another complete program. In
deploy/operator/internal/controller/dgd_disaggregatedset_program.go:52, pass a
scoped collaborator instead of the all-capable reconciler; in
deploy/operator/internal/controller/dgd_disaggregatedset_program.go:73-94,
remove componentProgram construction and invocation and implement fallback using
units owned by the DisaggregatedSet program. In
deploy/operator/internal/controller/dgd_component_program.go:60-63 and
deploy/operator/internal/controller/dgd_grove_program.go:65-68, inject concrete
legacy-cleanup collaborators instead of closures capturing the reconciler.
- Around line 85-95: Move restoreDisaggregatedSetServiceOwnershipToDCDs and
deleteDisaggregatedSetIfExists out of the unconditional post-componentProgram
path in componentProgram’s caller, and execute both only when the result state
is DGDStateSuccessful. Preserve pending fallback results without cleanup, and
add coverage for the DS-annotation path where replacement DCDs remain pending.
In
`@deploy/operator/internal/controller/dgd_disaggregatedset_workloads_reconciler.go`:
- Around line 29-41: Remove the owner field and *DynamoGraphDeploymentReconciler
parameter from newDisaggregatedSetWorkloadsReconciler, replacing them with
narrowly scoped collaborators for resource reconciliation and restart
observation. Update the call sites around lines 66, 133, and 179 to use those
injected collaborators directly, ensuring disaggregatedSetWorkloadsReconciler no
longer retains or invokes the all-capable outer controller.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.go`:
- Around line 111-144: Add t.Log headings before each distinct scenario block in
TestCheckDisaggregatedSetReadiness: stale observedGeneration, lagging decode
readiness, and fully ready status. Apply the same one-heading-per-step
convention to the other tests in this file, using concise messages that identify
each scenario and preserve the existing assertions and mutations.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go`:
- Around line 445-450: Guard the dcds[componentName] lookup in the loop that
builds selection.desiredReplicas before modifying or dereferencing the
deployment. If the component is absent, return the same explicit
missing-component error used by generateDisaggregatedSet and
reconcileDisaggregatedSetSideResources; otherwise preserve the existing replica
assignment and desiredComponentReplicas flow.
- Around line 315-322: Update reconcileDisaggregatedSetSideResources and its
Service cleanup logic so generated Services are deleted only for DCDs removed
from the current selection, not for selected DCDs. Account for generateService
normalizing dcd.Name when matching cleanup targets, while preserving Services
for selected components with Service enabled to prevent endpoint loss and
reconcile churn.
In `@deploy/operator/internal/testing/operatorenv/env.go`:
- Line 496: Replace the leading spaces on the filepath.Join entry in the
surrounding environment path list with a tab, matching adjacent lines and gofmt
formatting.
---
Nitpick comments:
In
`@deploy/operator/internal/controller/dgd_disaggregatedset_workloads_reconciler.go`:
- Around line 73-94: Add one-line story comments above each multi-line
restart-control block in the restart-status logic, including the terminal-state
preservation, already-processed handling, rolling-update supersession,
parallel/sequential computation, and the additionally referenced blocks. Each
comment should describe the resulting restart-state transition or coordination
behavior without changing the implementation.
In `@deploy/operator/internal/controller/dgd_workload_program_test.go`:
- Around line 119-125: Add a test-step heading immediately before the
DisaggregatedSet type assertion block in the relevant test, using t.Log with the
message “Verify the DisaggregatedSet program dependencies”. Keep the existing
dependency assertions unchanged.
- Around line 74-80: Add a test case in the workload program selection tests
alongside the explicit DisaggregatedSet opt-in case, setting groveEnabled to
true and retaining the DisaggregatedSet annotation; assert that wantProgram is
groveProgram to verify Grove takes precedence over DisaggregatedSet routing.
In `@deploy/operator/internal/controller/dgd_workload_program.go`:
- Around line 81-83: Add one-line story comments above each specified multi-line
semantic block: in deploy/operator/internal/controller/dgd_workload_program.go
lines 81-83, document DisaggregatedSet selection after Grove routing; in
deploy/operator/internal/controller/dgd_disaggregatedset_program.go lines
62-131, document failure handling, fallback, rollout, shared resources, restart,
workloads, and final result handling, and lines 139-143, document worker-hash
migration and unsupported-rollout handling; in
deploy/operator/internal/controller/dgd_component_program.go lines 60-63 and
140-144, document legacy cleanup wiring and cleanup after successful component
reconciliation; and in deploy/operator/internal/controller/dgd_grove_program.go
lines 65-68 and 136-140, document legacy cleanup wiring and cleanup after
successful Grove reconciliation.
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`:
- Around line 188-208: Add one-line story comments immediately above
workloadRoutingAnnotationsChanged and dgdOwnedServiceEventPredicate, explaining
that routing-annotation changes must bypass generation filtering and that owned
Service creation events are ignored, respectively.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.go`:
- Line 149: Update the annotation assignment in the reconcile test to mutate
current.Annotations in place rather than replacing the map, preserving existing
annotations across re-reads. Initialize the map when it is nil, then set
consts.KubeAnnotationEnableDisaggregatedSet to consts.KubeLabelValueTrue.
- Around line 371-384: Update ownedEnvtestCutoverDCDs to scope the
DynamoComponentDeploymentList call to dgd.Namespace by passing
client.InNamespace(dgd.Namespace) to k8sClient.List, while preserving the
existing controller-reference filtering and returned results.
- Around line 359-366: Update the service lookup in createComponentServices to
derive the name with dynamo.NormalizeKubeResourceName, matching the cutover
assertion’s lookup. Use the normalized name consistently when retrieving the
Service and recording its UID, while preserving the existing namespace and
ownership assertions.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.go`:
- Around line 146-176: Add status-only mutation cases to
TestDisaggregatedSetPredicatesObserveRoutingMetadata for both
disaggregatedSetStatusChanged and leaderWorkerSetStatusChanged, changing only
each object’s status while preserving labels, ownership, and other metadata;
assert the predicates return true for those status transitions.
- Around line 22-78: Refactor TestSelectDisaggregatedSetComponents into a
table-driven test, consolidating the shared DynamoGraphDeployment fixture and
varying only fields such as ScalingAdapter and expected selection or rejection
reason. Preserve the existing assertions for multinode role selection and
scaling-adapter rejection, while making additional cases easy to add.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go`:
- Around line 965-977: Extract the duplicated owner-reference replacement
algorithm into a shared helper such as replaceControllerOwnerReference(obj
client.Object, ownerRef metav1.OwnerReference). Move the existing filtering and
append logic from both call sites into that helper, then replace each inline
block with a helper call while preserving the current adoption behavior.
- Around line 1451-1480: Refactor the DCD helper delegations, especially
applyDCDCheckpointStartupPolicy and the methods on
DynamoGraphDeploymentReconciler, to use one explicitly constructed
componentWorkloadsReconciler with its syncer and rollout collaborators supplied
at construction. Hold and reuse that collaborator on the DS pathway instead of
rebuilding it per call or using the zero-value construction in
applyDCDCheckpointStartupPolicy, and pass the concrete dependency through the
relevant call sites.
🪄 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: a355c4e2-942a-42ef-9a85-e85acd7beaee
📒 Files selected for processing (19)
deploy/helm/charts/platform/components/operator/files/manager-role.yamldeploy/operator/config/rbac/role.yamldeploy/operator/internal/consts/consts.godeploy/operator/internal/controller/dgd_component_program.godeploy/operator/internal/controller/dgd_disaggregatedset_program.godeploy/operator/internal/controller/dgd_disaggregatedset_workloads_reconciler.godeploy/operator/internal/controller/dgd_grove_program.godeploy/operator/internal/controller/dgd_workload_program.godeploy/operator/internal/controller/dgd_workload_program_test.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.godeploy/operator/internal/controller/testing/disaggregatedset/disaggregatedsets.yamldeploy/operator/internal/controller/testing/disaggregatedset/leaderworkersets.yamldeploy/operator/internal/features/gates.godeploy/operator/internal/features/gates_test.godeploy/operator/internal/testing/operatorenv/env.go
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 06:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 06:56 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 07:06 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Thanks for the detailed checks. Updates pushed:
The earlier CI warning about an unused variable was on an older SHA and should be cleared by these commits. |
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 07:32 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Replying to CodeRabbit summary (https://github.com/ai-dynamo/dynamo/pull/12625#issuecomment-5175424178):\n\n- Fallback observability: added DisaggregatedSetEligible condition + warning event (7b4925e).\n- Service cleanup: no longer deletes active worker endpoints; only removes stale DS-selected services (f97e50a).\n- Fallback safety: DS cleanup deferred until after component fallback reconcile succeeds (f97e50a).\n- Feature gate: DS now honors explicit LWS opt-out (400f7ca).\n\nThanks for the review. |
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 08:22 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
panpan0000
temporarily deployed
to
external_collaborator
August 4, 2026 08:43 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 32475d9 |
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go (1)
780-795: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the rendered Services instead of rendering them twice.
reconcileDisaggregatedSetSideResourcesalready builds a renderer and callsrenderer.generateServicefor every selected component.deleteStaleDisaggregatedSetComponentServicesrepeats both steps to recompute the same names.generateServiceperforms client reads, so each successful reconcile pays the cost twice, and the two name sets can diverge if rendering later becomes non-deterministic.Return the desired Service names from
reconcileDisaggregatedSetSideResourcesand pass them into the cleanup function.🤖 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_disaggregatedset.go` around lines 780 - 795, Update reconcileDisaggregatedSetSideResources to return the desired Service names it already collects, and pass that set into deleteStaleDisaggregatedSetComponentServices. Remove the cleanup function’s duplicate renderer creation and generateService calls, using the returned names for stale-service deletion while preserving existing error and deletion handling.deploy/operator/internal/controller/dgd_workload_program_test.go (1)
74-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover Grove precedence over DisaggregatedSet opt-in.
The Grove case does not set the DisaggregatedSet annotation. It does not verify the new selection order. A reorder that selects DisaggregatedSet before Grove would pass this table.
Add a case with
groveEnabled: trueandKubeAnnotationEnableDisaggregatedSet: "true". Expect*groveProgram.Based on PR objectives, Grove-first routing is a required selection contract.
Proposed test case
{ name: "explicit DisaggregatedSet opt-in selects DS program", annotations: map[string]string{ commonconsts.KubeAnnotationEnableDisaggregatedSet: commonconsts.KubeLabelValueTrue, }, wantProgram: &disaggregatedSetProgram{}, }, + { + name: "Grove takes precedence over DisaggregatedSet opt-in", + groveEnabled: true, + annotations: map[string]string{ + commonconsts.KubeAnnotationEnableDisaggregatedSet: commonconsts.KubeLabelValueTrue, + }, + wantProgram: &groveProgram{}, + },🤖 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/dgd_workload_program_test.go` around lines 74 - 80, Add a table-driven test case alongside the existing DisaggregatedSet opt-in case with groveEnabled set to true and KubeAnnotationEnableDisaggregatedSet set to the true value, and assert that the selected program is groveProgram. This must verify Grove routing takes precedence when both options are enabled.deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.go (1)
181-198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover DisaggregatedSet opt-out updates.
The test covers only false-to-true routing. Add true-to-false and true-to-absent cases. If an annotation-only opt-out does not satisfy this predicate, the controller does not enter the component fallback path.
Proposed change
t.Log("enabling DisaggregatedSet triggers update predicate") newDGD = oldDGD.DeepCopy() newDGD.Annotations[consts.KubeAnnotationEnableDisaggregatedSet] = consts.KubeLabelValueTrue require.True(t, workloadRoutingAnnotationsChanged(updateEvent(oldDGD, newDGD))) + + t.Log("disabling DisaggregatedSet triggers update predicate") + oldDGD.Annotations[consts.KubeAnnotationEnableDisaggregatedSet] = consts.KubeLabelValueTrue + newDGD = oldDGD.DeepCopy() + newDGD.Annotations[consts.KubeAnnotationEnableDisaggregatedSet] = consts.KubeLabelValueFalse + require.True(t, workloadRoutingAnnotationsChanged(updateEvent(oldDGD, newDGD))) + + t.Log("removing DisaggregatedSet triggers update predicate") + newDGD = oldDGD.DeepCopy() + delete(newDGD.Annotations, consts.KubeAnnotationEnableDisaggregatedSet) + require.True(t, workloadRoutingAnnotationsChanged(updateEvent(oldDGD, newDGD))) }🤖 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_disaggregatedset_test.go` around lines 181 - 198, Add coverage to TestWorkloadRoutingAnnotationsChanged for DisaggregatedSet opt-out transitions: verify true-to-false and true-to-absent annotation changes make workloadRoutingAnnotationsChanged return true. Keep the existing false-to-true and unchanged cases intact, using DeepCopy to construct each updated object.
🤖 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/dgd_disaggregatedset_program.go`:
- Around line 40-45: Keep complete programs independent by replacing
disaggregatedSetProgram’s newComponentProgram dependency with a narrow
fallback-reconciliation collaborator and invoking it from
disaggregatedSetProgram.Reconcile instead of constructing or calling
componentProgram.Reconcile; update
deploy/operator/internal/controller/dgd_disaggregatedset_program.go lines 40-45
and 84-87. In deploy/operator/internal/controller/dgd_component_program.go lines
37, 60-63, and 140-144, replace the reconciler callback with an explicit
legacy-cleanup collaborator, inject it without capturing
DynamoGraphDeploymentReconciler, and invoke it after successful reconciliation.
Apply the same collaborator extraction, injection, and post-success invocation
to deploy/operator/internal/controller/dgd_grove_program.go lines 35, 65-71, and
139-143, ensuring neither program retains or calls the all-capable reconciler.
- Around line 96-100: Update the fallback reconciliation flow around
fallbackProgram.Reconcile so DisaggregatedSetEligible=False is set on
programResult before returning retErr. Ensure subsequent failure-status handling
preserves this condition and add a test covering fallback reconciliation errors
with the persisted ineligibility condition.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.go`:
- Around line 330-340: The reconcileCurrentDGDProgram helper bypasses
DynamoGraphDeploymentReconciler.Reconcile by calling selectWorkloadProgram
directly, preventing tests from verifying that the outer controller handles
status persistence correctly. Add a new focused test that calls the full
DynamoGraphDeploymentReconciler.Reconcile method to verify the outer controller
boundary: confirm that only the outer controller writes to request.DGD.Status
and that the inner programs do not mutate that field. Retain
reconcileCurrentDGDProgram as-is for program-only behavior verification in
existing tests.
- Around line 169-213: Update the handoff assertions in the test around
modelService and fallbackDCDs to verify controller ownership, not only Service
UID preservation. While fallback is pending after removing the disaggregated-set
annotation, assert each preserved component Service remains owned by the DGD;
after DS readiness and DCD cleanup, assert every preserved component Service is
owned by a fallback DCD and retain the existing UID check for modelService.
- Around line 423-428: Update the synthetic Available condition in the
dcd.Status.Conditions assignment to set ObservedGeneration to dcd.Generation,
ensuring it identifies the generation that produced the condition.
In `@deploy/operator/internal/features/gates.go`:
- Around line 215-219: The selectWorkloadProgram function continues to select
disaggregatedSetProgram based on the opt-in annotation even when the
DisaggregatedSet feature gate is disabled. Update the workload selection logic
to include a check using RuntimeConfig.Gate.Enabled with
features.DisaggregatedSet alongside the existing annotation check, ensuring the
program is only selected when both the gate is enabled and the condition for
disaggregatedSetProgram is met, since SetupWithManager only registers the
required watches when this gate is enabled.
---
Nitpick comments:
In `@deploy/operator/internal/controller/dgd_workload_program_test.go`:
- Around line 74-80: Add a table-driven test case alongside the existing
DisaggregatedSet opt-in case with groveEnabled set to true and
KubeAnnotationEnableDisaggregatedSet set to the true value, and assert that the
selected program is groveProgram. This must verify Grove routing takes
precedence when both options are enabled.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.go`:
- Around line 181-198: Add coverage to TestWorkloadRoutingAnnotationsChanged for
DisaggregatedSet opt-out transitions: verify true-to-false and true-to-absent
annotation changes make workloadRoutingAnnotationsChanged return true. Keep the
existing false-to-true and unchanged cases intact, using DeepCopy to construct
each updated object.
In
`@deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go`:
- Around line 780-795: Update reconcileDisaggregatedSetSideResources to return
the desired Service names it already collects, and pass that set into
deleteStaleDisaggregatedSetComponentServices. Remove the cleanup function’s
duplicate renderer creation and generateService calls, using the returned names
for stale-service deletion while preserving existing error and deletion
handling.
🪄 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: dff844a2-40ec-4c49-b13d-dca2a41b0434
📒 Files selected for processing (19)
deploy/helm/charts/platform/components/operator/files/manager-role.yamldeploy/operator/config/rbac/role.yamldeploy/operator/internal/consts/consts.godeploy/operator/internal/controller/dgd_component_program.godeploy/operator/internal/controller/dgd_disaggregatedset_program.godeploy/operator/internal/controller/dgd_disaggregatedset_workloads_reconciler.godeploy/operator/internal/controller/dgd_grove_program.godeploy/operator/internal/controller/dgd_workload_program.godeploy/operator/internal/controller/dgd_workload_program_test.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset_test.godeploy/operator/internal/controller/testing/disaggregatedset/disaggregatedsets.yamldeploy/operator/internal/controller/testing/disaggregatedset/leaderworkersets.yamldeploy/operator/internal/features/gates.godeploy/operator/internal/features/gates_test.godeploy/operator/internal/testing/operatorenv/env.go
julienmancuso
left a comment
julienmancuso
left a comment
Contributor
There was a problem hiding this comment.
I reviewed this against the composition-first DGD rules in deploy/operator/internal/AGENTS.md and the behavior carried forward from the earlier DisaggregatedSet implementation.
The third-program direction is right, but I found two P1 correctness blockers and four P2 correctness/architecture blockers. In particular, the existing feedback about program nesting and all-capable-controller coupling is still valid: narrowing the static interface does not resolve it while the implementation remains DynamoGraphDeploymentReconciler.
Details are inline. I would address the P1s first, then restructure the DisaggregatedSet program around concrete collaborators before merge.
Contributor
|
also, additional scope gaps:
|
panpan0000
temporarily deployed
to
external_collaborator
August 9, 2026 13:06 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
- delete stale DGD-owned model Services whose component was removed from the DisaggregatedSet selection, not just per-component discovery Services - document the uncached reader used by DisaggregatedSet compatibility cleanup - extract the DisaggregatedSetEligible condition type into a constant - note the two-digit slice-index assumption in the name budget Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Unify the install pages with the LWS knowledge-base wording: the DisaggregatedSet pathway requires an LWS release that serves disaggregatedset.x-k8s.io/v1, with v0.10.0 as the current operator dependency. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
The rebase replay against main's recorder and secret-retriever refactors mixed old API shapes into two DisaggregatedSet files. Restore the pathway files to their reviewed state and reapply the review follow-ups. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
force-pushed
the
feat/disaggregatedset-program
branch
from
August 24, 2026 11:38
9c4101f to
fab4167
Compare
panpan0000
temporarily deployed
to
external_collaborator
August 24, 2026 11:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
temporarily deployed
to
external_collaborator
August 24, 2026 11:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
temporarily deployed
to
external_collaborator
August 24, 2026 12:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
reworked based on @sttts's feedback. |
sttts
reviewed
Aug 24, 2026
sttts
reviewed
Aug 24, 2026
sttts
reviewed
Aug 24, 2026
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
temporarily deployed
to
external_collaborator
August 25, 2026 08:54 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
temporarily deployed
to
external_collaborator
August 25, 2026 09:40 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
@sttts , Dr.Stefan, thank you a lot , comments fixed again :-) |
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
panpan0000
temporarily deployed
to
external_collaborator
August 30, 2026 12:17 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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
4 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.
Summary
Rebased onto
mainafter #12942 and integrated DisaggregatedSet as the third durable DGD workload provider.component,grove, anddisaggregatedset.nvidia.com/workload-providerbefore workload side effects and adopt legacy DGDs from their owned DCD, PodCliqueSet, or DisaggregatedSet resources.Ready=Falsewithselected_workload_provider_unavailablewhen the selected provider is unavailable; there is no provider fallback or cross-provider cleanup.slices=1until the grouping/cardinality API is available.Capabilitiesconcept and update operator tests and multinode documentation for the durable-provider model.Review focus
deploy/operator/internal/controller/dgd_workload_provider.godeploy/operator/internal/controller/dgd_disaggregatedset_program.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset.godeploy/operator/internal/controller/dynamographdeployment_disaggregatedset_envtest_test.goValidation
go test ./internal/controller -count=1— passed.go test ./... -count=1— all non-e2e operator packages passed; the e2e package was blocked during Docker image metadata resolution becausenvcr.ioand the configured Docker mirror returned HTTP 403.git diff --check— passed.Related Issues