Update DSC status with the related components status" #1431

lburgazzoli · 2024-12-08T10:33:30Z

Description

This commit includes:

exposing common components status to the DSC as part of
status.components field
introduce a new Ready conditions that becomes true only when all the
components are also Ready
reduces DSC chattering, by reducing the number of updates per reconcile

Notes:

requires Fix linters #1432

https://issues.redhat.com/browse/RHOAIENG-15042

How Has This Been Tested?

e2e tests in progress

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
~~Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).~~
~~The developer has manually tested the changes and verified that the changes work~~

openshift-ci · 2024-12-08T10:33:35Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

github-actions · 2024-12-08T10:34:47Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12221051844

github-actions · 2024-12-08T10:36:18Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12221061488

github-actions · 2024-12-08T10:37:26Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12221069570

github-actions · 2024-12-08T10:38:16Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12221076816

codecov · 2024-12-08T10:39:28Z

Codecov Report

Attention: Patch coverage is 0.13986% with 714 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@5f456ad). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...atasciencecluster/datasciencecluster_controller.go	0.00%	191 Missing ⚠️
controllers/components/codeflare/codeflare.go	0.00%	43 Missing ⚠️
controllers/components/dashboard/dashboard.go	0.00%	42 Missing ⚠️
...nents/datasciencepipelines/datasciencepipelines.go	0.00%	42 Missing ⚠️
controllers/components/kserve/kserve.go	0.00%	42 Missing ⚠️
controllers/components/kueue/kueue.go	0.00%	42 Missing ⚠️
...rs/components/modelmeshserving/modelmeshserving.go	0.00%	42 Missing ⚠️
...trollers/components/modelregistry/modelregistry.go	0.00%	42 Missing ⚠️
controllers/components/ray/ray.go	0.00%	42 Missing ⚠️
...rs/components/trainingoperator/trainingoperator.go	0.00%	42 Missing ⚠️
... and 8 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1431   +/-   ##
=======================================
  Coverage        ?   19.12%           
=======================================
  Files           ?      149           
  Lines           ?    10353           
  Branches        ?        0           
=======================================
  Hits            ?     1980           
  Misses          ?     8149           
  Partials        ?      224

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-12-08T17:12:09Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12223711116

VaishnaviHire · 2024-12-09T18:31:24Z

Adding @grdryn, since this change will be helpful for setting Kserve status

lphiri

Minor nit on naming , but otherwise looks good to me.

apis/datasciencecluster/v1/datasciencecluster_types.go

openshift-ci · 2024-12-09T21:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lphiri

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:

~~OWNERS~~ [lphiri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grdryn

Generally LGTM, with some minor suggestions and questions 👍

grdryn · 2024-12-09T20:33:41Z

controllers/datasciencecluster/datasciencecluster_controller.go

 )

+// TODO: all the logic about the deletion configmap should be moved to another controller


Is this a TODO for this PR, or for the future? If future, would it make sense to have a Jira issue for it?

Added jira link

grdryn · 2024-12-09T21:32:45Z

controllers/datasciencecluster/datasciencecluster_controller.go

-	if err != nil {
-		return instance, fmt.Errorf("failed to update DataScienceCluster status after reconciling %s: %w", componentName, err)
+	default:
+		return nil, fmt.Errorf("unsuported management state: %s", ms)


Suggested change

return nil, fmt.Errorf("unsuported management state: %s", ms)

return nil, fmt.Errorf("unsupported management state: %s", ms)

/me hides 😶‍🌫️

grdryn · 2024-12-09T21:51:48Z

controllers/datasciencecluster/datasciencecluster_controller.go

-	if err != nil {
+func (r *DataScienceClusterReconciler) watchDeleteConfigMap(ctx context.Context, a client.Object) []reconcile.Request {
+	cm, ok := a.(*corev1.ConfigMap)
+	if !ok {
 		return nil


Would make sense as an alternative to pull out these conditions to a predicate to be used on the Watches method on L319, and just use handlers.Fn(r.watchDataScienceClusters) as the event handler? I'm not sure if I'm thinking about this correctly or not

done, let me know what you think

grdryn · 2024-12-09T22:05:58Z

pkg/componentsregistry/componentsregistry.go

 	NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error
+	// UpdateDSCStatus update the component specific status part od the DSC


Suggested change

// UpdateDSCStatus update the component specific status part od the DSC

// UpdateDSCStatus updates the component specific status part of the DSC

grdryn · 2024-12-09T22:12:44Z

pkg/controller/reconciler/reconciler_support.go

+/*
+	predicate.Funcs{
+		UpdateFunc: func(event event.UpdateEvent) bool {
+			fmt.Println(
+				">>>",
+				"gvk", event.ObjectNew.GetObjectKind().GroupVersionKind(),
+				"namespace", event.ObjectNew.GetNamespace(),
+				"name", event.ObjectNew.GetName(),
+				"diff", cmp.Diff(event.ObjectOld, event.ObjectNew),
+			)
+
+			return true
+		},
+	}),
+*/


Is this comment expected to be here?

grdryn · 2024-12-09T22:23:14Z

tests/e2e/codeflare_test.go

+	})
+
+	if err != nil {
+		return fmt.Errorf("error waiting Ready state for CodeFlare componetn in DSC: %w", err)


Suggested change

return fmt.Errorf("error waiting Ready state for CodeFlare componetn in DSC: %w", err)

return fmt.Errorf("error waiting on Ready state for CodeFlare component in DSC: %w", err)

and equivalent suggestion for the other tests that have this check

grdryn · 2024-12-09T22:28:09Z

apis/datasciencecluster/v1/datasciencecluster_types.go

+	// ModelMeshServing component status.
+	ModelMeshServing componentApi.DSCModelMeshServingStatus `json:"modelmeshserving,omitempty"`
+
+	// DataServicePipeline component status.


Suggested change

// DataServicePipeline component status.

// DataSciencePipeline component status.

grdryn · 2024-12-09T22:28:21Z

apis/datasciencecluster/v1/datasciencecluster_types.go

+	ModelMeshServing componentApi.DSCModelMeshServingStatus `json:"modelmeshserving,omitempty"`
+
+	// DataServicePipeline component status.
+	// Require OpenShift Pipelines Operator to be installed before enable component


Suggested change

// Require OpenShift Pipelines Operator to be installed before enable component

// Requires OpenShift Pipelines Operator to be installed before enable component

grdryn · 2024-12-09T22:29:42Z

apis/datasciencecluster/v1/datasciencecluster_types.go

+	DataSciencePipelines componentApi.DSCDataSciencePipelinesStatus `json:"datasciencepipelines,omitempty"`
+
+	// Kserve component status.
+	// Require OpenShift Serverless and OpenShift Service Mesh Operators to be installed before enable component


Suggested change

// Require OpenShift Serverless and OpenShift Service Mesh Operators to be installed before enable component

// Requires OpenShift Serverless and OpenShift Service Mesh Operators to be installed before enable component

I wonder if this is still true in all cases actually...I guess they shouldn't be absolutely necessary for RawDeployment mode? 🤔

This is something I don't know, the comment was there alrady (just copy pasted, and now removed since it does not make any sense on the status subresource)

grdryn · 2024-12-09T22:30:37Z

apis/datasciencecluster/v1/datasciencecluster_types.go

+	Kueue componentApi.DSCKueueStatus `json:"kueue,omitempty"`
+
+	// CodeFlare component status.
+	// If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabled component.


Suggested change

// If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabled component.

// If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabling component.

github-actions · 2024-12-09T23:12:09Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12245986753

github-actions · 2024-12-09T23:24:34Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12246154643

lburgazzoli · 2024-12-10T12:57:55Z

@grdryn @VaishnaviHire e2e tests are a little bit of a mess, I'll refactor them out once we get this and #1437 merged if that's fine with you

VaishnaviHire · 2024-12-10T18:36:53Z

/lgtm

lburgazzoli · 2024-12-10T19:49:33Z

@VaishnaviHire @grdryn mind another review ? rebased on top of main

ykaliuta · 2024-12-11T10:24:16Z

controllers/datasciencecluster/datasciencecluster_controller.go

+		}
+
+		readyCondition := meta.FindStatusCondition(ci.GetStatus().Conditions, status.ConditionTypeReady)
+		if readyCondition != nil && readyCondition.Status != metav1.ConditionTrue {


Is it ready if there is no ready condition?

good spot, let me fix it

Pushed a commit, now the logic is that:

if the component is not managed, skip checking its readiness

if the component is managed, then it is ready if the ready condition is present with status True

the message DSC's Ready condition message has been made generic as multiple factors could contribute to set it to ready (i.e. a cluster without managed components is still Ready)

VaishnaviHire · 2024-12-11T11:42:29Z

@lburgazzoli Branch needs a rebase

This commit includes: - exposing common components status to the DSC as part of status.components field - introduce a new Ready conditions that becomes true only when all the components are also Ready - reduces DSC chattering, by reducing the number of updates per reconcile Signed-off-by: Luca Burgazzoli <[email protected]>

lburgazzoli · 2024-12-11T11:44:50Z

@lburgazzoli Branch needs a rebase

done

VaishnaviHire · 2024-12-11T13:37:54Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress label Dec 8, 2024

lburgazzoli added the feature-operator-refactor label Dec 8, 2024

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch 2 times, most recently from 4c47d64 to 3eb275f Compare December 8, 2024 10:36

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from 3eb275f to 86d75d5 Compare December 8, 2024 10:37

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from 86d75d5 to c0f6ac5 Compare December 8, 2024 17:11

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from a052d93 to 5f019cd Compare December 9, 2024 10:20

VaishnaviHire added the odh-2.22 label Dec 9, 2024

openshift-merge-robot added the needs-rebase label Dec 9, 2024

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from d976c81 to dbb1137 Compare December 9, 2024 15:49

openshift-merge-robot removed the needs-rebase label Dec 9, 2024

lburgazzoli marked this pull request as ready for review December 9, 2024 15:54

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 9, 2024

openshift-ci bot requested review from grdryn and lphiri December 9, 2024 15:54

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from d693ade to 6de737a Compare December 9, 2024 20:13

lphiri approved these changes Dec 9, 2024

View reviewed changes

apis/datasciencecluster/v1/datasciencecluster_types.go Show resolved Hide resolved

openshift-ci bot assigned lphiri Dec 9, 2024

openshift-ci bot added the lgtm label Dec 9, 2024

openshift-ci bot added the approved label Dec 9, 2024

grdryn reviewed Dec 9, 2024

View reviewed changes

openshift-ci bot removed the lgtm label Dec 9, 2024

lburgazzoli requested review from VaishnaviHire and grdryn December 10, 2024 11:30

openshift-ci bot assigned VaishnaviHire Dec 10, 2024

openshift-ci bot added the lgtm label Dec 10, 2024

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from acb2d22 to fcfcba0 Compare December 10, 2024 19:48

openshift-ci bot removed the lgtm label Dec 10, 2024

ykaliuta reviewed Dec 11, 2024

View reviewed changes

lburgazzoli added 2 commits December 11, 2024 12:43

Fix finfings

238ae87

lburgazzoli force-pushed the RHOAIENG-15042-dsc branch from 195e7fa to 238ae87 Compare December 11, 2024 11:44

openshift-ci bot added the lgtm label Dec 11, 2024

openshift-merge-bot bot merged commit 817b465 into opendatahub-io:main Dec 11, 2024
10 checks passed

lburgazzoli deleted the RHOAIENG-15042-dsc branch December 11, 2024 13:53

		)

		// TODO: all the logic about the deletion configmap should be moved to another controller

	return nil, fmt.Errorf("unsuported management state: %s", ms)
	return nil, fmt.Errorf("unsupported management state: %s", ms)

		NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error
		// UpdateDSCStatus update the component specific status part od the DSC

	// UpdateDSCStatus update the component specific status part od the DSC
	// UpdateDSCStatus updates the component specific status part of the DSC

	return fmt.Errorf("error waiting Ready state for CodeFlare componetn in DSC: %w", err)
	return fmt.Errorf("error waiting on Ready state for CodeFlare component in DSC: %w", err)

	// DataServicePipeline component status.
	// DataSciencePipeline component status.

	// Require OpenShift Pipelines Operator to be installed before enable component
	// Requires OpenShift Pipelines Operator to be installed before enable component

	// Require OpenShift Serverless and OpenShift Service Mesh Operators to be installed before enable component
	// Requires OpenShift Serverless and OpenShift Service Mesh Operators to be installed before enable component

	// If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabled component.
	// If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabling component.

Update DSC status with the related components status" #1431

Update DSC status with the related components status" #1431

Conversation

lburgazzoli commented Dec 8, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Dec 8, 2024

github-actions bot commented Dec 8, 2024

This PR can't be merged just yet 😢

github-actions bot commented Dec 8, 2024

This PR can't be merged just yet 😢

github-actions bot commented Dec 8, 2024

This PR can't be merged just yet 😢

github-actions bot commented Dec 8, 2024

This PR can't be merged just yet 😢

codecov bot commented Dec 8, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 8, 2024

This PR can't be merged just yet 😢

VaishnaviHire commented Dec 9, 2024

lphiri left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 9, 2024

grdryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2024

This PR can't be merged just yet 😢

github-actions bot commented Dec 9, 2024

This PR can't be merged just yet 😢

lburgazzoli commented Dec 10, 2024 • edited Loading

VaishnaviHire commented Dec 10, 2024

lburgazzoli commented Dec 10, 2024

ykaliuta Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented Dec 11, 2024

lburgazzoli commented Dec 11, 2024

VaishnaviHire commented Dec 11, 2024

lburgazzoli commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

lburgazzoli commented Dec 10, 2024 •

edited

Loading

ykaliuta Dec 11, 2024 •

edited

Loading