feat: inject DGD id in planner env variables #2460
Conversation
julienmancuso
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughAdds parent graph deployment context (name/namespace) to pod/container generation. Extends GetBaseContainer and GenerateBasePodSpec signatures, wires values from GeneratePodSpecForComponent and controller paths, and sets K8S_PARENT_* env vars in planner containers. Introduces helper methods on DynamoComponentDeployment and corresponding unit tests; updates tests for new signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Graph as GenerateBasePodSpec
participant Defaults as ComponentDefaults
participant Kube as Kubernetes
Controller->>Graph: GenerateBasePodSpec(component, secrets, parentName, namespace, role, ...)
Graph->>Defaults: GetBaseContainer(numberOfNodes, parentName, namespace)
Defaults-->>Graph: corev1.Container (env/resources/probes)
Graph-->>Controller: corev1.PodSpec
Controller->>Kube: Create/Update Pod/Deployment
sequenceDiagram
participant Reconciler
participant DC as DynamoComponentDeployment
participant Graph as GenerateBasePodSpecForController
participant Planner as PlannerDefaults
Reconciler->>DC: GetParentGraphDeploymentName/Namespace()
Reconciler->>Graph: Call with parentName/namespace
Graph->>Planner: GetBaseContainer(..., parentName, namespace)
Planner-->>Graph: Container with K8S_PARENT_NAME/namespace env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
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.2.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/product/migration-guide for migration instructions Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types_test.go (1)
214-263: Add edge cases: wrong kind and multiple owner referencesTwo quick additions would harden the behavior:
- When OwnerReferences contain non-matching kinds, ensure we return "".
- When multiple OwnerReferences exist, ensure we select the intended one (your implementation returns the first matching kind).
Apply this diff to extend the table tests:
@@ tests := []struct { name string fields fields want string }{ { - name: "test", + name: "matching owner kind", fields: fields{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ { Kind: "DynamoGraphDeployment", Name: "name", }, }, }, }, want: "name", }, + { + name: "wrong owner kind", + fields: fields{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "SomethingElse", + Name: "name", + }, + }, + }, + }, + want: "", + }, + { + name: "multiple owner references (second is correct kind)", + fields: fields{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + {Kind: "SomethingElse", Name: "other"}, + {Kind: "DynamoGraphDeployment", Name: "graph-name"}, + }, + }, + }, + want: "graph-name", + }, { name: "no owner reference", fields: fields{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{}, }, }, want: "", }, }Additionally, consider adding a dedicated test for GetParentGraphDeploymentNamespace():
func TestDynamoComponentDeployment_GetParentGraphDeploymentNamespace(t *testing.T) { s := &DynamoComponentDeployment{ ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", }, } if got := s.GetParentGraphDeploymentNamespace(); got != "ns" { t.Fatalf("GetParentGraphDeploymentNamespace() = %q, want %q", got, "ns") } }deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (1)
264-271: Prefer controller owner when resolving parent graph; add doc commentIf multiple owners exist, Kubernetes convention is to mark the controller owner with OwnerReference.Controller=true. Prefer that when present, then fall back to the first matching kind. Also, a brief doc comment helps consumers.
Apply this diff:
-func (s *DynamoComponentDeployment) GetParentGraphDeploymentName() string { - for _, ownerRef := range s.ObjectMeta.OwnerReferences { - if ownerRef.Kind == "DynamoGraphDeployment" { - return ownerRef.Name - } - } - return "" -} +// GetParentGraphDeploymentName returns the name of the owning DynamoGraphDeployment, if any. +// It prefers the controller owner when present, and falls back to the first owner of that kind. +func (s *DynamoComponentDeployment) GetParentGraphDeploymentName() string { + // Prefer the controller owner + for _, ownerRef := range s.ObjectMeta.OwnerReferences { + if ownerRef.Kind == "DynamoGraphDeployment" && ownerRef.Controller != nil && *ownerRef.Controller { + return ownerRef.Name + } + } + // Fallback: first owner of the expected kind + for _, ownerRef := range s.ObjectMeta.OwnerReferences { + if ownerRef.Kind == "DynamoGraphDeployment" { + return ownerRef.Name + } + } + return "" +}Nit: consider moving "DynamoGraphDeployment" to a shared constant to avoid typos and ease future refactors.
deploy/cloud/operator/internal/dynamo/component_worker.go (1)
26-26: Optional: clarify unused parametersWorker doesn’t use the parent identifiers today. A short comment makes this explicit and avoids future confusion.
Apply this diff:
-func (w *WorkerDefaults) GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error) { +// parentGraphDeploymentName and parentGraphDeploymentNamespace are accepted to satisfy the ComponentDefaults interface +// but are not used by Worker today. +func (w *WorkerDefaults) GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error) {deploy/cloud/operator/internal/dynamo/component_frontend.go (1)
26-26: Optional: clarify unused parametersFrontend doesn’t use the parent identifiers. A brief comment helps future readers.
Apply this diff:
-func (f *FrontendDefaults) GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error) { +// parentGraphDeploymentName and parentGraphDeploymentNamespace are accepted to satisfy the ComponentDefaults interface +// but are not used by Frontend today. +func (f *FrontendDefaults) GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error) {deploy/cloud/operator/internal/dynamo/component_planner.go (1)
38-47: Append env vars instead of overwriting; consider centralizing keys as constantsOverwriting container.Env can discard defaults from getCommonContainer (if any). Appending is safer and future-proof. Also, consider promoting the env keys to shared constants to avoid typos.
Apply this diff to append instead of overwrite:
- container.Env = []corev1.EnvVar{ - { - Name: "K8S_PARENT_NAME", - Value: parentGraphDeploymentName, - }, - { - Name: "K8S_PARENT_NAMESPACE", - Value: parentGraphDeploymentNamespace, - }, - } + container.Env = append(container.Env, + corev1.EnvVar{ + Name: "K8S_PARENT_NAME", + Value: parentGraphDeploymentName, + }, + corev1.EnvVar{ + Name: "K8S_PARENT_NAMESPACE", + Value: parentGraphDeploymentNamespace, + }, + )Optional follow-ups:
- Define constants in internal/consts (e.g., EnvK8SParentName, EnvK8SParentNamespace) and use them here for consistency.
- If you need a stable identifier across recreate cycles, consider also injecting the parent UID (OwnerReference.UID) as an env (e.g., K8S_PARENT_UID).
I can open a small follow-up to introduce the constants and wire K8S_PARENT_UID if desired.
deploy/cloud/operator/internal/dynamo/component_planner_test.go (2)
75-78: Use standard cmp.Diff semantics and error message for clarityFlip cmp.Diff arguments and print the canonical (-want +got) form to make diffs easier to read during failures.
- diff := cmp.Diff(got, tt.want) - if diff != "" { - t.Errorf("PlannerDefaults.GetBaseContainer() = %v, want %v", diff, tt.want) - } + diff := cmp.Diff(tt.want, got) + if diff != "" { + t.Errorf("PlannerDefaults.GetBaseContainer() mismatch (-want +got):\n%s", diff) + }
32-42: Consider adding a negative test to ensure only Planner injects these env varsA small follow-up test that exercises BaseComponentDefaults (or Frontend/Worker defaults) and asserts K8S_PARENT_* are not set would prevent cross-component leakage in the future.
Happy to add such a test case if you want me to push it.
deploy/cloud/operator/internal/dynamo/component_common.go (1)
13-22: Document the new GetBaseContainer parameters for future maintainersThe interface comment mentions only numberOfNodes. Please add a brief note on the two new arguments to clarify their purpose (parent graph deployment name/namespace).
type ComponentDefaults interface { // GetBaseContainer returns the base container configuration for this component type - // The numberOfNodes parameter indicates the total number of nodes in the deployment - GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error) + // numberOfNodes: total number of nodes in the deployment + // parentGraphDeploymentName: name of the parent DynamoGraphDeployment (used e.g. for env injection) + // parentGraphDeploymentNamespace: namespace of the parent DynamoGraphDeployment + GetBaseContainer(numberOfNodes int32, parentGraphDeploymentName, parentGraphDeploymentNamespace string) (corev1.Container, error)deploy/cloud/operator/internal/dynamo/graph.go (3)
667-677: Name the parameter ‘parentGraphDeploymentNamespace’ for clarityThe new argument is used as the parent graph deployment namespace. Renaming the parameter improves readability and reduces confusion with service/component namespaces used elsewhere in this function.
func GenerateBasePodSpec( component *v1alpha1.DynamoComponentDeploymentOverridesSpec, backendFramework BackendFramework, secretsRetriever SecretsRetriever, - parentGraphDeploymentName string, - namespace string, + parentGraphDeploymentName string, + parentGraphDeploymentNamespace string, role Role, numberOfNodes int32, controllerConfig controller_common.Config, multinodeDeploymentType commonconsts.MultinodeDeploymentType, serviceName string, ) (corev1.PodSpec, error) {And update the GetBaseContainer invocation accordingly (see next comment).
680-683: Pass parent graph context through to component defaults (OK); align with param renameThis correctly threads the parent context to component defaults. If you accept the param rename above, adjust this line too.
- container, err := componentDefaults.GetBaseContainer(numberOfNodes, parentGraphDeploymentName, namespace) + container, err := componentDefaults.GetBaseContainer(numberOfNodes, parentGraphDeploymentName, parentGraphDeploymentNamespace)Additionally, update the secrets retrieval below to use the renamed parameter (outside this hunk):
Go (outside this selected range):
- secretsRetriever.GetSecrets(parentGraphDeploymentNamespace, component.ExtraPodSpec.MainContainer.Image)
1126-1140: Use dynComponent.GetParentGraphDeploymentNamespace() for symmetry and future-proofingYou’re already using GetParentGraphDeploymentName(); use the corresponding GetParentGraphDeploymentNamespace() instead of dynComponent.Namespace for clarity and resilience, even if they are currently equal.
podSpec, err := GenerateBasePodSpec( componentSpec, backendFramework, secretsRetriever, - dynComponent.GetParentGraphDeploymentName(), - dynComponent.Namespace, + dynComponent.GetParentGraphDeploymentName(), + dynComponent.GetParentGraphDeploymentNamespace(), role, numberOfNodes, controllerConfig, multinodeDeploymentType, serviceName, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go(1 hunks)deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types_test.go(1 hunks)deploy/cloud/operator/internal/dynamo/component_common.go(2 hunks)deploy/cloud/operator/internal/dynamo/component_frontend.go(1 hunks)deploy/cloud/operator/internal/dynamo/component_planner.go(2 hunks)deploy/cloud/operator/internal/dynamo/component_planner_test.go(1 hunks)deploy/cloud/operator/internal/dynamo/component_worker.go(1 hunks)deploy/cloud/operator/internal/dynamo/graph.go(4 hunks)deploy/cloud/operator/internal/dynamo/graph_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1308-1312
Timestamp: 2025-06-11T21:29:28.650Z
Learning: User julienmancuso expects replies in English; avoid switching languages unless explicitly requested.
🧬 Code Graph Analysis (2)
deploy/cloud/operator/internal/dynamo/component_planner_test.go (3)
deploy/cloud/operator/internal/dynamo/component_common.go (1)
BaseComponentDefaults(39-39)deploy/cloud/operator/internal/dynamo/graph.go (1)
Resources(81-86)deploy/cloud/operator/internal/dynamo/component_planner.go (1)
PlannerDefaults(15-17)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types_test.go (1)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (3)
DynamoComponentDeploymentSpec(40-54)DynamoComponentDeploymentStatus(158-168)DynamoComponentDeployment(179-187)
🔇 Additional comments (10)
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types_test.go (1)
214-263: Good coverage for owner reference name lookupThe test correctly exercises the happy path and the no-owner case for GetParentGraphDeploymentName().
deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go (1)
273-275: LGTM: namespace passthroughReturning the resource's namespace is correct for owners, as controller and controlled objects must be in the same namespace for namespaced resources.
deploy/cloud/operator/internal/dynamo/component_worker.go (1)
26-26: Signature update acknowledgedExpanding GetBaseContainer to accept parentGraphDeploymentName/Namespace aligns with the interface changes and keeps Worker in sync with callers.
deploy/cloud/operator/internal/dynamo/component_frontend.go (1)
26-26: Signature update acknowledgedFrontend now accepts parentGraphDeploymentName/Namespace to match the updated interface. No functional change in the body.
deploy/cloud/operator/internal/dynamo/component_planner.go (1)
23-23: LGTM: signature change enables passing parent contextThis aligns with the PR goal to inject DGD identifiers into Planner containers and keeps the interface consistent across components.
deploy/cloud/operator/internal/dynamo/component_planner_test.go (1)
16-63: Good coverage for parent context env injection in Planner base containerAsserting K8S_PARENT_NAME and K8S_PARENT_NAMESPACE is exactly in line with the PR goal. The container shape (name/command/resources/env) looks correct for a base Planner container.
deploy/cloud/operator/internal/dynamo/graph_test.go (1)
4125-4136: Signature update LGTMPassing deploymentName ("test-deployment") to GenerateBasePodSpec aligns with the new signature and wiring.
deploy/cloud/operator/internal/dynamo/component_common.go (1)
41-43: Base implementation correctly ignores the new contextReturning the common container and nil is fine for the base case; specialized defaults can consume the new parameters as needed.
deploy/cloud/operator/internal/dynamo/graph.go (2)
827-832: Wiring from GeneratePodSpecForComponent looks correctUsing dynamoDeployment.Name/Namespace as the parent context matches expectations for Grove path.
667-677: Signature & K8S_PARENT env injection — repo-wide sanity check OKQuick check results: repo search shows call sites updated for the signature changes and K8S_PARENT_* env vars are only injected by the Planner implementation.
Relevant locations:
- GenerateBasePodSpec definition: deploy/cloud/operator/internal/dynamo/graph.go:666 (function signature).
- Call sites: deploy/cloud/operator/internal/dynamo/graph.go:827 and deploy/cloud/operator/internal/dynamo/graph.go:1129; tests at deploy/cloud/operator/internal/dynamo/graph_test.go:4125.
- GetBaseContainer usage: deploy/cloud/operator/internal/dynamo/graph.go:680 (passes numberOfNodes, parentGraphDeploymentName, namespace); tests at deploy/cloud/operator/internal/dynamo/component_planner_test.go:70.
- K8S_PARENT_NAME / K8S_PARENT_NAMESPACE injection: deploy/cloud/operator/internal/dynamo/component_planner.go:40–45; validated in deploy/cloud/operator/internal/dynamo/component_planner_test.go:59–60.
No code changes required based on these checks.
biswapanda
left a comment
biswapanda
left a comment
There was a problem hiding this comment.
one minor naming suggestion
Signed-off-by: Hannah Zhang <[email protected]>
Overview:
inject DGD id in planner env variables
Summary by CodeRabbit
New Features
Tests