-
Notifications
You must be signed in to change notification settings - Fork 131
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
Fix Kserve Unmanaged Installation #860
Fix Kserve Unmanaged Installation #860
Conversation
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.
The main changes looks good to me
I didn't test this yet but the change looks good. |
i did not see this PR but made an early comments on the sync PR red-hat-data-services#192
|
For |
/retest |
1 similar comment
/retest |
I think the whole confusion comes from: |
The error in e2e is caused by https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/tests/e2e/dsc_creation_test.go#L188. |
For the time being, this is not a supported setup: KServe needs both Serverless and Service Mesh to be present in order to work properly. However, there are talks about reducing the footprint and this may change in the near term. |
@@ -108,13 +108,8 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, resC | |||
return err | |||
} | |||
} | |||
// check on dependent operators if all installed in cluster | |||
dependOpsErrors := checkDependentOperators(cli).ErrorOrNil() |
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.
What I remember, is that this check was left here in order to prevent KServe from being installed if either OSSM or Serverless were missing from the cluster, regardless of the management state of OSSM and Serverless. This was simply a proactive check so that we don't end up with a broken installation of KServe.
Moving this code into configreServerless
doesn't make much sense to me, because configureServerless
has its own protections.
So, I think it is better to, simply, fully remove checkDependentOperators
function. We may want to drop a note in the docs about that KServe installation will no longer test if there is a valid OSSM and Serverless setup.
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 agree we can remove the checkDependentOperators
function entirely. I will update the PR.
For downstream though, we had included the function in configreServerless
already for 2.7. We will remove the checkDependentOperators
in 2.8. The behavior will be same though,
For `Unmanaged`, operator will not check for dependent operators.
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.
So, I had a meeting today and now I have a much better understanding about why this is needed.
I agree we can remove the checkDependentOperators function entirely. I will update the PR.
We will remove the checkDependentOperators in 2.8. The behavior will be same
This seems to be the optimal way, at the moment. I appreciate your help @VaishnaviHire .
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.
Looked more into this, I think we should keep the checkDependentOperators
around since it looks for OperatorCondition
resource rather than CRD. OperatorCondition helps with verifying if the operator subscription is present which is not the case with crd
/cherry-pick main |
@VaishnaviHire: once the present PR merges, I will cherry-pick it on top of main in a new PR and assign it to you. In response to this:
Instructions 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/test-infra repository. |
033e05e
to
43d586c
Compare
43d586c
to
b4371f9
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zdtsw The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b4371f9
to
a1f2a42
Compare
a1f2a42
to
171385d
Compare
b467ed7
into
opendatahub-io:incubation
@VaishnaviHire: new pull request created: #874 In response to this:
Instructions 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/test-infra repository. |
* Fix Kserve Unmanaged Installation * Update tests for kserve (cherry picked from commit b467ed7)
* Fix Kserve Unmanaged Installation * Update tests for kserve (cherry picked from commit b467ed7)
Description
https://issues.redhat.com/browse/RHOAIENG-2951
How Has This Been Tested?
Merge criteria: