Repository navigation
fix(release): backport IPv6 fixes to 1.5.1 - #15430
jthomson04 wants to merge 3 commits into
Conversation
4d9de00 to
5a28947
Compare
This comment has been minimized.
This comment has been minimized.
|
|
||
| #[tokio::test] | ||
| async fn wildcard_bind_advertises_concrete_ipv4_with_bound_port() { | ||
| for host in ["0.0.0.0", "::ffff:0.0.0.0", " [::ffff:0.0.0.0] "] { |
There was a problem hiding this comment.
The three-host table reruns the same TcpStreamServer bind/register/connect path after resolve_host_or_interface has canonicalized each value to the same IPv4 wildcard advertisement. The retained 0.0.0.0 case protects TCP wildcard binding with the assigned port, while ip_resolver already covers IPv4-mapped wildcard spellings including bracketed whitespace, so the extra rows add no distinct TCP-server regression coverage.
🤖 AI Fix
Reduce this host table to the canonical 0.0.0.0 case and leave mapped-wildcard parsing to the resolver tests.
| async fn configured_ip_literals_bind_and_format_addresses() { | ||
| let ipv6_available = std::net::TcpListener::bind("[::1]:0").is_ok(); | ||
|
|
||
| for (host, expected_ip) in [ |
There was a problem hiding this comment.
This TCP integration test binds and registers a server for seven resolver spelling variants, but only the resulting concrete address affects the TcpStreamServer behavior under test. The existing IPv6 bind test protects bracketed IPv6 formatting, the wildcard test protects assigned-port advertisement, and ip_resolver covers whitespace, IPv4-mapped, and interface-alias resolution, so these extra rows duplicate lower-boundary coverage without distinguishing another TCP-server contract.
🤖 AI Fix
Consolidate this table to the minimal concrete bind cases needed at the TCP boundary, and rely on the resolver unit tests for spelling variants.
| fn direct_zmq_advertise_host_falls_back_to_ipv6_and_rejects_wildcards() { | ||
| let mut resolver = StubResolver::not_found(); | ||
| resolver.interfaces = vec![("eth0", "2001:db8::1".parse().unwrap())]; | ||
| assert_eq!( |
There was a problem hiding this comment.
The unset DYN_EVENT_PLANE_HOST IPv6 fallback assertion duplicates direct_zmq_automatic_host_selection, which already calls the same helper with the env var unset and verifies IPv6-only selection before adding an IPv4 candidate. The blank-env assertion in this test still protects the distinct empty-override path.
🤖 AI Fix
Remove the first unset-env fallback assertion from this test and keep the blank-env and wildcard-rejection checks.
| for host in [ | ||
| "0.0.0.0", | ||
| "::", | ||
| "[::]", |
There was a problem hiding this comment.
After resolve_host_or_interface returns, these bracketed and IPv4-mapped wildcard rows all exercise the same event-plane is_unspecified rejection already covered by retaining bare IPv4 and IPv6 wildcard cases. The spelling-specific parsing is protected in ip_resolver wildcard tests, so these rows multiply combinations without another event-plane observable contract.
🤖 AI Fix
Keep one IPv4 and one IPv6 wildcard rejection at the event-plane boundary and remove the bracketed/mapped wildcard rows.
| static LOCAL_IP_FOR_ADVERTISE: OnceLock<IpAddr> = OnceLock::new(); | ||
|
|
||
| /// Resolve the local IP for advertising endpoints, falling back to 127.0.0.1. | ||
| /// Read and normalize a host override from the environment. |
There was a problem hiding this comment.
This comment only paraphrases host_override_from_env and does not document what normalization means; the trimming and empty-value behavior are already explicit in the helper and tests.
🤖 AI Fix
Remove this redundant comment.
| Ok((!value.is_empty()).then(|| value.to_string())) | ||
| } | ||
|
|
||
| /// IP address operations used by the runtime. |
There was a problem hiding this comment.
This opening sentence merely restates the trait's name and methods. The following paragraph already preserves the useful reason for the abstraction: controllable resolution in tests.
🤖 AI Fix
Remove this opening sentence and retain the testability explanation below it.
| } | ||
| } | ||
|
|
||
| /// The system IP resolver. |
There was a problem hiding this comment.
The system IP resolver only expands the DefaultIpResolver identifier into a sentence and adds no contract or implementation constraint.
🤖 AI Fix
Remove this redundant doc comment.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approve at 5a28947ec671f2eb83c36e802bb1c8cbfdea2627. I found no P0, P1, or P2 defect. The three P3 items below do not block the merge.
- [P3] A snapshot restore does not apply
DYN_EVENT_PLANE_HOST.RESTORE_RUNTIME_ENV_NAMESincomponents/src/dynamo/common/snapshot/constants.py:27does not list it, so a restored worker keeps the value from the checkpoint.mainadded it in #11870. Please add it to the list. - [P3] On hosts with more than one IPv4 interface, the automatic address can change. Before this PR,
release/1.5.1used the source address of the default route. This branch uses the first interface that is up (lib/runtime/src/utils/ip_resolver.rs:230). Please name this change in the 1.5.1 release notes. - [P3] The new assert in
test_zmq_pubsub_basicchecks the string that the test built, so it cannot fail (lib/runtime/src/transports/event_plane/zmq_transport.rs:727). Please bindtcp://127.0.0.1:0and assert on the returned endpoint, asmaindoes since #14566.
The seven dynamo-review-agent comments ask to remove test rows and doc comments. That code is identical to main at #15383, and none of the comments names a defect. I suggest that this branch stays identical to main, and that such changes go to main first.
What I measured, at this head and at the base `a327f1ac1180`, on Linux x86_64 with Rust 1.96.1.
Cherry-picks. I replayed each merged commit on its parent with git merge-tree. All hand edits are in files that conflicted, except two in the third commit: the regenerated Helm README, and a Context as _ import that .with_context needs on this branch. The other differences are listed adaptations, or main code that this branch does not have. Examples are QUIC, DynamicZmqSubSocket, the ZMQ I/O thread setting, and the EMFILE hints. The first commit also brings #11870 behavior: DYN_TCP_RESPONSE_STREAM_HOST now accepts IP literals and wildcards, not only interface names.
Tests, run serially as in the PR description. The head passed 152 tests, with the 2 NATS tests ignored. The base passed 118. Each difference is a new test: resolver +13, transports +13, TCP server +4, system status +1, LLM HTTP +1, LLM ZMQ +2, router ZMQ +1. No IPv6 case was skipped, because ::1 is on lo. Whole-library runs at the head: runtime 686 passed (642 in parallel, then the 44 TCP server tests serially), LLM 2406 passed, router 1043 passed. On the base, one LLM run failed with Address already in use in direct_zmq_multi_node_replacement_isolated_by_global_rank. The head lets ZMQ choose the port, so that port race does not apply.
cargo check --locked --workspace --all-targets passed at the head.
Mutations. I broke one part at a time, ran the matching tests, and restored the file:
| Commit | Change I made | Result |
|---|---|---|
| #13126 | Remove the IPv6 fallback from automatic selection | 4 resolver tests fail |
| #13126 | Advertise the bind address of the TCP server | wildcard_bind_advertises_concrete_ipv4_with_bound_port fails |
| #13126 | Advertise the bound system status address | advertised_address_stays_in_the_bound_family fails |
| #13126 | Format the TCP transport without IPv6 brackets | tcp_transport_uses_concrete_ipv4_and_bracketed_ipv6_addresses fails |
| #13126 | Advertise the bind address in manager.rs |
no test fails (686 runtime tests pass) |
| #15324 | Bind tcp://0.0.0.0:0 for an IPv6 host |
direct_zmq_publisher_serves_advertised_endpoint::case_2 fails |
| #15324 | Return the requested :* endpoint from ZmqPubTransport::bind |
9 transports tests fail |
| #15383 | Turn off ZMQ_IPV6 for bracketed endpoints |
4 transports tests fail |
| #15383 | Select only IPv4 for the event host | 2 event plane tests fail |
| #15383 | Remove the bracket handling of the HTTP host | test_listen_socket_addr_accepts_ip_literals fails |
| #15383 | Turn off ZMQ_IPV6 in the router and LLM ZMQ helpers |
bracketed_ipv6_endpoints_connect fails in each crate |
No test covers the DYN_TCP_RPC_HOST wildcard advertisement in manager.rs. No test covers the IPv6 DNS fallback of the vLLM leader, because the Go test only finds the function name. CI ran the new SGLang test in all 4 cases on this head.
Locks. cargo tree --locked passes for all four manifests at the base and at the head. No package version changes. In the root lock, the nix features stay the same. In the kvbm and Python locks, nix gains net, socket, and memoffset. The examples lock gains nix and memoffset. The diff changes no pyproject.toml.
Helm README. helm-docs 1.14.2 does not change the head README. At the base, it rewrites the same three lines, and the operator job on release/1.5.1 failed on that diff.
Snapshot restore. With a restore context that sets both variables, the head applies DYN_SYSTEM_HOST and logs Ignoring unsupported snapshot restore env DYN_EVENT_PLANE_HOST. main applies both.
Address selection. I ran local_ip_for_advertise() from the base and from the head in one container, with eth0 on one network and eth1 on a second network. With the default route on eth1, the base returned 172.30.102.2 and the head returned 172.30.101.2. With the default route on eth0, both returned 172.30.101.2.
Test fix. The main version of test_zmq_pubsub_basic passes at the head. If bind returns the requested :* endpoint, that version fails, but the current version passes.
Not run: the IPv6-only container check, because the registry login on the test host expired. The two-pod vLLM GPU test is also not run, as the PR description says.
Not from this PR: in parallel runs, the TCP server tests share DYN_TCP_TLS_* through the process environment, and some of them fail at random. The base fails in the same way. The four new server tests are exposed to the same race.
Summary
Backport #13126, #15324, and #15383 to
release/1.5.1in three DCO-signed commits with upstream cherry-pick references. This fixes the IPv6-only deployment reported in #7619: workers advertise reachable metadata and transport addresses, models register, inference completes, and KV events reach the frontend. IPv6-only deployments still requireDYN_HTTP_HOST=::on the frontend andDYN_SYSTEM_HOST=::on workers; listener defaults stay unchanged.Includes the full merged fixes: unified IP resolution, typed advertised addresses, direct ZMQ IPv6 listeners and assigned ports, endpoint-specific ZMQ IPv6 settings that preserve IPv4 hostname connections, HTTP host parsing, system-status advertisement, SGLang local KV-event connections, and vLLM leader DNS fallback.
Release adaptations:
Preserve release TCP TLS, cancellation, accept backoff, and port parsing, plus its existing HTTP listener implementation.
Include the minimum event-host override and native ZMQ dynamic-port prerequisites. Use existing multi-endpoint subscriber APIs and adapt the mixed-family tests to them.
Preserve package versions and dependency pins. The resolver adds the required Unix
nixfeatures; the examples lockfile also gainsnixandmemoffset.Keep the relay address correction in the runtime reference because the upstream relay page is absent from this branch. Omit main-only QUIC and ZMQ context-setting documentation.
Regenerate the platform Helm README for the existing 1.5.1 chart values. The release baseline still contained three generated 1.5.0 values and failed the same operator generated-file check.
Validation
Validated on the release backport, based on
a327f1ac1180:go test ./internal/dynamo/...passed with Go 1.26.6. The extracted vLLM leader DNS helper also resolved IPv6-only and IPv4 container names.standalone-selection,metrics. Withoutmetrics, router Clippy reports an unusednew_with_indexer_metrics_and_cancel_token; reproduced the same failure on the unchanged release baseline.git diff --checkpassed. Docs lint: zero errors and eight existing warnings. Fern check: zero errors and three existing warnings. Fern broken-link check passed.Container acceptance used a frontend and two mocker workers with Qwen3-0.6B metadata, file discovery, default direct ZMQ events, and a separate client container. Only workers had model files, so frontend registration required worker metadata URLs. Each positive case passed remote worker health, model registration, four completions, applied stored KV events, and nonzero cache hits on repeated prompts.
::/v1/modelsempty locally, loopback warning emitted::Validation gap: the original two-pod vLLM/GPU test was not run. Kubernetes access required renewed Teleport authentication, and no accessible IPv6-only Kubernetes context was identified. The mocker checks do not replace this test.
Reproduction scripts and logs are retained in the release worktree under
.validation/; they are not part of the patch. No PR merge is included.