feat(inferenceservice): add PVC storage support for model volumes #288
Conversation
chethanuk
requested review from
CatherineSue,
YouNeedCryDear and
slin1237
as code owners
Summary of ChangesHello @chethanuk, 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 significantly enhances the InferenceService controller by introducing comprehensive support for models stored on Kubernetes PersistentVolumeClaims (PVCs). It modifies the controller to intelligently handle PVC-backed models, including dynamic volume mounting, optimized node selection, and robust validation. A new metadata extraction workflow, powered by Kubernetes Jobs, is implemented to ensure proper model lifecycle management and observability. This change improves flexibility for model storage and enhances the reliability of model serving infrastructure. 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 introduces support for PVC-backed models in the InferenceService controller, a significant feature enhancement. The changes include new logic to handle PVC volumes and node selectors, along with corresponding RBAC and CRD updates. The implementation is robust, featuring comprehensive validation, error handling, and retry mechanisms for PVC operations. My review focuses on improving code clarity, performance, and adherence to best practices in the new pvc_handler.go file. I've identified a couple of areas for improvement, mainly around error classification and the removal of a custom utility function in favor of the standard library.
| func classifyError(err error) ErrorType { | ||
| if err == nil { | ||
| return ErrorTypeTransient | ||
| } | ||
|
|
||
| errStr := err.Error() | ||
|
|
||
| // Security-related errors (don't retry) | ||
| if contains(errStr, "cross-namespace") || contains(errStr, "access denied") { | ||
| return ErrorTypeSecurity | ||
| } | ||
|
|
||
| // Validation errors (don't retry) | ||
| if contains(errStr, "not found") || contains(errStr, "not bound") || contains(errStr, "invalid") { | ||
| return ErrorTypeValidation | ||
| } | ||
|
|
||
| // Default to transient (can retry) | ||
| return ErrorTypeTransient | ||
| } |
There was a problem hiding this comment.
The classifyError function relies on string matching via a custom contains function, which is brittle. It's better to use errors.Is or functions like apierrors.IsNotFound where possible. For other cases, strings.Contains is more idiomatic and performant than the custom contains function. Please also add import "strings" to the file.
I've updated the function to use apierrors.IsNotFound and strings.Contains.
func classifyError(err error) ErrorType {
if err == nil {
return ErrorTypeTransient
}
if apierrors.IsNotFound(err) {
return ErrorTypeValidation
}
errStr := err.Error()
// Security-related errors (don't retry)
if strings.Contains(errStr, "cross-namespace") || strings.Contains(errStr, "access denied") {
return ErrorTypeSecurity
}
// Validation errors (don't retry)
if strings.Contains(errStr, "not bound") || strings.Contains(errStr, "invalid") {
return ErrorTypeValidation
}
// Default to transient (can retry)
return ErrorTypeTransient
}| app.kubernetes.io/component: metadata-extractor | ||
| {{- include "ome-resources.labels" . | nindent 4 }} | ||
| rules: | ||
| # Allow reading ConfigMaps for status updates |
There was a problem hiding this comment.
The comment on line 21 is misleading. It states 'Allow reading ConfigMaps for status updates', but the verbs ["get", "list", "create", "update", "patch"] grant full management permissions. To improve clarity and adhere to the principle of least astonishment, the comment should accurately reflect the granted permissions.
# Allow managing ConfigMaps for status updates| app.kubernetes.io/name: ome | ||
| app.kubernetes.io/component: metadata-extractor | ||
| rules: | ||
| # Allow reading ConfigMaps for status updates |
There was a problem hiding this comment.
The comment on line 19 is misleading. It states 'Allow reading ConfigMaps for status updates', but the verbs ["get", "list", "create", "update", "patch"] grant full management permissions. To improve clarity and adhere to the principle of least astonishment, the comment should accurately reflect the granted permissions.
# Allow managing ConfigMaps for status updates| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && (s == substr || len(substr) == 0 || | ||
| (len(s) > len(substr) && contains(s[1:], substr)) || | ||
| (len(s) >= len(substr) && s[:len(substr)] == substr)) | ||
| } |
There was a problem hiding this comment.
The custom contains function is a very inefficient and complex recursive reimplementation of strings.Contains. It should be removed and strings.Contains from the standard library should be used instead, as suggested for the classifyError function.
1 participant
What type of PR is this?
Implement PVC volume mounting and node selector handling for PVC-backed models in InferenceService controller.
What this PR does / why we need it:
Tests:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?