Allow sparse option for Kopia & Restic restore #7141

qiuming-best · 2023-11-23T08:04:29Z

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#6664

Modify the uploader API
Modify DataMoverConfig field type in DataDownload into map[string]string
Modify the ParallelFilesUpload API for Kopia & Restic backup
Add sparse option for Kopia & Restic restore

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-24T09:35:58Z

Codecov Report

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

Comparison is base (85482ae) 61.77% compared to head (1a237d3) 61.78%.
Report is 34 commits behind head on main.

Files	Patch %	Lines
pkg/uploader/provider/restic.go	42.85%	12 Missing and 4 partials ⚠️
pkg/uploader/kopia/snapshot.go	47.61%	9 Missing and 2 partials ⚠️
pkg/cmd/util/output/restore_describer.go	0.00%	7 Missing ⚠️
pkg/podvolume/restorer.go	0.00%	3 Missing and 1 partial ⚠️
pkg/uploader/util/uploader_config.go	90.00%	2 Missing and 1 partial ⚠️
pkg/cmd/util/output/backup_describer.go	33.33%	2 Missing ⚠️
pkg/controller/data_download_controller.go	0.00%	1 Missing ⚠️
pkg/controller/data_upload_controller.go	50.00%	0 Missing and 1 partial ⚠️
pkg/controller/pod_volume_backup_controller.go	0.00%	0 Missing and 1 partial ⚠️
pkg/controller/pod_volume_restore_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #7141    +/-   ##
========================================
  Coverage   61.77%   61.78%            
========================================
  Files         259      262     +3     
  Lines       27859    28167   +308     
========================================
+ Hits        17210    17403   +193     
- Misses       9445     9534    +89     
- Partials     1204     1230    +26

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

draghuram

Also note that the new uploaded config needs to be added at https://velero.io/docs/v1.12/api-types/restore/.

draghuram · 2023-11-27T18:14:24Z

design/velero-uploader-configuration.md

@@ -122,5 +122,44 @@ Roughly, the process is as follows:
 3. Each respective controller within the CRs calls the uploader, and the ParallelFilesUpload from UploaderConfig in CRs is passed to the uploader. 
 4. When the uploader subsequently calls the Kopia API, it can use the ParallelFilesUpload to set the MaxParallelFileReads parameter, and if the uploader calls the Restic command it would output one warning log for Restic does not support this feature.

+### Sparse Option For Kopia & Restic Restore
+In many system files, there are numerous zero bytes or empty blocks that still occupy physical storage space. Sparse backup employs a more intelligent approach by only backing up the actual data-containing portions. For those empty blocks or zero bytes, it merely records their presence without actually storing them. This can significantly reduce the storage space required for backups, especially in situations where there is a substantial amount of empty data in large file systems.


This is describing the backup but this section is about sparse restore?

I've removed the design, and I'll add the design later

design/velero-uploader-configuration.md

config/crd/v1/bases/velero.io_backups.yaml

pkg/cmd/cli/restore/create.go

pkg/controller/data_upload_controller.go

…pport-restore-sparse

config/crd/v1/bases/velero.io_backups.yaml

reasonerjt · 2023-12-06T04:01:51Z

pkg/apis/velero/v1/backup_types.go

+	UploaderConfigForBackup *UploaderConfigForBackup `json:"uploaderConfig,omitempty"`
+}
+
+// UploaderConfigForBackup defines the configuration for the backup.


inaccurate comment.

reasonerjt · 2023-12-06T04:11:01Z

pkg/cmd/util/output/restore_describer.go

@@ -177,6 +177,11 @@ func DescribeRestore(ctx context.Context, kbClient kbclient.Client, restore *vel
 		d.Println()
 		d.Printf("Preserve Service NodePorts:\t%s\n", BoolPointerString(restore.Spec.PreserveNodePorts, "false", "true", "auto"))

+		if restore.Spec.UploaderConfigForRestore != nil && *restore.Spec.UploaderConfigForRestore.WriteSparseFiles {


There is a risk of nil pointer b/c restore.Spec.UploaderConfigForRestore.WriteSparseFiles may also be nil, you should use boolptr.IsSetToTrue

reasonerjt · 2023-12-06T04:13:01Z

pkg/apis/velero/v1/restore_types.go

+	// UploaderConfig specifies the configuration for the restore.
+	// +optional
+	// +nullable
+	UploaderConfigForRestore *UploaderConfigForRestore `json:"uploaderConfig,omitempty"`


Trivial: for simplicity and consistency with json key and comment, the name of the Field in the struct should better be UploaderConfig, there won't be confusion, b/c normally the code would look like

restore.Spec.UploaderConfig .....

reasonerjt · 2023-12-06T04:19:45Z

pkg/util/uploaderconfig/uploaderconfig.go

+}
+
+func StoreRestoreConfig(config *velerov1api.UploaderConfigForRestore) map[string]string {
+	data := make(map[string]string)


Will this cause inconsistency for different uploaders?
Can we be sure that the default value is always false for different uploaders?

reasonerjt · 2023-12-06T04:23:42Z

pkg/util/uploaderconfig/uploaderconfig.go

+	return data
+}
+
+func GetBackupConfig(data map[string]string) (velerov1api.UploaderConfigForBackup, error) {


I'm not sure if reusing velerov1api.UploaderConfigForBackup is a good approach, there may be settings in PVB that is meaningful to the uploader but is not exposed in backup CR.

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

reasonerjt

lgtm

github-actions bot requested a review from shubham-pampattiwar November 23, 2023 08:04

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

github-actions bot requested a review from sseago November 23, 2023 08:04

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

qiuming-best marked this pull request as draft November 23, 2023 08:04

qiuming-best force-pushed the support-restore-sparse branch from af6adc7 to b2f8bbe Compare November 24, 2023 02:16

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

qiuming-best force-pushed the support-restore-sparse branch 8 times, most recently from fe45ccd to 076c9a3 Compare November 24, 2023 09:31

qiuming-best force-pushed the support-restore-sparse branch from 076c9a3 to 7cff2f1 Compare November 24, 2023 10:33

github-actions bot added the Area/Design Design Documents label Nov 24, 2023

qiuming-best force-pushed the support-restore-sparse branch 2 times, most recently from a34a85f to dfb8935 Compare November 24, 2023 10:37

qiuming-best marked this pull request as ready for review November 27, 2023 02:00

github-actions bot requested review from anshulahuja98 and reasonerjt November 27, 2023 02:00

qiuming-best requested review from Lyndon-Li, blackpiglet and ywk253100 November 27, 2023 02:05

qiuming-best force-pushed the support-restore-sparse branch from dfb8935 to fd77ba8 Compare November 27, 2023 02:10

draghuram reviewed Nov 27, 2023

View reviewed changes

qiuming-best force-pushed the support-restore-sparse branch from fd77ba8 to 0571fd2 Compare November 28, 2023 06:38

qiuming-best changed the title ~~Allow sparse option for Kopia & Restic restore~~ [WIP] Allow sparse option for Kopia & Restic restore Nov 28, 2023

qiuming-best force-pushed the support-restore-sparse branch 5 times, most recently from ae3461c to bfc6b68 Compare November 30, 2023 05:37

github-actions bot removed the Area/Design Design Documents label Nov 30, 2023

qiuming-best force-pushed the support-restore-sparse branch from bfc6b68 to 0fc7e5f Compare November 30, 2023 05:42

qiuming-best changed the title ~~[WIP] Allow sparse option for Kopia & Restic restore~~ Allow sparse option for Kopia & Restic restore Nov 30, 2023

Lyndon-Li reviewed Nov 30, 2023

View reviewed changes

pkg/cmd/cli/restore/create.go Outdated Show resolved Hide resolved

pkg/controller/data_upload_controller.go Outdated Show resolved Hide resolved

qiuming-best force-pushed the support-restore-sparse branch from 0fc7e5f to 7b95b3c Compare November 30, 2023 10:49

Merge branch 'main' of https://github.com/qiuming-best/velero into su…

0afaa70

…pport-restore-sparse

qiuming-best force-pushed the support-restore-sparse branch from 7b95b3c to 0afaa70 Compare November 30, 2023 10:56

reasonerjt added the target/FC label Dec 4, 2023

reasonerjt added this to the v1.13 milestone Dec 4, 2023

reasonerjt reviewed Dec 4, 2023

View reviewed changes

config/crd/v1/bases/velero.io_backups.yaml Outdated Show resolved Hide resolved

qiuming-best force-pushed the support-restore-sparse branch 4 times, most recently from 939def8 to ded375c Compare December 5, 2023 08:15

reasonerjt reviewed Dec 6, 2023

View reviewed changes

qiuming-best force-pushed the support-restore-sparse branch from ded375c to 2f00ea9 Compare December 6, 2023 08:37

Update API

1a237d3

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

qiuming-best force-pushed the support-restore-sparse branch from 2f00ea9 to 1a237d3 Compare December 6, 2023 08:59

reasonerjt approved these changes Dec 6, 2023

View reviewed changes

Lyndon-Li approved these changes Dec 6, 2023

View reviewed changes

Lyndon-Li merged commit 099acd2 into vmware-tanzu:main Dec 6, 2023
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sparse option for Kopia & Restic restore #7141

Allow sparse option for Kopia & Restic restore #7141

qiuming-best commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

draghuram left a comment

draghuram Nov 27, 2023

qiuming-best Dec 1, 2023

reasonerjt Dec 6, 2023

qiuming-best Dec 6, 2023

reasonerjt Dec 6, 2023

qiuming-best Dec 6, 2023

reasonerjt Dec 6, 2023

qiuming-best Dec 6, 2023

reasonerjt Dec 6, 2023

qiuming-best Dec 6, 2023

reasonerjt Dec 6, 2023 •

edited

Loading

qiuming-best Dec 6, 2023

reasonerjt left a comment

Allow sparse option for Kopia & Restic restore #7141

Allow sparse option for Kopia & Restic restore #7141

Conversation

qiuming-best commented Nov 23, 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 24, 2023 • edited Loading

Codecov Report

draghuram left a 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

reasonerjt Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reasonerjt left a comment

Choose a reason for hiding this comment

qiuming-best commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 24, 2023 •

edited

Loading

reasonerjt Dec 6, 2023 •

edited

Loading