feat(backend): add worker drain Admin API - #11118
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughThis PR adds manual worker drain/resume support end-to-end: the runtime routing client tracks a drained-id set alongside overloaded ids, WorkerSet stores a routing client, Model/ModelManager expose worker status and drain APIs, and a new admin HTTP router exposes GET /workers and POST drain/resume endpoints, with documentation updates. ChangesWorker drain/resume feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Related PRs: None identified. Suggested labels: enhancement, http-api, runtime, discovery Suggested reviewers: None identified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/http/service/service_v2.rs (1)
994-999: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftProtect the admin router with an auth boundary.
When
DYN_ENABLE_FRONTEND_ADMIN_APIis true, these drain/resume routes are merged into the same externally reachable frontend router as health/models and no auth or private-network boundary is added here. Any client that can reach the service can take workers out of rotation. Please wrap the admin routes in auth middleware or expose them on a separate private listener.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/llm/src/http/service/service_v2.rs` around lines 994 - 999, The admin routes added in the router assembly are exposed without any protection, so guard the `busy_threshold_router` and `worker_admin_router` paths with an auth/private-network boundary before pushing them into `system_routes`. Update the `admin_api_enabled` branch in `service_v2.rs` to either wrap those routers with the existing auth middleware used elsewhere in the service or move them onto a separate private listener, and make sure the fix is applied around the `busy_threshold::busy_threshold_router` and `worker_admin::worker_admin_router` integration points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/runtime/src/component/client.rs`:
- Around line 264-270: `Client::overloaded_instance_ids()` is no longer
overload-only because it now includes manual drains via `excluded_ids()`, which
changes the public contract used by `routing_instance_counts().overloaded` and
`routing_instance_snapshot().overloaded_ids`. Keep `overloaded_ids()` limited to
true overload signals only, and introduce a separate accessor for
excluded/scheduler-facing IDs so drained workers are not reported as overloaded.
Update the relevant helper(s) in `Client` and any callers around
`routing_instance_counts`/`routing_instance_snapshot` to use the new accessor
where appropriate.
---
Outside diff comments:
In `@lib/llm/src/http/service/service_v2.rs`:
- Around line 994-999: The admin routes added in the router assembly are exposed
without any protection, so guard the `busy_threshold_router` and
`worker_admin_router` paths with an auth/private-network boundary before pushing
them into `system_routes`. Update the `admin_api_enabled` branch in
`service_v2.rs` to either wrap those routers with the existing auth middleware
used elsewhere in the service or move them onto a separate private listener, and
make sure the fix is applied around the `busy_threshold::busy_threshold_router`
and `worker_admin::worker_admin_router` integration points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 220dc0d7-4b25-45b1-87fd-5c045e015c2b
📒 Files selected for processing (10)
docs/components/frontend/configuration.mdlib/llm/src/discovery.rslib/llm/src/discovery/model.rslib/llm/src/discovery/model_manager.rslib/llm/src/discovery/watcher.rslib/llm/src/discovery/worker_set.rslib/llm/src/http/service.rslib/llm/src/http/service/service_v2.rslib/llm/src/http/service/worker_admin.rslib/runtime/src/component/client.rs
9c14b87 to
fc6edd0
Compare
|
It looks like traffic removal before runtime shutdown is already implemented by the existing graceful-shutdown flow. On SIGTERM, a worker first unregisters its endpoints from discovery so routers stop assigning new requests, waits through the routing-convergence grace period, and then shuts down/drains in-flight work (see #6093 and #10705). With Grove Could you clarify which remaining failure mode requires this additional frontend-side drain API? If the intended distinction is administrator-controlled quiescing before initiating Pod deletion, it would be helpful to state that explicitly and explain why the existing discovery unregister path is insufficient, especially since this PR's drain state is frontend-local and in-memory. |
jthomson04
left a comment
There was a problem hiding this comment.
Source-only re-review at head 4ba7fb8. These comments are distinct from the existing availability findings and are submitted as a non-blocking review.
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test bce4014 |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test e8fedab |
Signed-off-by: xianlubird <xianlubird@gmail.com>
…in-api Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 4611297 |
jthomson04
left a comment
There was a problem hiding this comment.
Source-only review of the current PR head.
|
Can we simplify the status response by removing both There is currently no uniform Dynamo-facing engine API that reports KV transfers independently from request activity:
The engines are aware of transfer progress, but Dynamo cannot currently consume a consistent transfer-only state. Consequently, no production engine implements
As a follow-up, each engine adapter should implement
The worker can then publish Until those engine integrations exist, the public status should stick to |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 644c419 |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test e8a7b94 |
|
Thanks, agreed. I simplified the public status in 644c419 by removing both |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test c69da7e |
Signed-off-by: xianlubird <xianlubird@gmail.com> # Conflicts: # lib/backend-common/src/worker.rs
|
/ok to test b39e335 |
Background
This is PR 1 of the implementation plan in #11173. The design discussion concluded that lifecycle state must be owned by the worker rather than stored in each frontend replica.
This PR therefore removes the previous frontend-local prototype and introduces a worker-local Admin API. The existing SIGTERM graceful-shutdown flow remains the default fallback.
Summary
POST /engine/drainPOST /engine/resumeGET /engine/statusserving -> draining -> drained.safe_to_delete: trueonly after the worker reachesdrained.Out of Scope
The following work remains in separate follow-up PRs:
Validation
cargo fmt --all -- --checkcargo check -p dynamo-backend-commoncargo test -p dynamo-backend-common --lib— 137 passedcargo clippy -p dynamo-backend-common --all-targets -- -D warningspython3 docs/fern/scripts/check_asset_paths.pygit diff origin/main...HEAD --check