[router] build a general abstraction layer for both ModelRoute/ModelServer and Gateway API/Inference Extension #665
Conversation
volcano-sh-bot
added
kind/enhancement
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @git-malu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a fundamental architectural enhancement by creating a generic abstraction layer for routing and backend management. This change aims to provide a unified way to handle both ModelRoute/ModelServer and Gateway API/Inference Extension resources, making the router more flexible, maintainable, and easier to extend with new routing and backend types in the future. The core routing logic is now streamlined through this abstraction, reducing duplication and improving clarity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 general abstraction layer for handling both ModelRoute/ModelServer and Gateway API/Inference Extension resources. This is a significant improvement, as it unifies the routing and backend selection logic, making the doLoadbalance function in the router much cleaner and more maintainable. The new abstraction package effectively encapsulates the differences between the various resource types.
Overall, the changes are well-structured and achieve the stated goal of building a general abstraction layer. The conversion logic in converter.go is clear, and the refactoring in router.go demonstrates the benefits of this abstraction.
| return false | ||
| } | ||
| if sm.Regex != nil { | ||
| matched, _ := regexp.MatchString(*sm.Regex, value) |
There was a problem hiding this comment.
The error returned by regexp.MatchString is ignored here. If a user provides an invalid regular expression pattern in an HTTPRoute, this could lead to unexpected routing behavior (e.g., the regex might silently fail to match when it should, or vice-versa). It's best practice to at least log such errors to aid in debugging configuration issues.
matched, err := regexp.MatchString(*sm.Regex, value)
if err != nil {
klog.Errorf("invalid regex pattern '%s': %v", *sm.Regex, err)
return false // Or handle as appropriate, e.g., treat as no match
}
if !matched {| port := int32(0) | ||
| if len(ip.Spec.TargetPorts) > 0 { | ||
| port = int32(ip.Spec.TargetPorts[0].Number) | ||
| } |
There was a problem hiding this comment.
If ip.Spec.TargetPorts is empty, the port variable is initialized to 0. While this might be a valid default in some contexts, it's worth considering if a port of 0 is always a valid or intended value for a backend. If 0 is an invalid port, this could lead to connection issues downstream. It might be safer to return an error if no target ports are defined, or to ensure that 0 is handled gracefully by consumers of this Backend abstraction.
| // Get PDGroup from abstracted backend | ||
| var pdGroup *v1alpha1.PDGroup | ||
| if modelServer != nil && modelServer.Spec.WorkloadSelector != nil { | ||
| pdGroup = modelServer.Spec.WorkloadSelector.PDGroup | ||
| if backend.PDGroup != nil { | ||
| pdGroup = &v1alpha1.PDGroup{ | ||
| GroupKey: backend.PDGroup.GroupKey, | ||
| PrefillLabels: backend.PDGroup.PrefillLabels, | ||
| DecodeLabels: backend.PDGroup.DecodeLabels, | ||
| } |
There was a problem hiding this comment.
The PDGroup is converted from abstraction.PDGroupConfig back to v1alpha1.PDGroup here. This indicates a slight 'leak' in the abstraction, as the framework.Context still expects the concrete v1alpha1.PDGroup type. Ideally, the framework.Context and related scheduler components should also be updated to work with the abstract PDGroupConfig to fully leverage the abstraction layer. While functional, this conversion adds a small amount of overhead and couples the abstract layer to the concrete API type more than necessary.
git-malu
force-pushed
the
malu/dev/router
branch
from
0af8496 to
d59c763
Compare
2 participants
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
build a general abstraction layer for both ModelRoute/ModelServer and Gateway API/Inference Extension
Which issue(s) this PR fixes:
Fixes #
#546
Special notes for your reviewer:
Does this PR introduce a user-facing change?: