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

Make Kopia file parallelism configurable #7000

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

qiuming-best
Copy link
Contributor

@qiuming-best qiuming-best commented Oct 23, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Implementation of #7005

  • Add one generic API struct for the configuration of the Uploader
  • Add ParallelFilesUpload configuration for Kopia UploadPolicy

Does your change fix a particular issue?

Fixes #(issue)
#6607

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 marked this pull request as draft October 23, 2023 02:11
@qiuming-best qiuming-best force-pushed the kopia-parallelism branch 2 times, most recently from 90f6393 to 9e0a893 Compare October 24, 2023 10:00
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

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

Comparison is base (5c958d8) 61.68% compared to head (c2d4495) 61.69%.

Files Patch % Lines
pkg/cmd/util/output/backup_describer.go 42.85% 4 Missing ⚠️
pkg/uploader/provider/restic.go 0.00% 2 Missing and 1 partial ⚠️
pkg/controller/data_upload_controller.go 0.00% 0 Missing and 1 partial ⚠️
pkg/controller/pod_volume_backup_controller.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7000   +/-   ##
=======================================
  Coverage   61.68%   61.69%           
=======================================
  Files         258      258           
  Lines       27613    27636   +23     
=======================================
+ Hits        17034    17050   +16     
- Misses       9387     9393    +6     
- Partials     1192     1193    +1     

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

@qiuming-best qiuming-best force-pushed the kopia-parallelism branch 4 times, most recently from f444e7f to d754946 Compare October 25, 2023 08:28
Signed-off-by: Ming <mqiu@vmware.com>
@qiuming-best qiuming-best marked this pull request as ready for review October 26, 2023 02:32
@github-actions github-actions bot requested a review from blackpiglet October 26, 2023 02:32
@@ -477,6 +477,15 @@ spec:
description: TTL is a time.Duration-parseable string describing how
long the Backup should be retained for.
type: string
uploaderConfig:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for a late comment, didn't get a chance to review the design.
just wondering if we evaluated between passing this value as part of backup/restore API vs making it a server setting?
Any rationales around this.
From my thinking, a user might not really wish to use different uploaderconfig for each backup and might just prefer to have it set once at server level. This might be sufficient for most use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anshulahuja98
Thank you for your suggestion; we have internally discussed this matter. Due to the various configuration options in Velero, the placement of these configurations is relatively unclear. We believe that some configurations can be placed at the server level, while others are associated with backup or restore. To clearly define the location of configurations, we have decided to place configurations related to backup or restore in the respective Backup or Restore Custom Resource (CR). Currently, aspects such as concurrency control and upcoming tasks like Kopia Sparse and Ignore Folder parameters are all linked to specific backup or restore operations and will therefore be placed in the corresponding CRs. Of course, we will also consider how to better distinguish where these configurations should be placed in the future.

blackpiglet
blackpiglet previously approved these changes Nov 22, 2023
Lyndon-Li
Lyndon-Li previously approved these changes Nov 22, 2023
@qiuming-best qiuming-best dismissed stale reviews from Lyndon-Li and blackpiglet via c2d4495 November 22, 2023 03:52
@qiuming-best qiuming-best merged commit b276564 into vmware-tanzu:main Nov 22, 2023
24 checks passed
@navilg
Copy link

navilg commented Jul 19, 2024

@Lyndon-Li @blackpiglet If we do not set this value/flag during backup/schedule creation, What will be default value of parallel files upload ? I tried to find it, didn't find anywhere in doc.

Also if there is any document you can point me to about difference between node-agent concurrency and parallel files upload.

Thanks

@blackpiglet
Copy link
Contributor

@navilg
By default, it should be 0, meaning the parallel set is not enabled.

@navilg
Copy link

navilg commented Jul 22, 2024

Ohkay. Does parallel set value 1 means two files can be uploaded at a time ?

@blackpiglet
Copy link
Contributor

blackpiglet commented Jul 22, 2024

AFAIK, 1 means only one parallel. 2 means two files can be uploaded concurrently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants