Repository navigation
[Bugfix][TransferEngine] Enable TCP connection pooling by default - #4464
guptaishaan wants to merge 1 commit into
Conversation
One-shot TCP connections can exhaust the ephemeral port space when several instances share one node and destination IP. Enable the existing bounded connection lanes by default while preserving the explicit disable setting. Add a regression test that verifies requests reuse one connection when the pool setting is absent.
|
这是一封自动回复邮件已收到您的邮件,我将尽快处理,祝您生活愉快
|
he-yufeng
left a comment
There was a problem hiding this comment.
The behavioral half of this PR is already on main: 36fdd89 (#3569) flipped enable_connection_pool_ to true, so against current main this diff's tcp_transport.h change is a no-op and the PR will rebase down to the test updates.
Those test updates are worth keeping, though, and I'd suggest reframing the PR around them: OneLaneReusesCleanSocketInFifoOrder currently forces pooling via the env var, which stops being a meaningful exercise of the path users actually get now that pooling is the default. Pointing the test at the default configuration (with ScopedEnvVar gaining the unset support you added) is the right hardening. One nit on the rebase: keep the env override coverage somewhere, since MC_TCP_ENABLE_CONNECTION_POOL=0 is now the only way to get the unpooled behavior and that knob deserves a test that pins it.
he-yufeng
left a comment
There was a problem hiding this comment.
Thanks for picking up #3540. One heads-up on the base: the default-on flip already shipped in #3569 (merged Aug 24). Current main reads bool enable_connection_pool_ = true;, so the header hunk here rebases to a no-op. The PR looks like it was cut from a pre-flip tree.
The part that still adds value against main is the test hardening:
ScopedEnvVarlearning tounsetenvonnullptrDefaultConnectionPoolReusesCleanSocketpinning the default-on contract explicitly, so the reuse behavior is exercised even if whoever runs the suite hasMC_TCP_ENABLE_CONNECTION_POOLset in their shell
Worth rebasing onto current main and keeping just that part. Related: #4473 updates the docs for the same flip (flag is now an opt-out, 0/false/no).
|
Please fix conflicts before merging |
|
这是一封自动回复邮件已收到您的邮件,我将尽快处理,祝您生活愉快
|
Description
Fixes #3540.
TCP connection lanes were disabled unless
MC_TCP_ENABLE_CONNECTION_POOLwas set. Same-IP concurrent workloads therefore opened one connection per transfer and could exhaust the per-destination ephemeral port space.Enable the existing bounded connection lanes by default.
MC_TCP_ENABLE_CONNECTION_POOL=0remains an explicit opt-out.Thanks to @absoluteword for reporting the failure.
Module
mooncake-transfer-engine)mooncake-store)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Reproduced
connect: Cannot assign requested addressby submitting 30,000 outstanding Mooncake TCP writes to the node's non-loopback IP. The permanent regression test fails before the fix with eight accepted sockets and passes after the fix with one reused socket.Tested on Linux with 8 NVIDIA A40 GPUs present. The Transfer Engine build and tests were CPU-only.
The exact vLLM DP deployment, Docker setup, RTX PRO 6000 Blackwell, and B300 were not available. The legacy
tcp_transport_testalso requires a metadata service not present in this test build.Test commands:
Test results:
Checklist
./scripts/code_format.shAI Assistance Disclosure
OpenAI Codex reproduced the failure, traced the TCP connection path, implemented the change, and ran the tests. The human submitter must review every changed line and be able to defend the change.