Repository navigation
Conversation
The planner's Kubernetes calls (DGD get/list/patch, DGDSA scale get/patch, pod list, worker-metadata list) pass no _request_timeout, so kubernetes-client hands urllib3 timeout=None. Most of them run directly on the asyncio loop, so an apiserver connection that is accepted but never answered blocks the whole planner, including its control API, indefinitely. ai-dynamo#13245 fixed the same failure for Prometheus. Pass a (connect, read) timeout of (5, 30) seconds on every call. The client only honors an int or a 2-tuple; a float is silently ignored, so the test also pins the tuple shape. A request that never gets an answer now raises instead of blocking forever: a GET after 120s, because urllib3 retries reads three times, and a PATCH after 30s with a raw ReadTimeoutError, because urllib3 does not retry writes. The retry policy itself is left unchanged. Signed-off-by: Akira Lonske <akiralonske@gmail.com>
|
👋 Hi aylonsk! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. WalkthroughPlanner Kubernetes API calls now use a shared ChangesPlanner Kubernetes Request Timeouts
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The scoped Kubernetes requests now have finite timeouts, and no unresolved merge-blocking concern is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Overview:
None of the planner's Kubernetes calls set a request timeout. Most run synchronously on the planner's event loop, so an apiserver connection that is accepted but never answered blocks the planner, including its control API, forever. This PR bounds every call with a
(connect, read)timeout of(5, 30)seconds. It mirrors what #13245 did for Prometheus.Summary
REQUEST_TIMEOUT = (5, 30)inconnectors/clients/kubernetes_api.py. Pass it as_request_timeout=to all six calls there and to the worker-metadata list inconnectors/kubernetes.py.intor a 2-tuple; a bare float is silently ignored. So the new test also pins the tuple shape.urllib3is added toSTUB_MODULESso the marker-report pre-commit hook can collecttest_kube.py, the same as fix(planner): keep control APIs responsive during Prometheus collection #14377 did foraiohttp.test_utils.Details:
With the timeout, a request that never gets an answer now raises instead of blocking:
MaxRetryErrorafter 4 × 30 s = 120 s, because urllib3's defaultRetry(3)retries reads (measured 120.2 s).ReadTimeoutErrorafter 30 s, because urllib3 does not retry writes. The apiserver may already have applied the write.How each caller reacts, unchanged by this PR:
GPUShapeUnavailableError. The planner exits and the pod restarts, just as for other Kubernetes API errors today._apply_effects): the error reaches_mark_effect_submission_unknown, which disables automatic scaling until the planner restarts. This is the existing response to a scaling call with an unknown outcome. Previously a stalled call never got that far, because the loop was blocked. Note the trade-off:Values and policy:
(5, 60)would avoid that at the cost of a 240 s stall; I'm happy to switch if you prefer.Validation
test_stalled_apiserver_raises_instead_of_blockingintest_kube.py. It points a real client at a socket that accepts the connection and never answers, and expectsMaxRetryErrorwithin 10 s. Six existing exact-kwargs assertions gain_request_timeout=REQUEST_TIMEOUT.KubernetesAPI.get_graph_deploymentagainst that socket: still blocked after 20 s onmain;MaxRetryErrorafter 120.2 s with this change.pytest components/src/dynamo/planner/testsin a Linux container with ai-dynamo-runtime 1.6.0.dev20261006: 1498 passed, 2 failed. The same 2 tests (test_wait_for_unacknowledged_decision) fail onmainbecause the container has noetcdbinary.components/src/dynamo/global_planner/tests: 75 passed.mypy --explicit-package-bases components/src/dynamo/planner(the CI invocation, scoped to the planner): no issues.pytest-marker-report: passed.Where should the reviewer start?
components/src/dynamo/planner/connectors/clients/kubernetes_api.py:REQUEST_TIMEOUTand its call sites.Related Issues
Summary by CodeRabbit