[estroz]

Overview

Slice 4 of 8 of #17615, which stays open as the complete, CI-validated source of truth for the Multi-Cluster Services (KEP-1645) port. Review this PR for the public Go client of the registry and the in-process fake used by consumer tests; the other slices are listed under Issues and Ongoing Work.

pkg/client is the only way mcs-controller talks to the registry: it dials a list of registry URLs with failover, runs the watch loop with resume from the last revision and resync, and exports dynamo_mcs_client_* metrics. pkg/fakeregistry is a complete in-memory implementation of the dynamo.mcs.v1 service for tests, kept in step with the real server's error reasons and watch event shape. Both are public packages other modules may import, so their API is meant to stay additive.

Key Changes

• Add pkg/client: options (URLs, TLS, bearer token, timeouts, user agent global-discovery-mcs-client-go), connection failover, typed wrappers for every RPC, the watch loop with revision resume and periodic resync, dynamo_mcs_client_* metrics, and README.md.
• Add pkg/fakeregistry: gRPC server over bufconn or a loopback listener implementing clusters, leases, exports, imports and watches, with helpers to inject failures and read state in tests.
• Client tests run against the fake over loopback and bufconn with the race detector.

Depends on (not in this PR)

Links point at the parent branch estroz/mcs-port-module (#17615), where every piece is present together.

• Slice 01 for the build (deploy/global-discovery-mcs/go.mod, deploy/global-discovery-mcs/Makefile).
• Slice 02 for the generated API and constants: deploy/global-discovery-mcs/api.
• pkg/fakeregistry imports the record types of deploy/global-discovery-mcs/internal/registry from slice 03.
• The consumer of this client is mcs-controller, slice 05 (deploy/global-discovery-mcs/internal/mcs); the real server whose semantics the fake mirrors is slice 03 (deploy/global-discovery-mcs/internal/api).

Validation

Every slice of #17615 was validated together at 0fd4c96b on the parent branch, from a fresh checkout of deploy/global-discovery-mcs:

• make lint 0 issues; make test (unit plus envtest, -race, coverage gate 85 on ./internal/...) 95.7 percent; make test-integration against amazon/dynamodb-local all pass; make proto-check and make check no drift; Docker linter and tester stages exit 0.
• Chart: make -C deploy/helm/charts/global-discovery-mcs lint test, 57 unittest cases; server-side dry run and a real install, upgrade and uninstall.
• CI metadata: build_codeowners.py --strict 100 percent owned, emit_codeowners.py no diff, test-filters.js 87/87.
• Docs: docs_lint.py 0 errors, fern check 0 errors, fern docs broken-links passes.
• End-to-end on three kind clusters (make e2e E2E_PHASES="1 2 3"): 99 passed, 0 failed, 1 known gap (pool region equals cluster id until feat(global-router): take a pool's region from its EndpointSlice label in the mcs backend #17604 lands).

Issues and Ongoing Work

• Relates to Ship a standard Multi-Cluster Services implementation for the Global Router #17601
• Part of feat(global-router): add the Multi-Cluster Services registry and controller #17615 (the complete port, kept open as the source of truth). The other slices:
• feat(global-router): scaffold the global-discovery-mcs module and its CI #17629: slice 1, module scaffolding, Makefile, Dockerfile, README, CI wiring and code ownership
• feat(global-router): add the Multi-Cluster Services registry API contract #17630: slice 2, the dynamo.mcs.v1 proto contract, generated code and the label and condition constants
• feat(global-router): add mcs-registry, its stores and mcs-admin #17631: slice 3, mcs-registry (view, merge rules, memory and DynamoDB stores, handlers, server) and mcs-admin
• feat(global-router): add mcs-controller #17633: slice 5, mcs-controller (registration and lease, export reconciler, auto-export, importer, CRDs, RBAC)
• feat(global-router): add the global-discovery-mcs Helm chart #17634: slice 6, the Helm chart
• docs(global-router): document Multi-Cluster Services for the Global Router #17635: slice 7, the docs-site pages
• test(global-router): add the kind end-to-end test for global-discovery-mcs #17636: slice 8, the kind end-to-end test
• The /ok to test <sha> CI run and the module pre-merge jobs only pass once the slices they need are merged; see Depends on.

Pull Request Checklist

• I used Dynamo's /pr-checklist skill.

🤖 Generated with Claude Code

[devin-ai-integration]

❌ Dynamo PR CI failed — run 41131839133 (attempt 1) on 882f67dae3

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

Other	Jobs
changed-files	❌ 1

Failure details

1 job failed: changed-files rejects 29 new Go files under deploy/global-discovery-mcs/pkg/ that no CI filter covers, so all other jobs were skipped.

❌ changed-files: files not covered by any CI filter

Job: changed-files · Failed step: Check for changes · Logs: gh run view --job 124324431335 -R ai-dynamo/dynamo --log-failed

##[error]The following files are not covered by any CI filter:
"deploy/global-discovery-mcs/pkg/client/client.go"
"deploy/global-discovery-mcs/pkg/client/client_test.go"
"deploy/global-discovery-mcs/pkg/client/config.go"
...
"deploy/global-discovery-mcs/pkg/fakeregistry/views.go"
"deploy/global-discovery-mcs/pkg/fakeregistry/watch.go"
Add these paths to .github/filters.yaml. See .github/FILTERS.md.
##[error]Process completed with exit code 1.

The filter coverage check failed because 29 of the 30 changed files (deploy/global-discovery-mcs/pkg/client/*.go and deploy/global-discovery-mcs/pkg/fakeregistry/*.go; only the client README.md is covered) match no path filter in .github/filters.yaml. Adding these paths to a filter (see .github/FILTERS.md) should unblock the downstream build and test jobs; this is not an infrastructure failure.

For agents

{"pr": 17632, "run_id": 41131839133, "run_attempt": 1, "head_sha": "882f67dae3a8745b9c4cdf14e0b6df4df03fa9f3", "failures": [{"job": "changed-files", "job_id": 124324431335, "failed_step": "Check for changes", "signature": "29 files under deploy/global-discovery-mcs/pkg/{client,fakeregistry}/ not covered by any CI filter in .github/filters.yaml", "tests": [], "log_cmd": "gh run view --job 124324431335 -R ai-dynamo/dynamo --log-failed"}]}

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

[sttts]

Detailed inline review feedback. Correctness findings, non-blocking suggestions, and coverage requests are identified separately in the comments.

[sttts]

[P2] Require registry readiness before failback

GetServerInfo establishes reachability rather than readiness to serve registry traffic

GetServerInfo returns metadata independently of store availability and view freshness. This probe can succeed while the same registry reports gRPC health NOT_SERVING.

Client uses serving secondary
             |
Primary listener becomes reachable
Primary store/view remains unhealthy
             |
GetServerInfo succeeds
             |
Client moves connection and restarts watches
             |
Working secondary is replaced by unhealthy primary

For supported direct regional URLs, transport connectivity does not establish application readiness. The server's shutdown guard does not turn this metadata call into a readiness check, and pick_first does not automatically avoid a transport-connected registry because its store is unhealthy.

A readiness-filtering external load balancer can mitigate this, but the client also supports direct endpoints.

Requested change

Check actual registry readiness before committing failback. Also define how a connected but persistently unhealthy endpoint is avoided.

Verify the chosen mechanism against the selected gRPC load-balancing policy rather than assuming transport readiness or a service-config field alone provides the desired behavior.

Regression coverage

Keep the primary listener reachable while its health is NOT_SERVING; keep the secondary serving. Verify that the client stays on the secondary until the primary becomes ready.

[estroz]

Agreed. The registry already exposes the gRPC health service and flips it to NOT_SERVING when its view goes stale, the store is down or it is draining, so the failback probe now uses Health/Check on each registry service and requires SERVING (Client.probe). We also mark a connected endpoint that answers a call or a watch with UNAVAILABLE as unhealthy for a backoff (at least ConnectTimeout, growing with consecutive failures) and leave it out of the address list the resolver pushes, so the client moves on at once rather than relying on pick_first; returnToPrimary skips a primary that is still marked. Tests: TestFailbackRequiresServing (primary reachable but NOT_SERVING, secondary serving; the client stays on the secondary until the primary reports ready, the fake registry gained SetServing) and TestUnavailableSkipsEndpoint (in 882f67d).

[sttts]

[P3] Align the fake's omitted-generation behavior with the API

Generation zero has different meanings in the fake and real server

The public API defines omitted/zero generation as server-assigned. The real handler assigns a generation, but the fake stores literal zero and rejects the next zero as stale.

Request                  Real server             Fake
PutExport(generation=0)   Assign generation N      Store generation 0
PutExport(generation=0)   Assign newer generation  STALE_GENERATION

The endpoint-replacement path has the same mismatch, including returning the request generation rather than an assigned value.

The controller currently supplies nonzero generations, so this is a test-parity issue rather than an established production controller failure. Nevertheless, consumers using this documented API mode cannot rely on the fake's behavior.

Requested change

Align generation assignment and response semantics for this mode, preferably through behavioral cases shared with the real implementation.

Regression coverage

Repeat omitted-generation export and endpoint writes. Both implementations should accept them, return assigned nonzero generations and advance those generations.

[estroz]

Confirmed; the fake diverged from the documented server-assigned mode. The fake now reuses the server's assignment (registry.NextGeneration, max(stored + 1, unix ms)) for both exports and endpoint replacement when the generation is omitted, and returns the assigned value (putExportLocked, putEndpointsLocked, ReplaceEndpoints); explicit generations behave as before. TestGenerationsMatchRegistry runs the same cases, two omitted writes accepted with nonzero increasing generations and explicit stale ones rejected, against both the fake and the registry handlers over the memory store (in 882f67d).