feat(operator): add experimental grove.forceScalingGroup for single-node components - #11772
Conversation
1ef49bd to
5060ac2
Compare
This comment has been minimized.
This comment has been minimized.
5060ac2 to
8acaa5a
Compare
WalkthroughThe PR adds ChangesGrove force scaling group
Estimated code review effort: 4 (Complex) | ~45 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.go (1)
626-668: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd graph-level conversion round-trip coverage.
This test only exercises
DynamoComponentDeployment, whileDynamoGraphDeploymentSpec.components.experimental.grove.forceScalingGroupis also a conversion contract. Add a graph round-trip case to protect that path independently.🤖 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/api/v1alpha1/dynamocomponentdeployment_conversion_test.go` around lines 626 - 668, Extend TestDCD_RoundTrip_ExperimentalGrove with an independent DynamoGraphDeploymentSpec round-trip case covering components.experimental.grove.forceScalingGroup. Construct a graph containing the grove-only experimental field, run it through the graph conversion round-trip helper, and assert the original and converted graph specs are equivalent.
🤖 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.
Nitpick comments:
In `@deploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.go`:
- Around line 626-668: Extend TestDCD_RoundTrip_ExperimentalGrove with an
independent DynamoGraphDeploymentSpec round-trip case covering
components.experimental.grove.forceScalingGroup. Construct a graph containing
the grove-only experimental field, run it through the graph conversion
round-trip helper, and assert the original and converted graph specs are
equivalent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0fad3eeb-6acc-4f01-a596-4b928b567926
📒 Files selected for processing (16)
deploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.godeploy/operator/api/v1alpha1/shared_spec_conversion.godeploy/operator/api/v1beta1/common.godeploy/operator/api/v1beta1/dynamocomponentdeployment_types.godeploy/operator/api/v1beta1/zz_generated.deepcopy.godeploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yamldeploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yamldeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/dynamo/graph.godeploy/operator/internal/dynamo/graph_test.godeploy/operator/internal/dynamo/grove.godeploy/operator/internal/webhook/validation/dynamographdeployment_helpers.godeploy/operator/internal/webhook/validation/dynamographdeployment_test.godeploy/operator/internal/webhook/validation/shared_helpers.godeploy/operator/internal/webhook/validation/shared_v1beta1.go
8acaa5a to
62654f0
Compare
…ode components Add components[].experimental.grove.forceScalingGroup (bool) to the v1beta1 shared component spec. False or omitted means automatic selection: multi-node and inter-pod GMS components render as a Grove PodCliqueScalingGroup, other single-node components pool their replicas as pods of one standalone PodClique — exactly today's behavior. Setting forceScalingGroup: true opts a single-node component into the PCSG layout: the scaling-group replica count carries the horizontal scale, with a single one-pod PodClique per replica, so each replica is an independently gang-scheduled unit (its own PodGang beyond minAvailable) and scaling stamps out or removes whole PodGangs. - Centralize the previously duplicated "multinode || inter-pod GMS" PCSG decision as DynamoComponentDeploymentSharedSpec.UsesPCSG() (= isMultinode || isInterPodGMS || forceScalingGroup) and use it in PCS rendering, Grove readiness aggregation, restart progress tracking, reconcileGroveScaling, and resource-name-length validation. - Validation: Grove pathway only (same rule as minAvailable); the effective opt-in is immutable after creation (same pattern as the inter-pod GMS layout toggle); explicit false is equivalent to omitted and freely add/removable. - Conversion: the grove block has no v1alpha1 representation; preserve it across spoke round-trips, sparsely when it sits alongside alpha-representable experimental fields. API shape per review discussion in ai-dynamo#11774. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: joeltg <joel@reflection.ai>
62654f0 to
40464ac
Compare
julienmancuso
left a comment
There was a problem hiding this comment.
Two P2 findings from the operator API and validation review.
|
@julienmancuso any other feedback here? would love to get this in, would give us a lot more confidence in our ability to integrate with our scheduler :) |
|
@joeltg reading thru PR, given offline context |
|
@joeltg , sorry for the delay, please rebase and i'll review / approve with high priority |
…eplica-grouping Signed-off-by: joeltg <joel@reflection.ai> # Conflicts: # deploy/operator/internal/controller/dynamographdeployment_controller.go # deploy/operator/internal/dynamo/grove.go
|
/ok to test e064cf5 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
deploy/operator/internal/webhook/validation/shared_helpers.go (1)
185-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd story comments to the new multi-line Go blocks.
deploy/operator/internal/webhook/validation/shared_helpers.go#L185-L190: Add a comment that explains the nil ExperimentalSpec behavior.deploy/operator/internal/webhook/validation/shared_v1beta1.go#L232-L238: Add a comment that explains Grove create validation.deploy/operator/internal/webhook/validation/shared_v1beta1.go#L315-L320: Add a comment that explains the Grove-pathway rejection.deploy/operator/internal/webhook/validation/shared_v1beta1.go#L432-L438: Add a comment that explains removal handling.deploy/operator/internal/webhook/validation/shared_v1beta1.go#L515-L528: Add a comment that explains Grove update validation.deploy/operator/internal/webhook/validation/shared_v1beta1.go#L541-L549: Add a comment that explains effective opt-in immutability.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/webhook/validation/shared_helpers.go` around lines 185 - 190, Add one-line story comments above each specified multi-line block: in deploy/operator/internal/webhook/validation/shared_helpers.go lines 185-190, explain the nil ExperimentalSpec behavior; in deploy/operator/internal/webhook/validation/shared_v1beta1.go lines 232-238, 315-320, 432-438, 515-528, and 541-549, respectively explain Grove create validation, Grove-pathway rejection, removal handling, Grove update validation, and effective opt-in immutability. Keep the comments concise and describe the block’s behavior without changing its logic.Source: Coding guidelines
deploy/operator/internal/dynamo/graph_test.go (1)
5085-5100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
t.Logheadings for the test steps.The new tests use comments but do not log their setup, render, and assertion phases.
deploy/operator/internal/dynamo/graph_test.go#L5085-L5100: Addt.Logheadings before component setup, role expansion, and result comparison.deploy/operator/internal/dynamo/graph_test.go#L8793-L8847: Addt.Logheadings before deployment setup, beta-only configuration, rendering, and PCSG assertions.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 5085 - 5100, Add t.Log headings for each test step in deploy/operator/internal/dynamo/graph_test.go:5085-5100, before component setup, role expansion, and result comparison in TestExpandRolesForComponent_SingleNodeForceScalingGroup; and in deploy/operator/internal/dynamo/graph_test.go:8793-8847, before deployment setup, beta-only configuration, rendering, and PCSG assertions. Use one heading per block without changing test behavior.Source: Coding guidelines
deploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.go (1)
626-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Logfor the test story.This new test uses a paragraph comment to describe its steps. The Go test guidelines require
t.Logheadings before test-step blocks. Move the story into the subtest before fixture construction and round-trip validation.Proposed change
-// The grove block has no v1alpha1 representation and must survive the spoke -// round-trip both alone (whole hub-only block preserved) and alongside -// alpha-representable fields (sparse preservation merged back). func TestDCD_RoundTrip_ExperimentalGrove(t *testing.T) { ... t.Run(tt.name, func(t *testing.T) { + t.Log("Round-trip the Grove configuration through v1alpha1 conversion") src := &v1beta1.DynamoComponentDeployment{🤖 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/api/v1alpha1/dynamocomponentdeployment_conversion_test.go` around lines 626 - 628, Update the affected conversion test subtest to replace the paragraph comment describing the grove round-trip scenarios with a t.Log heading before fixture construction and round-trip validation, preserving the existing test behavior and story content.Source: Coding guidelines
deploy/operator/api/v1alpha1/conversion_field_coverage_test.go (1)
121-121: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd graph-deployment round-trip coverage.
The field set now acknowledges
DynamoGraphDeploymentSpec.components.experimental.grove.forceScalingGroup, but the supplied round-trip test covers onlyDynamoComponentDeployment. Add a graph-deployment round-trip case, or verify an existing test exercises this conversion path. Otherwise, a graph conversion regression could drop the preserved field while this acknowledgement test still passes.🤖 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/api/v1alpha1/conversion_field_coverage_test.go` at line 121, Add round-trip coverage for DynamoGraphDeploymentSpec conversion, specifically exercising components.experimental.grove.forceScalingGroup through both conversion directions and verifying it is preserved. Update the existing round-trip test suite in conversion_field_coverage_test.go or add a dedicated graph-deployment case alongside the DynamoComponentDeployment coverage.
🤖 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.
Nitpick comments:
In `@deploy/operator/api/v1alpha1/conversion_field_coverage_test.go`:
- Line 121: Add round-trip coverage for DynamoGraphDeploymentSpec conversion,
specifically exercising components.experimental.grove.forceScalingGroup through
both conversion directions and verifying it is preserved. Update the existing
round-trip test suite in conversion_field_coverage_test.go or add a dedicated
graph-deployment case alongside the DynamoComponentDeployment coverage.
In `@deploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.go`:
- Around line 626-628: Update the affected conversion test subtest to replace
the paragraph comment describing the grove round-trip scenarios with a t.Log
heading before fixture construction and round-trip validation, preserving the
existing test behavior and story content.
In `@deploy/operator/internal/dynamo/graph_test.go`:
- Around line 5085-5100: Add t.Log headings for each test step in
deploy/operator/internal/dynamo/graph_test.go:5085-5100, before component setup,
role expansion, and result comparison in
TestExpandRolesForComponent_SingleNodeForceScalingGroup; and in
deploy/operator/internal/dynamo/graph_test.go:8793-8847, before deployment
setup, beta-only configuration, rendering, and PCSG assertions. Use one heading
per block without changing test behavior.
In `@deploy/operator/internal/webhook/validation/shared_helpers.go`:
- Around line 185-190: Add one-line story comments above each specified
multi-line block: in
deploy/operator/internal/webhook/validation/shared_helpers.go lines 185-190,
explain the nil ExperimentalSpec behavior; in
deploy/operator/internal/webhook/validation/shared_v1beta1.go lines 232-238,
315-320, 432-438, 515-528, and 541-549, respectively explain Grove create
validation, Grove-pathway rejection, removal handling, Grove update validation,
and effective opt-in immutability. Keep the comments concise and describe the
block’s behavior without changing its logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a828284e-ca37-4156-b082-f4da91796c1e
📒 Files selected for processing (19)
deploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.godeploy/operator/api/v1alpha1/shared_spec_conversion.godeploy/operator/api/v1beta1/common.godeploy/operator/api/v1beta1/dynamocomponentdeployment_types.godeploy/operator/api/v1beta1/zz_generated.deepcopy.godeploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yamldeploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yamldeploy/operator/internal/controller/dgd_grove_restart_progress_resolver.godeploy/operator/internal/controller/dgd_grove_scaler.godeploy/operator/internal/dynamo/graph.godeploy/operator/internal/dynamo/graph_test.godeploy/operator/internal/dynamo/grove.godeploy/operator/internal/webhook/validation/dynamographdeployment_helpers.godeploy/operator/internal/webhook/validation/dynamographdeployment_validation_envtest_test.godeploy/operator/internal/webhook/validation/shared_helpers.godeploy/operator/internal/webhook/validation/shared_test.godeploy/operator/internal/webhook/validation/shared_v1beta1.godocs/fern/pages/reference/kubernetes-api/additional-resources/api-reference-k8s.md
julienmancuso
left a comment
There was a problem hiding this comment.
Two additional P2 findings from the Grove scheduling/API contract review.
…eplica-grouping Signed-off-by: joeltg <joel@reflection.ai> # Conflicts: # deploy/operator/internal/webhook/validation/shared_test.go # deploy/operator/internal/webhook/validation/shared_v1beta1.go
…apping accurately Address review feedback on the published API contract: - minAvailable: describe the Grove mapping in terms of scaling-group membership (multi-node, inter-pod GMS, or grove.forceScalingGroup; see UsesPCSG) instead of the stale single-node-vs-multi-node split. - forceScalingGroup: state the factual PodGang mapping — the first minAvailable replicas join the deployment's base PodGang together with its other base workloads — instead of promising fully independent replicas, which Grove's base-gang semantics cannot provide. Regenerate CRDs and the API reference from the updated comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: joeltg <joel@reflection.ai>
|
/ok to test a46cbda |
|
^ CI just needs a rerun for the failed workflow I think! thanks so much @julienmancuso |
|
/ok to test e6d6b52 |
1 similar comment
|
/ok to test e6d6b52 |
|
/ok to test 69faafd |
1 similar comment
|
/ok to test 69faafd |
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (65 commits) fix(frontend): emit SGLang stream role once (#12741) docs(fern): promote v1.3.1 to current release (#12752) fix(docs): remove duplicate unscoped community-rail CSS rules (#12615) feat(operator): migrate CRD storage to v1beta1 (#11904) fix: synchronize self-benchmark capacity across DP ranks (#12021) chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707) fix(frontend): preserve split UTF-8 characters (#12688) docs: align Kubernetes build selector with CLI (#12729) fix(frontend): preserve completion backend error status (#12706) fix(operator): replace snapshot pods after GMS restart (#11286) refactor(media): rename installer module, drop --packages per review fix(media): harden installer against three pre-redesign review findings fix(media): verify installs in a fresh interpreter; teach --pip-args= form test(serve): install test-time decoders at the validated bounds feat(media): explicit installer for additional media decoders docs(spica): correct kv_load_ratio support guidance (#12714) feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772) fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709) chore: bump trtllm to v1.3.0rc23 (#12532) perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
Adds an opt-in, per-component
experimental.grove.forceScalingGroupboolean (v1beta1) that lets a single-node component render as a GrovePodCliqueScalingGroupinstead of a standalonePodClique, so each replica becomes an independently gang-scheduled unit and horizontal scaling changes the scaling-group replica count.API shape follows the maintainer proposal in #11774:
falseor omitted means "use automatic selection" (multi-node and inter-pod GMS components use a PCSG, ordinary single-node components use a PodClique), not "force PodClique".Details:
Today the PCSG decision is hardcoded: only multi-node and inter-pod GMS components render as a PCSG; single-node components pool all replicas as pods of one standalone PodClique. With this change:
renders one
PodCliqueScalingGroupConfig(replicas: 4,minAvailable: 1) containing a single one-pod clique. The scheduler sees one base PodGang (PCSG replicas[0, minAvailable)plus standalone cliques) and an independent scaled PodGang per remaining replica, each with a single one-pod PodGroup. Scaling the component patches the PCSG replicas, adding/removing whole PodGangs.experimental.grove.forceScalingGroupfield;UsesPCSG()(=isMultinode || isInterPodGMS || forceScalingGroup) centralizes the previously duplicated PCSG decision (PCS rendering, Grove readiness aggregation, restart progress tracking,reconcileGroveScaling, resource-name-length validation).RoleMainclique withreplicas: 1; the existing PCSG config path carriescomponent.replicas/minAvailableunchanged.forceScalingGroup: trueon a multi-node component is a redundant no-op (automatic selection already yields a PCSG).minAvailable); the effective opt-in is immutable after creation (same pattern as the inter-pod GMS layout toggle since it re-shapes rendered resource names); explicitfalseis equivalent to omitted and can be added/removed freely.groveblock has no v1alpha1 representation; it is preserved across spoke round-trips, sparsely when set alongside alpha-representable experimental fields.Defaults are fully backward compatible: omitting the field keeps today's rendering exactly.
Where should the reviewer start?
deploy/operator/api/v1beta1/common.go— the field and its semanticsdeploy/operator/internal/dynamo/graph.go—expandSingleNodeScalingGroupRoles+UsesPCSG()call sitesdeploy/operator/internal/webhook/validation/shared_v1beta1.go— create/update rulesdeploy/operator/api/v1alpha1/shared_spec_conversion.go— hub-only preservationRelated Issues
🔗 This PR is linked to an issue:
🤖 Generated with Claude Code
Summary by CodeRabbit
forceScalingGroupoption for rendering single-node components with scaling-group support.