feat: update planner to use DYN_PARENT_DGD_K8S_NAME #2774
Conversation
Signed-off-by: Julien Mancuso <[email protected]>
julienmancuso
requested review from
hhzhang16,
jasonqinzhou and
tedzhouhk
as code owners
WalkthroughShifts KubernetesAPI to resolve the parent DynamoGraphDeployment via the DYN_PARENT_DGD_K8S_NAME environment variable. Updates KubernetesConnector to use the new API, adds set_component_replicas with readiness checks, and revises tests accordingly. Removes component-arg-based retrieval, simplifies error messages, and introduces delegation via get_parent_graph_deployment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant Planner as KubernetesAPI
participant K8s as Kubernetes API
Note over Planner: Retrieve parent graph deployment (new)
Env->>Planner: DYN_PARENT_DGD_K8S_NAME
alt Env var set
Planner->>K8s: _get_graph_deployment_from_name(name)
alt Found
K8s-->>Planner: Deployment dict
Planner-->>Caller: Deployment dict
else 404 Not Found
K8s-->>Planner: ApiException(404)
Planner-->>Caller: None
else Other error
K8s--xPlanner: ApiException(other)
Planner--xCaller: Propagate error
end
else Env var missing
Planner-->>Caller: None
end
sequenceDiagram
autonumber
participant Conn as KubernetesConnector
participant API as KubernetesAPI
participant K8s as Kubernetes API
Note over Conn: set_component_replicas(target_replicas, blocking)
Conn->>API: get_graph_deployment()
alt Deployment found
API-->>Conn: {name, ...}
Conn->>API: is_deployment_ready(name)
alt Not ready
API-->>Conn: False
Note over Conn: Log warning and return early
Conn-->>Caller: Return
else Ready
API-->>Conn: True
loop For each component in target_replicas
Conn->>API: update_graph_replicas(name, component, replicas)
API-->>Conn: ack
end
alt blocking=True
Conn->>API: wait_for_graph_deployment_ready(name)
API-->>Conn: ack
end
Conn-->>Caller: Return
end
else Not found
API-->>Conn: None
Conn--xCaller: ValueError("Parent DynamoGraphDeployment not found")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/planner/src/dynamo/planner/kube.py (2)
102-109: Handle 404s: align readiness check with API semanticsCustomObjectsApi raises ApiException on 404; current
if not graph_deploymentbranch won’t execute in real runs. Catch 404 and raise ValueError as intended.- graph_deployment = self._get_graph_deployment_from_name(graph_deployment_name) - - if not graph_deployment: - raise ValueError(f"Graph deployment {graph_deployment_name} not found") + try: + graph_deployment = self._get_graph_deployment_from_name(graph_deployment_name) + except client.ApiException as e: + if e.status == 404: + raise ValueError(f"Graph deployment {graph_deployment_name} not found") + raise
128-134: Handle 404s inside the wait loopSame issue as above: catch ApiException and translate 404 to ValueError.
- graph_deployment = self._get_graph_deployment_from_name( - graph_deployment_name - ) - - if not graph_deployment: - raise ValueError(f"Graph deployment {graph_deployment_name} not found") + try: + graph_deployment = self._get_graph_deployment_from_name( + graph_deployment_name + ) + except client.ApiException as e: + if e.status == 404: + raise ValueError(f"Graph deployment {graph_deployment_name} not found") + raise
🧹 Nitpick comments (5)
components/planner/test/kube.py (1)
267-272: Add assertion: ensure no API call when env var is unsetGuard against accidental calls by asserting _get_graph_deployment_from_name wasn’t invoked.
@@ - with patch.dict(os.environ, {}, clear=True): - result = await k8s_api.get_parent_graph_deployment() - assert result is None + with patch.dict(os.environ, {}, clear=True): + with patch.object(k8s_api, "_get_graph_deployment_from_name") as mock_get: + result = await k8s_api.get_parent_graph_deployment() + assert result is None + mock_get.assert_not_called()components/planner/src/dynamo/planner/kubernetes_connector.py (2)
35-39: Remove unnecessary f-stringString has no placeholders; flagged by Ruff F541.
- raise ValueError( - f"Parent DynamoGraphDeployment not found" - ) + raise ValueError("Parent DynamoGraphDeployment not found")
56-60: Remove unnecessary f-stringSame issue as above.
- raise ValueError( - f"Parent DynamoGraphDeployment not found" - ) + raise ValueError("Parent DynamoGraphDeployment not found")components/planner/test/kubernetes_connector.py (1)
146-168: Strengthen assertions for multi-component updatesAlso verify exact calls per component; optionally add tests for non-ready and non-blocking flows.
@@ -from dynamo.planner.kubernetes_connector import KubernetesConnector +from dynamo.planner.kubernetes_connector import KubernetesConnector +from unittest.mock import call @@ # Assert mock_kube_api.get_graph_deployment.assert_called_once() mock_kube_api.is_deployment_ready.assert_called_once_with("test-graph") - # Should be called twice, once for each component - assert mock_kube_api.update_graph_replicas.call_count == 2 + mock_kube_api.update_graph_replicas.assert_has_calls( + [call("test-graph", "component1", 3), call("test-graph", "component2", 2)], + any_order=True, + ) mock_kube_api.wait_for_graph_deployment_ready.assert_called_once_with("test-graph")Optionally add:
+@pytest.mark.asyncio +async def test_set_component_replicas_ignores_when_not_ready(kubernetes_connector, mock_kube_api): + target_replicas = {"c1": 2} + mock_kube_api.get_graph_deployment.return_value = {"metadata": {"name": "test-graph"}} + mock_kube_api.is_deployment_ready.return_value = False + await kubernetes_connector.set_component_replicas(target_replicas) + mock_kube_api.update_graph_replicas.assert_not_called() + mock_kube_api.wait_for_graph_deployment_ready.assert_not_called() + +@pytest.mark.asyncio +async def test_set_component_replicas_non_blocking_does_not_wait(kubernetes_connector, mock_kube_api): + target_replicas = {"c1": 2} + mock_kube_api.get_graph_deployment.return_value = {"metadata": {"name": "test-graph"}} + mock_kube_api.is_deployment_ready.return_value = True + await kubernetes_connector.set_component_replicas(target_replicas, blocking=False) + mock_kube_api.wait_for_graph_deployment_ready.assert_not_called()components/planner/src/dynamo/planner/kube.py (1)
143-147: Prefer logging over print for observabilityUse module logger instead of print to integrate with runtime logging.
- print( + logger.info( f"[Attempt {attempt + 1}/{max_attempts}] " f"(status: {ready_condition.get('status') if ready_condition else 'N/A'}, " f"message: {ready_condition.get('message') if ready_condition else 'no condition found'})" )Add once at top:
@@ -import os +import os +import logging @@ -from kubernetes import client, config +from kubernetes import client, config @@ +logger = logging.getLogger(__name__)
📜 Review details
Configuration used: Path: .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 (4)
components/planner/src/dynamo/planner/kube.py(2 hunks)components/planner/src/dynamo/planner/kubernetes_connector.py(3 hunks)components/planner/test/kube.py(2 hunks)components/planner/test/kubernetes_connector.py(5 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 (3)
components/planner/test/kube.py (1)
components/planner/src/dynamo/planner/kube.py (2)
get_parent_graph_deployment(57-77)get_graph_deployment(79-86)
components/planner/src/dynamo/planner/kubernetes_connector.py (1)
components/planner/src/dynamo/planner/kube.py (1)
get_graph_deployment(79-86)
components/planner/test/kubernetes_connector.py (2)
components/planner/src/dynamo/planner/kube.py (4)
is_deployment_ready(102-115)get_graph_deployment(79-86)update_graph_replicas(88-100)wait_for_graph_deployment_ready(117-153)components/planner/src/dynamo/planner/kubernetes_connector.py (2)
KubernetesConnector(27-119)set_component_replicas(75-106)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2774/merge) by julienmancuso.
components/planner/test/kube.py
[error] 1-1: isort: Import order issues detected. The file was auto-fixed by isort during pre-commit.
[error] 1-1: Black formatting applied. File reformatted by Black during pre-commit.
[error] 1-1: ruff: lint issues auto-fixed (3 issues).
components/planner/src/dynamo/planner/kubernetes_connector.py
[error] 1-1: Black formatting applied. File reformatted by Black during pre-commit.
[error] 1-1: ruff: lint issues auto-fixed (3 issues).
components/planner/test/kubernetes_connector.py
[error] 1-1: Black formatting applied. File reformatted by Black during pre-commit.
[error] 1-1: ruff: lint issues auto-fixed (3 issues).
🪛 Ruff (0.12.2)
components/planner/src/dynamo/planner/kubernetes_connector.py
38-38: f-string without any placeholders
Remove extraneous f prefix
(F541)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
85-85: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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). (6)
- GitHub Check: Build and Test - vllm
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
🔇 Additional comments (5)
components/planner/test/kube.py (2)
18-18: LGTM: import for env handlingos import is appropriate for env-var driven tests.
252-264: Good: happy-path env-var test covers name propagationTest asserts delegation to _get_graph_deployment_from_name with the env var value.
components/planner/test/kubernetes_connector.py (2)
29-30: LGTM: AsyncMock coverage includes readiness pathIncluding is_deployment_ready as AsyncMock enables the new flow.
66-71: Good: relax assertion on get_graph_deployment argsThis aligns with the new parameterless API.
components/planner/src/dynamo/planner/kube.py (1)
57-76: LGTM: env-var–based parent DGD resolutionBehavior matches PR objective (DYN_PARENT_DGD_K8S_NAME) and gracefully maps 404 to None.
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Jason Zhou <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Michael Shin <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: Julien Mancuso <[email protected]> Signed-off-by: nnshah1 <[email protected]>
Overview:
update planner to use DYN_PARENT_DGD_K8S_NAME
Summary by CodeRabbit
New Features
Improvements
Tests