add enablement flag for nim #1330
Conversation
|
Hi @trujillm. Thanks for your PR. I'm waiting for a opendatahub-io 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. 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-sigs/prow repository. |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11610782036 |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11610862112 |
bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
Outdated
Show resolved
Hide resolved
bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
Outdated
Show resolved
Hide resolved
|
/hold |
|
/ok-to-test |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## incubation #1330 +/- ##
=============================================
Coverage ? 19.08%
=============================================
Files ? 30
Lines ? 3369
Branches ? 0
=============================================
Hits ? 643
Misses ? 2657
Partials ? 69 ☔ View full report in Codecov by Sentry. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etirelli 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 |
|
New changes are detected. LGTM label has been removed. |
|
also by having this opendatahub-io/odh-model-controller#281 already in odh-model-controller , do we need to add rbac on it? |
bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
Outdated
Show resolved
Hide resolved
|
@israel-hdez @lburgazzoli After AM slack chat, we recognized that this configuration switches needs to be tested with |
| func getNimManagementFlag(obj metav1.Object) string { | ||
| removed := string(operatorv1.Removed) | ||
| un, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) | ||
| if err == nil { |
There was a problem hiding this comment.
it is usually a good practice in golang to keep the happy path on the left, so
if err != nil {
return removed
}
...This holds true also for the if/else below
| @@ -131,6 +132,12 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, | |||
| return err | |||
| } | |||
| } | |||
| extraParamsMap := map[string]string{ | |||
| "nim-state": string(k.NIM.ManagementState), | |||
There was a problem hiding this comment.
My understanding is that here we are updating the params.env only in the KServe is Managed, if not, the value in the params.env is untouched. However in the ModelMesh component reconciliation, the params.env value is updated regardless of the status of the KServe component.
It is probably not an issue but wonder if the behavior should be made consistent.
There was a problem hiding this comment.
The assumption is that NIM depends on Kserve. If Kserve is not managed, NIM shouldn't be either. If Kserve is not managed, is the Kserve component's reconciler running? How would you suggest making the behaviour consistent?
There was a problem hiding this comment.
There is a single reconciler for all the components, when one component is marked as Removed, the specific component reconcile is invoked and it deletes the related resources. In this case it would leave the nim-state as it was defined previously, which I don't think it would be a problem, since KServe would be set as Removed.
In the ModelMesh however, the nim-state is updated regardless of the fact that the ModelMesh is set to Managed or Removed, see https://github.com/opendatahub-io/opendatahub-operator/pull/1330/files#diff-be28ef017be36951371e5f700cafcdad5925b03de9ae41d754825781f213aa78R131-R137.
So for my POV, it should not be a problem, just want to double check.
There was a problem hiding this comment.
So if the Kserve's component has some cleanup logic in place, can we use it to set nim-state to removed if kserve is removed? That way, we can make both behaviours consistent.
There was a problem hiding this comment.
Yes. it is the same ReconcileComponent metho, there is a check enabled := k.GetManagementState() == operatorv1.Managed
There was a problem hiding this comment.
Is the ConfigMap being deleted when Kserve's component is removed?
There was a problem hiding this comment.
Yes it is, so yes, it is not an issue, my point was mainly about consistency in the code.
But I'm fine to leave it as it is.
|
/retest |
Description
Adding NIM flag under Kserve component in ODH
https://issues.redhat.com/browse/NVPE-23
How Has This Been Tested?
The flag will do nothing until the triggers is handled from the odh-model-controller
This first requires the enablement to be present at which point we can test the trigger from the odh-model-controller