[Mics]: optimize deleteReferenceGrant with label selector in ModelRouter controller #1589
Conversation
Summary of ChangesHello @googs1025, 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 an optimization to the Highlights
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 optimizes the deleteReferenceGrant function by using a label selector to check for existing model deployments, which is more efficient than listing all deployments and filtering on the client side. It also improves the idempotency of the delete operation by ignoring 'Not Found' errors. While the optimization is a good improvement, I've found a logical issue where it only checks for Deployment resources, while it should also check for ModelAdapter and RayClusterFleet resources before deleting a shared ReferenceGrant. I've provided a suggestion to address this.
| selector, err := labels.NewRequirement(modelIdentifier, selection.Exists, nil) | ||
| if err != nil { | ||
| klog.ErrorS(err, "deleteReferenceGrant: unable to list all deployments") | ||
| klog.ErrorS(err, "Failed to create label requirement", "namespace", namespace) | ||
| return | ||
| } | ||
| for _, deployment := range deploymentList.Items { | ||
| _, ok := deployment.Labels[modelIdentifier] | ||
| if !ok { | ||
| continue | ||
| } | ||
| klog.InfoS("ignore delete reference grant, at least one model deployment shares same reference grant in the namesapce", | ||
| "namespace", namespace, "deployment", deployment.Name) | ||
|
|
||
| listOpts := &client.ListOptions{ | ||
| Namespace: namespace, | ||
| LabelSelector: labels.SelectorFromSet(labels.Set{}).Add(*selector), | ||
| } | ||
|
|
||
| var deploymentList appsv1.DeploymentList | ||
| if err := m.Client.List(context.Background(), &deploymentList, listOpts); err != nil { | ||
| klog.ErrorS(err, "Failed to list model deployments", "namespace", namespace) | ||
| return | ||
| } | ||
| if len(deploymentList.Items) > 0 { | ||
| klog.InfoS("Skip deleting ReferenceGrant: model deployment still exists", | ||
| "namespace", namespace, "existingDeployments", len(deploymentList.Items)) | ||
| return | ||
| } |
There was a problem hiding this comment.
The current implementation only checks for existing Deployment resources before deleting a ReferenceGrant. However, this controller also manages routes for ModelAdapter and RayClusterFleet resources. If the last Deployment is removed from a namespace, but ModelAdapter or RayClusterFleet resources still exist, this function will incorrectly delete the shared ReferenceGrant, potentially causing routing failures for the remaining models.
To fix this, we should check for the existence of all three resource types (Deployment, ModelAdapter, and RayClusterFleet) with the model identifier label before deciding to delete the ReferenceGrant.
selector, err := labels.NewRequirement(modelIdentifier, selection.Exists, nil)
if err != nil {
klog.ErrorS(err, "Failed to create label requirement", "namespace", namespace)
return
}
listOpts := &client.ListOptions{
Namespace: namespace,
LabelSelector: labels.NewSelector().Add(*selector),
}
// Check for Deployments
var deploymentList appsv1.DeploymentList
if err := m.Client.List(context.Background(), &deploymentList, listOpts); err != nil {
klog.ErrorS(err, "Failed to list model deployments", "namespace", namespace)
return
}
if len(deploymentList.Items) > 0 {
klog.InfoS("Skip deleting ReferenceGrant: model deployment still exists",
"namespace", namespace, "existingDeployments", len(deploymentList.Items))
return
}
// Check for ModelAdapters
var modelAdapterList modelv1alpha1.ModelAdapterList
if err := m.Client.List(context.Background(), &modelAdapterList, listOpts); err != nil {
klog.ErrorS(err, "Failed to list model adapters", "namespace", namespace)
return
}
if len(modelAdapterList.Items) > 0 {
klog.InfoS("Skip deleting ReferenceGrant: model adapter still exists",
"namespace", namespace, "existingModelAdapters", len(modelAdapterList.Items))
return
}
// Check for RayClusterFleets
var fleetList orchestrationv1alpha1.RayClusterFleetList
if err := m.Client.List(context.Background(), &fleetList, listOpts); err != nil {
klog.ErrorS(err, "Failed to list rayclusterfleets", "namespace", namespace)
return
}
if len(fleetList.Items) > 0 {
klog.InfoS("Skip deleting ReferenceGrant: rayclusterfleet still exists",
"namespace", namespace, "existingFleets", len(fleetList.Items))
return
}There was a problem hiding this comment.
I understand this isn't necessary, but I'm just optimizing the original logic. 🤔
There was a problem hiding this comment.
But I'm not sure if changing it to "list pod" would be better?
There was a problem hiding this comment.
gemini's comment is true but it could be separate PR. check this for more details. https://aibrix.readthedocs.io/latest/getting_started/faq.html#gateway-referencegrant-issue
RayClusterFleet is not managed here. this is a feature gap
Signed-off-by: googs1025 <[email protected]>
Jeffwan
force-pushed
the
fix/modelrouter_controller
branch
from
15fec83 to
a0bdd9b
Compare
3 participants
Pull Request Description
[Please provide a clear and concise description of your changes here]
Related Issues
Resolves: #[Insert issue number(s)]
Important: Before submitting, please complete the description above and review the checklist below.
Contribution Guidelines (Expand for Details)
We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]: Corrections to existing functionality[CI]: Changes to build process or CI pipeline[Docs]: Updates or additions to documentation[API]: Modifications to aibrix's API or interface[CLI]: Changes or additions to the Command Line Interface[Misc]: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.