feat(global-router): reserve POST /routers until router registration is designed by atchernych · Pull Request #17283 · ai-dynamo/dynamo

atchernych · 2026-10-25T23:34:51Z

Summary

The design lists RegisterRouter among the Global Router's calls. This PR adds its endpoint, POST /routers, but no store takes router registrations yet. Every call answers 501 with the reason:

Kubernetes stores (--store karmada or --store mcs): not needed. These stores already list every router replica from the router Service's EndpointSlices that MCS imports.

router replicas need not register with this store: it lists them from the router Service's EndpointSlices, which MCS imports
Memory store: not decided yet. Router replicas still come from --router.

router registration is not implemented yet: how a router proves it is a router, and what it sends, are not decided. List router replicas with --router

DiscoveryStore gains router_source, which says where router replicas come from and picks the reason.

Why the memory store doesn't take registrations yet

The service trusts every caller. That was a V1 choice for pools, on a trusted network (Karmada, no mesh).
A fake or mistaken router entry is worse than a fake pool: every PoolRelay would connect to it and stream its pool's load and cache state there.
The memory store is for tests and the mode without Kubernetes, where a trusted network is least likely.

The first commit in this PR accepted registrations in the memory store; the last commit removes that.

Open questions for the design

How does a router prove it is a router: mesh mTLS (a SPIFFE ID), a token, or something else? This comes first.
What does it send? One GET /routers entry, {"replica": "<id>", "url": "<url>"}, may be enough.
Timing: a heartbeat with a lease, like POST /pools?

This PR and #17282 both add a field to DiscoveryStore, so whichever merges second resolves a small conflict in service.rs and main.rs.

Validation

cargo test -p dynamo-global-discovery-service: 44 passed. A new test checks both 501 answers. The MCS one is sent with an empty body, since it must not depend on the body.
The dynamo-global-ingress, dynamo-global-router and dynamo-global-discovery-client tests pass too. The global-ingress round-robin test builds this service.
cargo clippy --all-targets -- -D warnings and cargo fmt --check are clean for those four crates.
Ran the binary: POST /routers answered 501 with the reason in the memory store and with --store mcs, and GET /routers still listed the --router replica.

🤖 Generated with Claude Code

…service Add the design's `RegisterRouter` call as `POST /routers`. A Global Router replica sends `{"replica": "<id>", "url": "<url>"}`, the shape of one `GET /routers` entry, on a timer. Each call is also its heartbeat, and the reply carries the lease, as for `POST /pools`. - Memory store: `MemoryReplicaStore` lists registered replicas until their lease ends, plus the `--router` replicas. A registration replaces a `--router` entry with the same ID while it lasts. - Kubernetes stores: `POST /routers` answers 501 and says why. MCS already lists every router replica from the router Service's EndpointSlices, so replicas need not register. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>

dynamo-review-agent · 2026-10-25T23:44:40Z

+        self.ensure_sweeping();
+        let now = Instant::now();
+        let mut replicas = self.lock();
+        let changed = replicas.served(&endpoint.replica, now) != Some(&endpoint);


An expired registration can be replaced before the periodic sweep publishes its fallback/removal event. Here served() hides that expired entry behind the configured endpoint, so if the new registration equals the configured endpoint, changed is false: existing watchers remain stuck on the old registered URL even though get/list now return the new one. Treat replacing any expired entry as a change and emit Added; only suppress the event when the previous registration is still live and unchanged (or no registration exists and the configured endpoint is unchanged).

🤖 AI Fix

Change registration change detection to always publish Added when replacing an expired registered entry, and add a paused-time regression covering expiry before the next sweep followed by registration of the configured endpoint.

Fixed in 871b21c. register now treats replacing any expired registration as a change and publishes Added. It skips the event only for a live registration of the same endpoint, or for no registration when the configured endpoint is the same. replacing_a_registration_the_sweep_has_not_removed_is_a_change covers this case and fails on the previous code.

Update: 7391493 removes router registration from the memory store until its design settles (see the PR description), so this code and its fix are gone. POST /routers now answers 501 with the reason in every store.

dynamo-review-agent · 2026-10-25T23:44:40Z

+        self.ensure_sweeping();
+        let now = Instant::now();
+        let mut replicas = self.lock();
+        let changed = replicas.served(&endpoint.replica, now) != Some(&endpoint);


The change test ignores an expired registered override because served(..., now) has already fallen back to the configured endpoint. If watchers still contain override A, its lease expires, and the router registers configured endpoint B before the sweeper runs, this comparison reports no change; the live B entry then prevents the sweep from emitting the fallback event. list/get return B, but watch consumers remain on A indefinitely. A registration following an expired entry must publish the renewed endpoint even when it equals the configured fallback.

🤖 AI Fix

Treat an existing expired registration as changed and emit Added(endpoint) when replacing it, rather than deriving change solely from the time-filtered served value.

Same issue as the comment above; fixed in 871b21c.

Update: 7391493 removes router registration from the memory store until its design settles (see the PR description), so this code and its fix are gone. POST /routers now answers 501 with the reason in every store.

… expiry `MemoryReplicaStore::register` decided whether to publish `Added` from `served()`, which hides a registration that has expired but that the sweep has not removed yet. If such a registration overrode a `--router` entry and the router then registered the configured address again, no event went out, and the new live registration kept the sweep from publishing the fallback, so watchers kept the old URL while `list` and `get` returned the new one. Replacing any expired registration is now a change. Only a live registration of the same endpoint, or no registration with the same configured endpoint, skips the event. Also set the new `router_registrar` field in the global-ingress round-robin test, which no longer compiled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>

dynamo-review-agent · 2026-10-26T00:33:41Z

+}
+
+impl Replicas {
+    /// A live registration, else the configured replica.


This private helper’s doc comment merely paraphrases the immediately visible lookup order: use a live registration, otherwise fall back to configured. It records no additional invariant or constraint.

🤖 AI Fix

Remove this doc comment.

This code is gone: 7391493 removed router registration from the memory store.

…gn settles Router registration is not designed yet: how a router proves it is a router, and what it sends, are open. The service trusts every caller, a V1 choice made for pools on a trusted network, but a fake router entry is worse than a fake pool: every PoolRelay would connect to it and stream its pool's state there. The memory store, which took these registrations, is also where a trusted network is least likely. So no store takes router registrations, and `POST /routers` answers 501 with the reason: - MCS stores: router replicas need not register; MCS lists them. - Memory store: not implemented yet; list router replicas with --router. `DiscoveryStore::router_source` names where router replicas come from and replaces `router_registrar`. `MemoryReplicaStore`, the `ReplicaRegistrar` trait and the re-registration fix go away with it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>

dynamo-review-agent · 2026-10-26T00:55:02Z

 pub struct DiscoveryStore {
    pub pools: Arc<PoolDirectory>,
    pub routers: Arc<ReplicaDirectory>,
+    /// Where `routers` comes from. `POST /routers` gives it as the reason.


This field comment restates router_source and duplicates the nearby RouterSource documentation plus the register_router match that uses it as the response reason. It adds no invariant or constraint.

🤖 AI Fix

Remove this field comment.

Removed in a1b7d5c.

dynamo-review-agent · 2026-10-26T00:55:02Z


+    #[tokio::test]
+    async fn routers_cannot_register_and_the_answer_says_why() {
+        for (source, reason) in [


The nested loops create a four-case Cartesian product even though router_source only selects the reason text and register_router never reads the body. Retaining both bodies for Configured and one body for Mcs still detects body parsing becoming mandatory and protects both source-specific responses; the Mcs plus empty-body invocation adds no distinct observable contract.

🤖 AI Fix

Flatten these loops into three explicit cases covering both bodies for one source and one representative body for the other source.

Done in a1b7d5c, as two cases: MCS with an empty body, since that answer must not depend on the body, and the memory store with a registration body. Together they cover both reasons and that no body is parsed.

dynamo-review-agent · 2026-10-26T00:55:02Z

+                    "{source:?}"
+                );
+            }
+            // Listing still works.


This final GET assertion only repeats the status check already covered more strongly by service::tests::routers_use_the_client_shape, which also validates the returned router payload. The POST handler cannot mutate the store, and this assertion discards the payload, so running it after POST distinguishes no additional regression.

🤖 AI Fix

Remove the post-registration GET assertion and rely on routers_use_the_client_shape for the supported listing behavior.

Removed in a1b7d5c; routers_use_the_client_shape covers listing.

dynamo-review-agent · 2026-10-26T00:55:02Z

+                    "{source:?}"
+                );
+            }
+            // Listing still works.


This test comment merely narrates the immediately following GET /routers call and successful status assertion.

🤖 AI Fix

Remove this comment.

Removed in a1b7d5c, with the GET it described.

Address review comments. The MCS answer is checked with an empty body, since it must not depend on the body, and the memory-store answer with a registration body. Drop the GET /routers check, which `routers_use_the_client_shape` already covers, and a field comment that repeated `RouterSource`'s docs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>

devin-ai-integration · 2026-10-26T01:57:59Z

❌ Dynamo PR CI failed — run 40252246414 (attempt 1) on `a1b7d5c12f`

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ❌ dynamo-status-check

Other	Jobs
dynamo-runtime	❌ 2 ✅ 6

Failure details

2 jobs failed, both with the same single test: deploy/utils/tests/test_convert_api_version.py::test_examples_default_to_v1beta1 fails with a YAML ScannerError while parsing the Helm template examples/global-router/cdk/helm/dynamo-mocker/templates/dgd.yaml (raw {{- if ... }} directives at line 28). It failed on arm64 and amd64, and failed again on every auto-retry.

❌ dynamo-runtime / test / sequential cuda13.0, arm64: yaml.scanner.ScannerError in test_examples_default_to_v1beta1

Job: dynamo-runtime / test / sequential cuda13.0, arm64 · Failed step: Run CPU-only tests (parallelized) · Logs: gh run view --job 121349680865 -R ai-dynamo/dynamo --log-failed

manifest_path = PosixPath('/workspace/examples/global-router/cdk/helm/dynamo-mocker/templates/dgd.yaml')
...
FAILED deploy/utils/tests/test_convert_api_version.py::test_examples_default_to_v1beta1 - yaml.scanner.ScannerError: while scanning a simple key
  in "<unicode string>", line 28, column 5:
        {{- if .Values.runtimeVersionOve ...
        ^
could not find expected ':'
  in "<unicode string>", line 29, column 5:
        runtimeVersionOverride: {{ .Valu ...
        ^
== 1 failed, 5519 passed, 153 skipped, 2593 deselected in 1169.57s (0:19:29) ===

test_examples_default_to_v1beta1 collects manifests under examples/ and loads each with a YAML parser. It reached examples/global-router/cdk/helm/dynamo-mocker/templates/dgd.yaml, a Helm template with Go-template directives ({{- if .Values.runtimeVersionOverride }}), and that file is not valid YAML. The test failed again on all 3 auto-retries. The later "A pytest stage did not record a passing result" error is the stage check reporting this pre_merge_cpu failure.

❌ dynamo-runtime / test / sequential cuda13.0, amd64: same ScannerError in test_examples_default_to_v1beta1

Job: dynamo-runtime / test / sequential cuda13.0, amd64 · Failed step: Run CPU-only tests (parallelized) · Logs: gh run view --job 121349680892 -R ai-dynamo/dynamo --log-failed

FAILED deploy/utils/tests/test_convert_api_version.py::test_examples_default_to_v1beta1 - yaml.scanner.ScannerError: while scanning a simple key
  in "<unicode string>", line 28, column 5:
        {{- if .Values.runtimeVersionOve ...
        ^
could not find expected ':'

This is the same failure as the arm64 job: the same test fails on the same Helm template file.

For agents

{"pr": 17283, "run_id": 40252246414, "run_attempt": 1, "head_sha": "a1b7d5c12f5db091a8cbaed0461e7e45b407a40d",
 "failures": [
  {"job": "dynamo-runtime / test / sequential cuda13.0, arm64", "job_id": 121349680865, "failed_step": "Run CPU-only tests (parallelized)", "signature": "yaml.scanner.ScannerError parsing examples/global-router/cdk/helm/dynamo-mocker/templates/dgd.yaml (Helm template)", "tests": ["deploy/utils/tests/test_convert_api_version.py::test_examples_default_to_v1beta1"], "log_cmd": "gh run view --job 121349680865 -R ai-dynamo/dynamo --log-failed"},
  {"job": "dynamo-runtime / test / sequential cuda13.0, amd64", "job_id": 121349680892, "failed_step": "Run CPU-only tests (parallelized)", "signature": "yaml.scanner.ScannerError parsing examples/global-router/cdk/helm/dynamo-mocker/templates/dgd.yaml (Helm template)", "tests": ["deploy/utils/tests/test_convert_api_version.py::test_examples_default_to_v1beta1"], "log_cmd": "gh run view --job 121349680892 -R ai-dynamo/dynamo --log-failed"}
 ]}

_{Posted automatically by Devin for run 40252246414. Updated on every full-CI run of this PR.}

atchernych · 2026-10-26T18:54:08Z

Replaced by #17333, rebuilt for main on top of #17331. It now gives one 501 answer for every store instead of adding a setting. Closing this one, since it targets the global-router-v2 test branch.

…is designed POST /routers is RegisterRouter from the design. No store takes router registrations yet, so every call answers 501 with the reason. With MCS, router replicas need not register: the Kubernetes stores list them from the router Service's imported EndpointSlices. Otherwise, how a router proves it is a router, and what it sends, are not decided yet, and trusting every caller would let a fake router entry receive every pool's state. This replaces #17283, which targeted the global-router-v2 test branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Anna Tchernych <atchernych@nvidia.com>

atchernych requested a review from a team as a code owner October 25, 2026 23:34

pull-request-size Bot added the size/L label Oct 25, 2026

github-actions Bot added the feat label Oct 25, 2026

dynamo-review-agent Bot reviewed Oct 25, 2026

View reviewed changes

pull-request-size Bot added size/XL and removed size/L labels Oct 26, 2026

dynamo-review-agent Bot reviewed Oct 26, 2026

View reviewed changes

pull-request-size Bot added size/M and removed size/XL labels Oct 26, 2026

atchernych changed the title ~~feat(global-router): let router replicas register with the discovery service~~ feat(global-router): reserve POST /routers until router registration is designed Oct 26, 2026

dynamo-review-agent Bot reviewed Oct 26, 2026

View reviewed changes

atchernych mentioned this pull request Oct 26, 2026

feat(global-router): reserve POST /routers until router registration is designed #17333

Merged

atchernych closed this Oct 26, 2026

Conversation

atchernych commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Uh oh!

dynamo-review-agent Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynamo-review-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot commented Oct 26, 2026

❌ Dynamo PR CI failed — run 40252246414 (attempt 1) on a1b7d5c12f

Failure details

Uh oh!

atchernych commented Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

atchernych commented Oct 25, 2026 •

edited

Loading

❌ Dynamo PR CI failed — run 40252246414 (attempt 1) on `a1b7d5c12f`