fix: support ipv6 pod backend URLs#1071
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a PodEndpointURL utility function to standardize URL construction for pod endpoints, specifically improving support for IPv6 addresses. The sglang and vllm backends were updated to use this utility, and unit tests were added to cover various IP formats. Feedback suggests adding documentation for the exported function and ensuring the path parameter is correctly handled to prevent malformed URLs.
| return httpClient | ||
| } | ||
|
|
||
| func PodEndpointURL(podIP string, port uint32, path string) string { |
There was a problem hiding this comment.
Exported functions should have a doc comment explaining their purpose and behavior. This is a standard Go practice to maintain code readability and documentation quality, especially for shared utility functions.
| func PodEndpointURL(podIP string, port uint32, path string) string { | |
| // PodEndpointURL constructs a URL for a pod endpoint, correctly handling IPv6 addresses by wrapping them in brackets. | |
| func PodEndpointURL(podIP string, port uint32, path string) string { |
| } | ||
|
|
||
| func PodEndpointURL(podIP string, port uint32, path string) string { | ||
| return "http://" + net.JoinHostPort(podIP, strconv.FormatUint(uint64(port), 10)) + path |
There was a problem hiding this comment.
The current implementation assumes that the path argument always starts with a leading slash. If it doesn't, the resulting URL will be malformed (e.g., http://host:portpath). It is safer to ensure the slash is present or use a more robust URL construction method.
if len(path) > 0 && path[0] != '/' {
path = "/" + path
}
return "http://" + net.JoinHostPort(podIP, strconv.FormatUint(uint64(port), 10)) + path
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
Can you please also update the router.go?
I have updated it and that |
| return resp, nil | ||
| } | ||
|
|
||
| func podHostPort(podIP string, port int32) string { |
There was a problem hiding this comment.
I am thinking this function is redudant
There was a problem hiding this comment.
Fair point, it was only wrapping net.JoinHostPort. I removed the helper and now use net.JoinHostPort directly at the call sites.
I’ll also squash the commits before the next push.
|
@pm-ju You can and should squash the commits |
pm-ju
force-pushed
the
fix-router-backend-ipv6-pod-urls
branch
from
e5e5006 to
2965d39
Compare
volcano-sh-bot
added
size/XL
and removed
do-not-merge/contains-merge-commits
size/L
labels
Signed-off-by: pm-ju <pmdevops29@gmail.com>
pm-ju
force-pushed
the
fix-router-backend-ipv6-pod-urls
branch
from
2965d39 to
dc877ce
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind bug
What this PR does / why we need it:
vLLM and SGLang backend code built pod URLs with
fmt.Sprintf("http://%s:%d/...", podIP, port). This works for IPv4, but creates invalid URLs for IPv6 pod IPs because IPv6 literals must be bracketed.This PR adds a shared
PodEndpointURLhelper usingnet.JoinHostPortand uses it for:This keeps router backend metric/model fetching working in IPv6 or dual-stack Kubernetes clusters.
Which issue(s) this PR fixes:
None
Verification: