Node agent restart enhancement #7130

qiuming-best · 2023-11-21T02:29:51Z

Thank you for contributing to Velero!

Please add a summary of your change

Enhance the recoverability of backup & restore in data-mover

We could redo the data upload or data download which CRs in the Prepared phases when the node-agent restarts instead of canceling directly.
For those accepted CRs, we can retrieve them by label velero.io/accepted-by and cancel it when node-agent restarts.

Does your change fix a particular issue?

Fixes #(issue)
#6727

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-11-21T07:43:43Z

Codecov Report

Attention: 74 lines in your changes are missing coverage. Please review.

Comparison is base (e58a780) 61.19% compared to head (98a56eb) 61.70%.
Report is 21 commits behind head on main.

Files	Patch %	Lines
pkg/controller/data_download_controller.go	41.07%	28 Missing and 5 partials ⚠️
pkg/controller/data_upload_controller.go	40.00%	28 Missing and 5 partials ⚠️
pkg/cmd/cli/nodeagent/server.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7130      +/-   ##
==========================================
+ Coverage   61.19%   61.70%   +0.50%     
==========================================
  Files         258      258              
  Lines       27366    27740     +374     
==========================================
+ Hits        16747    17117     +370     
+ Misses       9434     9420      -14     
- Partials     1185     1203      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anshulahuja98 · 2023-11-27T05:22:36Z

I am thinking if this could lead to other issues.

if you think restart of node agent is likely when it is running out of mem / cpu due to high load in backing up/restoring certain large volumes

Here since today we cancel it, we atleast terminate and fail early. With your changes if we re-put, we might again cause the node agent to restart. This might be a vicious cycle.

qiuming-best · 2023-11-27T07:24:52Z

I am thinking if this could lead to other issues.

if you think restart of node agent is likely when it is running out of mem / cpu due to high load in backing up/restoring certain large volumes

Here since today we cancel it, we atleast terminate and fail early. With your changes if we re-put, we might again cause the node agent to restart. This might be a vicious cycle.

yes, What you considered is really a problem. we should cancel the in progress CR directly

Lyndon-Li · 2023-11-27T08:43:12Z

pkg/controller/data_download_controller.go

+		err = UpdateDataDownloadWithRetry(ctx, cli, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name},
+			r.logger.WithField("dataupload", dd.Name), func(dataDownload *velerov2alpha1api.DataDownload) {
+				dataDownload.Spec.Cancel = true
+				dataDownload.Status.Message = fmt.Sprintf("found a datadownload %s/%s is being deleted, mark it as cancel", dd.Namespace, dd.Name)


Message is not correct?

Sorry, Updated

Lyndon-Li · 2023-11-27T08:44:16Z

pkg/controller/data_download_controller.go

+		if err != nil {
+			r.logger.WithError(err).Errorf("failed to set cancel flag with error %s", err.Error())
+		}
+		r.logger.Warn(dd.Status.Message)


Make this log ahead, otherwise, when UpdateDataDownloadWithRetry returns an error, failed to set cancel flag with error %s will be first printed, which is confusing.

Lyndon-Li · 2023-11-27T08:45:33Z

pkg/controller/data_download_controller.go

+					})
+
+				if err != nil {
+					logger.WithError(errors.WithStack(err)).Errorf("failed to mark datadownload %q into prepared", dd.GetName())


The message, into prepared should be into cancelled?

Lyndon-Li · 2023-11-27T08:46:42Z

pkg/controller/data_upload_controller.go

+			r.logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q cancel", du.GetName())
+			continue
+		}
+		r.logger.WithField("dataupload", du.GetName()).Warn(du.Status.Message)


The same, move it ahead

Lyndon-Li · 2023-11-27T08:47:12Z

pkg/controller/data_upload_controller.go

+					})
+
+				if err != nil {
+					logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q into prepared", du.GetName())


into cancelled

Lyndon-Li · 2023-11-27T08:47:36Z

pkg/controller/data_download_controller.go

+					logger.WithError(errors.WithStack(err)).Errorf("failed to mark datadownload %q into prepared", dd.GetName())
+					continue
+				}
+				logger.WithField("datadownload", dd.GetName()).Debug("mark datadownload into prepared")


into cancelled

Lyndon-Li · 2023-11-27T08:48:04Z

pkg/controller/data_upload_controller.go

+					logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q into prepared", du.GetName())
+					continue
+				}
+				logger.WithField("dataupload", du.GetName()).Debug("mark dataupload into prepared")


into cancelled

anshulahuja98 · 2023-11-28T04:45:45Z

pkg/controller/data_download_controller.go

+// CancelAcceptedDataDownload will cancel the accepted data download
+func (r *DataDownloadReconciler) CancelAcceptedDataDownload(ctx context.Context, cli client.Client, ns string) {
+	r.logger.Infof("Canceling accepted data for node %s", r.nodeName)
+	dataDownloads, err := r.findAcceptDataDownloadsByNodeLable(ctx, cli, ns)


nit: label

thanks, updated

Signed-off-by: Ming Qiu <mqiu@vmware.com>

github-actions bot requested review from reasonerjt and sseago November 21, 2023 02:30

github-actions bot added the has-unit-tests label Nov 21, 2023

github-actions bot assigned qiuming-best Nov 21, 2023

qiuming-best marked this pull request as draft November 21, 2023 02:30

qiuming-best force-pushed the data-mover-recoverbility branch from 780edca to 51bb5f6 Compare November 21, 2023 07:34

github-actions bot added the has-changelog label Nov 21, 2023

qiuming-best force-pushed the data-mover-recoverbility branch 2 times, most recently from 294d16a to cc9b9b1 Compare November 21, 2023 07:41

qiuming-best force-pushed the data-mover-recoverbility branch from cc9b9b1 to e7e5da4 Compare November 21, 2023 07:52

qiuming-best marked this pull request as ready for review November 21, 2023 08:18

github-actions bot requested a review from shubham-pampattiwar November 21, 2023 08:18

qiuming-best requested review from anshulahuja98, Lyndon-Li, blackpiglet and ywk253100 November 21, 2023 08:19

qiuming-best force-pushed the data-mover-recoverbility branch from e7e5da4 to 3df177e Compare November 27, 2023 02:26

qiuming-best force-pushed the data-mover-recoverbility branch 2 times, most recently from d6807d4 to 56233a4 Compare November 27, 2023 07:42

Lyndon-Li reviewed Nov 27, 2023

View reviewed changes

qiuming-best force-pushed the data-mover-recoverbility branch 2 times, most recently from 4a01c2d to 17e3354 Compare November 28, 2023 01:35

anshulahuja98 reviewed Nov 28, 2023

View reviewed changes

Node agent restart enhancement

98a56eb

Signed-off-by: Ming Qiu <mqiu@vmware.com>

qiuming-best force-pushed the data-mover-recoverbility branch from 17e3354 to 98a56eb Compare November 28, 2023 05:50

Lyndon-Li approved these changes Nov 28, 2023

View reviewed changes

ywk253100 approved these changes Nov 28, 2023

View reviewed changes

ywk253100 merged commit 6ac7ff1 into vmware-tanzu:main Nov 28, 2023
23 of 24 checks passed

sseago mentioned this pull request Dec 1, 2023

Adding new cluster nodes during backup/restore causes failed DataUploads/DataDownloads #7170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node agent restart enhancement #7130

Node agent restart enhancement #7130

qiuming-best commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

anshulahuja98 commented Nov 27, 2023

qiuming-best commented Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

Lyndon-Li Nov 27, 2023

qiuming-best Nov 27, 2023

anshulahuja98 Nov 28, 2023

qiuming-best Nov 28, 2023

Node agent restart enhancement #7130

Node agent restart enhancement #7130

Conversation

qiuming-best commented Nov 21, 2023 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

anshulahuja98 commented Nov 27, 2023

qiuming-best commented Nov 27, 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

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

qiuming-best commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading