fix: vllm disagg hang - ip address based nixl sidechannel #2108
Conversation
biswapanda
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
grahamking,
ishandhanani,
jthomson04,
kkranen,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughThe update changes how node identification is handled for port reservation and side channel configuration. Hostnames are now resolved to IP addresses using socket operations, and these IPs are used throughout the logic, including environment variable settings and logging. The function signature for side channel setup is also simplified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/backends/vllm/src/dynamo/vllm/args.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/bindings/python)
🔇 Additional comments (2)
components/backends/vllm/src/dynamo/vllm/args.py (2)
140-140: LGTM: Consistent use of IP address in ETCD key.The change from hostname to IP address in the ETCD key is consistent with the PR objective and improves reliability in multi-node deployments.
242-242: Function signature usage verified—no issues foundA search for all calls to
set_side_channel_host_and_portshows only the one invocation incomponents/backends/vllm/src/dynamo/vllm/args.py, and it already uses the new single-parameter signature. No callers remain with the old two-parameter form, so this change is safe to merge.
|
/ok to test d608ed2 |
| logger.warning( | ||
| f"Hostname '{node_name}' cannot be resolved, falling back to '127.0.0.1'" | ||
| ) | ||
| node_ip = "127.0.0.1" |
There was a problem hiding this comment.
Can can 127.0.0.1 be set as a const? and all of its references refer to the const?
There was a problem hiding this comment.
localhost ~ this was the default value used by vllm but its not right for multi-node / k8s setup
There was a problem hiding this comment.
I'll rather throw an error because 127.0.0.1 will not work in k8s anyway
There was a problem hiding this comment.
I think throwing error makes sense in this case, because if the code path reaches this point, IIUC, the node is not discoverable (/ reachable) from other nodes within the network, which should be a fatal failure for the deployment and the user should resolve that before deploying dynamo.
I think we should add option for user to provide IP explicitly to bypass the IP discovery code here, for the cases where the user is well-aware of their node orchestration and the IP is not trivially discoverable by our code above.
There was a problem hiding this comment.
What's the behavior of gethostbyname in Kubernetes? Will it give us the routable hostname?
An alternative is that on Kubernetes we use the injected variables to set the hostname/IP of the pod/etc.
There was a problem hiding this comment.
yes, socket.gethostbyname gives a routable IP in k8s.
it was a quick solution to get to functional state/unblock Hannah's perf sweep today.
I'll update to use explicit IP address set as env var set from k8s layer.
biswapanda
changed the title
biswapanda
force-pushed
the
bis/vllm-disagg-nixl-sidechannel
branch
from
d608ed2 to
1e6709d
Compare
|
accidentally closed this PR - reopening after a rebase on top of main. |
|
Another PR #2135 for planner was rebased on top of this and its merged to main now. |
Overview:
fix: use ip_address instead of hostname for nixl sidechannel
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit