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

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

Codecov Report

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

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

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

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1340   +/-   ##
============================================================
  Coverage                             ?   26.63%           
============================================================
  Files                                ?       55           
  Lines                                ?     4464           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1189           
  Misses                               ?     3134           
  Partials                             ?      141

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

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-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?

I thought for DSP that it is always plural, but maybe I am wrong.

Ray operator == DataSciencePipelines operator ?

no, Ray is Ray, DSP is DSP, two different components

No sorry I didn't mean they are the same, when you refer to ray operator you say Ray, when you refer to DSP operator you say DataSciencePipelines, you use plural not singular. But I could be wrong with that, it is just that I seen that before and I thought I would copy that way

i was thinking here:

TypeMeta: metav1.TypeMeta{ Kind: componentsv1.DataSciencePipelinesKind, ============> DataSciencePipelines or DataSciencePipeline APIVersion: componentsv1.GroupVersion.String(), },

but now i see https://github.com/jackdelahunt/opendatahub-operator/blob/datasciencepipelines-refactor/bundle/manifests/components.opendatahub.io_datasciencepipelines.yaml#L13-L14
both singular and pulural are set to the same value :D
idk, let @lburgazzoli to tell if it should be
kind: DataSciencePipelines
or
kind: DataSciencePipeline

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

tests/e2e/datasciencepipelines_test.go

controllers/datasciencecluster/kubebuilder_rbac.go

controllers/datasciencecluster/datasciencecluster_controller.go

grdryn

Generally looks good to me 🚀

controllers/components/datasciencepipelines/datasciencepipelines_controller.go

tests/e2e/datasciencepipelines_test.go

openshift-ci · 2024-11-19T15:26:15Z

[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

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

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

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

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 890e650 to c2f6d0f Compare November 18, 2024 12:05

grdryn reviewed Nov 19, 2024

View reviewed changes

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

tests/e2e/datasciencepipelines_test.go Outdated Show resolved Hide resolved

tests/e2e/datasciencepipelines_test.go Outdated Show resolved Hide resolved

jackdelahunt force-pushed the datasciencepipelines-refactor branch 3 times, most recently from 739b5bf to 1ed662a Compare November 19, 2024 13:19

refactor: datasciencepipelines component refactor

1f7e63e

jackdelahunt force-pushed the datasciencepipelines-refactor branch from 1ed662a to 1f7e63e Compare November 19, 2024 15:03

lburgazzoli approved these changes Nov 19, 2024

View reviewed changes

openshift-ci bot assigned lburgazzoli Nov 19, 2024

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

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

openshift-merge-bot bot merged commit e980df6 into opendatahub-io:feature-operator-refactor Nov 19, 2024
10 checks passed

zdtsw mentioned this pull request Nov 27, 2024

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

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

datasciencepipelines component refactor #1340

datasciencepipelines component refactor #1340

jackdelahunt 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 13, 2024

jackdelahunt commented Nov 13, 2024

lburgazzoli commented Nov 14, 2024

zdtsw Nov 14, 2024

jackdelahunt Nov 18, 2024

zdtsw Nov 18, 2024

jackdelahunt Nov 18, 2024 •

edited

Loading

zdtsw Nov 18, 2024

zdtsw Nov 14, 2024

grdryn left a comment

openshift-ci bot commented Nov 19, 2024

datasciencepipelines component refactor #1340

datasciencepipelines component refactor #1340

Conversation

jackdelahunt commented Nov 4, 2024

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

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 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

jackdelahunt Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grdryn left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 19, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

jackdelahunt Nov 7, 2024 •

edited

Loading

jackdelahunt Nov 7, 2024 •

edited

Loading

jackdelahunt Nov 18, 2024 •

edited

Loading