-
Notifications
You must be signed in to change notification settings - Fork 219
feat: changed to support both v1 and v1a2 ip in EPP #1277
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
feat: changed to support both v1 and v1a2 ip in EPP #1277
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @capri-xiyue. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Please don't review it as it is just a draft |
|
didn't start the review. just verifying: I think this is the right thing to do! |
|
As an additional data point, supporting both for some time will make our life a lot easier, thanks for doing this! |
|
👍 I'll review when @capri-xiyue pulls off the WIP tag! Thanks for your work on this! We will give you space to work for now, feel free to pull of that WIP tag when ready! |
|
WRT the timeline, this is intended to be transitionary & give gateways time to migrate to v1. It wont be immediate, but this is not intended to be indefinite. We will work with our upstream partners but I'm thinking deprecation of v1a2 would probably be in the v1.1-v1.2 timeline |
9cec6c5 to
d4a8d67
Compare
|
/ok-to-test |
288e1b3 to
43727d1
Compare
|
I feel it is too much to refactor e2e and integration test here. Will create another issue to track it. #1283 I added basic UT here to make sure it works and I also verified it e2e via manual test
|
# Conflicts: # pkg/epp/controller/inferencemodel_reconciler.go # pkg/epp/datastore/datastore.go # pkg/epp/server/controller_manager.go # pkg/epp/server/runserver.go # Conflicts: # cmd/epp/runner/runner.go
Signed-off-by: Xiyue Yu <[email protected]>
Signed-off-by: Xiyue Yu <[email protected]>
76b97a9 to
3c84a17
Compare
| DefaultCertPath = "" // default for --cert-path | ||
| DefaultConfigFile = "" // default for --config-file | ||
| DefaultConfigText = "" // default for --config-text | ||
| DefaultPoolGroup = "inference.networking.k8s.io" // default for --pool-group |
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.
I'm wondering maybe I should use "inference.networking.x-k8s.io" to avoid unexpected break change when users change from release 0.5 to main branch?
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.
I personally think it's safer to switch the defaults to the new GA API and leave an option to use the older alpha API for backwards compatibility.
| logger.Info("InferencePool not found. Clearing the datastore") | ||
| if c.PoolGKNN.Group == v1alpha2.GroupName { | ||
| infPool := &v1alpha2.InferencePool{} | ||
| if err := c.Get(ctx, req.NamespacedName, infPool); err != nil { |
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.
can we combine this logic so we dont repeat? The only conditionals should be:
- creation of pool based on type
- conversion of v1a2 pool to v1 (do we need to unstructured middle step? would a conversion function that we define be more reliable?)
LMKWYT
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.
Updated to avoid duplicate code. To avoid duplicate code, I have to initialize the variable as a interface which both v1a2 and v1 Pool type can use, here I use client.Object.
I think using unstructured as middle step is quite reliable as the conversation function is handled by k8s runtime instead of self-authored written code. And self-written conversion code would be cumbersome as I need to self-write deep copy code by myself. Let me know what you think.
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.
Sounds great, and looks great as well. Thanks!
|
Looks good for the most part, left a single comment in the primary change (infPool reconciliation) |
Updated the code based on the comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: capri-xiyue, kfswain The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |


fixed #1278
we want to support CUJ (use case) that users can specify either "inference.networking.k8s.io" or "inference.networking.x-k8s.io" InferencePool in EPP. Please note EPP deployment and InferencePool still has 1:1 mapping.
--pool-groupto allow users to specify whether they configure eitherinference.networking.x-k8s.ioorhttp://inference.networking.k8s.ioInferencePoolinference.networking.x-k8s.ioorhttp://inference.networking.k8s.ioInferencePool