Repository navigation
feat(router): add a Global Discovery Service client crate - #17208
Conversation
Add dynamo-global-discovery-client with the two calls a PoolRelay makes to the Global Discovery Service in the Global Router design: - GlobalDiscoveryClient::routers lists every Global Router replica (GET /routers) as ReplicaEndpoint values. A URL that does not parse fails the whole call. - GlobalDiscoveryClient::register_pool registers or refreshes a pool (POST /pools). The body carries the pool's key and location, with the field names of PoolKey and PoolLocation, plus its model, capabilities, frontend URLs, and capacity. The PoolRelay calls it on a schedule, and each call is also the pool's heartbeat. The crate also defines the read-side types from the pool_discovery draft (#15933): ReplicaId, ReplicaEndpoint, WatchEvent, Snapshot, and Completeness. The client and the Global Discovery Service share these definitions. The paths and JSON are a strawman. The service does not serve these calls yet, and their shape is not agreed with its owners. Tests run the client against a local HTTP server. Refs: LLM-212 Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughThis change adds a Rust crate for Global Discovery Service calls. It defines shared discovery and watch types, implements router listing and pool registration over HTTP, and tests client behavior with endpoint checks and a fake service. ChangesGlobal Discovery Client
Priority: Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 6 files. (4 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change: adding a Global Discovery Service client crate for the router feature. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description provides a clear overview, detailed change summary, reviewer starting points, validation results, scope exclusions, and a valid Linear issue reference through "Closes LLM-… Full details: Description check
-
Explanation
The pull request description provides a clear overview, detailed change summary, reviewer starting points, validation results, scope exclusions, and a valid Linear issue reference through "Closes LLM-212". It uses "Summary" instead of the template's "Overview" heading and does not include a separate "Details" heading, but the required information is present and complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 6 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: Non-success response bodies are still buffered in full before constructing
Status, allowing an oversized or unbounded error response to exhaust memory. - Original discussion: The
/routerssuccess body is still collected without a byte limit, so repeated oversized 200 responses can exhaust the polling relay's memory. - Original discussion: Verified at the reviewed head:
routers()still callsResponse::json(), which buffers the complete successful/routersresponse without a byte limit. The scheduled polling path therefore remains vulnerable to payload-sized allocation from a faulty service or proxy. - Original discussion: Verified at the reviewed head:
send()still callsresponse.text()for every non-success response and stores the complete body in the displayableStatuserror. Error-body allocation and any resulting log entry remain unbounded. The separate thread at discussion_r4363471773 reports the same underlying defect. - Original discussion: Verified still present:
routers()usesResponse::json()without a byte limit, so a fast oversized successful response can cause unbounded buffering in the future polling client. - Original discussion: Verified still present:
send()collects every non-success response withresponse.text()without a byte limit, allowing an oversized error body to cause unbounded allocation. The later discussion atdiscussion_r4363471773reports the same underlying defect.
…scovery client Disable automatic redirects. A followed 301, 302, or 303 turns the registration POST into a GET, so register_pool returned Ok even though nothing was registered. A 3xx now returns a Status error. Read the /routers body before parsing it. A body cut off mid-transfer now returns a Request error, and Decode only covers JSON that does not match the expected shape. reqwest reports both cases as decode errors, so the client parses with serde_json itself. Remove the redundant RoutersResponse doc comment. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The non-success path still buffers the complete response body into
DiscoveryClientError::Status; cap the diagnostic body to prevent oversized error responses from exhausting memory. - Original discussion: Verified still present at the reviewed head:
routers()collects the complete successful/routersresponse withResponse::bytes()before deserialization. The request timeout does not bound a fast response by bytes, so the scheduled polling path can make payload-sized allocations from a faulty service or proxy. - Original discussion: Verified still present at the reviewed head:
send()collects every non-success body withresponse.text()and retains it in the displayableStatuserror. A fast oversized error response can therefore cause an unbounded allocation and an unbounded downstream log entry; the later discussion atdiscussion_r4363471773is the same underlying defect. - Original discussion: Verified still present:
routers()collects the complete successful/routersresponse withResponse::bytes()without a byte limit, so an oversized 200 response can cause unbounded allocation in the polling client. - Original discussion: Verified still present:
send()collects every non-success response withresponse.text()without a byte limit and retains it inStatus, allowing an oversized error response to cause unbounded allocation. Discussion r4363471773 reports the same underlying defect. - Original discussion:
routers()still buffers the complete successful/routersresponse without a byte limit, so a large fast response can cause payload-sized allocation despite the request timeout.
Every other public type in the crate implements Debug. Clone is cheap because reqwest::Client is reference-counted, so a PoolRelay can give its registration loop and its routers poll their own clones instead of wrapping the client in an Arc. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
- registers_a_pool now compares the JSON the service received with a literal, so renaming a field fails the test. It used to decode the body into the same struct the client encoded. - a_redirect_is_an_error checks that a 301 comes back as a Status error instead of being followed. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
07f71dc to
0738961
Compare
❌ Dynamo PR CI failed — run 40719751361 (attempt 1) on
|
| Framework | Build | 1-GPU amd64 |
|---|---|---|
| vLLM | ❌ 1 | ➖ |
| SGLang | ✅ 3 | ❌ 1 |
| Other | Jobs |
|---|---|
| planner | ❌ 1 ✅ 4 |
Failure details
The vLLM build failed fetching a CUDA package index, the planner CPU stage ended without recording an exit code, and the SGLang GPU job failed the frontend API-surface compliance test because its request trace lacked parent agent context.
❌ vllm-runtime / Build multi-arch cuda13.0: CUDA apt index size mismatch
Job: vllm-runtime / Build multi-arch cuda13.0 · Failed step: Build and Push Image · Logs: gh run view --job 122927407601 -R ai-dynamo/dynamo --log-failed
> [linux/arm64 pre_runtime 15/38] RUN set -eux; apt-get update; ...
#190 0.881 Err:5 https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2404/sbsa Packages
#190 0.881 File has unexpected size (1835129 != 1831864). Mirror sync in progress? [IP: 23.212.62.197 443]
#190 3.509 E: Some index files failed to download. They have been ignored, or old ones used instead.
#190 ERROR: process "/bin/sh -c set -eux; apt-get update; ..." did not complete successfully: exit code: 100
The arm64 image's apt-get update rejected the CUDA repository's Packages.gz because its downloaded size did not match the index metadata, aborting the multi-architecture build before package installation. This is a package-repository infrastructure signature; rerunning this failed job after the repository metadata settles is likely enough.
❌ planner / CPU Test cpu, amd64: CPU stage cut off without an exit code
Job: planner / CPU Test cpu, amd64 · Failed step: Verify test stages completed · Logs: gh run view --job 122932122827 -R ai-dynamo/dynamo --log-failed
❌ pre_merge_cpu: reported success but recorded no exit code.
Its script was cut off mid-run, so the result is
unknown and cannot be treated as a pass.
⏭️ pre_merge_cpu_e2e: did not run
⏭️ pre_merge_gpu_parallel: did not run
⏭️ pre_merge_gpu: did not run
##[error]A pytest stage did not record a passing result. See this step's log for the stage.
The CPU pytest output stops at 98% without a final pytest result or a recorded stage exit code, so the verification step correctly refused to treat the apparent success as a pass. No failing test assertion or explicit interruption cause is shown in the available log; this is an incomplete test execution, not an identified test failure.
❌ sglang-runtime / Test cuda13.0, amd64: missing parent agent_context in Codex subagent trace
Job: sglang-runtime / Test cuda13.0, amd64 · Failed step: Run GPU tests (parallel) · Logs: gh run view --job 122932691717 -R ai-dynamo/dynamo --log-failed
[w0] tests/frontend/test_frontend_api_surface_compliance.py::test_frontend_api_surface_compliance RETRY FAILED (Auto Test Retries) [100%]
[w0] tests/frontend/test_frontend_api_surface_compliance.py::test_frontend_api_surface_compliance FAILED [100%]
[w0] tests/frontend/test_frontend_api_surface_compliance.py:1072: in _run_codex_subagent_smoke
[w0] _assert_agent_parent_context_in_trace(
[w0] tests/frontend/test_frontend_api_surface_compliance.py:841: in _assert_agent_parent_context_in_trace
[w0] pytest.fail(
[w0] E Failed: request trace did not contain parent agent_context after 'codex subagent' within 30.0s; saw [...]
[w0] !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
[w0] ======================== 1 failed in 225.61s (0:03:45) =========================
tests/frontend/test_frontend_api_surface_compliance.py::test_frontend_api_surface_compliance failed in the Codex subagent smoke check: the request trace did not contain the required parent agent_context within 30 seconds. Automatic retries also failed, and the parallel GPU test runner stopped after this test failure; the later container-execution errors are consequences of the nonzero test exit.
For agents
{"pr":17208,"run_id":40719751361,"run_attempt":1,"head_sha":"e4f6dcde29aef68cf28648dabb3f7f99001d3b3b","failures":[{"job":"vllm-runtime / Build multi-arch cuda13.0","job_id":122927407601,"failed_step":"Build and Push Image","signature":"arm64 CUDA apt Packages.gz unexpected size (1835129 != 1831864); apt-get update exit 100","tests":[],"log_cmd":"gh run view --job 122927407601 -R ai-dynamo/dynamo --log-failed"},{"job":"planner / CPU Test cpu, amd64","job_id":122932122827,"failed_step":"Verify test stages completed","signature":"pre_merge_cpu reported success but recorded no exit code; script cut off mid-run","tests":[],"log_cmd":"gh run view --job 122932122827 -R ai-dynamo/dynamo --log-failed"},{"job":"sglang-runtime / Test cuda13.0, amd64","job_id":122932691717,"failed_step":"Run GPU tests (parallel)","signature":"request trace did not contain parent agent_context after 'codex subagent' within 30.0s","tests":["tests/frontend/test_frontend_api_surface_compliance.py::test_frontend_api_surface_compliance"],"log_cmd":"gh run view --job 122932691717 -R ai-dynamo/dynamo --log-failed"}]}Posted automatically by Devin for run 40719751361. Updated on every full-CI run of this PR.
There was a problem hiding this comment.
Please don't drop the POST /pools reply. It is how the service tells the Relay its timing: {"heartbeat_interval_s": 5, "lease_duration_s": 15}. If the Relay ignores it, its interval and the service's --lease-duration-secs are set separately, and if the interval ever reaches the lease, pools drop out of GET /pools between heartbeats with no warning. Nothing calls register_pool yet, so changing its return type now is free.
What to do with the reply:
- In the client: parse it and return it,
register_pool(...) -> Result<Lease, DiscoveryClientError>, withLease { heartbeat_interval: Duration, lease_duration: Duration }. Treat a missing body, a zero interval, or an interval not below the lease asDecode, so a bad reply can't make the Relay call in a tight loop. - In the Relay: wait the returned
heartbeat_intervalbefore the nextregister_poolcall, instead of a fixed interval. After an error, retry sooner. If calls keep failing for longer thanlease_duration, the pool has dropped out ofGET /pools, which is worth a warning.
The copy of this crate on global-router-v2 needs the same change.
Add dynamo-global-discovery-client with the two calls a PoolRelay makes to the Global Discovery Service in the Global Router design: - GlobalDiscoveryClient::routers lists every Global Router replica (GET /routers) as ReplicaEndpoint values. A URL that does not parse fails the whole call. - GlobalDiscoveryClient::register_pool registers or refreshes a pool (POST /pools). The body carries the pool's key and location, with the field names of PoolKey and PoolLocation, plus its model, capabilities, frontend URLs, and capacity. The PoolRelay calls it on a schedule, and each call is also the pool's heartbeat. The crate also defines the read-side types from the pool_discovery draft (#15933): ReplicaId, ReplicaEndpoint, WatchEvent, Snapshot, and Completeness. The client and the Global Discovery Service share these definitions. The paths and JSON are a strawman. The service does not serve these calls yet, and their shape is not agreed with its owners. Tests run the client against a local HTTP server. Refs: LLM-212 Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
The Global Discovery Service answers POST /pools with
{"heartbeat_interval_s": 5, "lease_duration_s": 15}. register_pool now
parses that reply and returns it as a Lease, so the PoolRelay can take
its heartbeat interval from the service instead of configuring it
separately. If the interval ever reached the lease, pools would drop out
of GET /pools between heartbeats.
A missing body, a zero interval, or an interval not below the lease
fails to parse and returns Decode, so a bad reply cannot make the relay
register in a tight loop.
routers() and register_pool() share one helper that reads the body and
then parses it.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
…version main bumped the workspace version to 1.7.0. The merge kept 1.6.0 in this crate's Cargo.lock entry, so cargo metadata --locked failed in CI. Regenerated the lock; only that version line changes. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
|
Thanks @atchernych, done in 4fa71a6 as you described. This PR has no Relay to change. I added Relay steps to LLM-214, Hannah's PoolRelay wiring ticket. The |
`register_pool(®).await?;` compiled without a warning and threw away the service's timing, which is the failure the review on #17208 asked to prevent. #[must_use] on Lease makes that line warn. The Lease doc now says what happens when lease_duration passes with no successful registration: the pool drops out of GET /pools. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
The crate and watch module docs said the service shares these types, but the service does not depend on this crate. The watch doc also fixed a start-up event order that nothing here emits and that is not agreed yet. The Decode doc now lists an invalid Lease as a cause. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
`register_pool(®).await?;` compiled without a warning and threw away the service's timing, which is the failure the review on #17208 asked to prevent. #[must_use] on Lease makes that line warn. The Lease doc now says what happens when lease_duration passes with no successful registration: the pool drops out of GET /pools. On global-router-v2, the global-ingress round_robin test now keeps the lease in a binding, so the new warning does not fail clippy. Signed-off-by: Jacob Ioffe <jioffe@nvidia.com> (cherry picked from commit 9f0f234)
The one-day cap made the shared `Lease` stricter than the client in #17208: once that client reads the shared type, a reply with a longer lease would fail to parse. The cap only protects the service's own clock arithmetic, so the service now bounds the lease it grants (#17371), and `Lease` keeps the checks a client needs: the interval is above zero and below the lease. Also drops `LeaseError`'s doc comment, which restated its use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>
nnshah1
left a comment
nnshah1
left a comment
There was a problem hiding this comment.
Approving. One question and one note.
Is PoolKey.site_id the same thing as the relay's cluster_id from #17215, in string form? If so the two should share a name so a pool key and a relay identity line up without a translation table, and PoolLocation.cluster needs a doc line saying how it differs. If not, what does site_id identify? No change needed here, since the types move to dynamo-global-common in #17330; I just want the answer recorded before the rename lands there.
On that move: #17330 says this client switches to the shared types later with no JSON change. Could that switch ride in #17330 itself once this merges, rather than a separate follow-up, so the duplicate PoolKey definitions never coexist on main?
🤖 Generated with Claude Code, reviewed and posted by @nnshah1.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
|
Thanks @nnshah1! As far as I can tell, Switching the client in #17330 makes sense to me too, so two |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
No P0 or P1 found. This review found two P3 items. Both are test checks that are missing. Head reviewed: 7f9a3a961fd3f5434d55f63786f93c89a2b96d76.
- [P3]
client.rs:133: no test covers theRequestresult for a body that ends early. - [P3]
relay_calls.rs:153: the non-2xx test does not check the error body.
What this review measured.
- The 7 crate tests pass on this head. They also pass on a merge of this head with
mainatc49f1bee493db24fe2f3721d640d016327ccabf9, which has no conflict. The newermaincommit6e918af4d5changes none of the crate, lock or manifest files.cargo tree --locked,clippy -D warnings,rustdoc -D warningsandfmt --checkpass. - The
Cargo.lockchange adds only dependency edges to versions thatmainalready locks. - The three merge commits add nothing beyond
main. - Eight deliberate breaks each fail a test. Four change the requests: no
pop_if_empty,PUTin place ofPOST, every status as success, and redirects followed. Four change the JSON: a renamedPoolKeyfield, a renamed lease field, a zero interval accepted, and an interval equal to the lease accepted. lib/global-discovery-serviceonmainholds no code yet. The paths, the request JSON and the lease reply match the service onglobal-router-v2at80701ae7eb.- A dropped
Leasegives theunused_must_usewarning. - CI on this head:
PR,Pre Mergeand the other source-branch checks are green.rust-tests (.)ran all 7 crate tests. The skipped jobs are docs, deploy, operator, snapshot and dev-image lanes, which do not build this crate.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
No P0 or P1 found at 7f9a3a961fd3f5434d55f63786f93c89a2b96d76. Two P3 items stay open in my comment review. Both are missing test checks, and each has a tested fix.
The 7 crate tests pass on this head and on a merge with main at c49f1bee493db24fe2f3721d640d016327ccabf9. The paths and the JSON match the Global Discovery Service on global-router-v2, because the service crate on main holds no code yet. The Cargo.lock change adds only dependency edges to versions that main already locks.
…ror body Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Main now has the Global Discovery Service client from #17208 in lib/global-discovery-client, which the next commits move onto this crate's types, and the PoolRelay rename from #17215. No conflicts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>
Neelay Shah and Jacob Ioffe asked on #17208 that its client move onto dynamo-global-common in this PR, so two PoolKey definitions never sit on main together. lib/global-discovery-client drops its own copies of the JSON types, router replicas and watch events. It re-exports the shared types its calls send and return, with their error types; the watch types come from dynamo-global-common. Its calls and errors do not change. What it sends follows the shared types: the key's cluster_id, a location without cluster, and capacity in workers. A PoolKey is built with PoolKey::new, which checks its fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>
Summary
Adds
lib/global-discovery-client, the HTTP client a PoolRelay uses to talk to the Global Discovery Service:routers()callsGET /routersand returns every Global Router replica as aReplicaEndpoint. Each call returns the full current list. If any entry fails to parse, the whole call fails, so the relay never acts on a partial list.register_pool()callsPOST /poolswith the pool's key, location, model, capabilities, frontend URLs, and capacity. Each call is also the pool's heartbeat. It returns the service'sLease, which says when to register again and how long a registration lasts. A missing or inconsistent lease fails asDecode, so a bad reply can't make the relay register in a tight loop.The crate also holds
ReplicaId,ReplicaEndpoint,WatchEvent,Snapshot, andCompletenessfrom thepool_discoverydraft in #15933, so the discovery service can import them instead of defining its own.PoolKeyandPoolLocationuse the same field names aslib/kv-router/src/global_viewon the Global Router branches.The Global Discovery Service on
global-router-v2serves both calls, and the paths and JSON here match it, including the lease in thePOST /poolsreply. It isn't onmainyet. If its shapes change before it lands, I'll update the client to match.Not in this PR: calling the client from the PoolRelay, including waiting
heartbeat_intervalbetween registrations, and pollingrouters()intoWatchEventvalues.Where should the reviewer start?
src/client.rs, thensrc/wire.rsfor the JSON.tests/relay_calls.rsruns the client against an in-process axum server.Validation
clippy -D warnings,rustdoc -D warnings,fmt --check, pre-commit, and the strict CODEOWNERS check.PUT, treating a non-2xx status as success, following redirects, renaming a registration field, and accepting a heartbeat interval at or above the lease.Related Issues
Summary by CodeRabbit