Uploader Implementation: Kopia backup and restore #5221

qiuming-best · 2022-08-16T10:05:37Z

Thank you for contributing to Velero!

Please add a summary of your change

Uploader Implementation: Kopia backup and restore
It's about uploader module implementation, which related to kopia implementation of backup and restore
which reference to #5074 #5073

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.

pkg/uploader/provider/kopia.go

pkg/repository/mocks/repository_writer.go

pkg/uploader/provider/kopia.go

pkg/uploader/provider/provider.go

pkg/uploader/kopia/snapshot.go

pkg/uploader/provider/kopia.go

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

…loader-kopia

codecov-commenter · 2022-08-19T08:20:46Z

Codecov Report

Merging #5221 (9cb7a15) into main (1ba7b3d) will decrease coverage by 0.39%.
The diff coverage is 39.55%.

@@            Coverage Diff             @@
##             main    #5221      +/-   ##
==========================================
- Coverage   41.79%   41.39%   -0.40%     
==========================================
  Files         225      231       +6     
  Lines       19172    19659     +487     
==========================================
+ Hits         8012     8137     +125     
- Misses      10574    10928     +354     
- Partials      586      594       +8

Impacted Files	Coverage Δ
pkg/cmd/util/output/backup_describer.go	`0.00% <0.00%> (ø)`
pkg/controller/pod_volume_backup_controller.go	`41.93% <0.00%> (-2.14%)`	⬇️
pkg/controller/pod_volume_restore_controller.go	`21.62% <0.00%> (-1.24%)`	⬇️
pkg/uploader/kopia/progress.go	`0.00% <0.00%> (ø)`
pkg/uploader/provider/provider.go	`0.00% <0.00%> (ø)`
pkg/uploader/provider/restic.go	`0.00% <0.00%> (ø)`
pkg/uploader/types.go	`100.00% <ø> (ø)`
pkg/uploader/kopia/snapshot.go	`37.50% <37.50%> (ø)`
pkg/uploader/provider/kopia.go	`54.38% <54.38%> (ø)`
pkg/uploader/kopia/shim.go	`0.00% <0.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ywk253100 · 2022-08-22T01:37:35Z

pkg/uploader/provider/kopia.go

+}
+
+//NewKopiaUploaderProvider initialized with open or create a repository
+func NewKopiaUploaderProvider(


Isn't the velerov1.BackupRepo a parameter of the function? Or how does the uploader know where to put the files?

And seems only the BSL's UID is used, how does the uploader know the access information(e.g. URL) of the storage?

Yes, I am considering this problem, we need to use BSL+VolumeNamespace to identify a repository. On the other hand, actually, if we have the velerov1.BackupRepo, it is enough to just use velerov1.BackupRepo's UUID to identify a repository, which is quite simple. However, in the current code, PVB/PVR doesn't get the velerov1.BackupRepo.

Let me consider this for some more time and discuss with you guys later. In the current PR, let's make it as is.

Let me add some more explanation here:

During the Connect operation of unified repo, a config file containing all the connecting information, except the credentials, is stored in a local file

During the backup/restore, as long as the uploader could provide the name of the config file, it could connect to the repo and then write/read data to/from the repo

The Connection operation is done during repo initialization, that is, by the velerov1.BackupRepo reconciler

We need a unique file name for the local config so that different repos don't mix up their configuration

At present, the config file is named by the BSL's UUID

As mentioned above BSL's UUID is not enough, velerov1.BackupRepo's UUID is more suitable, however, it is not currently available in PVB/PVR

yeah, I've changed into velerov1.BackupRepo's UUID

pkg/uploader/types.go

ywk253100 · 2022-08-22T02:14:35Z

pkg/uploader/provider/kopia.go

+//CheckContext check context status periodically
+//check if context is timeout or cancel
+func (kp *kopiaProvider) CheckContext(ctx context.Context) {
+	for {


The for loop should be stopped once the backup/restore is finished

ok, I've add one finishChan chan to stop the loop once finished

Seems the for loop isn't needed. By removing the default section, we can use select without the for loop

ywk253100 · 2022-08-22T02:16:10Z

pkg/uploader/provider/kopia.go

+		return errors.Wrapf(err, "Failed to run kopia restore")
+	}
+
+	updater.UpdateProgress(&uploader.UploaderProgress{


Why do we need to update the progress here? Could you add comments about it if it is needed indeed?

To ensure that the statistic data of TotalBytes is equal to BytesDone when finished, I've added one annotation

ywk253100 · 2022-08-22T02:21:23Z

pkg/uploader/provider/kopia.go

+	prorgess := new(kopia.KopiaProgress)
+	prorgess.InitThrottle(backupProgressCheckInterval)
+	prorgess.Updater = updater
+	kp.uploader.Progress = prorgess


Seems the Progress of the shared uploader will be overwritten when calling Runbackups()
more than once.

yes, I've removed the uploader from the kopiaProvider struct. which will make the progress is independent for every backup

ywk253100 · 2022-08-22T02:24:19Z

pkg/uploader/provider/kopia.go

+	bkRepo        udmrepo.BackupRepo
+	credGetter    *credentials.CredentialGetter
+	uploader      *snapshotfs.Uploader
+	restoreCancel chan struct{}


Is it only for one specific restore? If it is, it should be not the field of kopiaProvider as kopiaProvider are shared by different RunRestore() actions

I've removed it from the kopiaProvider struct.

ywk253100 · 2022-08-22T02:25:48Z

pkg/uploader/provider/kopia.go

+		"parentSnapshot": parentSnapshot,
+	})
+	repoWriter := kopia.NewShimRepo(kp.bkRepo)
+	kp.uploader = snapshotfs.NewUploader(repoWriter)


Is the uploader shared with other function calls?

I've removed uploader from kp

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

github-actions bot assigned qiuming-best Aug 16, 2022

github-actions bot added the has-unit-tests label Aug 16, 2022

github-actions bot requested review from dsu-igeek and Lyndon-Li August 16, 2022 10:06

qiuming-best force-pushed the uploader-kopia branch from 2ef9d12 to 37c646d Compare August 16, 2022 10:21

github-actions bot added the has-changelog label Aug 16, 2022

qiuming-best requested review from ywk253100 and reasonerjt and removed request for dsu-igeek August 17, 2022 06:45

Lyndon-Li reviewed Aug 17, 2022

View reviewed changes

qiuming-best force-pushed the uploader-kopia branch 3 times, most recently from ffe390d to 67c5d38 Compare August 18, 2022 03:40

Lyndon-Li reviewed Aug 18, 2022

View reviewed changes

qiuming-best added 2 commits August 19, 2022 06:39

Uploader Implementation: Kopia backup and restore

262de19

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

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

839c2ed

…loader-kopia

qiuming-best force-pushed the uploader-kopia branch 3 times, most recently from db0b2e8 to 4060bec Compare August 19, 2022 08:11

qiuming-best force-pushed the uploader-kopia branch 5 times, most recently from e69a372 to 8d5e38f Compare August 19, 2022 09:12

Lyndon-Li previously approved these changes Aug 19, 2022

View reviewed changes

ywk253100 reviewed Aug 22, 2022

View reviewed changes

pkg/uploader/types.go Show resolved Hide resolved

ywk253100 reviewed Aug 22, 2022

View reviewed changes

qiuming-best dismissed Lyndon-Li’s stale review via 69713c6 August 22, 2022 07:44

qiuming-best force-pushed the uploader-kopia branch 3 times, most recently from 272c30c to fee188f Compare August 22, 2022 09:15

update upstream codes

2bf054a

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

qiuming-best force-pushed the uploader-kopia branch from fee188f to 2bf054a Compare August 22, 2022 09:20

ywk253100 approved these changes Aug 22, 2022

View reviewed changes

Lyndon-Li approved these changes Aug 22, 2022

View reviewed changes

qiuming-best merged commit 082d680 into vmware-tanzu:main Aug 22, 2022

Lyndon-Li mentioned this pull request Sep 5, 2022

Velero Kopia Integration - Prevent stale configuration after BSL's storage config updated for Kopia path #5272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uploader Implementation: Kopia backup and restore #5221

Uploader Implementation: Kopia backup and restore #5221

qiuming-best commented Aug 16, 2022 •

edited

codecov-commenter commented Aug 19, 2022 •

edited

ywk253100 Aug 22, 2022

Lyndon-Li Aug 22, 2022 •

edited

Lyndon-Li Aug 22, 2022 •

edited

qiuming-best Aug 22, 2022

ywk253100 Aug 22, 2022

qiuming-best Aug 22, 2022 •

edited

ywk253100 Aug 22, 2022

ywk253100 Aug 22, 2022

qiuming-best Aug 22, 2022

ywk253100 Aug 22, 2022

qiuming-best Aug 22, 2022

ywk253100 Aug 22, 2022

qiuming-best Aug 22, 2022

ywk253100 Aug 22, 2022

qiuming-best Aug 22, 2022

Uploader Implementation: Kopia backup and restore #5221

Uploader Implementation: Kopia backup and restore #5221

Conversation

qiuming-best commented Aug 16, 2022 • edited

Please add a summary of your change

Please indicate you've done the following:

codecov-commenter commented Aug 19, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Lyndon-Li Aug 22, 2022 • edited

Choose a reason for hiding this comment

Lyndon-Li Aug 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiuming-best Aug 22, 2022 • edited

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

qiuming-best commented Aug 16, 2022 •

edited

codecov-commenter commented Aug 19, 2022 •

edited

Lyndon-Li Aug 22, 2022 •

edited

Lyndon-Li Aug 22, 2022 •

edited

qiuming-best Aug 22, 2022 •

edited