Store PodVolumeBackups in obj storage & use as source of truth #1577

carlisia · 2019-06-12T20:57:11Z

Deprecate the annotation on the pod within the backup tarball (eventually remove)
Serialize the PodVolumeBackups as <backup-name>-podvolumebackups.json.gz that stores a serialized list of PodVolumeBackups and store it in object storage
Sync to the cluster through the backup sync controller
Modify the code that relies on original annotation to map pods and volumes to restic snapshots/Modify restore to use this source, but maintain existing code so we can restore “old” backups
Implementation needs to be backwards compatible with 1.0 ability for restoring backups

Signed-off-by: Carlisia carlisiac@vmware.com

skriss

Added some specific comments, but to summarize - users will still use the backup.velero.io/backup-volumes annotation to indicate which pod volumes they want to back up using restic. That's not changing. What is changing is that we will no longer use the snapshot.velero.io/<volume-name> annotation to store the restic snapshot ID when a backup is taken; the <backup-name>-podvolumebackups.json.gz file will be used instead.

pkg/restic/common.go

site/docs/master/restic.md

pkg/restic/common.go

skriss · 2019-06-13T18:56:37Z

👍 everything so far looks fine now.

I would definitely refer to the code for the <backup-name>-volumesnapshots.json.gz file as you go through the rest of this - while this won't be identical, it will be similar and tracing how that file gets created/saved/restored from should give you a pretty good idea of what code needs to change.

A few relevant references for the current volumesnapshots code:
https://github.com/heptio/velero/blob/master/pkg/backup/item_backupper.go#L448-L459
https://github.com/heptio/velero/blob/master/pkg/controller/backup_controller.go#L568-L588
https://github.com/heptio/velero/blob/master/pkg/persistence/object_store.go#L193
https://github.com/heptio/velero/blob/master/pkg/controller/restore_controller.go#L435
https://github.com/heptio/velero/blob/master/pkg/restore/restore.go#L832-L869

carlisia · 2019-06-13T19:14:43Z

I super appreciate the links, thank you! 🙏

carlisia · 2019-06-18T04:08:58Z

The content of the json file looks exactly like the podvolumebackups CR.

carlisia · 2019-06-18T13:53:24Z

@skriss, @nrb question: do we want to add any podvolumebackup info to the velero backup describe output? If so, what? I'm thinking useful options could be number of PodVolumeBackup items, as in:

Pod Volume Backups: 1

skriss · 2019-06-18T19:33:10Z

Per slack we're already showing it, just labeled as "restic backups"

skriss

Few more comments - overall looks like it's headed in the right direction!

pkg/controller/backup_controller.go

pkg/backup/request.go

pkg/controller/backup_controller.go

pkg/persistence/object_store.go

pkg/persistence/object_store_layout.go

changelogs/unreleased/1577-carlisia

Deprecating the annotation on the pod within the backup tarball since we are going to stop using this annotation to identify which volumes are restic backups. Signed-off-by: Carlisia <carlisiac@vmware.com>

Signed-off-by: Carlisia <carlisiac@vmware.com>

carlisia · 2019-07-13T00:09:53Z

@skriss question wrt Sync to the cluster through the backup sync controller: I'm not clear on what is new that needs to be done with the sync that is already not being done.

skriss · 2019-07-15T00:31:42Z

question wrt Sync to the cluster through the backup sync controller: I'm not clear on what is new that needs to be done with the sync that is already not being done.

The backup sync controller will need to be updated so when it's syncing a backup from object storage into the cluster, it also reads the contents of the PodVolumeBackups file and ensures that the custom resources contained in that file exist in the cluster - if they don't exist, they'll need to be created.

site/docs/master/restic.md

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss

Added another round of comments.

I'm thinking we may want to implement the changes to pkg/restore in a separate PR to keep this one from getting too big. This one should be fine on its own.

pkg/backup/builderBackup.go

pkg/backup/builderPodVolumes.go

pkg/backup/item_backupper.go

pkg/backup/item_backupper_test.go

pkg/backup/request.go

pkg/persistence/object_store.go

pkg/restic/backupper.go

pkg/restic/common.go

skriss · 2019-07-17T15:33:36Z

Let me know if you want to talk through/pair on any of the comments!

Signed-off-by: Carlisia <carlisiac@vmware.com>

carlisia · 2019-07-22T22:50:51Z

I tried to address the comments, PTAL.

I'm stuck on how to resolve this mess, any hint: https://github.com/heptio/velero/pull/1577/files#diff-d0a3ff018929aec3abbaa4c8a2c299ccR575

I'm suspecting the answer will be initialize the entire struct with mocked objects for each element?

skriss

Code's looking good! I had a few comments but they should be pretty minor.

I haven't reviewed the tests again yet - doing that next.

pkg/controller/backup_sync_controller.go

pkg/persistence/object_store.go

pkg/restic/backupper.go

pkg/restic/common.go

skriss · 2019-07-23T15:51:20Z

pkg/controller/backup_controller_test.go

+			// 	// VolumeSnapshots:  mock.Anything,
+			// }
+			// TODO: [carlisia] resolve this mock
+			backupStore.On("PutBackup", mock.Anything).Return(nil)


I think what you want to do here is define a function that can be used as an arg to mock.MatchedBy -- so something like the following:

func hasNameAndCompletionTimestamp(info persistence.BackupInfo) bool { return info.Name == test.backup.Name && strings.Contains(info.Metadata.String(), `"completionTimestamp": "2006-01-02T22:04:05Z"`) }

And then this line becomes:

backupStore.On("PutBackup", mock.MatchedBy(hasNameAndCompletionTimestamp)).Return(nil)

pkg/controller/backup_sync_controller.go

pkg/persistence/object_store.go

Signed-off-by: Carlisia <carlisiac@vmware.com>

pkg/controller/backup_controller_test.go

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss

Couple more comments, all test-related. If needed we can merge as-is and then follow up.

pkg/backup/item_backupper_test.go

skriss · 2019-07-24T14:54:36Z

pkg/controller/backup_controller_test.go


 			// Ensure we have a CompletionTimestamp when uploading.
 			// Failures will display the bytes in buf.
-			completionTimestampIsPresent := func(buf *bytes.Buffer) bool {
-				return strings.Contains(buf.String(), `"completionTimestamp": "2006-01-02T22:04:05Z"`)
+			completionTimestampIsPresent := func(info persistence.BackupInfo) bool {


Let's rename to hasNameAndCompletionTimestamp, and also update the comment just above.

pkg/controller/backup_controller_test.go

carlisia · 2019-07-24T16:28:44Z

Thank you, making those changes!

Signed-off-by: Carlisia <carlisiac@vmware.com>

carlisia · 2019-07-24T16:50:33Z

I'm trying to make sense of this output that I'm getting intermittently: https://gist.github.com/carlisia/8eb210744758a70e41af47216a4107ae

It happens when I run TestBackupItemNoSkips on the item_backupper_test.

skriss · 2019-07-24T17:44:03Z

I see:

expected: "resources/pods/cluster/.json"
actual  : "resources/pods/namespaces/foo/bar.json"

Which looks related to #1577 (comment) - you need to fully revert changes that test case.

I also see a typo error related to the backup controller tests -- "mock.Anythin" (should be "mock.Anything")

carlisia · 2019-07-24T18:05:10Z

Ah yes, I had reverted that line but not expectedTrackedPVCs: sets.NewString(key("foo", "bar"), key("foo", "baz")),. Added it, pushing it in a sec.

If you search my diff, there's no resources/pods/cluster/.json anywhere. Same when I do a global search in my branch. This is puzzling.

There is no mock.Anythin in my diff. I did have this typo yesterday, but corrected it. It's as if an old test result is being outputted.

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss · 2019-07-24T18:07:49Z

There is no mock.Anythin in my diff. I did have this typo yesterday, but corrected it. It's as if an old test result is being outputted.

Often this implies the tests are failing to compile - I'll try your pushed code locally

carlisia · 2019-07-24T18:09:17Z

Still getting the failure intermittently (as I expected since I didn't change anything relevant).

I think it's about this:

mock: Unexpected Method Call
-----------------------------

PutBackup(persistence.BackupInfo)
		0: persistence.BackupInfo{Name:"backup-1", Metadata:(*bytes.Buffer)(0xc000600000), Contents:(*os.File)(0xc0000105a0), Log:(*os.File)(0xc0000d4610), PodVolumeBackups:(*bytes.Buffer)(0xc000374f90), VolumeSnapshots:(*bytes.Buffer)(0xc000374e10)}

The closest call I have is: 

PutBackup(persistence.BackupInfo)
		0: persistence.BackupInfo{Name:"backup-1", Metadata:io.Reader(nil), Contents:io.Reader(nil), Log:io.Reader(nil), PodVolumeBackups:io.Reader(nil), VolumeSnapshots:io.Reader(nil)}

Which I think it's complaining that the BackupInfo is not being built as expected when I'm calling that PutBackup mock.

skriss · 2019-07-24T18:13:40Z

Everything runs fine for me - tried running things 100x to look for flakes. It's also passing CI

nrb · 2019-07-24T18:15:52Z

What commands are you both running for these tests?

carlisia · 2019-07-24T18:16:17Z

Yeah, good to go!

I think vscode was caching something. Closed it and opened it and I don't get it anymore.

I ran it a few times on my terminal and ran fine.

skriss

LGTM! @nrb @prydonius PTAL. We'll be following up with #1691 for the restore side of things.

carlisia requested review from nrb, prydonius and skriss June 12, 2019 20:57

carlisia added Restic Relates to the restic integration Restic - GA needed for restic integration to be considered GA labels Jun 12, 2019

carlisia changed the title ~~Deprecate pod annotation system~~ Store PodVolumeBackups in obj storage & use as source of truth Jun 12, 2019

skriss reviewed Jun 12, 2019

View reviewed changes

carlisia force-pushed the c-podvolumebackups branch from d49bcc0 to 0a1feaa Compare June 12, 2019 21:51

skriss reviewed Jun 13, 2019

View reviewed changes

pkg/restic/common.go Outdated Show resolved Hide resolved

skriss reviewed Jun 13, 2019

View reviewed changes

pkg/restic/common.go Outdated Show resolved Hide resolved

skriss reviewed Jun 18, 2019

View reviewed changes

nrb reviewed Jun 24, 2019

View reviewed changes

changelogs/unreleased/1577-carlisia Outdated Show resolved Hide resolved

Carlisia added 5 commits July 12, 2019 10:48

Deprecate pod annotation system

c1d1a3e

Deprecating the annotation on the pod within the backup tarball since we are going to stop using this annotation to identify which volumes are restic backups. Signed-off-by: Carlisia <carlisiac@vmware.com>

Remove unused code

5d232fd

Signed-off-by: Carlisia <carlisiac@vmware.com>

Fix doc + add changelog

a3b78bf

Signed-off-by: Carlisia <carlisiac@vmware.com>

Serialize PodVolumeBackups and send to storage

cac5347

Signed-off-by: Carlisia <carlisiac@vmware.com>

Address code reviews

cb8f214

Signed-off-by: Carlisia <carlisiac@vmware.com>

carlisia force-pushed the c-podvolumebackups branch from 28cfac7 to cb8f214 Compare July 12, 2019 18:41

Fix import aliases

0abba30

Signed-off-by: Carlisia <carlisiac@vmware.com>

nrb reviewed Jul 16, 2019

View reviewed changes

site/docs/master/restic.md Outdated Show resolved Hide resolved

Carlisia added 3 commits July 16, 2019 14:18

Add PodVolumeBackups to the sync process

49d1a59

Signed-off-by: Carlisia <carlisiac@vmware.com>

Fix restic restore documentation

e71bf91

Signed-off-by: Carlisia <carlisiac@vmware.com>

Remove unused code

1af290c

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss suggested changes Jul 17, 2019

View reviewed changes

Address code reviews

81a899c

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss suggested changes Jul 23, 2019

View reviewed changes

pkg/controller/backup_sync_controller.go Show resolved Hide resolved

pkg/persistence/object_store.go Show resolved Hide resolved

pkg/restic/backupper.go Outdated Show resolved Hide resolved

pkg/restic/common.go Outdated Show resolved Hide resolved

skriss reviewed Jul 23, 2019

View reviewed changes

nrb mentioned this pull request Jul 23, 2019

Add resource limits to restic init container #1677

Merged

1 task

nrb reviewed Jul 23, 2019

View reviewed changes

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

pkg/persistence/object_store.go Show resolved Hide resolved

carlisia mentioned this pull request Jul 23, 2019

Helper function for GetBackupVolumeSnapshots #1696

Closed

Another round of addressing code reviews

2c42605

Signed-off-by: Carlisia <carlisiac@vmware.com>

carlisia commented Jul 23, 2019

View reviewed changes

pkg/controller/backup_controller_test.go Show resolved Hide resolved

Bug fix

5d138f3

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss suggested changes Jul 24, 2019

View reviewed changes

Code fixins

107904f

Signed-off-by: Carlisia <carlisiac@vmware.com>

Add back bit of test

4c8e4f2

Signed-off-by: Carlisia <carlisiac@vmware.com>

skriss approved these changes Jul 24, 2019

View reviewed changes

nrb merged commit 3b9af8c into vmware-tanzu:master Jul 24, 2019

carlisia deleted the c-podvolumebackups branch July 24, 2019 20:08

dymurray mentioned this pull request Aug 8, 2019

[WIP] Update Restic Restore to work with Velero 1.1 Beta migtools/mig-controller#252

Closed

reasonerjt mentioned this pull request Sep 2, 2022

Kopia Pod Volume Backup/Restore #5259

Merged

Lyndon-Li mentioned this pull request Sep 5, 2022

Remove the deprecated snapshot ID recording/retrieving method for pod volume backup #5277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store PodVolumeBackups in obj storage & use as source of truth #1577

Store PodVolumeBackups in obj storage & use as source of truth #1577

carlisia commented Jun 12, 2019 •

edited

Loading

skriss left a comment

skriss commented Jun 13, 2019 •

edited

Loading

carlisia commented Jun 13, 2019

carlisia commented Jun 18, 2019

carlisia commented Jun 18, 2019

skriss commented Jun 18, 2019

skriss left a comment

carlisia commented Jul 13, 2019

skriss commented Jul 15, 2019

skriss left a comment

skriss commented Jul 17, 2019

carlisia commented Jul 22, 2019

skriss left a comment

skriss Jul 23, 2019

skriss left a comment

skriss Jul 24, 2019

carlisia commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

nrb commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss left a comment

Store PodVolumeBackups in obj storage & use as source of truth #1577

Store PodVolumeBackups in obj storage & use as source of truth #1577

Conversation

carlisia commented Jun 12, 2019 • edited Loading

skriss left a comment

Choose a reason for hiding this comment

skriss commented Jun 13, 2019 • edited Loading

carlisia commented Jun 13, 2019

carlisia commented Jun 18, 2019

carlisia commented Jun 18, 2019

skriss commented Jun 18, 2019

skriss left a comment

Choose a reason for hiding this comment

carlisia commented Jul 13, 2019

skriss commented Jul 15, 2019

skriss left a comment

Choose a reason for hiding this comment

skriss commented Jul 17, 2019

carlisia commented Jul 22, 2019

skriss left a comment

Choose a reason for hiding this comment

skriss Jul 23, 2019

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

skriss Jul 24, 2019

Choose a reason for hiding this comment

carlisia commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss commented Jul 24, 2019

nrb commented Jul 24, 2019

carlisia commented Jul 24, 2019

skriss left a comment

Choose a reason for hiding this comment

carlisia commented Jun 12, 2019 •

edited

Loading

skriss commented Jun 13, 2019 •

edited

Loading