Delete related VSRs after restore completes. #269

mrnold · 2023-05-16T19:37:30Z

OADP-1872: delete VSRs after a restore completes, and clean up VSRs from completed restores and restores that no longer exist.

openshift-ci · 2023-05-16T19:39:43Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Matthew Arnold <marnold@redhat.com>

openshift-ci · 2023-05-18T19:37:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mrnold

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

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold · 2023-05-18T21:51:40Z

Okay, I think this satisfies all the requirements, and it's working okay. It is very slow though, so I will keep working to try to have it not hammer the API so hard.

shubham-pampattiwar · 2023-05-18T22:08:39Z

pkg/datamover/datamover.go

+		return err
+	}
+
+	for _, restore := range restoreList.Items {


We are calling the CleanupRestoreVSRs function for the restores with completed phase, IMO we should only cleanup VSRs for this current restore and not touch any other VSRs.

@shubham-pampattiwar Hmm. I guess that depends on whether we want to clean up old non-cleaned restores or not. If we're only ever concerned with removing VSRs for the current restore, then this should be simpler.

clean up only the vsr's associated w/ the restore in process. Matching the vsb and vsr behavior would be the right thing to do. Customers can clean any other $vsr's themselves.

I put this back to only removing VSRs for the current restore.

This reverts commit bfbb435. Signed-off-by: Matthew Arnold <marnold@redhat.com>

This reverts commit 8582710. Signed-off-by: Matthew Arnold <marnold@redhat.com>

sseago · 2023-05-19T14:13:52Z

pkg/controller/restore_controller.go

@@ -575,6 +576,9 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu
 			r.logger.Debug("Restore completed")
 			restore.Status.Phase = api.RestorePhaseCompleted
 			r.metrics.RegisterRestoreSuccess(restore.Spec.ScheduleName)
+			if err := datamover.DeleteVSRsIfComplete(restore.Name, r.logger); err != nil {
+				r.logger.WithError(err).Error("Error removing VSRs after completed restore")
+			}


We need the same above after line 568/569, otherwise a single restore error will prevent VSR cleanup.

Also, what if there are inProgress operations, does the cleanup just never happen, or is there another place which will call the same code?

I think I have addressed these with cc1c0 and 7e77a now, let me know whether or not that's what you had in mind.

sseago · 2023-05-19T14:22:57Z

pkg/datamover/datamover.go

+				// check for a failed VSR
+				for _, cond := range currentVSR.Status.Conditions {
+					if cond.Status == metav1.ConditionFalse && cond.Reason == ReconciledReasonError && cond.Type == ConditionReconciled && (currentVSR.Status.Phase == SnapMoverBackupPhaseFailed || currentVSR.Status.Phase == SnapMoverBackupPhasePartiallyFailed) {
+						return false, errors.Errorf("volumesnapshotrestore %s has failed status", currentVSR.Name)


Why do we return error if VSR failed? This causes the check command to fail out, which means we only clean up VSRs if they're all successful. Is this the desired behavior, or do we always want to clean up post-restore?

@sseago I think we decided to only clean up completed CRs for debugging purposes

Disregard this comment, as we want the current behavior here.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

shubham-pampattiwar · 2023-05-19T16:16:54Z

@mrnold Unit tests are failing please make sure you sun make update and make test` locally and see if there are any changes needed

sseago · 2023-05-19T16:38:53Z

pkg/controller/restore_operations_controller.go

+			phase = " partially failed "
+		}
+		if err = datamover.DeleteVSRsIfComplete(restore.Name, log); err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "error cleaning up after%srestore", phase)


I don't think we really need the extra phase text in the message. That makes the flow slightly more confusing, and it doesn't really add much.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

openshift-ci · 2023-05-19T19:33:15Z

@mrnold: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/unit-test	`734897b`	link	false	`/test unit-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

weshayutin · 2023-05-19T22:08:00Z

The unit test https://github.com/openshift/velero/blob/konveyor-dev/pkg/controller/restore_operations_controller_test.go#L70

https://github.com/openshift/velero/blob/konveyor-dev/pkg/controller/restore_operations_controller_test.go#L308

is failing via cli invocation, goland invocation but passing vscode.
this is silly..
waiving test for 1.2 branch

weshayutin · 2023-05-19T22:08:34Z

@sseago @kaovilai @mrnold @shubham-pampattiwar on a call waiving.

mrnold mentioned this pull request May 16, 2023

Delete VSR when matching restore is complete. migtools/volume-snapshot-mover#218

Closed

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2023

Delete VSRs after completed Restore.

571322d

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold force-pushed the oadp1872 branch from 2dfcd7f to 571322d Compare May 18, 2023 17:49

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2023

mrnold changed the base branch from konveyor-dev to oadp-1.2 May 18, 2023 17:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2023

Clean up VSRs from all completed Restores.

8582710

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold force-pushed the oadp1872 branch from 7ed2dc6 to 8582710 Compare May 18, 2023 20:12

Remove VSRs from non-existent Restores.

bfbb435

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold marked this pull request as ready for review May 18, 2023 21:52

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2023

shubham-pampattiwar reviewed May 18, 2023

View reviewed changes

mrnold added 2 commits May 18, 2023 20:06

Revert "Remove VSRs from non-existent Restores."

1d0f575

This reverts commit bfbb435. Signed-off-by: Matthew Arnold <marnold@redhat.com>

Revert "Clean up VSRs from all completed Restores."

f0a9bf1

This reverts commit 8582710. Signed-off-by: Matthew Arnold <marnold@redhat.com>

sseago reviewed May 19, 2023

View reviewed changes

mrnold added 2 commits May 19, 2023 11:42

Also clean up VSRs after partially failed restore.

cc1c0c2

Signed-off-by: Matthew Arnold <marnold@redhat.com>

Also clean VSRs after restore plugin operations.

7e77a30

Signed-off-by: Matthew Arnold <marnold@redhat.com>

sseago reviewed May 19, 2023

View reviewed changes

Remove phase string from error message.

734897b

Signed-off-by: Matthew Arnold <marnold@redhat.com>

weshayutin merged commit a147573 into openshift:oadp-1.2 May 19, 2023
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete related VSRs after restore completes. #269

Delete related VSRs after restore completes. #269

mrnold commented May 16, 2023 •

edited by openshift-ci bot

openshift-ci bot commented May 16, 2023

openshift-ci bot commented May 18, 2023

mrnold commented May 18, 2023

shubham-pampattiwar May 18, 2023

sseago May 18, 2023

weshayutin May 18, 2023 •

edited

mrnold May 19, 2023

sseago May 19, 2023

sseago May 19, 2023

mrnold May 19, 2023

sseago May 19, 2023

eemcmullan May 19, 2023

sseago May 19, 2023

shubham-pampattiwar commented May 19, 2023

sseago May 19, 2023

openshift-ci bot commented May 19, 2023

weshayutin commented May 19, 2023

weshayutin commented May 19, 2023

Delete related VSRs after restore completes. #269

Delete related VSRs after restore completes. #269

Conversation

mrnold commented May 16, 2023 • edited by openshift-ci bot

openshift-ci bot commented May 16, 2023

openshift-ci bot commented May 18, 2023

mrnold commented May 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshayutin May 18, 2023 • edited

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

shubham-pampattiwar commented May 19, 2023

Choose a reason for hiding this comment

openshift-ci bot commented May 19, 2023

weshayutin commented May 19, 2023

weshayutin commented May 19, 2023

mrnold commented May 16, 2023 •

edited by openshift-ci bot

weshayutin May 18, 2023 •

edited