Skip to content

fix(operator): allow component-scoped topology - #12448

Merged
julienmancuso merged 1 commit into
ai-dynamo:mainfrom
alexeldeib:ace/allow-component-scoped-topology
Jul 31, 2026
Merged

fix(operator): allow component-scoped topology#12448
julienmancuso merged 1 commit into
ai-dynamo:mainfrom
alexeldeib:ace/allow-component-scoped-topology

Conversation

@alexeldeib

@alexeldeib alexeldeib commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Overview

Align DGD webhook validation with the existing component-scoped topology design. This allows GPU workers to be rack-packed while CPU frontends and auxiliary components remain topology-free.

Details

  • Allow a deployment topology without packDomain when at least one component defines its own topology constraint.
  • Continue rejecting configurations with neither a deployment-level packDomain nor any component-scoped constraint.
  • Add positive and negative admission coverage while preserving independent topology error aggregation.

This matches the documented service-level-only topology behavior and the existing Grove render path.

Where should the reviewer start?

  • deploy/operator/internal/webhook/validation/dynamographdeployment.go
  • deploy/operator/internal/webhook/validation/dynamographdeployment_validation_envtest_test.go

Validation

  • make envtest ENVTEST_PACKAGES=./internal/webhook/validation
  • go vet ./internal/webhook/validation

Related Issues

🔗 This PR is linked to an issue:

Related design: ai-dynamo/enhancements#69


Open in Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation for deployment and component topology constraints.
    • Component-level constraints with a pack domain now allow other components to remain unconstrained.
    • Added clearer required-field validation when no deployment- or component-level topology constraint is configured.
    • Updated validation error reporting for more accurate field paths and aggregated errors.

@alexeldeib
alexeldeib requested a review from a team as a code owner July 30, 2026 22:30
@copy-pr-bot

copy-pr-bot Bot commented Jul 30, 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.

@alexeldeib
alexeldeib temporarily deployed to external_collaborator July 30, 2026 22:30 — with GitHub Actions Inactive
@alexeldeib
alexeldeib temporarily deployed to external_collaborator July 30, 2026 22:30 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi alexeldeib! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor fix deployment::k8s Relates to dynamo deployment in kubernetes labels Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6c72c97a-93e5-49cf-b896-613820707d5b

📥 Commits

Reviewing files that changed from the base of the PR and between 6cc7c4e and dc83c5a.

📒 Files selected for processing (2)
  • deploy/operator/internal/webhook/validation/dynamographdeployment.go
  • deploy/operator/internal/webhook/validation/dynamographdeployment_validation_envtest_test.go

Walkthrough

The DynamoGraphDeployment validator now recognizes topology constraints at both deployment and component scope. Missing pack-domain validation and environment-test expectations were updated accordingly.

Changes

Topology validation

Layer / File(s) Summary
Constraint detection and required-field validation
deploy/operator/internal/webhook/validation/dynamographdeployment.go
Topology validation considers deployment-level or component-level constraints, and only requires deployment packDomain when no component constraint exists.
Environment validation coverage
deploy/operator/internal/webhook/validation/dynamographdeployment_validation_envtest_test.go
Tests cover component-provided pack domains, the no-constraint required-field error, and updated aggregated topology errors.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: enabling component-scoped topology.
Description check ✅ Passed The description includes overview, details, reviewer start points, validation, and a linked issue, matching the template well.
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.

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 help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 2 potential issues.

Open in Devin Review

@alexeldeib
alexeldeib force-pushed the ace/allow-component-scoped-topology branch from dc83c5a to 8c787c2 Compare July 30, 2026 22:41
@alexeldeib
alexeldeib temporarily deployed to external_collaborator July 30, 2026 22:41 — with GitHub Actions Inactive
@alexeldeib
alexeldeib force-pushed the ace/allow-component-scoped-topology branch from 8c787c2 to 25950c0 Compare July 30, 2026 23:00
@alexeldeib
alexeldeib temporarily deployed to external_collaborator July 30, 2026 23:00 — with GitHub Actions Inactive
@julienmancuso

Copy link
Copy Markdown
Contributor

/ok to test 25950c0

Signed-off-by: Ace Eldeib <aeldeib@coreweave.com>
@alexeldeib
alexeldeib force-pushed the ace/allow-component-scoped-topology branch from 25950c0 to 8ef5c1f Compare July 31, 2026 00:01
@alexeldeib
alexeldeib temporarily deployed to external_collaborator July 31, 2026 00:01 — with GitHub Actions Inactive
@athreesh

Copy link
Copy Markdown
Contributor

/ok to test 8ef5c1f

@julienmancuso
julienmancuso merged commit 26b83a9 into ai-dynamo:main Jul 31, 2026
92 checks passed
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 external-contribution Pull request is from an external contributor fix size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants