Fix cant initiate failover of applicationSet based apps after hub recovery #861

GowthamShanmugam · 2023-05-23T16:27:18Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2209288

Fix explanation:

Earlier, Failover/Relocate component is using placement ownerRef to match placement and its placement decision. It is failing in some cases. So Switching to label-based matching.

Other errors observed from QE setup once after fixing the above one:
2. Failover/Relocation readiness text in UI should not rely only on DRPC peer-ready conditions and available conditions. It is common for all errors, So if any error occurs the readiness should display "Not Ready".
3. While Failing/Relocating ramen is removing the cluster name from the placement rule, and adding it back once the failover/relocation operation is completed. In between if the cluster went down, and if a user wants to trigger Failover, Then UI is unable to find out the target cluster and deployment cluster.
The fix is, Find the deployment cluster name from DRPC, which is only possible at the time of failover and relocation operation is ongoing.

If the status is in the FailingOver state then the target cluster can be found using failoverCluster spec from DRPC.
If the status is in the Relocating state then the target cluster can be found using preferedCluster spec from DRPC.
If a target cluster is found then finding a deployment cluster is possible using the list of DR Clusters.

GowthamShanmugam · 2023-05-23T16:27:26Z

/hold

openshift-ci · 2023-05-23T16:27:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam

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:

~~packages/mco/OWNERS~~ [GowthamShanmugam]

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

GowthamShanmugam · 2023-05-24T13:23:27Z

/hold cancel

SanjalKatiyar · 2023-05-24T17:12:51Z

packages/mco/utils/disaster-recovery.tsx

+      // While failing over failoverCluster cluster is the target cluster
+      // Find deployment cluster using dr cluster list
+    } else if (currStatus === DRPC_STATUS.FailingOver) {


nits

Suggested change

// While failing over failoverCluster cluster is the target cluster

// Find deployment cluster using dr cluster list

} else if (currStatus === DRPC_STATUS.FailingOver) {

} else if (currStatus === DRPC_STATUS.FailingOver) {

// While failing over failoverCluster cluster is the target cluster

// Find deployment cluster using dr cluster list

SanjalKatiyar · 2023-05-24T17:15:22Z

packages/mco/utils/disaster-recovery.tsx

+      const cluster = findCluster(
+        drClusters,
+        drpc?.spec?.preferredCluster,
+        false


this is confusing:

"matchCluster" argument for the "findCluster" function is false by default, so no need to pass "false" again.

second argument for the "findCluster" function is named as "deploymentClusterName", but in reality u want to figure out the deployment cluster, parameter that u are passing is target cluster naming convention is wrong/confusing).

correct, the name is wrong.

SanjalKatiyar · 2023-05-24T17:15:52Z

packages/mco/utils/disaster-recovery.tsx

+      const cluster = findCluster(
+        drClusters,
+        drpc?.spec?.failoverCluster,
+        false


same 2 points as above...

SanjalKatiyar · 2023-05-24T17:17:16Z

packages/mco/utils/disaster-recovery.tsx

@@ -488,3 +489,36 @@ export const filterPVCDataUsingAppsets = (

 export const filterDRAlerts = (alert: Alert) =>
  alert?.annotations?.alert_type === 'DisasterRecovery';
+
+export const findDeploymentClusterName = (
+  plsDecision: ACMPlacementDecisionKind,


no issue with current approach, but if "drpc" is the best way to determine the deployment cluster, do we even need to have check for "plsDecision" ?? Can't we always check based upon "drpc" only ??

PlacementDecision is our source of truth to find out the deployment cluster.

Finding the deployment cluster name from DRPC won't work always, User may accidently change the "Prefered cluster / Failover cluster" on the DRPC spec without changing the spec "Action".

Also, if we find from DRPC, these are the following challenges:

If the current DRPC status is Deployed, then the deployment cluster is a preferred cluster spec.

If the current DRPC status is Initiating, then:
If the Action is Failover, then the deploy deployment cluster is the opposite cluster from the failover cluster.
If the Action is Relocate, then the deploy deployment cluster is the opposite cluster from the preferred cluster.

If the current DRPC status is FailingOver, then the deployment cluster is the opposite cluster from the failover cluster.

If the current DRPC status is Relocating, then the deployment cluster is the opposite cluster from the preferred cluster.

If the current DRPC status is FailedOver, then the deployment cluster is the preferred cluster.

If the current DRPC status is Relocated, then the deployment cluster is the preferred cluster.

If any new state comes then we need to handle that also, If the user changes the preferred cluster/failover cluster/action spec then it's a problem.

Instead of checking these many conditions, I decided to check placementRule. Which always gives me the correct deployment cluster name.

But one corner case is happening here, ramen is removing the cluster names from placementRule, I decided to check DRPC only for this corner case, (i.e. Relocating/Failing Over). I am assuming that, while Relocating/FailingOver users won't change the DRPC spec.

Another alternate solution is to ask the user to input the cluster name from UI, But if we introduce bulk failover and relocate in the future this will be a problem.

Again it is not a solid solution, We need to discuss with the DR about this.

This fix will at least save us from raising blocker BZ-like failover/relocation is blocked (Or) like UI is not displaying target cluster name on the UI.

SanjalKatiyar · 2023-05-24T17:19:43Z

packages/mco/components/modals/app-failover-relocate/failover-relocate-modal.tsx

@@ -53,7 +53,7 @@ export const FailoverRelocateModal: React.FC<FailoverRelocateModalProps> = (
    ModalFooterStatus.INITIAL
  );
  const [placement, setPlacement] = React.useState<PlacementProps>({});
-  const [canInitiate, setCanInitiate] = React.useState(false);
+  const [canInitiate, setCanInitiate] = React.useState<Boolean>(undefined);


nits: if we are defining the type to be Boolean, then shouldn't; we initialise it with false ??

GowthamShanmugam · 2023-05-25T08:17:07Z

/test images

…overy BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2209288 Signed-off-by: Gowtham Shanmugasundaram <gshanmug@redhat.com>

SanjalKatiyar · 2023-05-25T08:24:56Z

/lgtm

GowthamShanmugam · 2023-05-25T09:58:42Z

/test odf-console-e2e-aws

GowthamShanmugam · 2023-05-25T13:07:10Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-05-25T14:31:14Z

/cherry-pick release-4.13

SanjalKatiyar · 2023-05-25T14:31:26Z

/cherry-pick release-4.13-compatibility

openshift-cherrypick-robot · 2023-05-25T14:31:54Z

@SanjalKatiyar: #861 failed to apply on top of branch "release-4.13":

Applying: Fix cant initiate failover of applicationSet based apps after hub recovery
Using index info to reconstruct a base tree...
M	packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
M	packages/mco/components/modals/app-failover-relocate/failover-relocate-modal-body.tsx
Falling back to patching base and 3-way merge...
Auto-merging packages/mco/components/modals/app-failover-relocate/failover-relocate-modal-body.tsx
Auto-merging packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
CONFLICT (content): Merge conflict in packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix cant initiate failover of applicationSet based apps after hub recovery
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2023-05-25T14:32:03Z

@SanjalKatiyar: #861 failed to apply on top of branch "release-4.13-compatibility":

Applying: Fix cant initiate failover of applicationSet based apps after hub recovery
Using index info to reconstruct a base tree...
M	packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
M	packages/mco/components/modals/app-failover-relocate/failover-relocate-modal-body.tsx
Falling back to patching base and 3-way merge...
Auto-merging packages/mco/components/modals/app-failover-relocate/failover-relocate-modal-body.tsx
Auto-merging packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
CONFLICT (content): Merge conflict in packages/mco/components/modals/app-failover-relocate/argo-application-set.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix cant initiate failover of applicationSet based apps after hub recovery
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13-compatibility

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/hold label May 23, 2023

openshift-ci bot added the approved label May 23, 2023

GowthamShanmugam force-pushed the bz_2209288 branch 5 times, most recently from 3fb47ac to 3ce32ef Compare May 24, 2023 13:08

openshift-ci bot removed the do-not-merge/hold label May 24, 2023

GowthamShanmugam changed the title ~~Fix cant initiate failover of applicationSet based apps after hub recovery - WIP~~ Fix cant initiate failover of applicationSet based apps after hub recovery May 24, 2023

SanjalKatiyar reviewed May 24, 2023

View reviewed changes

GowthamShanmugam force-pushed the bz_2209288 branch 2 times, most recently from a594bde to 1f0ea80 Compare May 25, 2023 08:06

Fix cant initiate failover of applicationSet based apps after hub rec…

e68ed2d

…overy BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2209288 Signed-off-by: Gowtham Shanmugasundaram <gshanmug@redhat.com>

GowthamShanmugam force-pushed the bz_2209288 branch from 1f0ea80 to e68ed2d Compare May 25, 2023 08:18

openshift-ci bot assigned SanjalKatiyar May 25, 2023

openshift-ci bot added the lgtm label May 25, 2023

openshift-merge-robot merged commit 11f4070 into red-hat-storage:master May 25, 2023
6 checks passed

SanjalKatiyar mentioned this pull request May 25, 2023

Bug 2209288: [release-4.13] Fix cant initiate failover of applicationSet based apps after hub recovery #867

Merged

This was referenced Aug 9, 2023

Bug 2209288: [release-4.14] After hub recovery, cant initiate failover #957

Merged

Bug 2209288: [release-4.14-compatibility] After hub recovery, cant initiate failover #956

Merged

Fix cant initiate failover of applicationSet based apps after hub recovery #861

Fix cant initiate failover of applicationSet based apps after hub recovery #861

GowthamShanmugam commented May 23, 2023 •

edited

GowthamShanmugam commented May 23, 2023

openshift-ci bot commented May 23, 2023

GowthamShanmugam commented May 24, 2023

SanjalKatiyar May 24, 2023

SanjalKatiyar May 24, 2023

GowthamShanmugam May 25, 2023

SanjalKatiyar May 24, 2023

SanjalKatiyar May 24, 2023

GowthamShanmugam May 25, 2023 •

edited

GowthamShanmugam May 25, 2023

SanjalKatiyar May 24, 2023

GowthamShanmugam commented May 25, 2023

SanjalKatiyar commented May 25, 2023

GowthamShanmugam commented May 25, 2023

GowthamShanmugam commented May 25, 2023

SanjalKatiyar commented May 25, 2023

SanjalKatiyar commented May 25, 2023

openshift-cherrypick-robot commented May 25, 2023

openshift-cherrypick-robot commented May 25, 2023

Fix cant initiate failover of applicationSet based apps after hub recovery #861

Fix cant initiate failover of applicationSet based apps after hub recovery #861

Conversation

GowthamShanmugam commented May 23, 2023 • edited

GowthamShanmugam commented May 23, 2023

openshift-ci bot commented May 23, 2023

GowthamShanmugam commented May 24, 2023

SanjalKatiyar May 24, 2023

Choose a reason for hiding this comment

SanjalKatiyar May 24, 2023

Choose a reason for hiding this comment

GowthamShanmugam May 25, 2023

Choose a reason for hiding this comment

SanjalKatiyar May 24, 2023

Choose a reason for hiding this comment

SanjalKatiyar May 24, 2023

Choose a reason for hiding this comment

GowthamShanmugam May 25, 2023 • edited

Choose a reason for hiding this comment

GowthamShanmugam May 25, 2023

Choose a reason for hiding this comment

SanjalKatiyar May 24, 2023

Choose a reason for hiding this comment

GowthamShanmugam commented May 25, 2023

SanjalKatiyar commented May 25, 2023

GowthamShanmugam commented May 25, 2023

GowthamShanmugam commented May 25, 2023

SanjalKatiyar commented May 25, 2023

SanjalKatiyar commented May 25, 2023

openshift-cherrypick-robot commented May 25, 2023

openshift-cherrypick-robot commented May 25, 2023

GowthamShanmugam commented May 23, 2023 •

edited

GowthamShanmugam May 25, 2023 •

edited