-
Notifications
You must be signed in to change notification settings - Fork 151
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
Show KServe defaultDeploymentMode in DSC status #1465
Show KServe defaultDeploymentMode in DSC status #1465
Conversation
/test images |
4679db4
to
3635ba3
Compare
/retest |
2 similar comments
/retest |
/retest |
@biswassri I think #1455 is a misunderstanding of what's needed. I've asked about it in the jira issue. |
/retest |
3635ba3
to
64128f9
Compare
/retest ray and kueue e2e test flakes |
/retest ds pipelines test flake 🤔 |
@@ -157,6 +157,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. | |||
WithAction(deploy.NewAction( | |||
deploy.WithCache(), | |||
)). | |||
WithAction(setStatusFields). |
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 the update function be done in UpdateDSCStatus() https://github.com/opendatahub-io/opendatahub-operator/blob/b0a5efa5db3c0d9379720fdfc52f14580f91003c/controllers/components/kserve/kserve.go#L87C31-L87C49 or have to create a new setStatusFields() ?
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.
Hmm, I'm not sure 🤔 it looks like that will get called only during the reconciliation in the DSC controller, right? I think we should also have this set in the Kserve CR status for consistency, and to do that I think it needs to be updated here. WDYT?
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.
hmmmmmm, why we do not add the "set status" in customizeKserveConfigMap() directly?
e.g afterinferenceServiceConfigMap.Data[DeployConfigName] = string(deployDataBytes)
then we can have a default value "Serverless" for defaultDeploymentMode , only if deployData.DefaultDeploymentMode != string(defaultmode) {
it sets a different value.
--
tbh, i am not fond of setting component's status in DSC.
if possible, it should only be in the kserve CR 's .status. or we are duplicating all things in both places.
e.g modelreg namespace is only in the DSC .spec and ModelReg CR .spec , but not DSC .status.
it would be good for dashboard to check component CR to get each's .status.
but this is out of the scope for this PR, if the requirment is to get it done in DSC .status.
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.
why we do not add the "set status" in customizeKserveConfigMap() directly?
The main reason that I chose to keep this separate, and after the deploy action, is that we want to read the ConfigMap from the cluster rather than relying on it from the resources list, because the user may decide to manage it themselves, and change the value from what the operator would have set it to (the value that would be in the resources list). Doing it after the deploy action should mean that the ConfigMap at least always exists on the cluster at this point. If there wasn't that edge case of unmanaged configmap, then it would make sense to do it in customizeKserveConfigMap().
it would be good for dashboard to check component CR to get each's .status.
Yeah maybe, I'm not sure what's best there. 🤔 @lburgazzoli do you have any thoughts on this?
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.
let me get back from PTO :)
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.
@lburgazzoli ah apologies 🙈 in any case I think this is not a blocking 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.
don't worry 😉
64128f9
to
e0bf6e4
Compare
JIRA: https://issues.redhat.com/browse/RHOAIENG-16240 The dashboard would like to be able to read this value, and should only read it from the status. The value for the status is always read from the KServe ConfigMap, rather than ever assuming that what is set in the DSC, or what the operator defaults to, is correct: this is because that ConfigMap can be annotated to be unmanaged by the operator, in which case a user may have changed the value to something else, and the status should reflect this reality.
e0bf6e4
to
e051140
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asanzgom 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 |
/retest kueue test flake (I guess these flakes are timeouts) |
/retest training operator this time. 🔍 we should try to figure out why all of these tests are failing intermittently now. |
/retest Ray this time. It often seems to be at the "Validate_Disabling_<Name>_Component" check. === RUN TestOdhOperator/validate_installation_of_ray_component/e2e-test-dsc/Validate_Disabling_Ray_Component ray_test.go:75: Error Trace: /go/src/github.com/opendatahub-io/opendatahub-operator/tests/e2e/ray_test.go:75 Error: Received unexpected error: component kuberay-operator deployment rayis not ready Test: TestOdhOperator/validate_installation_of_ray_component/e2e-test-dsc/Validate_Disabling_Ray_Component Messages: error testing ray component enabled field |
/retest data science pipelines this time: === RUN TestOdhOperator/validate_installation_of_datasciencepipelines_component/e2e-test-dsc/Validate_Disabling_DataSciencePipelines_Component datasciencepipelines_test.go:81: Error Trace: /go/src/github.com/opendatahub-io/opendatahub-operator/tests/e2e/datasciencepipelines_test.go:81 Error: Received unexpected error: component DataSciencePipelines is disabled, should not have deployments in NS opendatahub any more Test: TestOdhOperator/validate_installation_of_datasciencepipelines_component/e2e-test-dsc/Validate_Disabling_DataSciencePipelines_Component Messages: error testing DataSciencePipelines component enabled field |
@grdryn I do have some idea about the reason of sic failures, but I need to validate them. I'll do so on Tuesday when back |
/retest data science pipelines again: === RUN TestOdhOperator/validate_installation_of_datasciencepipelines_component/e2e-test-dsc/Validate_Disabling_DataSciencePipelines_Component datasciencepipelines_test.go:81: Error Trace: /go/src/github.com/opendatahub-io/opendatahub-operator/tests/e2e/datasciencepipelines_test.go:81 Error: Received unexpected error: component DataSciencePipelines is disabled, should not have deployments in NS opendatahub any more Test: TestOdhOperator/validate_installation_of_datasciencepipelines_component/e2e-test-dsc/Validate_Disabling_DataSciencePipelines_Component Messages: error testing DataSciencePipelines component enabled field |
Description
JIRA: https://issues.redhat.com/browse/RHOAIENG-16240
The dashboard would like to be able to read this value, and should
only read it from the status.
The value for the status is always read from the KServe ConfigMap,
rather than ever assuming that what is set in the DSC, or what the
operator defaults to, is correct: this is because that ConfigMap can
be annotated to be unmanaged by the operator, in which case a user may
have changed the value to something else, and the status should
reflect this reality.
How Has This Been Tested?
Merge criteria