-
Notifications
You must be signed in to change notification settings - Fork 381
[Misc] feature: use kvcache webhook #1187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
50c303c
to
ed00058
Compare
return backend | ||
} | ||
|
||
// TODO: Move validation logic to webhook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inspired by this todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure --disable-webhook
mode can be supported as well. there're some users want to use individual controllers. they want the minimum setup
default: | ||
return false | ||
} | ||
return kv.Annotations[constants.KVCacheLabelKeyBackend] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we have KVCache
webhook, then we can just get from Annotations
ed00058
to
0c24134
Compare
added |
0c24134
to
49ed023
Compare
61cb366
to
a51508a
Compare
part of #710 |
will add some integration test this week |
Signed-off-by: googs1025 <[email protected]>
a51508a
to
565c887
Compare
Signed-off-by: googs1025 <[email protected]>
9d165dd
to
2332900
Compare
if backend == "" { | ||
// In some case where webhooks are disabled (e.g., via --disable-webhook), | ||
// mutating webhooks cannot inject default values into the spec. Using annotations | ||
// ensures that users can still explicitly specify the backend, even when no webhook is active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/vllm-project/aibrix/pull/1187/files#r2155736323
We have made it compatible here and used comment to explain
cmpopts.IgnoreFields(orchestrationapi.KVCacheSpec{}, "Service"), | ||
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "UID", "ResourceVersion", "Generation", "CreationTimestamp", "ManagedFields"))) | ||
}, | ||
ginkgo.Entry("apply kvcache with no backend specified", &testDefaultingCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add mutating webhook test case
}), | ||
) | ||
|
||
type testValidatingCase struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add valid webhook case
8cff218
to
71b2848
Compare
Signed-off-by: googs1025 <[email protected]>
71b2848
to
d97cb90
Compare
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.