Skip to content

feat(operator): add DisaggregatedSet pathway - #11645

Closed
panpan0000 wants to merge 33 commits into
ai-dynamo:mainfrom
panpan0000:feat/disaggregatedset-draft-pathway
Closed

feat(operator): add DisaggregatedSet pathway#11645
panpan0000 wants to merge 33 commits into
ai-dynamo:mainfrom
panpan0000:feat/disaggregatedset-draft-pathway

Conversation

@panpan0000

@panpan0000 panpan0000 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • detect disaggregatedset.x-k8s.io/v1 with its required LWS API independently from the existing LWS + Volcano pathway
  • route workloads with explicit precedence: Grove first, opt-in DisaggregatedSet second, and the standard DCD pathway as fallback
  • render eligible multinode worker roles into one DisaggregatedSet while retaining regular DCDs for non-selected components
  • reuse the existing LWS leader/worker pod-template rendering path
  • emit schema-valid LWS role specs with explicit rollout/restart policies and render every role with the stable DGD-level Kubernetes discovery identity
  • normalize desired DisaggregatedSets through an API-server dry run so reconciliation converges after CRD defaulting, including defaults added by compatible future CRDs
  • reconcile/adopt component and model Services, preserve shared model-Service ownership, and remove stale DGD-owned component Services only after the replacement is ready
  • restore component and shared model Service ownership to replacement DCDs before deleting a stale DisaggregatedSet during fallback; recreate a missing shared model Service under its selected replacement DCD before cleanup
  • coordinate checkpoint startup gating across every selected DS role, keeping legacy DCDs serving until checkpoint readiness and DS cutover
  • aggregate readiness and replica status across target and draining child LWS revisions; use status.roleStatuses directly when the installed DS API exposes it and require exact replica convergence
  • watch DS/LWS label and controller-owner changes used by routing, readiness, and event mapping
  • preserve unmanaged DS labels, annotations, and non-controller owner references while applying desired operator metadata during reconciliation
  • validate the DS API's 10-role limit and use stable hash truncation so upstream <set>-<revision>-<role> child names always fit the 63-character DNS label limit
  • migrate between DS and Grove/DCD pathways without deleting the serving workload before its replacement is ready
  • close lifecycle gaps identified in review: annotation-only pathway changes now reconcile, stale Grove PodCliqueSets are removed after DS cutover, DS restart coalescing only runs when DS wins routing, restart annotations survive consecutive requests after DCD cleanup, non-selected multinode roles require LWS, and reparented Services trigger DGD reconciliation
  • complete coalesced sequential DS restarts without waiting on an already-restarted sibling role, and accept a shared model Service already owned by any replacement DCD controlled by the same DGD
  • add generated RBAC, envtest fixtures, unit coverage, and Kubernetes documentation

Workload routing is ordered, not mutually exclusive: Grove > opt-in DS > DCD. On a Grove-enabled cluster, selecting DS requires both nvidia.com/enable-grove: "false" and nvidia.com/enable-disaggregatedset: "true"; installing the DS API alone never changes existing workload routing.

Compatibility and scope

  • requires LWS v0.9.0 or newer; Dynamo currently builds and tests against v0.9.0
  • treats status.roleStatuses as authoritative when accompanied by generation-observation evidence and otherwise reads target-revision child LeaderWorkerSets; if neither source proves readiness, cutover remains pending and the previous workload stays serving
  • keeps DS as a supported, explicit per-DGD opt-in below the default Grove pathway; the annotations are the per-DGD routing opt-in

The Dynamo platform chart intentionally does not add a synthetic disaggregatedset.enabled value. DS is supplied by an external LWS installation and detected at runtime.

Validation

  • KUBEBUILDER_ASSETS="$PWD/bin/k8s/1.30.0-darwin-arm64" go test ./internal/controller ./internal/controller_common/... ./cmd/... -count=1
    • controller envtest suite passed, including API-server-backed DCD-to-DS-to-DCD cutover, component and shared model Service ownership restoration, recreation of a missing DGD-owned shared model Service before DS fallback cleanup, checkpoint coordination, child-revision draining, metadata preservation, bounded child names, role limits, stale Service cleanup, annotation routing, Grove cleanup and precedence, multinode guard, and consecutive sequential restart regressions
  • real pinned LWS v0.9.0 CRD validation/defaulting test:
    • KUBEBUILDER_ASSETS="$PWD/bin/k8s/1.30.0-darwin-arm64" go test ./internal/controller -run '^TestDisaggregatedSetRealLWSCRDValidationAndConvergence$' -count=1 -v
    • initial creation passes the real schema and an identical second reconciliation returns modified=false
  • focused unit regressions cover explicit LWS defaults, DGD-level ServiceAccount rendering, DS/LWS metadata predicates, exact parent replica convergence, and safe DGD-to-DCD owner transfer including refusal to steal foreign-owned resources
  • go vet ./internal/controller ./internal/controller_common/... ./cmd/...
  • GOTOOLCHAIN=go1.26.3 go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.62.2 run --timeout=5m
  • uvx pre-commit run --files <changed files>
  • current controller package coverage: 71.4%; the new convergence and ownership helpers are exercised by API-server-backed tests
  • make manifests
  • make generate-helm-docs
  • CI-equivalent container linter:
    • docker buildx build --platform linux/amd64 --target linter --build-context snapshot=../snapshot --build-context operator=../operator .
  • CI-equivalent full operator test target:
    • docker buildx build --platform linux/amd64 --target tester --build-context snapshot=../snapshot --build-context operator=../operator .
  • Live Kubernetes validation on OrbStack with LWS v0.9.0:
    • a generated two-role DS created two child LWS objects and four Ready pods
    • every pod used the existing DGD-level Kubernetes discovery ServiceAccount
    • coordinated rollout moved both roles from revision d65d7965 to b47ae5a3, then removed the old revision
    • confirmed v0.9.0 leaves the parent DS status empty, exercising the child-LWS readiness/watch compatibility requirement
  • on head 7854e10ff5, focused DisaggregatedSet regressions and the controller DisaggregatedSet test set pass; lightweight CI is rerunning after the latest lint-only commit
  • stacked Draft test(operator): add DisaggregatedSet live e2e #12352 runs the real live-cluster suite with LWS v0.9.0 and Volcano v1.14.0; two immediate consecutive runs passed 7/7 (48.1s and 49.6s), covering DS/LWS readiness, metadata watch, restart completion, DS-to-DCD fallback, deleted model-Service recreation, DCD readiness, and Service ownership handoff

Follow-up testing

Draft #12352 implements the dependent phase-2 live-cluster E2E suite and remains intentionally outside this feature PR. It adds pinned dependency preparation plus make test-e2e-disaggregatedset for ordinary kubeconfig-backed clusters; it must not merge before this PR.

Review focus

  • deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go
  • deploy/operator/internal/controller/dynamographdeployment_controller.go
  • deploy/operator/internal/controller/dynamocomponentdeployment_controller.go

Related issues

Relates to #10205

Addresses the correctness findings in yankay#4

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@copy-pr-bot

copy-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 05:23 — with GitHub Actions Inactive
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 05:23 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor feat deployment::k8s Relates to dynamo deployment in kubernetes labels Jul 14, 2026
@datadog-official

This comment has been minimized.

Move the leader/worker pod template rendering out of
generateLeaderWorkerSet so the DisaggregatedSet pathway can reuse it
without instantiating a separate DynamoComponentDeploymentReconciler.
No behavior change for the existing LWS pathway.

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 07:55 — with GitHub Actions Inactive
Reduce controller complexity in the DGD reconcile path, remove an unused
DisaggregatedSet helper, add the missing SPDX header for the envtest CRD
fixture, and replace the stale SGLang diffusion doc links that were failing
lychee.

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 08:32 — with GitHub Actions Inactive
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 14, 2026
Run the DisaggregatedSet envtest against the existing controller suite environment so the CRD conversion webhook is available.

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 08:55 — with GitHub Actions Inactive
@panpan0000
panpan0000 marked this pull request as ready for review July 14, 2026 09:01
@panpan0000
panpan0000 requested review from a team as code owners July 14, 2026 09:01
devin-ai-integration[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The operator now discovers and authorizes the DisaggregatedSet API, selects eligible multinode components, reconciles DisaggregatedSet resources and readiness, routes DGD reconciliation accordingly, adds envtest coverage, and updates SGLang diffusion documentation links.

Changes

DisaggregatedSet operator integration

Layer / File(s) Summary
Capability discovery and permissions
deploy/operator/internal/..., deploy/operator/cmd/main.go, deploy/operator/config/rbac/role.yaml, deploy/helm/.../manager-role.yaml
Runtime discovery, configuration, startup logging, annotation constants, and RBAC permissions are added for DisaggregatedSet.
Selection and resource generation
deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go, deploy/operator/internal/controller/dynamocomponentdeployment_controller.go
Eligible multinode components are mapped to roles, replica constraints are enforced, and DisaggregatedSet roles reuse rendered leader/worker pod templates.
Resource reconciliation and readiness
deploy/operator/internal/controller/dynamographdeployment_disaggregatedset.go, deploy/operator/internal/controller/*disaggregatedset*test.go, deploy/operator/internal/controller/testing/disaggregatedset/*, deploy/operator/internal/controller/suite_test.go
DisaggregatedSet objects, services, ownership, selected DCD cleanup, readiness, restart progress, and envtest coverage are implemented.
Controller routing and watches
deploy/operator/internal/controller/dynamographdeployment_controller.go
DGD reconciliation routes between Grove, DisaggregatedSet, and legacy DCD paths, with conditional DisaggregatedSet watches and standardized not-found handling.

Documentation link updates

Layer / File(s) Summary
SGLang diffusion references
docs/backends/sglang/sglang-diffusion.md
Two diffusion documentation links now target the consolidated SGLang documentation site.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding a DisaggregatedSet pathway to the operator.
Description check ✅ Passed The description covers overview, validation, review focus, and issue linkage, though it does not mirror the template headings exactly.

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

Document how DisaggregatedSet works with the LWS multinode path, including installation, selection, and fallback behavior.

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 10:16 — with GitHub Actions Inactive
@panpan0000
panpan0000 marked this pull request as draft July 14, 2026 12:45
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 14, 2026 12:57 — with GitHub Actions Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>

Copy link
Copy Markdown
Contributor Author

Phase-2 live-cluster E2E is now open as Draft #12352 (stacked on this PR).

Running it against OrbStack with real LWS v0.9.0 + Volcano v1.14.0 found two controller issues, both fixed here in focused commits:

  • cf043609ce: complete a coalesced DisaggregatedSet sequential restart instead of leaving the second role permanently Restarting
  • 1a33ed81f1: accept a shared model Service already controlled by a sibling replacement DCD owned by the same DGD

The full lifecycle suite now passes twice consecutively, 7/7 each run: real DS/LWS creation and readiness, metadata propagation, restart, annotation removal, DS→DCD fallback, deleted model-Service recreation, replacement readiness, and Service ownership handoff.

@panpan0000
panpan0000 temporarily deployed to external_collaborator July 29, 2026 10:31 — with GitHub Actions Inactive
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 29, 2026 10:31 — with GitHub Actions Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 force-pushed the feat/disaggregatedset-draft-pathway branch from 360f225 to 7854e10 Compare July 29, 2026 10:31
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 29, 2026 10:31 — with GitHub Actions Inactive
@athreesh

Copy link
Copy Markdown
Contributor

/ok to test 7854e10

@athreesh athreesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs lgtm

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator July 31, 2026 03:09 — with GitHub Actions Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator August 2, 2026 04:47 — with GitHub Actions Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator August 3, 2026 01:06 — with GitHub Actions Inactive
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator August 3, 2026 01:31 — with GitHub Actions Inactive
@athreesh

athreesh commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

/ok to test 84e63e5

Keep the new DisaggregatedSet envtest suite out of the clustertest build so
operator integration does not fail on the envtest-only k8sClient setup.

Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
@panpan0000
panpan0000 temporarily deployed to external_collaborator August 3, 2026 09:02 — with GitHub Actions Inactive
@panpan0000

Copy link
Copy Markdown
Contributor Author

Pushed a fix for the Operator Integration compile failure on the latest head (8763efe).

Pre-merge operator checks are green now.

Could you please re-run full CI with /ok to test 8763efe7de?

@panpan0000

Copy link
Copy Markdown
Contributor Author

Adding context for reviewers: follow-up live-cluster e2e coverage is in #12352, which depends on this PR and stays unmerged until this one lands. I also reran the DisaggregatedSet live tests on a real Kubernetes cluster after the latest fixes; both consecutive runs passed 7/7 (54.8s and 49.9s).

@julienmancuso

Copy link
Copy Markdown
Contributor

Now that #12283 has merged (associated with #12035), the DGD controller has a composition-first architecture. I think this PR should be reworked on top of current main rather than resolving the old monolithic-controller conflicts mechanically.

The key design point is that DisaggregatedSet should become a third complete graph-level workload program:

  • Grove program
  • DisaggregatedSet program
  • Component/DCD program

DisaggregatedSet is a graph-level program because one DisaggregatedSet can coordinate multiple DGD components while unselected components remain ordinary DCDs. LWS itself is not another DGD program: it remains a workload mechanism used by DCD and by the DisaggregatedSet implementation.

The outer DGD reconciler should only select the program, invoke it, and persist its returned status/events. The DisaggregatedSet program should own its sequencing and compose concrete collaborators for:

  • selecting and validating participating components and role names;
  • rendering roles using the existing read-only dcdWorkloadRenderer;
  • synchronizing the DisaggregatedSet resource;
  • reconciling unselected components as DCDs;
  • reconciling Services and ownership handoff;
  • resolving readiness across the DisaggregatedSet, target/draining LWS revisions, and unselected DCDs;
  • grouping selected roles as one provider-specific restart unit;
  • performing DisaggregatedSet-specific compatibility cleanup;
  • returning complete authoritative DGD status, including Ready.

In particular, the DisaggregatedSet implementation should not construct or call a complete DCD reconciler. The renderer extraction previously included in this PR is also no longer needed because #12283 introduced the narrower dcdWorkloadRenderer, including multinode pod-template rendering.

The existing large DisaggregatedSet controller file should ideally be decomposed by responsibility rather than moving all its methods unchanged onto another large type. Nested reconcilers should have narrow dependencies and must not write DGD status directly. Status changes and queued events should be returned through the workload-program result and persisted once by the outer controller.

Restart coalescing for selected roles should remain DisaggregatedSet-specific. It should not alter the common restart flow to introduce generic provider lifecycle behavior. Similarly, DCD/Grove/DisaggregatedSet transitions and Service ownership handoff should be implemented as narrow provider-specific compatibility cleanup, not as a universal zero-downtime cutover abstraction.

I suggest rebuilding the implementation from current main, using the existing PR as the behavioral specification. The current tests should be retained and reorganized around the new owners, especially coverage for mixed DS/DCD graphs, checkpoint gating, restart grouping, target/draining LWS readiness, Service adoption, metadata preservation, and DCD → DS → DCD transitions.

This should preserve the behavior and feature scope already implemented here while making DisaggregatedSet coherent with the architecture now present on main.

@panpan0000

Copy link
Copy Markdown
Contributor Author

rework moved to #12625

@tmonty12

Copy link
Copy Markdown
Contributor

Closing, implementation of DisaggSet support refactored in this PR: #12625

@tmonty12 tmonty12 closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment::k8s Relates to dynamo deployment in kubernetes documentation Improvements or additions to documentation external-contribution Pull request is from an external contributor feat size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants