update: DSC status update and delete component CR if it is not set Managed #1390

zdtsw · 2024-11-26T08:48:17Z

delete component CR when it is either set to Removed or not set in DSC
remove old status updates"Component is disabled/enabled), only update when component CR create successful or failed (should have follow up to add check if CR status is ready or notready)
fix a bug in datasciencpipeline :D datasciencepipelines component refactor #1340

Description

split out changes not related to modelmesh from #1338

How Has This Been Tested?

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

lburgazzoli · 2024-11-26T08:55:13Z

controllers/datasciencecluster/datasciencecluster_controller.go

 		instance, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dscv1.DataScienceCluster) {
-			status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, fmt.Sprintf("Component reconciliation failed: %v", err), corev1.ConditionFalse)
+			status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, fmt.Sprintf("Component creation failed: %v", err), corev1.ConditionFalse)


nit: this method does not only creates a component, but would also update it so I think reconciliation is more appropriate

correct. let me revert it

lburgazzoli · 2024-11-26T08:58:43Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -78,6 +78,9 @@ type DataScienceClusterConfig struct {

 const (
 	finalizerName = "datasciencecluster.opendatahub.io/finalizer"
+	RHOAIAppNS    = "redhat-ods-applications"
+	ODHAppNS      = "opendatahub"
+	MonitoringNS  = "redhat-ods-monitoring"


Those changes do not seem to be related with the PR :)

yeah, the lint is annoying, to get rid of the //nolint:goconst
but i can revert it

codecov · 2024-11-26T10:42:20Z

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature-operator-refactor@7da9176). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...atasciencecluster/datasciencecluster_controller.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1390   +/-   ##
============================================================
  Coverage                             ?   27.16%           
============================================================
  Files                                ?       60           
  Lines                                ?     4804           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1305           
  Misses                               ?     3347           
  Partials                             ?      152

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

lburgazzoli · 2024-11-26T12:02:38Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -360,7 +352,7 @@ func (r *DataScienceClusterReconciler) apply(ctx context.Context, dsc *dscv1.Dat
 	}

 	managementStateAnn, exists := obj.GetAnnotations()[annotations.ManagementStateAnnotation]
-	if exists && managementStateAnn == string(operatorv1.Removed) {
+	if exists && (managementStateAnn == string(operatorv1.Removed) || managementStateAnn == "Unknown") {


When is this case happening ? I wonder if we should only handle Managed and Removed, and return an error or skip otherwise ?

we can set it in DSC as

components: codeflare: {}

which is the same

components: codeflare: managementState: Removed

currently we tell user, do not set managementState in the component is the same as set it to Removed.
to make it easier i always create DSC with

spec: components: modelmeshserving: managementState: Managed

only ;D , then i found other new components get created

so I guess the mapping could be done in the components' NewCRObject ?

yeah, either do in all components NewCRObject(), or a central place.

thinking it out, if one day, we add Force or Unmanaged as supported case, we will need it in NewCRObject(), then DSC need logic to handle that as well.

Now since components have GetManagementState, maybe we can move it to a central place.

zdtsw · 2024-11-26T18:52:38Z

/test opendatahub-operator-e2e

- delete component CR when it is either set to Removed or not set in DSC - remove old status updates, only update when component CR create successful or failed Signed-off-by: Wen Zhou <[email protected]>

- change return as string, used by NewCRObject() - move logic check on Managed, Removed, not set component and others into GetManagementState() Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2024-11-27T10:38:24Z

controllers/components/dashboard/dashboard.go

+	case operatorv1.Managed, operatorv1.Removed:
+		return string(dsc.Spec.Components.Dashboard.ManagementState)
+	default: // Force and Unmanaged case for unknown values, we do not support these yet
+		return "Unknown"


Wonder if this should return operatorv1.Removed, in other words, how do we generally handle the case where the ManagementState is not set ?

i think it falls into the s == nil case ?

Now, i think i understand your comments better:
i can think of two approaches:

remove this default for now: since enum is only for Removed and Managed. till we add support for Force and Unmanaged, we do not need to worry about "default"

func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) operatorv1.ManagementState { if s == nil { return operatorv1.Removed } return dsc.Spec.Components.Dashboard.ManagementState }

change the whole funciton to

func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) operatorv1.ManagementState { if dsc.Spec.Components.Dashboard.ManagementState == operatorv1.Managed { return operatorv1.Managed } return operatorv1.Removed }

yep something like option 2

lburgazzoli · 2024-11-27T10:39:25Z

controllers/components/datasciencepipelines/datasciencepipelines.go

-	return dsc.Spec.Components.Dashboard.ManagementState
+func (s *componentHandler) GetManagementState(dsc *dscv1.DataScienceCluster) string {
+	if s == nil {
+		return "Removed"


I would do return string(operatorv1.Removed) so it is always consistent with the constant

i first made the one with return string(operatorv1.Removed)
then i thought to convert a const to string, is that extra step to just return a string.
but yeah, i get your point, i can get that reverted

lburgazzoli · 2024-11-27T10:40:05Z

pkg/componentsregistry/componentsregistry.go

@@ -21,7 +20,7 @@ type ComponentHandler interface {
 	// GetName and GetManagementState sound like pretty much the same across
 	// all components, but I could not find a way to avoid it
 	GetName() string
-	GetManagementState(dsc *dscv1.DataScienceCluster) operatorv1.ManagementState
+	GetManagementState(dsc *dscv1.DataScienceCluster) string


If we always return a known state, then we don't need tor return a plain string

yeah, lets revert back to operatorv1.ManagementState

…ementState() - return Managed if component is managed, other case, return Removed Signed-off-by: Wen Zhou <[email protected]>

openshift-ci · 2024-11-27T15:56:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lburgazzoli

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~~ [lburgazzoli]

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

zdtsw added the feature-operator-refactor label Nov 26, 2024

openshift-ci bot requested review from jackdelahunt and StevenTobin November 26, 2024 08:48

zdtsw requested review from lburgazzoli and VaishnaviHire and removed request for StevenTobin and jackdelahunt November 26, 2024 08:48

lburgazzoli reviewed Nov 26, 2024

View reviewed changes

zdtsw force-pushed the for_3 branch from 9f19efc to 5fb1e21 Compare November 26, 2024 09:13

lburgazzoli reviewed Nov 26, 2024

View reviewed changes

zdtsw force-pushed the for_3 branch from 5fb1e21 to 2fbf3bd Compare November 26, 2024 15:19

zdtsw added 3 commits November 27, 2024 09:27

update: DSC status and remove component CR's case

e0e15b3

- delete component CR when it is either set to Removed or not set in DSC - remove old status updates, only update when component CR create successful or failed Signed-off-by: Wen Zhou <[email protected]>

update: update GetManagementState return type and logic

a2e2fa8

- change return as string, used by NewCRObject() - move logic check on Managed, Removed, not set component and others into GetManagementState() Signed-off-by: Wen Zhou <[email protected]>

fix: wrong component is set in DSPO

00973f8

Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the for_3 branch from 2fbf3bd to 00973f8 Compare November 27, 2024 08:32

zdtsw requested a review from jackdelahunt November 27, 2024 08:37

zdtsw changed the title ~~update: DSC status update and remove component CR's case~~ update: DSC status update and delete component CR if it is not set Managed Nov 27, 2024

zdtsw requested a review from lburgazzoli November 27, 2024 10:20

lburgazzoli reviewed Nov 27, 2024

View reviewed changes

update: revert back to return operatorv1.ManagementState for GetManag…

3e7a0c0

…ementState() - return Managed if component is managed, other case, return Removed Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the for_3 branch from bc74af2 to 3e7a0c0 Compare November 27, 2024 12:32

zdtsw requested a review from lburgazzoli November 27, 2024 14:35

lburgazzoli approved these changes Nov 27, 2024

View reviewed changes

openshift-ci bot assigned lburgazzoli Nov 27, 2024

openshift-ci bot added the lgtm label Nov 27, 2024

openshift-ci bot added the approved label Nov 27, 2024

openshift-merge-bot bot merged commit 7384fe6 into opendatahub-io:feature-operator-refactor Nov 27, 2024
10 checks passed

lburgazzoli mentioned this pull request Nov 27, 2024

feat: codeflare component #1387

Merged

5 tasks

This was referenced Dec 8, 2024

A reconciler should not require the DSCI to perform cleanup action to avoid deadlock #1428

Closed

Refactor DSC controller #1430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: DSC status update and delete component CR if it is not set Managed #1390

update: DSC status update and delete component CR if it is not set Managed #1390

zdtsw commented Nov 26, 2024 •

edited

Loading

lburgazzoli Nov 26, 2024

zdtsw Nov 26, 2024

zdtsw Nov 26, 2024

lburgazzoli Nov 26, 2024

zdtsw Nov 26, 2024

zdtsw Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

lburgazzoli Nov 26, 2024

zdtsw Nov 26, 2024

zdtsw Nov 26, 2024

lburgazzoli Nov 26, 2024

zdtsw Nov 26, 2024

lburgazzoli Nov 26, 2024

zdtsw commented Nov 26, 2024

lburgazzoli Nov 27, 2024

zdtsw Nov 27, 2024

zdtsw Nov 27, 2024 •

edited

Loading

lburgazzoli Nov 27, 2024

zdtsw Nov 27, 2024

lburgazzoli Nov 27, 2024

zdtsw Nov 27, 2024

lburgazzoli Nov 27, 2024

zdtsw Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

update: DSC status update and delete component CR if it is not set Managed #1390

update: DSC status update and delete component CR if it is not set Managed #1390

Conversation

zdtsw commented Nov 26, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

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

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

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

zdtsw commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdtsw Nov 27, 2024 • edited Loading

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

openshift-ci bot commented Nov 27, 2024

zdtsw commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

zdtsw Nov 27, 2024 •

edited

Loading