Begin to use annotations in VllmConfig#404
Conversation
Signed-off-by: Jun Duan <jun.duan.phd@outlook.com>
Signed-off-by: Jun Duan <jun.duan.phd@outlook.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the dual-pods controller to rely on VllmConfig annotations (specifically inference-port) when determining an instance’s inference port, and centralizes the annotation keys as constants.
Changes:
- Introduces constants for
isc-nameandinference-portannotation keys. - Switches instance port detection from parsing
Optionsto readingVllmConfig.Annotations["inference-port"]. - Improves logging context when an instance’s port cannot be determined.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/controller/dual-pods/launcherclient.go | Adds shared constants for VllmConfig annotation keys. |
| pkg/controller/dual-pods/inference-server.go | Uses the annotation-based port lookup for launcher instance selection and uses the new constants when populating VllmConfig.Annotations. |
| port, err := strconv.ParseInt(value, 10, 32) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("parse annotations[%s] value %q: %w", VllmConfigInferencePortAnnotationKey, value, err) | ||
| } |
There was a problem hiding this comment.
getVLLMInstancePort parses the annotation value as a signed 32-bit integer but does not validate that it’s a valid TCP port (e.g., 1–65535). Since this value is later used to avoid port conflicts and to form wake-up endpoints, consider rejecting non-positive or out-of-range ports (and possibly use ParseUint with an explicit upper bound) to prevent silent misbehavior when the annotation is malformed or user-specified port is invalid.
| port, err := strconv.ParseInt(value, 10, 32) | |
| if err != nil { | |
| return 0, fmt.Errorf("parse annotations[%s] value %q: %w", VllmConfigInferencePortAnnotationKey, value, err) | |
| } | |
| port, err := strconv.ParseUint(value, 10, 16) | |
| if err != nil { | |
| return 0, fmt.Errorf("parse annotations[%s] value %q as TCP port: %w", VllmConfigInferencePortAnnotationKey, value, err) | |
| } | |
| if port == 0 { | |
| return 0, fmt.Errorf("annotations[%s] value %q is not a valid TCP port", VllmConfigInferencePortAnnotationKey, value) | |
| } |
There was a problem hiding this comment.
We have bigger problems if we do not trust the instance annotations. Problems for later.
There was a problem hiding this comment.
We need to put access control on all the access to the launcher's control port.
|
/ok-to-test |
|
🚀 E2E tests triggered by /ok-to-test |
This PR uses the "inference-port" annotation to find the port cleanly.