fix(sidecar): rank-endpoint P2P pulls and wide-EP port-base compensation by nilig · Pull Request #2234 · llm-d/llm-d-router

nilig · 2026-07-30T07:53:21Z

What type of PR is this?
/kind bug
/kind documentation

What this PR does / why we need it:

Two changes that make the P2P source pull work on multi-rank pods:

The self-pull guards compared only hosts, so two DP ranks on the same pod (10.0.6.107:8003 vs 10.0.6.107:8007) were treated as one engine and the pull was skipped exactly where it is needed - addP2PPullToPrefill on the prefill leg, and decodeWithP2PSource on the decoder-only path (which collapsed identity to POD_IP). Both guards now compare full serving endpoints through a scheme-agnostic normalizeEndpoint: a same-pod cross-rank source is a valid peer and injects, while the selected rank's own endpoint still skips. The injected remote_kv_source params derive from the normalized endpoint, so scheme-qualified sources produce a bare remote_host and the rank-correct remote_port.
docs/disaggregation.md documented multi-pod DP groups (LWS wide-EP) as not covered. vLLM binds the P2P and KV-events listeners at configured base + global data_parallel_index, while the router addresses pod IP + pod-local rank; the docs now state the compensation contract that reconciles the two - each pod subtracts its global start rank from both configured bases (P2P_BASE=$((7777 - START_RANK)), KV_EVENTS_BASE=$((5557 - START_RANK))) - with the LWS formulas and the resulting per-pod port ranges. The base-port formulas themselves belong in deployment sources (guides), not router code.

Which issue(s) this PR fixes:
Fixes #2227

Test plan:

Prefill and decoder-only guards skip same-endpoint pulls and inject remote_kv_source for same-pod different-rank sources; scheme-qualified sources produce the correct remote_host and rank-specific remote_port (ginkgo, -race)
Live on a GLM-5.2 wide-EP cell with compensated bases: every pod binds P2P 7777-7784 / KV events 5557-5564 / serving 8000-8007; a same-pod cross-rank pull (global rank 11 -> 14) completed end to end - source accept on the rank-correct port, one consumer load of tokens x 92.6 KB/token, HTTP 200 in 2.9 s

Release note:

NONE

…4-arm corrected record The c32 load-spill benchmark (precise w1/queue w3, fresh 70K prefixes) reproduces Maroon's result on independently built llm-d#2233/llm-d#2234 images: 7.855 -> 2.557 s mean TTFT, 3.80 -> 10.10 req/s, 576/576 ok. Designated for the guide and blog. The weka 4-arm campaign is recorded with its adversarial-review corrections; surviving claims are approx+P2P crash -> clean and no-regression on the approx pair. Signed-off-by: nilig <nili.ifergan@gmail.com>

elevran

Verified the fix locally - checked out the branch and ran the full pkg/sidecar/proxy suite plus a couple of scratch tests for edge cases; all pass. Good, minimal fix that matches the issue. Two minor notes below, neither blocking.

elevran · 2026-07-30T11:58:27Z

 func (s *Server) addP2PPullToPrefill(prefillKVParams map[string]any, kvCacheSource, prefillPodHostPort string) {
-	if kvCacheSource != "" && extractHost(kvCacheSource) != extractHost(prefillPodHostPort) {
+	source, _ := strings.CutPrefix(kvCacheSource, "http://")
+	prefiller, _ := strings.CutPrefix(prefillPodHostPort, "http://")


nit:
only http:// is stripped, Not an issue today since header producers (disagg_headers_handler.go, disagg_profile_handler.go, p2psource/producer.go) always emit plain ip:port via net.JoinHostPort, but if that ever changes the same-endpoint guard would silently stop matching. Consider reusing extractHost for the comparison instead of a local CutPrefix, for consistency with the rest of the file.

extractHost drops the port, so reusing it for this comparison would make 10.0.6.107:8003 and 10.0.6.107:8007 compare equal again - the exact bug this PR fixes. The guard needs the full serving endpoint. Every producer of both headers emits plain host:port via net.JoinHostPort (disagg_headers_handler.go, p2psource/producer.go), so the http:// trim keeps the sidecar's existing tolerance without discarding the rank-bearing port. I'd keep the comparison as is.

I see, thanks.
My concern was that stripping just http would fail if using TLS on the communication or any another protocol. Right now either works since everything is just ip:port without a protocol spec but this is error prone if/when we add a protocol specification to the values.
If you really care just for the host and port, you can use url.Parse() to handle the protocol, then net.SplitHostPort() to isolate the server and the port.

Makes sense. The comparison has to keep the full host:port - the port identifies the rank - so I'll add a small normalization helper: parse a scheme-qualified URL when one is present (url.Parse handles https:// and anything else), fall back to treating the value as plain host:port, and compare the normalized endpoints. The same-host/different-port regression case stays in the tests, plus a scheme-variant case. Would that address your concern?

that works, thanks

Done - normalizeEndpoint parses a scheme-qualified URL when one is present and falls back to the raw host:port; both self-pull guards (prefill leg and decoder-only path) compare through it, and the injected params derive from the normalized endpoint as of d7e63d29.

elevran · 2026-07-30T11:58:27Z

 })
+
+var _ = Describe("addP2PPullToPrefill", func() {
+	It("injects a pull when source and prefiller are different ranks on the same pod", func() {


nit: this covers the cross-rank injection case, but there is no explicit unit test asserting the same-rank/same-endpoint case still skips. The test plan describes it as covered, but the only place it is implicitly exercised is the pre-existing full-flow test, which does not set a KV cache source at all. A short second It block mirroring this one but with equal host:port would close that gap.

Agreed - the cross-rank test proves injection but nothing pins the same-endpoint skip directly. Added an explicit equal host:port case asserting remote_kv_source is absent in bba473b.

The guard compared hosts only, so a source and a selected prefiller on the same pod were treated as one engine and the pull was skipped even when they were different DP ranks. Compare the full serving endpoints so same-pod cross-rank pulls inject remote_kv_source; a same-rank match still skips. Part of llm-d#2227. Co-authored-by: Maroon Ayoub <Maroonay@gmail.com> Signed-off-by: nilig <nili.ifergan@gmail.com>

vLLM offsets the P2P and KV-events listeners by the global DP rank while the router addresses pod IP plus pod-local rank, so multi-pod DP groups bind outside the addressed range unless each pod subtracts its global start rank from the configured bases. Document the compensation contract with the LWS formulas and the resulting per-pod port ranges. Fixes llm-d#2227. Co-authored-by: Maroon Ayoub <Maroonay@gmail.com> Signed-off-by: nilig <nili.ifergan@gmail.com>

Review follow-up: the cross-rank case proves injection; this asserts an equal host:port source is skipped, the behavior the guard exists for. Signed-off-by: nilig <nili.ifergan@gmail.com>

… guard Review follow-up: the guard's comparison now canonicalizes both values - scheme-qualified endpoints (any scheme) reduce to their host:port via url.Parse, bare values go through SplitHostPort/JoinHostPort - so a future protocol specification in the values cannot silently break the same-endpoint check. Covered by scheme-variant guard cases and a normalizeEndpoint table. Signed-off-by: nilig <nili.ifergan@gmail.com>

Signed-off-by: nilig <nili.ifergan@gmail.com>

nilig requested review from a team, roytman and shmuelk as code owners July 30, 2026 07:53

nilig requested review from liu-cong and vMaroon July 30, 2026 07:53

github-actions Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. labels Jul 30, 2026

roytman previously approved these changes Jul 30, 2026

View reviewed changes

nilig force-pushed the fix/p2p-sidecar-rank-endpoint branch from 54464c0 to a181797 Compare July 30, 2026 10:01

elevran enabled auto-merge (squash) July 30, 2026 10:44

elevran reviewed Jul 30, 2026

View reviewed changes

elevran self-assigned this Jul 30, 2026

nilig dismissed roytman’s stale review via bba473b July 30, 2026 13:06

github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 30, 2026

nilig and others added 4 commits July 30, 2026 22:01

test(sidecar): pin the same-endpoint skip in the P2P self-pull guard

1102d96

Review follow-up: the cross-rank case proves injection; this asserts an equal host:port source is skipped, the behavior the guard exists for. Signed-off-by: nilig <nili.ifergan@gmail.com>

nilig force-pushed the fix/p2p-sidecar-rank-endpoint branch from 50e0a1e to 3e6853b Compare July 30, 2026 19:02

fix(sidecar): apply endpoint guard to decoder-only P2P pulls

d7e63d2

Signed-off-by: nilig <nili.ifergan@gmail.com>

elevran approved these changes Jul 30, 2026

View reviewed changes

github-actions Bot removed kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. labels Jul 30, 2026

github-actions Bot added kind/bug Categorizes issue or PR as related to a bug. kind/documentation Categorizes issue or PR as related to documentation. labels Jul 30, 2026

elevran merged commit 1026043 into llm-d:main Jul 30, 2026
36 of 39 checks passed

This was referenced Jul 30, 2026

[Blog]: Peer-to-Peer KV Cache Sharing in llm-d llm-d/llm-d.github.io#424

Open

blog: move the KV, not the work - P2P cache sharing in llm-d llm-d/llm-d.github.io#458

Draft

Navigation Menu

Uh oh!

fix(sidecar): rank-endpoint P2P pulls and wide-EP port-base compensation - #2234

fix(sidecar): rank-endpoint P2P pulls and wide-EP port-base compensation#2234
elevran merged 5 commits into
llm-d:mainfrom
nilig:fix/p2p-sidecar-rank-endpoint

nilig commented Jul 30, 2026 •

edited

Loading

Uh oh!

elevran left a comment

Uh oh!

elevran Jul 30, 2026

Uh oh!

nilig Jul 30, 2026

Uh oh!

elevran Jul 30, 2026

Uh oh!

nilig Jul 30, 2026

Uh oh!

elevran Jul 30, 2026

Uh oh!

nilig Jul 30, 2026

Uh oh!

elevran Jul 30, 2026

Uh oh!

nilig Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nilig commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elevran left a comment

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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nilig commented Jul 30, 2026 •

edited

Loading