Add check for existing Argo Workflows #955

VaishnaviHire · 2024-04-04T22:20:20Z

Description

Jira Issue: https://issues.redhat.com/browse/RHOAIENG-4570
This PR introduces changes to handle existing Argo workflows for new install and upgrade. Following is the expected workflow

For version n

1. New Install
    - Argo Workflow exists
        - All components in n deployed successfully except DSP
        - New Dashboard(n) shows warning banner

2. Upgrade from n-1 --> n
   - Argo workflow exists and DSP is enabled
      - Do not upgrade any component. All components in (n-1)
      - Dashboard will not show any warning
      - DSC(Operator) will trigger events stating remove Argo or disable DSP to upgrade to new version
      - Note, (n-1) manifests are no longer managed here

How Has This Been Tested?

New Install

Deploy Workflow CRD .
Deploy operator using catalogsource: quay.io/vhire/opendatahub-operator-catalog:v2.10.1
Create DSCI and DSC with DataSciecnePipelines enabled
Verify all components except DSP are updated
Verify conditiontype CapabilityDSPv2Argo is triggered .
Verify DSP status is False

Upgrade

Install an ODH version with v1 dsp pipelines(e.g uay.io/vhire/opendatahub-operator-catalog:v2.7.0-711)
Deploy Workflow CRD
Create DSCi and DSC with DataSciecnePipelines enabled
Replace catalog source image with new version quay.io/vhire/opendatahub-operator-catalog:v2.10.1
Verify no components are upgraded to new version
Verify conditiontype CapabilityDSPv2Argo is triggered .
Verify conditiontype Upgradeable is set to False

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
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

zdtsw

some nitty gritty but i am fine with the change.

one more thing, what will happen from Dashboard side.
if we keep throwing error of "CRD already there, go remove it" will user still be able to see the feature of enable DSPV2 from dashboard?

components/datasciencepipelines/datasciencepipelines.go

zdtsw · 2024-04-05T05:51:13Z

/test opendatahub-operator-e2e

VaishnaviHire · 2024-04-05T14:21:36Z

some nitty gritty but i am fine with the change.

one more thing, what will happen from Dashboard side. if we keep throwing error of "CRD already there, go remove it" will user still be able to see the feature of enable DSPV2 from dashboard?

We still will have v1 manifests deployed. @lucferbux Do you know what will be the dashboard behavior in this case?

kywalker-rh · 2024-04-05T14:30:18Z

Can you provide a screenshot of where/when this will be shown? Also, it would be good to provide the user with a link to further documentation about why exactly they need to remove the CRD/operator.

zdtsw · 2024-04-05T14:33:55Z

/onhold
just to get more review comments, feel free to remove label @VaishnaviHire

VaishnaviHire · 2024-04-05T16:08:56Z

Can you provide a screenshot of where/when this will be shown? Also, it would be good to provide the user with a link to further documentation about why exactly they need to remove the CRD/operator.

This is shown under Details for DataScienceCluster resource. I have attached a screen grab
CheckArgoWorkflow.mov.zip

To resolve this error users will need to delete the Workflow crd by running -

oc delete crd workflows.argoproj.io

kywalker-rh · 2024-04-05T18:49:07Z

Thanks for sharing that @VaishnaviHire, I'm worried that's pretty buried though. Will an admin typically check those statuses after starting an upgrade?

With the status set as Ready I wouldn't expect to need to dig into it further to find out something failed. I want to prevent users being frustrated and confused as to why Pipelines won't work. Would it make sense to add a Banner for admins to see in the dashboard?

zdtsw · 2024-04-06T07:15:23Z

Thanks for sharing that @VaishnaviHire, I'm worried that's pretty buried though. Will an admin typically check those statuses after starting an upgrade?

With the status set as Ready I wouldn't expect to need to dig into it further to find out something failed. I want to prevent users being frustrated and confused as to why Pipelines won't work. Would it make sense to add a Banner for admins to see in the dashboard?

should we move this discussion into original JIRA to get more eyes on?
changes on Dashboard side, will need UI team's work

zdtsw · 2024-04-12T05:50:38Z

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -281,12 +296,15 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context
 		instance = r.reportError(err, instance, "failed to reconcile "+componentName+" on DataScienceCluster")
 		instance, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dsc.DataScienceCluster) {
 			if enabled {
-				status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, fmt.Sprintf("Component reconciliation failed: %v", err), corev1.ConditionFalse)
+				if componentName == datasciencepipelines.ComponentName && strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD) {


we probably no need check componentname here,

Suggested change

if componentName == datasciencepipelines.ComponentName && strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD) {

if strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD) {

Gkrumbach07 · 2024-04-12T14:55:01Z

This condition looks good from my side where we are testing against:

condition.type === 'CapabilityDSPv2Argo' && condition.status === 'False',

zdtsw · 2024-04-12T14:56:18Z

tested again with latest code in the negative case

openshift-ci · 2024-04-12T15:17:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asanzgom, zdtsw

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~~ [asanzgom,zdtsw]

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

openshift-ci · 2024-04-12T17:57:59Z

New changes are detected. LGTM label has been removed.

VaishnaviHire · 2024-04-12T19:11:16Z

@etirelli Added the error phase

openshift-ci · 2024-04-12T21:22:35Z

New changes are detected. LGTM label has been removed.

* Add check for existing Argo Workflows * Move resource cleanup before DSC creation * Add separate workflow for upgrades and installs * Remove conditions that are not required * Remove check for DSP component * Add more specific error comparison * Fix linter * Add Error phase * Fix linter (cherry picked from commit 7389fcb)

bartoszmajsak · 2024-04-15T09:20:06Z

controllers/datasciencecluster/datasciencecluster_controller.go

+	DeleteFunc: func(e event.DeleteEvent) bool {
+		if e.Object.GetName() == datasciencepipelines.ArgoWorkflowCRD {
+			labels := e.Object.GetLabels()
+			if _, ok := labels["app.opendatahub.io/"+datasciencepipelines.ComponentName]; !ok {


We could use labels.ODH.Component(datasciencepipelines.ComponentName)

i have it updated in https://github.com/opendatahub-io/opendatahub-operator/pull/967/files#diff-ddd4fc79031732c6d6cbc28c4f3fae64ee035ff75633e6d19097f717f36311b6

openshift-ci bot requested review from andrewballantyne and etirelli April 4, 2024 22:20

VaishnaviHire requested review from zdtsw and removed request for etirelli and andrewballantyne April 4, 2024 22:20

VaishnaviHire force-pushed the add_dsp_precond branch from d605260 to a4ba3e9 Compare April 4, 2024 23:23

VaishnaviHire requested review from gregsheremeta, etirelli and HumairAK April 4, 2024 23:24

zdtsw reviewed Apr 5, 2024

View reviewed changes

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

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

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

VaishnaviHire force-pushed the add_dsp_precond branch from a4ba3e9 to 0adabf8 Compare April 5, 2024 14:26

zdtsw approved these changes Apr 5, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Apr 5, 2024

openshift-ci bot added lgtm approved labels Apr 5, 2024

VaishnaviHire added the do-not-merge/hold label Apr 5, 2024

VaishnaviHire force-pushed the add_dsp_precond branch from 0adabf8 to 847f35f Compare April 12, 2024 03:04

openshift-ci bot removed the lgtm label Apr 12, 2024

VaishnaviHire requested review from Gkrumbach07 and asanzgom April 12, 2024 03:13

VaishnaviHire force-pushed the add_dsp_precond branch from 847f35f to 1ca992f Compare April 12, 2024 03:27

zdtsw reviewed Apr 12, 2024

View reviewed changes

zdtsw removed the approved label Apr 12, 2024

openshift-ci bot added the lgtm label Apr 12, 2024

VaishnaviHire force-pushed the add_dsp_precond branch from 3b4a902 to c866a19 Compare April 12, 2024 14:47

openshift-ci bot removed the lgtm label Apr 12, 2024

asanzgom approved these changes Apr 12, 2024

View reviewed changes

openshift-ci bot added the lgtm label Apr 12, 2024

openshift-ci bot removed the lgtm label Apr 12, 2024

VaishnaviHire added lgtm and removed do-not-merge/hold labels Apr 12, 2024

VaishnaviHire added 9 commits April 12, 2024 17:22

Add check for existing Argo Workflows

a362918

Move resource cleanup before DSC creation

a691d70

Add separate workflow for upgrades and installs

69cf659

Remove conditions that are not required

a419b69

Remove check for DSP component

7283d39

Add more specific error comparison

b52efb1

Fix linter

effba14

Add Error phase

3d14e02

Fix linter

525c610

VaishnaviHire force-pushed the add_dsp_precond branch from ff12f53 to 525c610 Compare April 12, 2024 21:22

openshift-ci bot removed the lgtm label Apr 12, 2024

VaishnaviHire merged commit 7389fcb into opendatahub-io:incubation Apr 12, 2024
6 of 8 checks passed

VaishnaviHire mentioned this pull request Apr 12, 2024

[04/12/24] Sync upstream changes red-hat-data-services/rhods-operator#243

Merged

zdtsw mentioned this pull request Apr 13, 2024

fix(dspv2): label should be check on existance and value as well #967

Merged

3 tasks

bartoszmajsak reviewed Apr 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for existing Argo Workflows #955

Add check for existing Argo Workflows #955

VaishnaviHire commented Apr 4, 2024 •

edited

zdtsw left a comment

zdtsw commented Apr 5, 2024

VaishnaviHire commented Apr 5, 2024

kywalker-rh commented Apr 5, 2024

zdtsw commented Apr 5, 2024

VaishnaviHire commented Apr 5, 2024 •

edited

kywalker-rh commented Apr 5, 2024

zdtsw commented Apr 6, 2024

zdtsw Apr 12, 2024

Gkrumbach07 commented Apr 12, 2024

zdtsw commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

VaishnaviHire commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

bartoszmajsak Apr 15, 2024

zdtsw Apr 15, 2024

	if componentName == datasciencepipelines.ComponentName && strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD) {
	if strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD) {

Add check for existing Argo Workflows #955

Add check for existing Argo Workflows #955

Conversation

VaishnaviHire commented Apr 4, 2024 • edited

Description

How Has This Been Tested?

New Install

Upgrade

Merge criteria:

zdtsw left a comment

Choose a reason for hiding this comment

zdtsw commented Apr 5, 2024

VaishnaviHire commented Apr 5, 2024

kywalker-rh commented Apr 5, 2024

zdtsw commented Apr 5, 2024

VaishnaviHire commented Apr 5, 2024 • edited

kywalker-rh commented Apr 5, 2024

zdtsw commented Apr 6, 2024

zdtsw Apr 12, 2024

Choose a reason for hiding this comment

Gkrumbach07 commented Apr 12, 2024

zdtsw commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

VaishnaviHire commented Apr 12, 2024

openshift-ci bot commented Apr 12, 2024

bartoszmajsak Apr 15, 2024

Choose a reason for hiding this comment

zdtsw Apr 15, 2024

Choose a reason for hiding this comment

VaishnaviHire commented Apr 4, 2024 •

edited

VaishnaviHire commented Apr 5, 2024 •

edited