Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted #6914

shubham-pampattiwar · 2023-10-03T21:16:55Z

Thank you for contributing to Velero!

Please add a summary of your change

Adds changes proposed here: #6913 (comment)

Does your change fix a particular issue?

Fixes #6913

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

codecov · 2023-10-03T21:29:15Z

Codecov Report

Merging #6914 (ee271b7) into main (ad114f8) will increase coverage by 0.25%.
Report is 34 commits behind head on main.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #6914      +/-   ##
==========================================
+ Coverage   60.78%   61.04%   +0.25%     
==========================================
  Files         250      252       +2     
  Lines       26629    26855     +226     
==========================================
+ Hits        16187    16394     +207     
- Misses       9293     9306      +13     
- Partials     1149     1155       +6

Files	Coverage Δ
pkg/controller/data_download_controller.go	`75.27% <100.00%> (ø)`
pkg/controller/data_upload_controller.go	`68.38% <66.66%> (-0.07%)`	⬇️

... and 20 files with indirect coverage changes

kaovilai

Could be useful to future data movers to keep Canceling phase

pkg/apis/velero/v2alpha1/data_upload_types.go

sseago · 2023-10-04T18:12:48Z

@kaovilai The DU/DD controllers don't do anything with this phase, so I'm not sure what value there is in defining it as valid.

Lyndon-Li · 2023-10-11T00:53:04Z

pkg/controller/data_upload_controller.go

 			original := du.DeepCopy()
-			du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceling


There are two cases:

If the dataPath is still there (fsBackup is not nil), we cannot update the DUCR to cancelled directly. Instead, we need to notify the dataPath (by calling fsBackup.Cancel()), wait for it to cancel the data transfer and then it will call back OnDataUploadCancelled

If the dataPath is not there, we need to cancel the DUCR in the main flow

Both of the cases must be supported. However:

For the existing code, case 2 is not supported

For the changes in this PR, case 1 will not be supported

So please reconstruct the code to support the both case

Lyndon-Li · 2023-10-11T00:54:15Z

pkg/controller/data_upload_controller.go

 			original := du.DeepCopy()
-			du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceling
+			du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceled


For case 2, instead of patching the DUCR directly here, let's call OnDataUploadCancelled instead, it does something more:

Update the completionTimeStamp

Cleanup intermediate object

@shubham-pampattiwar I think the updated version still doesn't handle case 2.
When there is no fsbackup, then we set Canceled directly, but when there is, we need to cancel the fsbackup rather than just setting the phase.

@shubham-pampattiwar
For 1, we need to call fsBackup.Cancel() and NOT set du.Status.Phase (the same as the original code).
For 2, we just need to call OnDataUploadCancelled

Lyndon-Li · 2023-10-11T00:56:29Z

Please also rebase the code, the codespell error will disappear.

Lyndon-Li · 2023-10-11T00:57:47Z

@shubham-pampattiwar As discussed, please also help to take care of the DataDownload part.

shubham-pampattiwar · 2023-10-12T22:47:41Z

@Lyndon-Li @sseago Added fix for data download controller too, PTAL, Thanks !

shubham-pampattiwar · 2023-10-25T16:37:04Z

@Lyndon-Li Updated the PR, Please take another look.

mateusoliveira43 · 2023-10-25T16:39:16Z

pkg/controller/data_download_controller.go

 				return ctrl.Result{}, nil
 			}
-
 			log.Info("Data download is being canceled")


move this to line 290, to be similar to Data upload case?

Lyndon-Li · 2023-10-26T01:28:12Z

pkg/controller/data_download_controller.go

 			original := dd.DeepCopy()
-			dd.Status.Phase = velerov2alpha1api.DataDownloadPhaseCanceling
+			dd.Status.Phase = velerov2alpha1api.DataDownloadPhaseCanceled


Before fsRestore cancels itself, we should not set DD to DataDownloadPhaseCanceled. We need to wait fsRestore to cancel itself and then it calls OnDataDownloadCancelled.

So just rollback to the original code for this case.

Done, Updated the PR.

Lyndon-Li · 2023-10-26T01:28:44Z

pkg/controller/data_download_controller.go

@@ -289,18 +289,17 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		if dd.Spec.Cancel {
 			fsRestore := r.dataPathMgr.GetAsyncBR(dd.Name)
 			if fsRestore == nil {
+				r.OnDataDownloadCancelled(ctx, dd.GetNamespace(), dd.GetName())


Let's add a log here.

Re-positioned the earlier log

Lyndon-Li · 2023-10-26T01:31:29Z

pkg/controller/data_upload_controller.go

 			original := du.DeepCopy()
-			du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceling
+			du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceled


The same as DD, rollback to the original code and wait fsBackup to cancel itself

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> add changelog file Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> keep canceling phase const Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> fix data download as well Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> address PR feedback Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> minor fixes Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

github-actions bot requested review from anshulahuja98 and Lyndon-Li October 3, 2023 21:17

github-actions bot added the has-unit-tests label Oct 3, 2023

github-actions bot assigned shubham-pampattiwar Oct 3, 2023

shubham-pampattiwar force-pushed the fix-ducr-cancel branch from 87a8a6b to 4e203b2 Compare October 3, 2023 21:19

github-actions bot added the has-changelog label Oct 3, 2023

shubham-pampattiwar requested a review from sseago October 3, 2023 21:19

sseago previously approved these changes Oct 3, 2023

View reviewed changes

kaovilai suggested changes Oct 3, 2023

View reviewed changes

pkg/apis/velero/v2alpha1/data_upload_types.go Outdated Show resolved Hide resolved

shubham-pampattiwar dismissed sseago’s stale review via 4b54c89 October 4, 2023 17:34

shubham-pampattiwar force-pushed the fix-ducr-cancel branch from 4e203b2 to 4b54c89 Compare October 4, 2023 17:34

Lyndon-Li requested a review from qiuming-best October 10, 2023 07:08

sseago previously approved these changes Oct 10, 2023

View reviewed changes

Lyndon-Li reviewed Oct 11, 2023

View reviewed changes

shubham-pampattiwar dismissed sseago’s stale review via 6f9ce0b October 12, 2023 22:46

shubham-pampattiwar force-pushed the fix-ducr-cancel branch from 4b54c89 to 6f9ce0b Compare October 12, 2023 22:46

shubham-pampattiwar requested review from Lyndon-Li and sseago October 12, 2023 22:47

shubham-pampattiwar force-pushed the fix-ducr-cancel branch from 6f9ce0b to 8b91a91 Compare October 25, 2023 16:35

mateusoliveira43 reviewed Oct 25, 2023

View reviewed changes

Lyndon-Li reviewed Oct 26, 2023

View reviewed changes

shubham-pampattiwar force-pushed the fix-ducr-cancel branch from 8b91a91 to ee271b7 Compare October 26, 2023 04:39

github-actions bot removed the has-unit-tests label Oct 26, 2023

shubham-pampattiwar requested a review from Lyndon-Li October 26, 2023 04:40

Lyndon-Li approved these changes Oct 26, 2023

View reviewed changes

qiuming-best approved these changes Oct 26, 2023

View reviewed changes

sseago approved these changes Oct 26, 2023

View reviewed changes

sseago merged commit 1e0fc77 into vmware-tanzu:main Oct 26, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted #6914

Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted #6914

shubham-pampattiwar commented Oct 3, 2023

codecov bot commented Oct 3, 2023 •

edited

kaovilai left a comment •

edited

sseago commented Oct 4, 2023

Lyndon-Li Oct 11, 2023 •

edited

Lyndon-Li Oct 11, 2023

sseago Oct 12, 2023

Lyndon-Li Oct 13, 2023 •

edited

Lyndon-Li commented Oct 11, 2023

Lyndon-Li commented Oct 11, 2023

shubham-pampattiwar commented Oct 12, 2023

shubham-pampattiwar commented Oct 25, 2023

mateusoliveira43 Oct 25, 2023

Lyndon-Li Oct 26, 2023

shubham-pampattiwar Oct 26, 2023

Lyndon-Li Oct 26, 2023

shubham-pampattiwar Oct 26, 2023

Lyndon-Li Oct 26, 2023

shubham-pampattiwar Oct 26, 2023

		original := du.DeepCopy()
		du.Status.Phase = velerov2alpha1api.DataUploadPhaseCanceling

Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted #6914

Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted #6914

Conversation

shubham-pampattiwar commented Oct 3, 2023

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Oct 3, 2023 • edited

Codecov Report

kaovilai left a comment • edited

Choose a reason for hiding this comment

sseago commented Oct 4, 2023

Lyndon-Li Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyndon-Li Oct 13, 2023 • edited

Choose a reason for hiding this comment

Lyndon-Li commented Oct 11, 2023

Lyndon-Li commented Oct 11, 2023

shubham-pampattiwar commented Oct 12, 2023

shubham-pampattiwar commented Oct 25, 2023

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

codecov bot commented Oct 3, 2023 •

edited

kaovilai left a comment •

edited

Lyndon-Li Oct 11, 2023 •

edited

Lyndon-Li Oct 13, 2023 •

edited