Perf improvements for existing resource restore #6723

sseago · 2023-08-30T16:01:37Z

Use informer cache with dynamic client for Get calls on restore
When enabled, also make the Get call before create attempt.

Add server and install parameter to allow disabling this feature, but enable by default

This change results in significant restore performance improvements in cases where resources to be restored already exist in the cluster. As a test example, backed up a namespace with 31k secrets in it (the number was chosen to approximately match the total number of items in a real backup of a user who was concerned with restore performance numbers for the existing resource case).

Without this change:
restore into new namespace: 52 minutes
restore with all resources existing (existingResourcePolicy=none): 52 minutes
restore with all resources existing (existinResourcePolicy=update): 1 hour, 18 minutes

With this change:
restore into new namespace: 52 minutes
restore with all resources existing (existingResourcePolicy=none): 26 minutes
restore with all resources existing (existinResourcePolicy=update): 26 minutes

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

[x ] Accepted the DCO. Commits without the DCO will delay acceptance.
[x ] 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-08-30T16:58:48Z

Codecov Report

Merging #6723 (7750e12) into main (b316101) will increase coverage by 0.11%.
The diff coverage is 88.72%.

@@            Coverage Diff             @@
##             main    #6723      +/-   ##
==========================================
+ Coverage   60.69%   60.81%   +0.11%     
==========================================
  Files         249      249              
  Lines       26493    26607     +114     
==========================================
+ Hits        16081    16180      +99     
- Misses       9268     9278      +10     
- Partials     1144     1149       +5

Files	Coverage Δ
pkg/controller/restore_controller.go	`66.09% <100.00%> (+0.12%)`	⬆️
pkg/install/deployment.go	`88.79% <100.00%> (+0.33%)`	⬆️
pkg/restore/request.go	`100.00% <ø> (ø)`
pkg/cmd/server/server.go	`22.17% <66.66%> (+0.17%)`	⬆️
pkg/client/dynamic.go	`0.00% <0.00%> (ø)`
pkg/install/resources.go	`75.39% <0.00%> (-0.90%)`	⬇️
pkg/restore/restore.go	`65.93% <91.74%> (+1.54%)`	⬆️

weshayutin · 2023-09-05T15:39:53Z

@shubham-pampattiwar @kaovilai please review as you have time :)

pkg/test/fake_dynamic.go

pkg/cmd/server/server.go

Lyndon-Li · 2023-09-13T04:02:23Z

@sseago
Could you help to open an issue for this PR, since the 1.13 tasks are tracked by issues? We still need some time to review this PR, an issue could help us timely track the review status and priority.

sseago · 2023-09-19T22:11:32Z

@Lyndon-Li issue is here: #6845

pkg/restore/restore.go

Lyndon-Li · 2023-09-20T02:12:48Z

pkg/cmd/cli/install/install.go

@@ -120,6 +121,7 @@ func (o *Options) BindFlags(flags *pflag.FlagSet) {
 	flags.BoolVar(&o.DefaultVolumesToFsBackup, "default-volumes-to-fs-backup", o.DefaultVolumesToFsBackup, "Bool flag to configure Velero server to use pod volume file system backup by default for all volumes on all backups. Optional.")
 	flags.StringVar(&o.UploaderType, "uploader-type", o.UploaderType, fmt.Sprintf("The type of uploader to transfer the data of pod volumes, the supported values are '%s', '%s'", uploader.ResticType, uploader.KopiaType))
 	flags.BoolVar(&o.DefaultSnapshotMoveData, "default-snapshot-move-data", o.DefaultSnapshotMoveData, "Bool flag to configure Velero server to move data by default for all snapshots supporting data movement. Optional.")
+	flags.BoolVar(&o.UseInformerCacheForGet, "use-informer-cache-for-get", o.UseInformerCacheForGet, "Use informer cache for Get calls on restore. This will speed up restore in cases where there are backup resources which already exist in the cluster, but for very large clusters this will increase velero memory usage. Default is true. Optional.")


Can we give a user friendly name for this flag?

Any suggestions?

@Lyndon-Li @kaovilai I agree it's a bit awkward. But I couldn't come up with a better name at the time. Is shortening it to "use-informer-cache" OK? Maybe the "for get" is implied, since that's really the only call where the informer cache helps us much. I'll make that change unless someone comes up with a better name before I get to it.

yeah use-informer-cache sounds good, or maybe enable-informer-cache ?

Hmm. Actually, since we're defaulting it to enabled, maybe we should call it disable-informer-cache and if false, we use informer cache, if true we don't?

yeah works for me.

sseago · 2023-10-03T21:07:06Z

Updated based on review feedback.

mateusoliveira43 · 2023-10-05T14:52:41Z

pkg/cmd/cli/install/install.go

@@ -122,6 +123,7 @@ func (o *Options) BindFlags(flags *pflag.FlagSet) {
 	flags.BoolVar(&o.DefaultVolumesToFsBackup, "default-volumes-to-fs-backup", o.DefaultVolumesToFsBackup, "Bool flag to configure Velero server to use pod volume file system backup by default for all volumes on all backups. Optional.")
 	flags.StringVar(&o.UploaderType, "uploader-type", o.UploaderType, fmt.Sprintf("The type of uploader to transfer the data of pod volumes, the supported values are '%s', '%s'", uploader.ResticType, uploader.KopiaType))
 	flags.BoolVar(&o.DefaultSnapshotMoveData, "default-snapshot-move-data", o.DefaultSnapshotMoveData, "Bool flag to configure Velero server to move data by default for all snapshots supporting data movement. Optional.")
+	flags.BoolVar(&o.DisableInformerCache, "disable-informer-cache", o.DisableInformerCache, "Disable informer cache for Get calls on restore. With this enabled, it will speed up restore in cases where there are backup resources which already exist in the cluster, but for very large clusters this will increase velero memory usage. Default is false (don't disable). Optional.")


I think cobra already append default info to help

actually no it doesnt. it just appends what type things are.. ie string or bool.

right, so better to keep the default text, thanks @kaovilai !

Lyndon-Li · 2023-10-11T00:39:20Z

@sseago Could you help to rebase from the main branch? The codespell errors have been fixed in the latest code.

Use informer cache with dynamic client for Get calls on restore When enabled, also make the Get call before create. Add server and install parameter to allow disabling this feature, but enable by default Signed-off-by: Scott Seago <[email protected]>

sseago · 2023-10-11T15:03:23Z

I set DCO to pass because the errors that showed up weren't for my PR.

Commit sha: [7f73aca](https://github.com/vmware-tanzu/velero/pull/6723/commits/7f73acab1644f7170779b7225a5d7336b20f0868), Author: Guang Jiong Lou, Committer: GitHub; Can not find "Guang Jiong Lou [[email protected]](mailto:[email protected])", in ["lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])", "lou [[email protected]](mailto:[email protected])"].
Commit sha: [e5e99c7](https://github.com/vmware-tanzu/velero/pull/6723/commits/e5e99c75a0cb2e0958fabd54707db82137073b99), Author: Yang Gang, Committer: GitHub; Expected "Yang Gang [[email protected]](mailto:[email protected])", but got "yanggang [[email protected]](mailto:[email protected])".

sseago · 2023-10-11T15:13:10Z

@Lyndon-Li done

weshayutin · 2024-01-05T20:54:14Z

@sseago I believe we wanted to CP this to v.1.11 for oadp-1.2

kaovilai · 2024-01-08T06:21:26Z

@weshayutin @sseago keep in mind that there's an issue with the functionality noted in #7263

sseago · 2024-01-08T16:52:50Z

@kaovilai Looks like the bug in #7263 was only introduced in 1.13 development, although that's been fixed.

sseago marked this pull request as draft August 30, 2023 16:01

github-actions bot requested review from Lyndon-Li and ywk253100 August 30, 2023 16:01

github-actions bot added the has-unit-tests label Aug 30, 2023

github-actions bot assigned sseago Aug 30, 2023

sseago marked this pull request as ready for review August 30, 2023 16:08

github-actions bot requested a review from blackpiglet August 30, 2023 16:08

sseago added the 1.13-candidate issue/pr that should be considered to target v1.13 minor release label Aug 30, 2023

sseago force-pushed the restore-get-perf branch from 76b72c1 to 346f742 Compare August 30, 2023 16:34

github-actions bot added the has-changelog label Aug 30, 2023

sseago force-pushed the restore-get-perf branch 2 times, most recently from 7c07591 to e2f2d83 Compare August 30, 2023 16:45

sseago force-pushed the restore-get-perf branch from e2f2d83 to 0425f4a Compare August 30, 2023 17:15

kaovilai requested changes Sep 5, 2023

View reviewed changes

pkg/test/fake_dynamic.go Outdated Show resolved Hide resolved

pkg/cmd/server/server.go Outdated Show resolved Hide resolved

sseago force-pushed the restore-get-perf branch from 0425f4a to 25ff2b4 Compare September 5, 2023 17:26

kaovilai requested changes Sep 5, 2023

View reviewed changes

pkg/cmd/server/server.go Outdated Show resolved Hide resolved

sseago force-pushed the restore-get-perf branch from 25ff2b4 to 106074b Compare September 5, 2023 19:17

kaovilai approved these changes Sep 5, 2023

View reviewed changes

sseago force-pushed the restore-get-perf branch from 106074b to b9b3105 Compare September 8, 2023 14:46

shubham-pampattiwar previously approved these changes Sep 8, 2023

View reviewed changes

reasonerjt added this to the v1.13 milestone Sep 13, 2023

blackpiglet previously approved these changes Sep 14, 2023

View reviewed changes

mateusoliveira43 mentioned this pull request Sep 19, 2023

Design: ACM performance use case - restore 30k secrets openshift/oadp-operator#1044

Closed

6 tasks

sseago mentioned this pull request Sep 19, 2023

Restore of existing resources is too slow #6845

Closed

reasonerjt removed the 1.13-candidate issue/pr that should be considered to target v1.13 minor release label Sep 20, 2023

Lyndon-Li previously approved these changes Sep 28, 2023

View reviewed changes

sseago dismissed stale reviews from Lyndon-Li, blackpiglet, and shubham-pampattiwar via f0f8f58 October 3, 2023 21:06

sseago force-pushed the restore-get-perf branch from b9b3105 to f0f8f58 Compare October 3, 2023 21:06

sseago force-pushed the restore-get-perf branch 2 times, most recently from cf7904f to 930f0dd Compare October 3, 2023 21:12

shubham-pampattiwar approved these changes Oct 4, 2023

View reviewed changes

mateusoliveira43 reviewed Oct 5, 2023

View reviewed changes

kaovilai approved these changes Oct 5, 2023

View reviewed changes

Lyndon-Li added the target/1.12.1 label Oct 11, 2023

sseago force-pushed the restore-get-perf branch from 930f0dd to 7750e12 Compare October 11, 2023 14:53

Lyndon-Li approved these changes Oct 12, 2023

View reviewed changes

shubham-pampattiwar merged commit ad114f8 into vmware-tanzu:main Oct 12, 2023
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf improvements for existing resource restore #6723

Perf improvements for existing resource restore #6723

sseago commented Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

weshayutin commented Sep 5, 2023

Lyndon-Li commented Sep 13, 2023

sseago commented Sep 19, 2023

Lyndon-Li Sep 20, 2023

kaovilai Sep 28, 2023

sseago Sep 28, 2023

shubham-pampattiwar Sep 28, 2023

sseago Sep 28, 2023

shubham-pampattiwar Sep 28, 2023

sseago commented Oct 3, 2023

mateusoliveira43 Oct 5, 2023

kaovilai Oct 5, 2023

mateusoliveira43 Oct 5, 2023

Lyndon-Li commented Oct 11, 2023

sseago commented Oct 11, 2023

sseago commented Oct 11, 2023

weshayutin commented Jan 5, 2024

kaovilai commented Jan 8, 2024

sseago commented Jan 8, 2024

Perf improvements for existing resource restore #6723

Perf improvements for existing resource restore #6723

Conversation

sseago commented Aug 30, 2023

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

weshayutin commented Sep 5, 2023

Lyndon-Li commented Sep 13, 2023

sseago commented Sep 19, 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

sseago commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyndon-Li commented Oct 11, 2023

sseago commented Oct 11, 2023

sseago commented Oct 11, 2023

weshayutin commented Jan 5, 2024

kaovilai commented Jan 8, 2024

sseago commented Jan 8, 2024

codecov bot commented Aug 30, 2023 •

edited

Loading