[v0.11.1] Fix controller setup - add schemas based on CRDs available #265
Conversation
|
Skipping CI for Draft Pull Request. |
|
@zdtsw Tagging you just FYI :-) |
cmd/manager/main.go
Outdated
|
|
||
| ksvcFound, ksvcCheckErr := utils.IsCrdAvailable(cfg, knservingv1.SchemeGroupVersion.String(), constants.KnativeServiceKind) | ||
| if ksvcCheckErr != nil { | ||
| log.Error(ksvcCheckErr, "error when checking if Knative Services are available") |
There was a problem hiding this comment.
Knative Services -> Knative Service kind
| @@ -146,13 +147,27 @@ func main() { | |||
| log.Error(err, "unable to get ingress config.") | |||
| os.Exit(1) | |||
| } | |||
| if deployConfig.DefaultDeploymentMode == string(constants.Serverless) { | |||
There was a problem hiding this comment.
The upstream PR where all related changes are present should make it clear. I'll refer to it here to give clarity.
I'll take as an example the changes to the inference controller here: https://github.com/kserve/kserve/pull/3472/files#diff-f14605793ed521dabc5c4794584c2961365d2d8324ad41f009aa44b1513d50feR321.
Notice that an equivalent condition around deployConfig.DefaultDeploymentMode is also being removed in the controller.
So, this removal aligns the conditionals that were modified in the controllers (that were merged in #233).
cmd/manager/main.go
Outdated
| if ingressConfig.DisableIstioVirtualHost == false { | ||
| } | ||
|
|
||
| if ingressConfig.DisableIstioVirtualHost == false { |
There was a problem hiding this comment.
Can be simplified !ingressConfig.DisableIstioVirtualHost
In opendatahub-io#233 an enhancement was done to KServe controllers to watch resources based on available CRDs. A similar change in the setup of the manager was overlooked: it is also needed to add schemas to the manager based on available CRDs rather than only the values in the inferenceservice-config ConfigMap. This would keep both manager setup and controller setup in sync with regards schemas and watches around the CRDs. Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>
israel-hdez
force-pushed
the
jira-4196-fix
branch
from
0ccbe18
to
a1fbd38
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: israel-hdez, spolti, terrytangyuan 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 |
openshift-merge-bot
bot
merged commit b1dd255
into
opendatahub-io:release-v0.11.1
|
Tested it in an OpenShift 4.14.13 cluster. Passed the test per instructions. |
…ster update konflux nightly
What this PR does / why we need it:
In #233 an enhancement was done to KServe controllers to watch resources based on available CRDs.
A similar change in the setup of the manager was overlooked: it is also needed to add schemas to the manager based on available CRDs rather than only the values in the inferenceservice-config ConfigMap. This would keep both manager setup and controller setup in sync with regards schemas and watches around the CRDs.
This is, basically, a cherry-pick of commti 5180ec5 from upstream PR (not merged yet).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes https://issues.redhat.com/browse/RHOAIENG-4196
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
Recommended testing is by using OpenShift.
./test/scripts/openshift-ci/deploy.ossm.sh(available in this repo / the checked out code)../test/scripts/openshift-ci/deploy.serverless.shoc new-project opendatahuboc apply -k 'github.com/red-hat-data-services/kserve/config/overlays/odh?ref=rhoai-2.8'Once KServe is deployed, you should be able to look at the logs and it is going to be running fine. Let's do the problematic setup:
oc delete knativeserving -n knative-serving knative-servingoc delete smcp -n istio-system basicoc patch cm -n opendatahub inferenceservice-config --type merge -p '{"data": {"deploy": "{\"defaultDeploymentMode\": \"RawDeployment\"}"}}'oc delete pod -n opendatahub -l control-plane=kserve-controller-managerOnce it restarts, you should notice
kind must be registered to the Schemeerrors in the logs of the pod and it will enter in a CrashLoop state. Now, deploy the fixed image:export IMG=quay.io/edgarhz/kserve-controller:pr-265make docker-build docker-pushoc set image -n opendatahub deploy/kserve-controller-manager manager=$IMGOnce the KServe pod restarts with the fixed image, you should observe it is stable and there are no errors in the logs.