feat(operator): add SGLang selection topology controller#11036
Conversation
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
|
👋 Hi yankay! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
feat
external-contribution
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93cc0d415b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| targetURL := parsedSelectorURL.parsed | ||
| targetURL.Host = net.JoinHostPort(endpoint.Addresses[0], strconv.Itoa(int(targetPort))) | ||
| selectorTargetURLSet.Insert(targetURL.String()) |
There was a problem hiding this comment.
When the selector URL is an https:// Kubernetes Service DNS name, this rewrites the request URL to the Pod IP, so Go sends SNI and verifies the certificate against that IP rather than the original Service hostname. In clusters that put TLS on the selection service with a normal selector.namespace.svc certificate, every fan-out request fails and workers never register; either avoid endpoint-IP fan-out for HTTPS or preserve the original server name/Host when dialing replicas.
Useful? React with 👍 / 👎.
WalkthroughAdds a ChangesSelectionTopology Controller + SGLang Worker Registration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 4
🤖 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 `@deploy/operator/internal/enginemetadata/sglang/kv_events.go`:
- Around line 18-22: The endpoint selection in kvEventsEndpoints currently
prefers the legacy KVEvents descriptor when both KVEvents and KVEventsConfig are
present, which can reject valid mixed payloads. Update kvEventsEndpoints to
treat KVEventsConfig as the प्राथमिक source and only fall back to
kvEventsDescriptorEndpoints when KVEventsConfig is absent or empty, keeping the
existing kvEventsConfigEndpoints path and preserving BuildWorkerRequest
behavior.
In `@deploy/operator/internal/enginemetadata/sglang/worker_test.go`:
- Around line 153-172: The test currently only exercises the legacy kv_events
path, so it does not validate the structured kv_events_config branch in
BuildWorkerRequest. Update
TestBuildWorkerRequestReadsRealStructuredKVEventsMetadata to remove or nil out
metadata.ServerInfo.KVEvents, then assert the structured payload is decoded via
KVEventsConfig instead of KVEventsEndpoints; alternatively, change
realStructuredKVEventsFixtureSnapshot to provide an object-shaped
kv_events_config fixture so the test explicitly covers structured metadata
handling.
In `@docs/components/router/standalone-selection.md`:
- Around line 84-90: Update the standalone-selection documentation to state that
PATCHing metadata replaces the entire map rather than merging entries. In the
section describing POST /workers, PATCH /workers/{worker_id}, and GET /workers
behavior, clarify that the selection service’s metadata handling (as implemented
by selection/types.rs in the metadata setter) overwrites the existing map, so
clients must resend all desired keys when updating worker metadata.
In `@examples/runtime-free-router/sglang/kustomization.yaml`:
- Around line 11-14: The kustomization for the selector overlay still points to
the plain dynamo-frontend image, so the deployed Pod may not contain
dynamo.select_service. Update the image reference in the overlay to a published
selector-capable image that includes the select-service feature, or make the
image overridable/required before deployment. Use the existing kustomization
fields newName and newTag to locate the image settings and ensure the referenced
image is compatible with running python -m dynamo.select_service.
🪄 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: ec5f4f4d-8d8c-4a5c-8d49-41c87206f8ff
📒 Files selected for processing (30)
deploy/operator/cmd/main.godeploy/operator/internal/consts/consts.godeploy/operator/internal/controller/selection_topology_adapter_sglang.godeploy/operator/internal/controller/selection_topology_controller.godeploy/operator/internal/controller/selection_topology_controller_test.godeploy/operator/internal/enginemetadata/sglang/client.godeploy/operator/internal/enginemetadata/sglang/client_test.godeploy/operator/internal/enginemetadata/sglang/kv_events.godeploy/operator/internal/enginemetadata/sglang/testdata/README.mddeploy/operator/internal/enginemetadata/sglang/testdata/model_info_generation.jsondeploy/operator/internal/enginemetadata/sglang/testdata/models.jsondeploy/operator/internal/enginemetadata/sglang/testdata/server_info_qwen3_huggingface_legacy_kv_events_config.jsondeploy/operator/internal/enginemetadata/sglang/testdata/server_info_qwen3_huggingface_structured_kv_events.jsondeploy/operator/internal/enginemetadata/sglang/types.godeploy/operator/internal/enginemetadata/sglang/worker.godeploy/operator/internal/enginemetadata/sglang/worker_test.godeploy/operator/internal/selectionservice/client.godeploy/operator/internal/selectionservice/client_test.godeploy/operator/internal/selectionservice/types.godocs/components/router/standalone-selection.mddocs/kubernetes/dynamo-operator.mdexamples/README.mdexamples/runtime-free-router/README.mdexamples/runtime-free-router/sglang/README.mdexamples/runtime-free-router/sglang/kustomization.yamlexamples/runtime-free-router/sglang/selection-service.yamlexamples/runtime-free-router/sglang/sglang-worker.yamllib/kv-router/src/services/selection/core/mod.rslib/kv-router/src/services/selection/tests.rslib/kv-router/src/services/selection/types.rs
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
yankay
force-pushed
the
feat/sglang-selection-adapter
branch
from
377757d to
7b2be42
Compare
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
|
Ran the standalone-selection tests on this branch, all 24 pass including the new round-trip test. One semantics question on the PATCH path: apply() replaces the whole metadata map (types.rs, the new if let Some(metadata) block), so a second writer PATCHing just one key drops everything the operator wrote, and "metadata": {} silently clears the map while omitting the field leaves it untouched. If the map is meant to be operator-owned that's fine, but it'd be worth a line in the API docs saying PATCH replaces wholesale (or switching to a merge), plus a test pinning the {} -> clear behavior either way. |
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Thanks for catching this, that is a good point. I kept the current whole-map replacement semantics for metadata, and added a docs clarification that {} clears it while omitting the field leaves it unchanged. I also added test coverage for the replace/omit/clear behavior. |
|
Re-ran the suite at 63b85cb -- all green here (643 lib tests pass, including the 24 selection ones), and the new replace/omit/clear coverage plus the docs line are exactly what I was hoping for. Looks good from my side, thanks! |
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
yankay
force-pushed
the
feat/sglang-selection-adapter
branch
from
63b85cb to
ba2e48a
Compare
yankay
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
Hi @waynehacking8 |
2 participants
Summary
This adds a Kubernetes selection topology controller and
external-sglangadapter for the runtime-free standalone selection service. The operator discovers annotated external SGLang worker Services, probes/v1/models,/model_info, and/server_info, normalizes the result intoWorkerRequest, and reconciles the selection service/workerscatalog.This builds on the HTTP selection service from #10641.
Scope is intentionally narrow: this is a control-plane catalog path for aggregated/raw SGLang workers. It does not add Gateway/EPP forwarding, reservation lifecycle handling, P/D orchestration, multinode SGLang aggregation, or other engine adapters.
Details
external-sglangadapter and selection topology controller.DELETE /workers/{worker_id}semantics.worker_idcollisions.Known gaps:
dp_size > 1SGLang deployments, follower KV-event aggregation, and restart-stable logical worker IDs are follow-ups.localhost, host URLs, and external load balancers are treated as single fixed HTTP targets.nvidia.com/dynamo-selection-require-kv-eventscontrols operator-side metadata validation only. It does not override selection-service schedulability.unschedulabletombstones; purge, TTL/GC, and active-only catalog views are follow-ups.Related Issues
Validation
git diff --checkpassed./workers,/ready,/select, KV event ingestion, selector Service fan-out/replay, stale/orphan deactivation, and fail-closed metadata handling.