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

OADP-645: VSR performance improvements #37

Merged
merged 6 commits into from Sep 16, 2022

Conversation

eemcmullan
Copy link

@eemcmullan eemcmullan commented Aug 5, 2022

image: quay.io/emcmulla/csi-plugin:perf

To test:

  • take backup using data mover of an app with multiple PVCs, such as robot-shop
  • delete app and VSCs after backup completes
  • in DPA, override CSI image with quay.io/emcmulla/csi-plugin:perf and Velero image with quay.io/emcmulla/velero:perf
  • create restore with data mover
  • in Velero logs, you should notice waiting for all VSRs to complete simultaneously

@openshift-ci 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 Aug 5, 2022
@eemcmullan eemcmullan changed the title VSR performance improvements OADP-645: VSR performance improvements Aug 5, 2022
@eemcmullan eemcmullan marked this pull request as ready for review August 29, 2022 21:17
@openshift-ci 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 Aug 29, 2022
@weshayutin
Copy link

@eemcmullan is it possible to post some testing instructions in the comments here. I'd like to run through taking a volume snapshot and restore w/ and w/o this patch.

Thank you in advance!

@eemcmullan
Copy link
Author

@weshayutin updated description with instructions

util.WaitVolumeSnapshotBackup: "true",
},
},
Driver: "ebs.csi.aws.com",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be hardcoded to ebs? What about other providers?

Copy link
Author

@eemcmullan eemcmullan Sep 8, 2022

Choose a reason for hiding this comment

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

This driver isn't actually used for anything since this VSClass is only needed for blocking VSR. It is only there because creating a VSClass will fail without this field. However I need to look into whether or not this will error on other drivers not available, such as Azure.

Copy link
Member

Choose a reason for hiding this comment

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

A safer one IMO would be to copy an existing voluemsnapshotclass or get driver name from existing storageclass, change the name, and proceed. This would be a more generic way to achieve what you want across providers.

Copy link
Member

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

Tested the changes on latest volsync main branch, worked for me ! Just a couple of nits ! Thank you @eemcmullan

},
},
// dummy driver as it is not used, but the field is required
Driver: "foo",
Copy link
Member

Choose a reason for hiding this comment

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

This works on 4.11? If yes then we're good to go.

Copy link
Author

Choose a reason for hiding this comment

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

tested with success on 4.11

@eemcmullan eemcmullan merged commit e69b7dd into openshift:main Sep 16, 2022
@eemcmullan
Copy link
Author

/cherry-pick oadp-1.1

@openshift-cherrypick-robot

@eemcmullan: #37 failed to apply on top of branch "oadp-1.1":

Applying: VSR perf improvements
Using index info to reconstruct a base tree...
M	internal/util/labels_annotations.go
M	internal/util/util.go
Falling back to patching base and 3-way merge...
Auto-merging internal/util/util.go
CONFLICT (content): Merge conflict in internal/util/util.go
Auto-merging internal/util/labels_annotations.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 VSR perf improvements
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick oadp-1.1

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.

@eemcmullan eemcmullan deleted the vsr-perf branch February 1, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants