Add Ability to configure resources in Kserve #965
Conversation
pkg/deploy/deploy.go
Outdated
| return nil | ||
| } | ||
| // do not patch resources field in Kserve deployment i.e allows users to update resources field | ||
| if found.GetKind() == "Deployment" { |
There was a problem hiding this comment.
Redundant check, it's done in the function
There was a problem hiding this comment.
+1
can remove the in-function check if we know it is deployment already from the caller
There was a problem hiding this comment.
I would rather leave it in the func and remove it from here :) the func says "deployment" in the name, so it needs to make sure whatever is passed is a deployment.
|
I really hope it is temporary :) |
|
@bartoszmajsak @LaVLaS so, what's you opinion, is it "plugin" or "patching"? ;) |
|
I have tested the build, i think the supported case for kserve will be:
|
It is. The reason for this being temporary fix is we do not yet have a requirement or list of accepted whitelisted fields for other components. The long term solution I see is to introduce a custom plugin that takes up list of whitelisted fields and applies them to all the resources, |
bb142a0 to
cc54b6a
Compare
@israel-hdez Can you confirm if this behavior for deployment updates look good to you? |
There's nothing as permanent as a temporary solution ;) We have to be vigilant and pay this back sooner than later. |
pkg/deploy/deploy.go
Outdated
| return nil | ||
| } | ||
| // do not patch resources field in Kserve deployment i.e allows users to update resources field | ||
| if found.GetKind() == "Deployment" { |
There was a problem hiding this comment.
I would rather leave it in the func and remove it from here :) the func says "deployment" in the name, so it needs to make sure whatever is passed is a deployment.
These are two different things :) |
cc54b6a to
6e9ac29
Compare
6e9ac29 to
0b7e6d3
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak, zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test opendatahub-operator-e2e |
|
@dhirajsb We see e2e failures when deploying Model registry controller image Do we need to update the default branch or the tag needs to be updated?? |
I just tried, and it is working fine. |
|
/test opendatahub-operator-e2e |
5b6147b
into
opendatahub-io:incubation
(cherry picked from commit 5b6147b)
Description
This is a very targeted change for Kserve component. The PR adds ability to configure
resourcesfield in Deployments objects of the component.Eventually we will move to a generalize solution to whitelist fields for all components. See [wip]ADR
Jira Issue: https://issues.redhat.com/browse/RHOAIENG-280
How Has This Been Tested?
resourcesfield inkserve-controller-managerDeploymentresourcesfield in Deployment of any other componentMerge criteria: