Repository navigation
feat(runtime): default event plane to ZMQ for all backends - #10902
Conversation
Previously the event-plane default was backend-driven: local backends (file/mem) defaulted to ZMQ while distributed backends (etcd/kubernetes) defaulted to NATS. Make ZMQ the default for every discovery backend; NATS remains an explicit opt-in via DYN_EVENT_PLANE=nats. Since the request plane already defaults to TCP, a standard etcd deployment now needs no NATS connection by default. - discovery::EventTransportKind: #[default] and from_env*() -> Zmq - DiscoveryBackend::resolve_event_transport_kind: unset/empty/invalid -> Zmq - durable KV events stay NATS-only; the subscriber now bails with an actionable message (set DYN_EVENT_PLANE=nats or drop --durable-kv-events) instead of an opaque error - tests: pin DYN_EVENT_PLANE=nats when durable_kv_events=True so the JetStream path keeps working under the new ZMQ default - docs/CLI help updated to reflect ZMQ-default / NATS-opt-in Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughSwitches the default ChangesZMQ as universal default event plane
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/conftest.py`:
- Line 1172: Move the function-local import os statement from line 1172 to the
top of the conftest.py file with the other imports. Then, replace all direct
os.environ mutation assignments (lines 1188-1209) with monkeypatch.setenv()
calls. Ensure the fixture receives monkeypatch as a parameter so it can call
monkeypatch.setenv(key, value) instead of os.environ[key] = value for each
environment variable being set.
🪄 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: 2a06e340-4619-46ca-bc0a-79e6cbe8a4ee
📒 Files selected for processing (8)
components/src/dynamo/common/configuration/groups/runtime_args.pycomponents/src/dynamo/frontend/frontend_args.pydocs/components/router/router-guide.mddocs/design-docs/event-plane.mdlib/llm/src/kv_router/indexer/recovery/subscriber.rslib/runtime/src/discovery/mod.rslib/runtime/src/distributed.rstests/conftest.py
This comment has been minimized.
This comment has been minimized.
The router e2e tests (mocker/vLLM/SGLang/TRT-LLM) never set DYN_EVENT_PLANE and relied on the old per-backend default (etcd -> NATS). With ZMQ now the default, the in-process harness delivers KV events between a separate engine subprocess and an in-process router over a naive direct ZMQ PUB/SUB path with no broker, which drops events published before the subscriber connects (slow-joiner). The router then sees zero KV blocks and the decision/overload/ sync assertions fail. Pin DYN_EVENT_PLANE=nats in runtime_services_dynamic_ports' etcd branch (where NATS is already started), reproducing the pre-existing etcd->NATS default for the whole integration suite. file/mem backends stay on the ZMQ default and explicit event_plane="zmq" overrides still win. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on the etcd branch of runtime_services_dynamic_ports: - Drop the redundant function-local `import os` (os is already imported at module top). - Replace the manual os.environ save/set/restore for NATS_SERVER, ETCD_ENDPOINTS, and DYN_EVENT_PLANE with monkeypatch.setenv(), which auto-restores at teardown (including any values set by the session-scoped fixture) and removes the boilerplate. monkeypatch is function-scoped like this fixture, so it tears down after the fixture's server context managers exit; env vars are restored to their prior (session) values. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI on the ZMQ default flagged 12 failures, all in test_router_e2e_with_mockers.py (test_router_decisions, test_indexers_sync[nats_core], the overload_503 tests, and test_router_decisions_disagg_round_robin_prefill_dp_rank). All GPU router e2e tests (vLLM/SGLang) passed on ZMQ. Root cause: the mock engine publishes KV events instantly, before the in-process router's ZMQ subscription connects (ZMQ slow-joiner), so the router sees zero events. Real engines start slowly enough to avoid this, which is why their router e2e tests pass on ZMQ and cover the new default. Add a mocker-scoped autouse fixture that pins DYN_EVENT_PLANE=nats for etcd-backed mocker tests. file-backed variants keep the ZMQ default; an explicit DYN_EVENT_PLANE (e.g. durable_kv_events) is left untouched. Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
black 23.1.0 requires two blank lines between the new autouse fixture and the module-level pytestmark. No behavior change. Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the mocker NATS pin with a short ZMQ subscription warm-up so the mocker tests also exercise the (now default) ZMQ event plane. Root cause was a ZMQ slow-joiner: the mock engine publishes KV events before the router's event-plane SUB has connected, so the router missed them and the routing assertions failed (real engines start slowly enough to avoid this, which is why the vLLM/SGLang/TRT-LLM router e2e tests already pass on ZMQ). _zmq_event_plane_warmup() sleeps briefly (only when not on NATS) to let the SUB attach before the test starts sending. Called from _create_kv_router_with_timeout (covers the in-process KvRouter tests) and the two overload_503 tests (frontend HTTP path). The mocker-scoped NATS autouse fixture is removed. durable_kv_events still pins NATS (JetStream is NATS-only); file-backed variants keep the ZMQ default. Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…inning NATS" This reverts commit 6ef420d. The ZMQ warm-up did not fix the mocker failures: the real root cause is that a worker running multiple dp_ranks in one process registers one ZMQ EventChannel per publisher, but they collide on a per-process discovery key, so the router discovers only one and misses the other dp_ranks' KV events (a warm-up of any length can't help — verified up to 10s). Restore the mocker NATS pin so this PR (ZMQ default) stays green, and defer the multi-publisher-per-process discovery-key fix to a follow-up (candidates: PR #10972 unique-key, or the share-publishers-across-dp-ranks approach). Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Overview
Make ZMQ the default event plane for every discovery backend. Previously the default was backend-driven — local backends (
file/mem) used ZMQ while distributed backends (etcd/kubernetes) used NATS. Now ZMQ is the default everywhere; NATS remains a first-class opt-in viaDYN_EVENT_PLANE=nats.Since the request plane already defaults to TCP, a standard
etcddeployment now opens no NATS connection by default — NATS is only connected when explicitly requested (DYN_EVENT_PLANE=nats,NATS_SERVERset,DYN_REQUEST_PLANE=nats, durable KV events, or the NATS audit sink).Changes
discovery::EventTransportKind:#[default]andfrom_env()/from_env_or_default()now resolve toZmqwhenDYN_EVENT_PLANEis unset/empty.DiscoveryBackend::resolve_event_transport_kind(authoritative resolver): unset/empty/invalid →Zmqunconditionally (dropped the backend-driven branch).DYN_EVENT_PLANE=natsor drop--durable-kv-events.tests/conftest.pypinsDYN_EVENT_PLANE=natswhendurable_kv_events=Trueso the durable/JetStream path keeps working under the new default.CLAUDE.md, and the--event-planehelp text updated to describe ZMQ-default / NATS-opt-in.Compatibility / blast radius
DYN_EVENT_PLANE=natsrestores prior behavior.Testing
cargo test --locked -p dynamo-runtime --lib— 395 passed (incl. ZMQ transport tests)cargo clippy --locked --no-deps -p dynamo-runtime -p dynamo-llm— cleandynamo-runtime+dynamo-llm— green🤖 Generated with Claude Code
Summary by CodeRabbit
Configuration Changes
Documentation & Help Text