Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix data mover controller bugs #6616

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

qiuming-best
Copy link
Contributor

@qiuming-best qiuming-best commented Aug 7, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#6615
#6612
#6550
#6563
#6600

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.

@qiuming-best qiuming-best added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Aug 7, 2023
@qiuming-best qiuming-best force-pushed the add-accept-label branch 3 times, most recently from 80abc03 to 7005dcc Compare August 7, 2023 14:27
blackpiglet
blackpiglet previously approved these changes Aug 8, 2023
@qiuming-best qiuming-best changed the title Fix data mover bugs #6550 #6563 #6600 [WIP] Fix data mover bugs #6550 #6563 #6600 Aug 9, 2023
@qiuming-best qiuming-best force-pushed the add-accept-label branch 5 times, most recently from b50ba81 to 8c7c199 Compare August 9, 2023 14:01
@qiuming-best qiuming-best changed the title [WIP] Fix data mover bugs #6550 #6563 #6600 [WIP] Fix data mover bugs Aug 10, 2023
}
return ctrl.Result{}, errors.Wrap(err, "getting datadownload")
}
if dd.Spec.Cancel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't check for dd.Spec.Cancel but for isDataDownloadInFinalState(dd).
For all terminal statuses, we need to do this, e.g., when prepare timeout happens, the CR will be set to Failed instead of Cancelled or dd.Spec.Cancel = true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


// we need to clean up resources as resources created in Expose it later than cancel action
// and need to clean up resources again
if du.Spec.Cancel {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, check for isDataUploadInFinalState(du)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

func ExclusiveUpdateDataUpload(ctx context.Context, client client.Client, du *velerov2alpha1api.DataUpload,
updateFunc func(*velerov2alpha1api.DataUpload)) (*velerov2alpha1api.DataUpload, bool, error) {
original := du.DeepCopy()
updateFunc(du)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExclusiveUpdateDataUpload is a fundermental function, we don't expect all the callers expect a updated du, so it is risky to update the du directly.

Actually, I suggest we leave ExclusiveUpdateDataUpload as is, and change the function of acceptDataUpload, the latter is more pure and it is safe to change it.

Copy link
Contributor Author

@qiuming-best qiuming-best Aug 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed back to exclusiveUpdateDataUpload.

But for this function, in the update action, it is not a general usage that coping it before updating, leading to many related UT tests failing with conflict errors.

So I removed the DeepCopy function for the UT, I know your original purpose would be not willing to update the du directly, but for our current usage scenario there is no effect as we always get the latest CR from the API server instead of memory in each Reconcile

@codecov
Copy link

codecov bot commented Aug 13, 2023

Codecov Report

Merging #6616 (5485616) into main (06628cf) will increase coverage by 0.13%.
Report is 27 commits behind head on main.
The diff coverage is 61.88%.

@@            Coverage Diff             @@
##             main    #6616      +/-   ##
==========================================
+ Coverage   60.18%   60.31%   +0.13%     
==========================================
  Files         242      242              
  Lines       25639    25924     +285     
==========================================
+ Hits        15431    15637     +206     
- Misses       9135     9186      +51     
- Partials     1073     1101      +28     
Files Changed Coverage Δ
pkg/cmd/cli/nodeagent/server.go 8.91% <0.00%> (-0.12%) ⬇️
pkg/cmd/server/server.go 21.80% <0.00%> (-0.12%) ⬇️
pkg/controller/data_upload_controller.go 68.25% <63.51%> (+2.28%) ⬆️
pkg/controller/data_download_controller.go 75.18% <73.45%> (+1.08%) ⬆️

... and 10 files with indirect coverage changes

@qiuming-best qiuming-best force-pushed the add-accept-label branch 3 times, most recently from b409b69 to bbbdbcd Compare August 14, 2023 08:35
@qiuming-best qiuming-best changed the title [WIP] Fix data mover bugs Fix data mover controller bugs Aug 14, 2023
@qiuming-best qiuming-best merged commit 411bd54 into vmware-tanzu:main Aug 15, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants