datasciencepipelines component refactor #1340

jackdelahunt · 2024-11-04T20:37:03Z

Description

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

openshift-ci · 2024-11-04T20:37:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jackdelahunt. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2024-11-04T21:46:06Z

Codecov Report

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

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

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

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1340   +/-   ##
============================================================
  Coverage                             ?   25.86%           
============================================================
  Files                                ?       55           
  Lines                                ?     4384           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1134           
  Misses                               ?     3111           
  Partials                             ?      139

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

config/crd/bases/components.opendatahub.io_codeflares.yaml

lburgazzoli · 2024-11-05T06:52:57Z

you may also need to re-generate some additional metadata:

make generate manifest api-docs bundle

bundle/manifests/opendatahub-operator.clusterserviceversion.yaml

tests/e2e/helper_test.go

controllers/components/datasciencepipelines/datasciencepipelines_controller.go

jackdelahunt · 2024-11-06T12:42:35Z

controllers/components/datasciencepipelines/datasciencepipelines_controller_actions.go

+func devFlags(ctx context.Context, rr *odhtypes.ReconciliationRequest) error {
+	dsp, ok := rr.Instance.(*componentsv1.DataSciencePipelines)
+	if !ok {
+		return fmt.Errorf("resource instance %v is not a componentsv1.Ray)", rr.Instance)
+	}
+
+	if dsp.Spec.DevFlags == nil {
+		return nil
+	}
+
+	// Implement devflags support logic
+	// If dev flags are set, update default manifests path
+	if len(dsp.Spec.DevFlags.Manifests) != 0 {
+		manifestConfig := dsp.Spec.DevFlags.Manifests[0]
+		if err := odhdeploy.DownloadManifests(ctx, componentsv1.DataSciencePipelinesComponentName, manifestConfig); err != nil {
+			return err
+		}
+		if manifestConfig.SourcePath != "" {
+			rr.Manifests[0].Path = odhdeploy.DefaultManifestPath
+			rr.Manifests[0].ContextDir = componentsv1.DataSciencePipelinesComponentName
+			rr.Manifests[0].SourcePath = manifestConfig.SourcePath
+		}
+	}
+
+	return nil


Do we know if there is anything additional needed here looking at the old component implementation I don't think so but I am not sure

lburgazzoli · 2024-11-06T20:17:07Z

it seems this part https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/components/datasciencepipelines/datasciencepipelines.go#L147-L168 has not been implemented, am I wrong ?

jackdelahunt · 2024-11-06T20:29:54Z

controllers/components/datasciencepipelines/datasciencepipelines.go

+func UnmanagedArgoWorkFlowExists(ctx context.Context, cli client.Client) error {
+	workflowCRD := &apiextensionsv1.CustomResourceDefinition{}
+	if err := cli.Get(ctx, client.ObjectKey{Name: ArgoWorkflowCRD}, workflowCRD); err != nil {
+		if k8serr.IsNotFound(err) {
+			return nil
+		}
+		return fmt.Errorf("failed to get existing Workflow CRD : %w", err)
+	}
+	// Verify if existing workflow is deployed by ODH with label
+	odhLabelValue, odhLabelExists := workflowCRD.Labels[labels.ODH.Component(componentsv1.DataSciencePipelinesComponentName)]
+	if odhLabelExists && odhLabelValue == "true" {
+		return nil
+	}
+
+	return fmt.Errorf("%s CRD already exists but not deployed by this operator. "+
+		"Remove existing Argo workflows or set `spec.components.datasciencepipelines.managementState` to Removed to proceed ", ArgoWorkflowCRD)
+}
+
+func SetExistingArgoCondition(conditions *[]conditionsv1.Condition, reason, message string) {
+	status.SetCondition(conditions, string(status.CapabilityDSPv2Argo), reason, message, corev1.ConditionFalse)
+	status.SetComponentCondition(conditions, componentsv1.DataSciencePipelinesComponentName, status.ReconcileFailed, message, corev1.ConditionFalse)
+}


@lburgazzoli I added that here

but that is the used in the DSC controller ? I'm a little confused about if this is a DSP concern or a generat DSC concern. in the first case, the DSP controlelr should set its own condition, the DSC controller can eventually copy them.

@VaishnaviHire do you remember what is the logic here ?

@jackdelahunt the UnmanagedArgoWorkFlowExists is also called here: https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/components/datasciencepipelines/datasciencepipelines.go#L108-L110

I am kind of confused but here is what I think you were saying?:

UnmanagedArgoWorkFlowExists can be a new action in the DSP controller

We can also set the first condition in the DSP controller

And if that condition is there we can set the component condition in the DSC controller

New commit has those changes with this logic translated into and action in the DSP controller but I have some questions:

I have this at the top of the new action, in terms of upgrading does this work the same way or because now this is an action do things act a bit differently

// Check preconditions if this is an upgrade if rr.Instance.GetStatus().Phase != status.PhaseReady { return nil }

I set the CapabilityDSPv2Argo condition for the DSC in this action which is what I think you were looking for, is updating the DSC from a component okay? Just wondering what are the rule around component/DSC interactions

The second part of SetExistingArgoCondition

status.SetComponentCondition(conditions, componentsv1.DataSciencePipelinesComponentName, status.ReconcileFailed, message, corev1.ConditionFalse)

I am not sure where to put this in the DSC, I think failing the new UnmanagedArgoWorkFlowExists action already cause the component's reconcile to be in a failed state because it is erroring. So it this already done for us when we process the componentErrors? Or maybe things are acting differently I am not sure

This is my first PR on the operator so my understanding is limited but hopefully growing 😃

jackdelahunt · 2024-11-11T09:26:04Z

@lburgazzoli rebasing and re-testing now

controllers/components/datasciencepipelines/datasciencepipelines_controller_actions.go

jackdelahunt · 2024-11-13T12:07:48Z

/retest

jackdelahunt · 2024-11-13T15:25:36Z

/retest

lburgazzoli · 2024-11-14T15:58:41Z

LGTM we would probably need to add a test for the preconditions step, but can be done as a follow up

I let @zdtsw doing the final review

zdtsw · 2024-11-14T16:05:31Z

apis/components/v1/datasciencepipelines_types.go

+	DataSciencePipelinesComponentName = "data-science-pipelines-operator"
+	// value should match whats set in the XValidation below
+	DataSciencePipelinesInstanceName = "default-datasciencepipelines"
+	DataSciencePipelinesKind         = "DataSciencePipelines"


do we want to use plural?

zdtsw · 2024-11-14T16:06:09Z

apis/components/v1/datasciencepipelines_types.go

@@ -71,3 +76,11 @@ type DataSciencePipelinesList struct {
 func init() {
 	SchemeBuilder.Register(&DataSciencePipelines{}, &DataSciencePipelinesList{})
 }
+
+// DSCDataSciencePipelines contains all the configuration exposed in DSC instance for DataSciencePipelines component
+type DSCDataSciencePipelines struct {


same for here

zdtsw · 2024-11-14T16:08:18Z

tests/e2e/datasciencepipelines_test.go

+	}
+
+	if err = tc.testCtx.wait(func(ctx context.Context) (bool, error) {
+		// Verify ray CR is deleted


zdtsw · 2024-11-14T16:09:35Z

tests/e2e/datasciencepipelines_test.go

+	})
+
+	if err != nil {
+		return fmt.Errorf("unable to find Ray CR instance: %w", err)


zdtsw · 2024-11-14T16:17:59Z

controllers/datasciencecluster/kubebuilder_rbac.go

@@ -115,6 +115,11 @@ package datasciencecluster
 // +kubebuilder:rbac:groups="user.openshift.io",resources=users,verbs=list;watch;patch;delete;get
 // +kubebuilder:rbac:groups="console.openshift.io",resources=consolelinks,verbs=create;get;patch;delete

+// DataSciencePipelines


zdtsw · 2024-11-14T16:23:31Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -662,10 +656,10 @@ func (r *DataScienceClusterReconciler) getRequestName(ctx context.Context) (stri
 // argoWorkflowCRDPredicates filters the delete events to trigger reconcile when Argo Workflow CRD is deleted.
 var argoWorkflowCRDPredicates = predicate.Funcs{
 	DeleteFunc: func(e event.DeleteEvent) bool {
-		if e.Object.GetName() == datasciencepipelines.ArgoWorkflowCRD {
+		if e.Object.GetName() == datasciencepipelinesctrl.ArgoWorkflowCRD {


i am thinking, should not this argoWorkflowCRDPredicates moved into component, instead of having in DSC controller?

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

openshift-ci bot requested review from AjayJagan and ykaliuta November 4, 2024 20:37

lburgazzoli reviewed Nov 5, 2024

View reviewed changes

config/crd/bases/components.opendatahub.io_codeflares.yaml Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Nov 5, 2024

lburgazzoli reviewed Nov 5, 2024

View reviewed changes

bundle/manifests/opendatahub-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

jackdelahunt commented Nov 5, 2024

View reviewed changes

tests/e2e/helper_test.go Show resolved Hide resolved

jackdelahunt force-pushed the datasciencepipelines-refactor branch from bc7f2c7 to cb49d4b Compare November 5, 2024 09:39

openshift-merge-robot removed the needs-rebase label Nov 5, 2024

lburgazzoli reviewed Nov 5, 2024

View reviewed changes

controllers/components/datasciencepipelines/datasciencepipelines_controller.go Show resolved Hide resolved

jackdelahunt force-pushed the datasciencepipelines-refactor branch 3 times, most recently from 911378c to 836fa5d Compare November 5, 2024 14:12

openshift-merge-robot added the needs-rebase label Nov 5, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 836fa5d to 6e1164c Compare November 5, 2024 15:32

openshift-merge-robot removed the needs-rebase label Nov 5, 2024

lburgazzoli reviewed Nov 5, 2024

View reviewed changes

controllers/components/datasciencepipelines/datasciencepipelines_controller.go Outdated Show resolved Hide resolved

jackdelahunt force-pushed the datasciencepipelines-refactor branch from c2e67eb to f7178e9 Compare November 6, 2024 12:29

jackdelahunt commented Nov 6, 2024

View reviewed changes

jackdelahunt force-pushed the datasciencepipelines-refactor branch from f7178e9 to 138cd52 Compare November 6, 2024 12:43

jackdelahunt changed the title ~~WIP refactor: datasciencepipelines component refactor~~ datasciencepipelines component refactor Nov 6, 2024

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

jackdelahunt force-pushed the datasciencepipelines-refactor branch from aae608e to 5f72374 Compare November 6, 2024 14:36

jackdelahunt commented Nov 6, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Nov 7, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from e6cc628 to 68f4aba Compare November 11, 2024 09:05

openshift-merge-robot removed the needs-rebase label Nov 11, 2024

lburgazzoli added the feature-operator-refactor label Nov 11, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 4c12f0a to f6ffc95 Compare November 11, 2024 09:30

lburgazzoli reviewed Nov 11, 2024

View reviewed changes

controllers/components/datasciencepipelines/datasciencepipelines_controller_actions.go Outdated Show resolved Hide resolved

lburgazzoli reviewed Nov 11, 2024

View reviewed changes

controllers/components/datasciencepipelines/datasciencepipelines_controller_actions.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Nov 11, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from c0e1c33 to 8acd4a2 Compare November 11, 2024 14:02

openshift-merge-robot removed the needs-rebase label Nov 11, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 8acd4a2 to 32989a4 Compare November 12, 2024 12:59

openshift-merge-robot added the needs-rebase label Nov 12, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 32989a4 to f4996a9 Compare November 12, 2024 13:11

openshift-merge-robot removed the needs-rebase label Nov 12, 2024

jackdelahunt force-pushed the datasciencepipelines-refactor branch 2 times, most recently from 4330711 to 0619f16 Compare November 12, 2024 16:31

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 0619f16 to 165f799 Compare November 13, 2024 12:26

refactor: datasciencepipelines component refactor

890e650

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 165f799 to 890e650 Compare November 14, 2024 12:34

jackdelahunt requested a review from lburgazzoli November 14, 2024 15:06

lburgazzoli requested a review from zdtsw November 14, 2024 15:58

zdtsw reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datasciencepipelines component refactor #1340

datasciencepipelines component refactor #1340

jackdelahunt commented Nov 4, 2024

openshift-ci bot commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

lburgazzoli commented Nov 5, 2024

jackdelahunt Nov 6, 2024

lburgazzoli commented Nov 6, 2024

jackdelahunt Nov 6, 2024

lburgazzoli Nov 6, 2024

lburgazzoli Nov 6, 2024

jackdelahunt Nov 7, 2024 •

edited

Loading

lburgazzoli Nov 7, 2024

jackdelahunt Nov 7, 2024 •

edited

Loading

jackdelahunt commented Nov 11, 2024

jackdelahunt commented Nov 13, 2024

jackdelahunt commented Nov 13, 2024

lburgazzoli commented Nov 14, 2024

zdtsw Nov 14, 2024

zdtsw Nov 14, 2024

zdtsw Nov 14, 2024

zdtsw Nov 14, 2024

zdtsw Nov 14, 2024

zdtsw Nov 14, 2024

datasciencepipelines component refactor #1340

Are you sure you want to change the base?

datasciencepipelines component refactor #1340

Conversation

jackdelahunt commented Nov 4, 2024

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Nov 4, 2024

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

lburgazzoli commented Nov 5, 2024

Choose a reason for hiding this comment

lburgazzoli commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdelahunt Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdelahunt Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

jackdelahunt commented Nov 11, 2024

jackdelahunt commented Nov 13, 2024

jackdelahunt commented Nov 13, 2024

lburgazzoli commented Nov 14, 2024

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 4, 2024 •

edited

Loading

jackdelahunt Nov 7, 2024 •

edited

Loading

jackdelahunt Nov 7, 2024 •

edited

Loading