fix: ensure auto-generated components inherit graph dynamo namespace #3020
Conversation
GavinZhu-GMI
requested review from
atchernych,
biswapanda,
hhzhang16,
hutm,
ishandhanani,
julienmancuso,
mohammedabdulwahhab and
nnshah1
as code owners
|
👋 Hi GavinZhu-GMI! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
the
external-contribution
WalkthroughAdds graph-level DynamoNamespace resolution with conflict detection and defaulting, and forces all generated DynamoComponentDeployments to inherit that namespace. The resolver scans components, detects mismatches among explicit namespaces, derives a single graph namespace (explicit or default), and applies it during deployment generation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator
participant Graph as DynamoGraph
participant Resolver as getDynamoNamespace
participant Default as GetDefaultDynamoNamespace
participant DeployGen as GenerateDynamoComponentsDeployments
participant K8s as Kubernetes
Operator->>Resolver: Resolve graph DynamoNamespace (Graph)
Resolver->>Graph: Scan services/components
Note over Resolver: Collect components with/without explicit DynamoNamespace
alt Multiple explicit namespaces differ
Resolver-->>Operator: Error (namespace mismatch)
else Any explicit namespace present
Resolver-->>Operator: Resolved = first explicit namespace
else No explicit namespaces
Resolver->>Default: Request default namespace
Default-->>Resolver: Default namespace
Resolver-->>Operator: Resolved = default
end
Operator->>DeployGen: Generate component deployments (Resolved namespace)
loop For each component
DeployGen->>DeployGen: Set deployment.Spec.DynamoNamespace = Resolved
DeployGen->>K8s: Create/Apply component deployment
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
deploy/cloud/operator/internal/dynamo/graph.go (5)
133-136: Prefer ptr.To for string pointers; avoid taking address of local.Using &graphDynamoNamespace works but aliases the same pointer across all deployments and relies on escape analysis. Use ptr.To for clarity and idiomatic K8s code.
- deployment.Spec.DynamoNamespace = &graphDynamoNamespace + deployment.Spec.DynamoNamespace = ptr.To(graphDynamoNamespace)
194-205: Remove unused tracking slices.componentsWithNamespace and componentsWithoutNamespace are collected but never used; they add noise and risk future confusion.
- componentsWithNamespace := make([]string, 0) - componentsWithoutNamespace := make([]string, 0) - - // First pass: collect components with explicit namespace settings + // Iterate services and resolve the graph namespace from explicit settings for componentName, component := range parentDynamoGraphDeployment.Spec.Services { dynamoNamespace := "" if component.DynamoNamespace != nil && *component.DynamoNamespace != "" { dynamoNamespace = *component.DynamoNamespace - componentsWithNamespace = append(componentsWithNamespace, componentName) - } else { - componentsWithoutNamespace = append(componentsWithoutNamespace, componentName) }
197-216: Stabilize conflict detection by iterating over sorted service names.Map iteration order is random; sorting yields deterministic, testable error messages.
- // First pass: collect components with explicit namespace settings - for componentName, component := range parentDynamoGraphDeployment.Spec.Services { + // Iterate deterministically for stable behavior/messages + serviceNames := make([]string, 0, len(parentDynamoGraphDeployment.Spec.Services)) + for name := range parentDynamoGraphDeployment.Spec.Services { + serviceNames = append(serviceNames, name) + } + sort.Strings(serviceNames) + for _, componentName := range serviceNames { + component := parentDynamoGraphDeployment.Spec.Services[componentName] dynamoNamespace := "" if component.DynamoNamespace != nil && *component.DynamoNamespace != "" { dynamoNamespace = *component.DynamoNamespace }
930-937: Use ptr.To and confirm in-place mutation is intended.
- Use ptr.To for consistency with K8s style.
- This mutates the input CR (dynamoDeployment.Spec.Services[...]); verify this side-effect is desired.
- component.DynamoNamespace = &dynamoNamespace + component.DynamoNamespace = ptr.To(dynamoNamespace)If mutation is not desired, deep-copy the component before modification and pass the copy downstream.
1010-1031: Add KubeLabelDynamoNamespace to Grove path for parity with component deployments.Component CRs set the label; Grove cliques/pods don’t. Adding it keeps selectors and tooling consistent.
labels[commonconsts.KubeLabelDynamoGraphDeploymentName] = dynamoDeployment.Name if component.ComponentType != "" { labels[commonconsts.KubeLabelDynamoComponentType] = component.ComponentType } + if component.DynamoNamespace != nil && *component.DynamoNamespace != "" { + labels[commonconsts.KubeLabelDynamoNamespace] = *component.DynamoNamespace + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/cloud/operator/internal/dynamo/graph.go(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/3020/merge) by GavinZhu-GMI.
deploy/cloud/operator/internal/dynamo/graph.go
[error] 1-1: Command failed: pre-commit run --show-diff-on-failure --color=always --all-files. Trailing-whitespace check failed; the hook modified deploy/cloud/operator/internal/dynamo/graph.go. Re-run 'pre-commit run --all-files' and commit the changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
Signed-off-by: Gavin.Zhu <[email protected]>
Signed-off-by: Gavin.Zhu <[email protected]>
Signed-off-by: Gavin.Zhu <[email protected]>
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
1 participant
Overview:
When the planner automatically adds VllmDecodeWorker and VllmPrefillWorker
components via Kubernetes API patches, these components were not inheriting
the dynamoNamespace configuration from existing components in the same
DynamoGraphDeployment. This caused namespace mismatch errors like:
"namespace mismatch for component VllmDecodeWorker: graph uses namespace
dynamo but component specifies dynamo-xxx"
Details:
Modified the getDynamoNamespace() function in deploy/cloud/operator/internal/dynamo/graph.go to:
Where should the reviewer start?
Maybe start from getDynamoNamespace() func.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit