You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(global-router): add GET /pools and a pool cache to the Global Discovery client - #17702
Add GlobalDiscoveryClient::pools() for GET /pools, and PoolCache, which keeps the pool list current and merges partial lists the way the Global Discovery Service asks. The cache publishes the pools as a watch::Receiver<HashMap<PoolKey, PoolListing>>. Nothing reads it yet: the Global Frontend still takes its pools from --pool flags, and switching it over is a follow-up.
About 150 lines of core logic; tests, docs and comments excluded.
Stacked on #17688, which adds RouterCache to the same file. The Files tab shows only this PR's changes.
GET /pools can return a partial list (complete: false), for example while the service's memory store refills after a restart, or while its Kubernetes watch keeps failing. The service's contract documents how a caller that keeps its own list should handle one (lib/global-discovery-service/src/pool_discovery.rs, from #17331), and PoolCache applies those rules:
A complete list replaces the known pools.
A partial list adds new pools and updates changed ones, but keeps known pools that are missing.
Pools are removed only on a complete list.
A cache that knows no pools takes a partial list too. Some pools beat none.
PoolCache has the same shape as RouterCache. spawn reads immediately, then every interval, until its cancel token fires or the cache is dropped. A failed read keeps the last pools and reports Freshness::Stale with the error. A partial list counts as a successful read. A refresh that adds, updates, and removes nothing publishes nothing. The cache logs each change with the pools added, updated, and removed, and logs once when the service starts or stops listing every pool.
The published value leaves out complete. The cache already applies the only rule that depends on it, and a Global Router routes on a partial view, so no consumer needs it yet. A getter can add it later without changing the published type.
RouterCache and PoolCache share ticks(), mark_stale(), and mark_current(). So both handle a zero interval, a slow refresh, an outage, and a recovery the same way. Both publish a list before marking it Current. Moving these out of RouterCache is its own commit, with one behavior change. spawn now builds the timer before it spawns the task, so a runtime without timers panics in the caller instead of in the task.
PoolKey now implements Display as <cluster_id>/<namespace>/<dgd_name>, the name the Global Router already gives a pool in metrics. The cache logs pools by it.
End to end against the Global Discovery Service binary from feat(global-router): add the Global Discovery HTTP service #17371, with --pool-discovery registrations --registration-storage memory, a 1 s heartbeat, and a 4 s lease. Pools a and d register with register_pool() every 1 s and 3 s, and b stops after 2 s. The cache reads every 250 ms. The service was killed at 9 s and restarted empty at 11 s.
Time
GET /pools
Cache
0.03 s
[d], partial
Takes [d] while empty
0.28 s
[a, b, d], partial
Adds a and b
4.05 s
[a, b, d], complete
No change
5.10 s
[a, d], complete, after b's lease ran out
Removes b
9.01 s
Unreachable
Stale, keeps a and d
11.06 s
[a], partial, after the restart
Current, keeps d
12.10 s
[a, d], partial
No change
15.11 s
[a, d], complete
No change
CI can't run this until the service is on main.
The new tests catch:
a_complete_list_replaces_the_known_pools: a complete list that doesn't remove or update a pool.
a_partial_list_removes_nothing_until_a_complete_one: a partial list that removes a missing pool, a partial list with nothing new that still publishes, and a complete list that doesn't remove.
an_empty_cache_takes_a_partial_list: a cache that waits for a complete list.
the_last_listing_of_a_key_wins: a key listed twice.
keeps_pools_through_an_outage_and_reports_them_stale: an outage that drops pools, an unchanged list that still publishes, and a channel that stays open after cancel.
a_partial_list_after_an_outage_keeps_the_missing_pools: a restarted service's partial list that removes pools.
a_complete_empty_list_removes_every_pool: a cache that ignores an empty complete list.
dropping_the_pool_cache_stops_it: a refresh task that outlives its cache.
lists_pools_and_whether_the_list_is_complete and one_bad_pool_fails_the_whole_list: the call against an HTTP service.
a_key_displays_as_its_name_for_people: the PoolKey format.
Summary by CodeRabbit
New Features
Pool Relay can now optionally connect to Global Discovery to retrieve and keep a list of Global Router replicas current.
Added a global discovery URL setting through the command line or environment variable. Relay health reports whether discovery is enabled and, when applicable, why the router list is stale.
Added support for retrieving and caching pool listings, including their completeness status.
Documentation
Updated Pool Relay configuration and architecture references with the discovery setting and health information.
A PoolRelay keeps the Global Router replicas in a HashSet, the directory
PoolViewBroadcaster::run takes, so RouterReplica needs Hash.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
RouterCache reads GET /routers now and then on an interval, and
publishes the Global Router replica set on a watch channel. A refresh
that lists the same routers publishes nothing. A failed refresh keeps
the last set and reports the error through Freshness::Stale, so a
discovery outage does not drop routers a caller already knows. The
cache logs each new error once, the recovery once, and the router IDs
added or removed.
The refresh task stops when its cancel token fires or the cache is
dropped. A zero interval panics at spawn instead of inside the task.
The cache lives in the client so the PoolRelay and the Global Frontend
share one refresh loop. GET /pools with merge-on-partial rules can
follow the same way.
Refs LLM-213.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
A PoolRelay sends its state to every Global Router replica, so it needs
the current replica list. With --global-discovery-url or
DYN_RELAY_GLOBAL_DISCOVERY_URL set, PoolRelay::start builds a
GlobalDiscoveryClient and starts a RouterCache that reads GET /routers
every 5 s. subscribe_global_routers() returns the cache's watch channel
of HashSet<RouterReplica>, the input PoolViewBroadcaster::run takes.
Nothing reads it yet; LLM-228 hands it to the broadcaster.
The cache starts after every fallible step, so a failed start leaves no
task behind, and it stops with the Relay. health() reports
global_discovery_enabled and global_routers_stale_reason. A stale list
does not change healthy, so probes do not restart the Relay during a
discovery outage.
Closes LLM-213.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
…rent
RouterCache now publishes a changed set before it marks the list
current, so a reader never sees Current next to the previous set. Its
change log names routers as <id>=<url>, so a URL change under the same
ID shows up. A unit test covers the error-cause chain that goes into
the stale reason.
Refs LLM-213.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
DiscoveryClientError::Status kept the whole response body. RouterCache
now puts that error in logs and in PoolRelay health, so a proxy's error
page would fill both. The client keeps the first 1 KiB, cut on a
character boundary.
Refs LLM-213.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
The test dialed 127.0.0.1:9, which can be in use or filtered. It now
binds 127.0.0.1:0 for an unused port and checks that the stale reason
carries the connection error's cause.
Refs LLM-213.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
GlobalDiscoveryClient::pools() calls GET /pools and returns the
service's PoolsResponse: the pools it lists and whether the list is
complete. A partial list can miss pools that still exist, so a caller
that keeps a list removes pools only on a complete one. If any entry
does not parse, the whole call fails.
The crate now re-exports PoolListing and PoolsResponse from
dynamo-global-common.
Refs LLM-233.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Move RouterCache's tick setup into ticks() and its Freshness updates
into mark_stale and mark_current, so a second cache refreshes and
reports outages the same way: a zero interval panics at spawn, an
overrunning refresh delays the next tick, each new failure reason and
the recovery are logged once, and Current is set after the new list is
published. Rename refresh to refresh_routers to say which list it
reads. No behavior change.
Refs LLM-233.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
A PoolKey now displays as <cluster_id>/<namespace>/<dgd_name>, the name
the Global Router already gives a pool in metrics. The Global Discovery
client's pool cache logs pools by it.
Refs LLM-233.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
PoolCache reads GET /pools immediately, then every interval, and
publishes the pools by PoolKey on a watch channel. It applies the
service's rules for partial lists:
- A complete list replaces the known pools.
- A partial list adds new pools and updates changed ones, but keeps
known pools that are missing. Pools are removed only on a complete
list.
- A cache that knows no pools takes a partial list too.
A failed refresh keeps the last pools and reports Freshness::Stale, as
RouterCache does. A refresh that changes nothing publishes nothing.
The cache logs each change with the pools added, updated and removed,
and logs once when the service starts or stops listing every pool.
The published value leaves out `complete`. The cache already applies
the only rule that depends on it, and a Global Router routes on a
partial view, so no consumer needs it yet. A getter can add it later
without changing the published type.
Refs LLM-233.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Each refresh collected the listed pools into a new map, cloning every
key, even when nothing changed. Look each pool up in the known map
instead, so only a new pool's key is cloned, and borrow the listed keys
for the removal pass.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
…st, and a partial recovery
Add tests that dropping a PoolCache stops its task, that a complete
empty list removes every pool, and that a partial list after an outage
keeps the missing pools. Note in the docs that Freshness keeps its last
value after a cache stops, and that the complete flag starts true.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
The client cut an error body to 1 KiB only after reading all of it, so a
large or endless error page was buffered whole on every refresh. Read
chunks until the limit instead. The test serves an error body that never
ends; the old read waits for the timeout.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
…ing comments
error_chain's test now nests two sources, so a version that appends only
the first cause fails. Remove comments that repeat a constant's name or
the assertions that follow them.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
lib/bindings/kvbm resolves dynamo-llm by path, so its lockfile now lists
dynamo-global-discovery-client and dynamo-global-common, and
`cargo metadata --locked` passes there.
Drop the zero-interval test, which only guarded a constant, and the HTTP
RouterCache test, which repeated the unit and host tests. One test now
serves an endless error body with a multibyte character at the cut.
Remove a needless clone and a comment about start order: RouterCache
aborts its task on drop, so the order does not matter.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
…pool-cache
Takes the review fixes from #17688. Like its RouterCache twin there, the
HTTP PoolCache test goes too: the PoolCache unit tests cover the merge
rules and the outage, and lists_pools_and_whether_the_list_is_complete
covers decoding over HTTP. The fake service drops the fields only that
test used.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Build on the router and pool caches from #17688 and #17702. Project cached pool listings into frontend candidates, accepting partial additions and updates while retaining omitted pools until a complete listing arrives.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
No actionable comments were generated in the recent review.
Walkthrough
Pool Relay can optionally read Global Router replicas from Global Discovery. The Global Discovery client adds pool listing and periodic router and pool caches. Pool Relay exposes router subscriptions and discovery freshness through its health data.
Changes
Global Discovery client and caches
Layer / File(s)
Summary
Discovery client and caches Cargo.toml, lib/global-common/src/*, lib/global-discovery-client/*
The client adds pool listing and limits the response body included in HTTP status errors. Router and pool caches refresh periodically, expose freshness and subscriptions, and retain cached data after failed refreshes. Tests cover cache updates, incomplete pool listings, outages, and cancellation.
Pool Relay integration
Layer / File(s)
Summary
Pool Relay runtime integration lib/llm/Cargo.toml, lib/llm/src/pool_relay/host.rs, lib/llm/src/pool_relay/docs/architecture.md
Pool Relay optionally starts a router cache from its Global Discovery URL and exposes the router subscription. Health data reports whether discovery is enabled and whether the router list is unread or stale.
Relay configuration and entry points components/src/dynamo/pool_relay/*, lib/bindings/python/rust/llm/pool_relay.rs, lib/bindings/python/src/dynamo/_core.pyi, docs/fern/pages/reference/components/pool-relay-configuration.md
The CLI and Python constructor accept the optional URL. CLI parsing supports DYN_RELAY_GLOBAL_DISCOVERY_URL, prioritizes the CLI option, and rejects empty values. The configuration reference describes the option and health fields.
No issue requiring a fix before merge was identified. The change is ready for normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title "feat(global-router): add GET /pools and a pool cache to the Global Discovery client" matches the main change. The PR adds GlobalDiscoveryClient::pools() for GET /pools and a PoolCache that …
Description check
The description covers the core intent: GET /pools support, PoolCache merge rules for partial and complete lists, the Freshness behavior, the PoolKey Display change, and the stacking on #17688. It inc…
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.
Full details: Title check
Explanation
The title "feat(global-router): add GET /pools and a pool cache to the Global Discovery client" matches the main change. The PR adds GlobalDiscoveryClient::pools() for GET /pools and a PoolCache that publishes pools through a watch channel. The title is specific and concise. It does not mention the PoolKey Display implementation or the shared refresh helpers, which are secondary.
Full details: Description check
Explanation
The description covers the core intent: GET /pools support, PoolCache merge rules for partial and complete lists, the Freshness behavior, the PoolKey Display change, and the stacking on #17688. It includes a Review section that lists files and a Validation section with test, clippy, fmt, and end-to-end results. It does not follow the repository template exactly. The template has Overview, Key Changes, Validation, and Issues and Ongoing Work headings, plus a 'Closes #XXXX' line and a checklist. The description uses Summary, Design, Review, and Validation headings. It has a 'Closes LLM-233' line, but that is a Linear-style reference rather than the '#XXXX' issue link the template expects. It also omits the Pull Request Checklist block. These gaps are non-critical, so the description is mostly complete.
Full details: Docstring Coverage
Explanation
Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 12 files. (5 skipped: 5 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
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Build on the router and pool caches from #17688 and #17702. Project cached pool listings into frontend candidates, accepting partial additions and updates while retaining omitted pools until a complete listing arrives.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Build on the router and pool caches from #17688 and #17702. Project cached pool listings into frontend candidates, accepting partial additions and updates while retaining omitted pools until a complete listing arrives.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Build on the router and pool caches from #17688 and #17702. Project cached pool listings into frontend candidates, accepting partial additions and updates while retaining omitted pools until a complete listing arrives.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Summary
Add
GlobalDiscoveryClient::pools()forGET /pools, andPoolCache, which keeps the pool list current and merges partial lists the way the Global Discovery Service asks. The cache publishes the pools as awatch::Receiver<HashMap<PoolKey, PoolListing>>. Nothing reads it yet: the Global Frontend still takes its pools from--poolflags, and switching it over is a follow-up.About 150 lines of core logic; tests, docs and comments excluded.
Stacked on #17688, which adds
RouterCacheto the same file. The Files tab shows only this PR's changes.Closes LLM-233.
Design
GET /poolscan return a partial list (complete: false), for example while the service's memory store refills after a restart, or while its Kubernetes watch keeps failing. The service's contract documents how a caller that keeps its own list should handle one (lib/global-discovery-service/src/pool_discovery.rs, from #17331), andPoolCacheapplies those rules:PoolCachehas the same shape asRouterCache.spawnreads immediately, then every interval, until its cancel token fires or the cache is dropped. A failed read keeps the last pools and reportsFreshness::Stalewith the error. A partial list counts as a successful read. A refresh that adds, updates, and removes nothing publishes nothing. The cache logs each change with the pools added, updated, and removed, and logs once when the service starts or stops listing every pool.The published value leaves out
complete. The cache already applies the only rule that depends on it, and a Global Router routes on a partial view, so no consumer needs it yet. A getter can add it later without changing the published type.RouterCacheandPoolCacheshareticks(),mark_stale(), andmark_current(). So both handle a zero interval, a slow refresh, an outage, and a recovery the same way. Both publish a list before marking itCurrent. Moving these out ofRouterCacheis its own commit, with one behavior change.spawnnow builds the timer before it spawns the task, so a runtime without timers panics in the caller instead of in the task.PoolKeynow implementsDisplayas<cluster_id>/<namespace>/<dgd_name>, the name the Global Router already gives a pool in metrics. The cache logs pools by it.Review
lib/global-discovery-client/src/client.rs:pools().lib/global-discovery-client/src/cache.rs:PoolCache, the merge inrefresh_pools(), the shared helpers, and the tests.lib/global-discovery-client/tests/relay_calls.rs:pools()against an HTTP fake.lib/global-common/src/pool.rs:DisplayforPoolKey.Validation
cargo test -p dynamo-global-common -p dynamo-global-discovery-client --locked: 32 passed, 11 of them new.cargo clippy -p dynamo-global-common -p dynamo-global-discovery-client -p dynamo-global-router -p dynamo-global-frontend --all-targets --locked -- -D warningscargo fmt --all -- --checkEnd to end against the Global Discovery Service binary from feat(global-router): add the Global Discovery HTTP service #17371, with
--pool-discovery registrations --registration-storage memory, a 1 s heartbeat, and a 4 s lease. Poolsaanddregister withregister_pool()every 1 s and 3 s, andbstops after 2 s. The cache reads every 250 ms. The service was killed at 9 s and restarted empty at 11 s.GET /pools[d], partial[d]while empty[a, b, d], partialaandb[a, b, d], complete[a, d], complete, afterb's lease ran outbStale, keepsaandd[a], partial, after the restartCurrent, keepsd[a, d], partial[a, d], completeCI can't run this until the service is on
main.The new tests catch:
a_complete_list_replaces_the_known_pools: a complete list that doesn't remove or update a pool.a_partial_list_removes_nothing_until_a_complete_one: a partial list that removes a missing pool, a partial list with nothing new that still publishes, and a complete list that doesn't remove.an_empty_cache_takes_a_partial_list: a cache that waits for a complete list.the_last_listing_of_a_key_wins: a key listed twice.keeps_pools_through_an_outage_and_reports_them_stale: an outage that drops pools, an unchanged list that still publishes, and a channel that stays open after cancel.a_partial_list_after_an_outage_keeps_the_missing_pools: a restarted service's partial list that removes pools.a_complete_empty_list_removes_every_pool: a cache that ignores an empty complete list.dropping_the_pool_cache_stops_it: a refresh task that outlives its cache.lists_pools_and_whether_the_list_is_completeandone_bad_pool_fails_the_whole_list: the call against an HTTP service.a_key_displays_as_its_name_for_people: thePoolKeyformat.Summary by CodeRabbit