Essential checks introduction for hooks and groups in recipe #1795

asn1809 · 2025-02-03T20:11:40Z

When using recipe, hooks and group can be either essential or not. It is defined either as group.Essential or hook.Essential. It is used in combination with workflow.FailOn.

The supported values for FailOn are:

"any-error" -- Meaning for any of the error seen in the workflow steps mentioned in sequence, fail the workflow.
"essential-error" -- Meaning if hook or group is marked as essential and any of essential steps fail, then the workflow fails.
"full-error" -- Meaning if all the essential hooks or groups in the sequence fail, then only workflow fails. If any one of the essential passes, then the overall workflow should be marked as successful.

Fixes #1762
Fixes #1760

BenamarMk · 2025-02-18T10:56:14Z

internal/controller/volumereplicationgroup_controller.go

+	// to determine if the cluster data is protected. If it is nil, then it is
+	// considered as success. So we should set it as false here and set it as
+	// true if protection is not required or if protection is successful.
+	v.kubeObjectsCaptureFailed("KubeObjectsCaptureNotStarted", "Kube objects capture has not started")


This function is poorly named for this purpose. you should probably call directly kubeObjectsCaptureStatusFalse instead.

BenamarMk · 2025-02-18T11:34:33Z

internal/controller/vrg_kubeobjects.go

+
+		if isEssentialStep {
+			// shows that at least one essential step has succeeded
+			allEssentialStepsFailed = false


This is disabled for all captureSteps. In other words, if isEssentialSteps is always true, line 295 seems redundant. I'm not sure what the code is trying to determine.

ShyamsundarR · 2025-02-19T00:03:53Z

internal/controller/volumereplicationgroup_controller.go

@@ -1198,7 +1204,7 @@ func (v *VRGInstance) processAsPrimary() ctrl.Result {
 			v.log.Info("Kube objects restore failed", "error", err)
 			v.errorConditionLogAndSet(err, "Failed to restore kube objects", setVRGKubeObjectsErrorCondition)

-			return v.updateVRGStatus(v.result)
+			return v.updateVRGConditionsAndStatus(v.result)
 		}

 		v.log.Info("Kube objects restored")


Follow the shouldRestoreClusterData pattern and update status here, before progressing with further processing (which includes a new backup and VRG upload conditionally).

As e2e reports an error where restore of k8s resources is triggered twice, at this point returning to update status to ensure no further processing is done would be better as suggested by @raghavendra-talur

ShyamsundarR

Conditional LGTM based on addressing the early return as mentioned.

nirs · 2025-02-19T17:07:51Z

"full-error" -- Meaning if all the essential hooks or groups in the sequence fail, then only workflow fails. If any one of the essential passes, then the overall workflow should be marked as successful.

I don't understand this

nirs

The first commit is big and complicated and I don't fully understand it.

The second commit is smaller but I could not find the change described by the commit message. It looks like bunch of unrelated changes.

nirs · 2025-02-19T17:23:11Z

internal/controller/kubeobjects/velero/requests.go

@@ -232,7 +232,7 @@ func backupDummyStatusProcessAndRestore(
 	restoreName string,
 	labels map[string]string,
 ) (*velero.Restore, error) {
-	backupStatusLog(backup, w.log)
+	backupStatusLog("restore", backup, w.log)


Is this related to "initialize kubeObjectProtected condition to false"?

nirs · 2025-02-19T17:31:44Z

internal/controller/vrg_recipe.go

@@ -47,6 +49,7 @@ func captureWorkflowDefault(vrg ramen.VolumeReplicationGroup, ramenConfig ramen.

 	captureSpecs := []kubeobjects.CaptureSpec{
 		{
+			Name: DefaultCaptureRecoverGroupName,


Is this related to "initialize kubeObjectProtected condition to false"?

internal/controller/vrg_recipe.go

internal/controller/vrg_kubeobjects.go

internal/controller/volumereplicationgroup_controller.go

nirs · 2025-02-19T17:39:17Z

internal/controller/vrg_kubeobjects.go

@@ -210,52 +210,106 @@ func (v *VRGInstance) kubeObjectsCaptureStartOrResume(
 	captureNumber int64,
 	pathName, capturePathName, namePrefix, veleroNamespaceName string,
 	interval time.Duration,
-	labels map[string]string,


Why was this removed?

We seems to create new labels here, but it is not clear way this change is needed, and how it is related to supporting the new essential checks.

It is not related to essential checks but during the changes found that this refactoring would help reduce the arguments required as labels can be easily retrieved using util package using vrg instance.

nirs · 2025-02-19T17:41:37Z

internal/controller/vrg_kubeobjects.go

+	allEssentialStepsFailed, err := v.executeCaptureSteps(result, pathName, capturePathName, namePrefix,
+		veleroNamespaceName, captureInProgressStatusUpdate, annotations, requests, log)
+	if err != nil {
+		return


Return silently on errors? is the error already logged?

In case of error, vrg status is updated. If you suggest additional log, will add.

nirs · 2025-02-19T17:44:14Z

internal/controller/vrg_kubeobjects.go

+	annotations := map[string]string{vrgGenerationKey: strconv.FormatInt(generation, vrgGenerationNumberBase)}
+	labels := util.OwnerLabels(v.instance)
+
+	allEssentialStepsFailed, err := v.executeCaptureSteps(result, pathName, capturePathName, namePrefix,


Why do care about all essential steps failure and not about any essential failure? If something is essential than a failure should not be ignored.

Essentiality of steps (group/hook) depends on the workflow.FailOn and is expected work in combination.
If FailOn is "any-error", we don't care whether step is essential or not and fail immediately.

If FailOn is "essential-error", we look for essential step and if it fails, workflow is marked as failure. If all essential steps pass, workflow is treated as successful.

If FailOn is "full-error", we look for essential steps. Here, only if all the essential steps fail, workflow is treated as failure. In case any one essential passes, workflow is treated as successful.

nirs · 2025-02-19T17:55:57Z

internal/controller/vrg_kubeobjects.go

 				result, cg, pathName, capturePathName, namePrefix, veleroNamespaceName,
 				captureInProgressStatusUpdate,
 				labels, annotations, requests, log,
 			)
+			requestsCompletedCount += loopCount


loopCount is the number of requests completed by the call to v.kubeObjectsGroupCapture?

nirs · 2025-02-19T17:57:59Z

internal/controller/vrg_kubeobjects.go


-				return
+				return false, err


Why return false here in all case?

As mentioned here, in case failOn is "any-error", we should immediately stop the execution of the workflow.

raghavendra-talur · 2025-02-19T18:28:29Z

@nirs Yes, I was trying to do too many things in this PR. Moved the unrelated commits to a new PR #1848 .

Signed-off-by: Annaraya Narasagond <[email protected]>

asn1809 requested a review from raghavendra-talur February 3, 2025 20:11

asn1809 force-pushed the asn-hook-essential-fix branch 2 times, most recently from 0f931ff to c91198f Compare February 3, 2025 20:24

asn1809 requested review from ShyamsundarR and BenamarMk February 3, 2025 20:29

asn1809 force-pushed the asn-hook-essential-fix branch from c91198f to 7f4602b Compare February 4, 2025 05:42

asn1809 marked this pull request as ready for review February 4, 2025 05:47

asn1809 force-pushed the asn-hook-essential-fix branch 2 times, most recently from d94908e to 8ef861a Compare February 4, 2025 19:02

raghavendra-talur approved these changes Feb 5, 2025

View reviewed changes

asn1809 force-pushed the asn-hook-essential-fix branch from 8ef861a to 29fadac Compare February 7, 2025 18:33

raghavendra-talur force-pushed the asn-hook-essential-fix branch 4 times, most recently from cf4362b to ec06119 Compare February 17, 2025 18:57

BenamarMk reviewed Feb 18, 2025

View reviewed changes

ShyamsundarR reviewed Feb 19, 2025

View reviewed changes

raghavendra-talur force-pushed the asn-hook-essential-fix branch 2 times, most recently from 9f32517 to 924cf98 Compare February 19, 2025 12:37

nirs reviewed Feb 19, 2025

View reviewed changes

raghavendra-talur force-pushed the asn-hook-essential-fix branch from 924cf98 to b9b560e Compare February 19, 2025 18:27

asn1809 force-pushed the asn-hook-essential-fix branch 2 times, most recently from a842042 to 9ee584b Compare March 3, 2025 18:22

recipe: adding failOn and essential checks

a0658f1

Signed-off-by: Annaraya Narasagond <[email protected]>

asn1809 force-pushed the asn-hook-essential-fix branch from 9ee584b to a0658f1 Compare March 3, 2025 18:32

raghavendra-talur approved these changes Mar 3, 2025

View reviewed changes

raghavendra-talur merged commit 484d5eb into RamenDR:main Mar 3, 2025
23 checks passed

asn1809 deleted the asn-hook-essential-fix branch March 4, 2025 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Essential checks introduction for hooks and groups in recipe #1795

Essential checks introduction for hooks and groups in recipe #1795

asn1809 commented Feb 3, 2025 •

edited by nirs

Loading

BenamarMk Feb 18, 2025

BenamarMk Feb 18, 2025

ShyamsundarR Feb 19, 2025

ShyamsundarR left a comment

nirs commented Feb 19, 2025

nirs left a comment

nirs Feb 19, 2025

nirs Feb 19, 2025

nirs Feb 19, 2025

asn1809 Feb 20, 2025

nirs Feb 19, 2025

asn1809 Feb 20, 2025

nirs Feb 19, 2025

asn1809 Feb 20, 2025

nirs Feb 19, 2025

asn1809 Feb 20, 2025

nirs Feb 19, 2025

asn1809 Feb 20, 2025

raghavendra-talur commented Feb 19, 2025

Essential checks introduction for hooks and groups in recipe #1795

Essential checks introduction for hooks and groups in recipe #1795

Conversation

asn1809 commented Feb 3, 2025 • edited by nirs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR left a comment

Choose a reason for hiding this comment

nirs commented Feb 19, 2025

nirs left a 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

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

raghavendra-talur commented Feb 19, 2025

asn1809 commented Feb 3, 2025 •

edited by nirs

Loading