Repository navigation
Conversation
…e host at once (opt-in) A P2P peer is a host:port, and a pod with several DP ranks exposes one port per rank. When one rank's session died, loads to the pod's other ranks ran until their own load timeout (10 s or 30 s). New tier option group_peers_by_host: the reap also tears down the sessions to the dead peer's other ports on that host. The host then gets no new outbound sessions for 10 s, so those requests prefill locally at once. It is off by default, because independent instances can share a host. The load-timeout log line now prints the job id and the seconds since submit (vllm-project#56828). Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Documentation preview: https://vllm--64678.org.readthedocs.build/en/64678/ |
liranschour
left a comment
There was a problem hiding this comment.
Thanks @sohom-cs. I'm not sure host grouping is worth the complexity:
- A whole host down is already caught per port, in ~1s (sockets closed) or ~10s (heartbeat).
- The issue was a partly alive pod, and #63781/#64256/#64501 already shorten that wait. Does it still reproduce on main?
- It can misfire: an idle eviction or a single handshake failure takes down the whole host, it cascades to the healthy side, and co-located instances break it.
I prefer to close this PR.
|
@liranschour Thanks for looking at this. It still does, in a smaller form. On main (a11d3a1), after one port on a host is reaped, a load to another port on that host still runs to its own load timeout: 10 s for P2P, 30 s for PD (40 s before #63781). This PR's test shows it on main: Would you take a narrower change that drops host grouping and keeps only the job id and age on the load-timeout log line, or should I close it? |
|
I prefer to close this PR. Thanks |
Overview
Fixes 1 and 3 of #56828.
A P2P peer is identified by
host:port(vllm/v1/kv_offload/tiering/p2p/manager.py:128-133), and each DP rank binds its own port (manager.py:344-348). So a source pod with 8 DP ranks is 8 unrelated sessions on the consumer. When one rank's session dies,_reap_dead_sessionsreaps only thatpeer_id(manager.py:872,:878). The sessions to the pod's other ranks stay up, and so do their loads.With the new P2P tier option
group_peers_by_host, a dead peer also takes down the sessions to the other ports on its host, and that host gets no new outbound sessions for 10 s. The option is off by default. Separately, the load-timeout warning now prints the job id and the seconds since submit.Claims
group_peers_by_host: true, when a session dies, every other session to the same host is closed in the same reap, through the same teardown. Their in-flight loads fail at once. Their kv_request_ids go into_failed_req_ids, solookup()returns MISS (manager.py:427) and the requests prefill locally._get_or_create_sessionreturns None for any port on that host.on_new_requestthen marks the request failed (manager.py:473-475), so a new request naming that host prefills locally instead of opening a session.session/client.py:527now readsP2PSession <peer>: <req> round=0 job_id=7 timed out 10.0s after submit, sending abort. Since [KV Offload][P2P] Fail faster: per-mode load timeout, shorter abort-ack and lookup timeouts #63781 a load times out after 10 s (symmetric P2P) or 30 s (PD) (session/client.py:37,:40), so the old line did not say which applied. Bytes moved are not logged: the fetch protocol has no progress signal to read them from.Validation
Linux, CPU torch,
VLLM_TARGET_DEVICE=cpu, on maine49ea84e92:TestGroupPeersByHostruns the manager's real reap and session-open code with fake sessions: a dead session to10.0.0.1:7777, a live one to10.0.0.1:7778and a live one to10.0.0.2:7777, each with one load in flight.test_dead_peer_fails_loads_to_other_ports_on_its_host: after one reap, the loads to both10.0.0.1ports have failed, both request ids are failed, both peers are removed from the data plane,10.0.0.2:7777is still up, and the log line names the dead peer.test_new_request_to_quarantined_host_prefills_locally: a new PD request naming10.0.0.1:7779opens no connection, andlookup()returns MISS. After_HOST_QUARANTINE_Sthe host is dialed again.test_off_by_default: with the option off, the reap leaves10.0.0.1:7778up, and a new request to the host connects, as on main.With main's code and the new tests, the first two fail at the symptom:
e49ea84e92tests/v1/kv_offload/tiering/p2p/tests/v1/kv_offload/The 32 failures are
cpu/test_gpu_worker.py::test_transferand::test_transfer_multi_group, which need a GPU. They fail the same way on main.ruff checkandruff format --checkpass on the changed files.CI: the new tests run in the "V1 KV Offload" step (
.buildkite/test_areas/misc.yaml:129-137,pytest -v -s v1/kv_offload). Every step in.buildkite/test_areas/disaggregated.yamlis commented out on main ("Disabled until the KV connectors installation is locked", line 5), so no GPU P/D or P2P run covers this. I have no GPU and have not run it end to end. I have not measured the time to fallback on a real pod failure. The tests pin the manager's behavior, not latency.Details
The issue's timeline, mapped to main:
_reap_dead_sessionsreaps that session only (manager.py:872,:878).manager.py:757-759, called from:473), and the PD path returns HIT (manager.py:451).session/client.py:525). Since [KV Offload][P2P] Fail faster: per-mode load timeout, shorter abort-ack and lookup timeouts #63781 and [Bugfix][KV Offload][P2P] Fail a timed-out load at once, defer releasing its slots #64256 that is 10 s for symmetric P2P and 30 s for PD, down from 36-43 s. But every load to a sibling rank still pays it.Why the host, and why opt-in. Nothing in the P2P tier knows which ports make up one pod. The host part of
peer_idis the only key they share. It is right for one pod per IP, as in the issue's llm-d deployment. It is wrong when independent instances share a host:run_accuracy_test.shstarts every instance on127.0.0.1(line 103), and the open fault slice #64511 checks that pulls from a healthy co-located source keep hitting while the other source faults. Turned on there, this would fail the healthy source's loads for up to 10 s. So it is a tier option, documented inkv_offloading_usage.md. If you would rather have an explicit failure domain (for example a field next toremote_hostthat the router sets), I can do that instead.Quarantine. It is time-based only: 10 s, the ZMQ heartbeat timeout (
control/zmq.py:27). An inbound connection from the host does not lift it, because a sibling rank that is still control-alive could reconnect at once. It only blocks new outbound sessions. Inbound connections are accepted as before.Teardown. The siblings go through the same path as the dead session:
close()sends each oneDisconnectMsg(session/session.py:416), their running transfers are retained (#64250), andremove_remote_peerderegisters them. No reconnect backoff is scheduled for them, since they had no data-plane fault. The peer-down line names the dead peer and the number of loads failed.Fix 2 of the issue (abort a load with no data-path progress) is not here. It needs a progress signal in the fetch protocol.
Not a duplicate. No open or merged PR references #56828 or groups sessions by host. Closest merged: #63781 (per-mode load timeout), #64256 (fail a timed-out load at once), #64501 (prefill locally while a handshake is pending), #64251 (fault a peer whose source WRITE stalls). Each bounds one peer's own wait; none touches the sessions to its sibling ports. Closest open: #64060 (ZMQ reconnect handover), #61667 and its splits #64298, #64299, #64300 (consumer slots, buffered terminal messages, failed ids), #64006 (IPv6 literals;
_host_ofuses the same last-colon split as itshost:portparsing).AI assistance
AI-assisted; reviewed and tested by me.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.