don't snapshot volumes that have been backed up with restic #608

skriss · 2018-06-25T18:43:54Z

Signed-off-by: Steve Kriss steve@heptio.com

Fixes #604

nrb · 2018-06-26T21:10:40Z

pkg/backup/volume_snapshot_tracker.go

+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+// volumeSnapshotTracker keeps track of volumes that have been snapshotted.


I'd recommend stating that it tracks restic volumes that have been snapshotted.

ncdc · 2018-06-27T20:58:30Z

pkg/backup/item_backupper.go

+		// get the volumes to backup using restic, and add any of them that are PVCs to the pvc snapshot
+		// tracker, so that when we backup PVCs/PVs via an item action in the next step, we don't snapshot
+		// PVs that will have their data backed up with restic.
+		if pod, podVolumeBackups, err = getPodVolumesToBackup(obj, ib.pvcSnapshotTracker); err != nil {


Having getPodVolumesToBackup also add to the tracker seems like it's doing more work than it should. What if we did this?

pod, err = podFromUnstructured(obj) // check err resticVolumesToBackup := restic.GetVolumesToBackup(pod) ib.pvcSnapshotTracker.Track(pod, resticVolumesToBackup) // this has the looping logic

(I think resticVolumesToBackup is more descriptive than podVolumeBackups.)

Good idea. I think I was tired when I wrote this. updated.

ncdc · 2018-06-27T21:25:50Z

pkg/backup/item_backupper.go

-func backupPodVolumes(log logrus.FieldLogger, backup *api.Backup, obj runtime.Unstructured, backupper restic.Backupper) (runtime.Unstructured, []error) {
-	if backupper == nil {
-		log.Warn("No restic backupper, not backing up pod's volumes")
+func (ib *defaultItemBackupper) backupPodVolumes(log logrus.FieldLogger, obj runtime.Unstructured, pod *corev1api.Pod, volumes []string) (runtime.Unstructured, []error) {


Do you think this function would be cleaner if it didn't require both obj (an unstructured pod) and pod? We could have it return volumeSnapshots and call restic.SetPodSnapshotAnnotation outside of here.

Yes. updated and this is now much cleaner :)

nrb · 2018-06-27T22:00:24Z

pkg/backup/pvc_snapshot_tracker.go

+
+// pvcSnapshotTracker keeps track of persistent volume claims that have been snapshotted
+// with restic.
+type pvcSnapshotTracker struct {


Thanks for the comment on this. On a second pass, I'm wondering if having restic in the name might be helpful.

For example, in backup.go, L289, it's near snapshotService, which is for cloud volumes. Looking at the two names could get confusing, given they're for different types of snapshots.

I don't want to encode too much information in names, but the closeness here seems a bit off.

I renamed the variable to resticSnapshotTracker and left the type name as pvcSnapshotTracker. I think that makes it pretty clear. let me know what you think.

ncdc · 2018-06-28T14:39:26Z

I've been thinking a bit about how we're encoding more and more custom per-resource logic into the backupper/restorer instead of using item actions.

One way we could achieve this with an item action would be to create one that applies to pods and pvs. We would also need a prioritized order for resources when backing up (#586), where we make sure we do pods before pvs. We'd maybe want to consider creating a special-case interface specifically for snapshotting, that would run after the normal item actions. In this interface we'd need a way for 1 plugin to signal to ark either a) that it's handling the snapshot, or b) that no other plugins should execute a snapshot. If we have all that, we could have a theoretical resticSnapshotAction:

The plugin would get its own copy of a restic repo manager (maybe a bad thing, but let's say it's ok)
Upon seeing a pod, the plugin would create PVBs for all appropriate volumes and add the ns/pvcName to its tracking map
Upon seeing a pv, if the pv's claimRef is in the tracking map, tell ark it's "handling" the snapshot / no other plugins should execute a snapshot

We should not do this now - this PR is fine to go in. But we should start to think about ways that we can make bits and pieces of logic like this a bit more self contained as plugins, instead of having to modify the core backup/restore functions.

ncdc · 2018-06-28T14:42:24Z

pkg/backup/item_backupper.go

@@ -355,6 +359,23 @@ func (ib *defaultItemBackupper) takePVSnapshot(pv runtime.Unstructured, backup *
 		return nil
 	}

+	// If this PV is claimed, see if we've already taken a (restic) snapshot of the contents
+	// of this PV. If so, don't take a snapshot.
+	if collections.Exists(pv.UnstructuredContent(), "spec.claimRef") {


Do you want to convert to an actual PV here?

sure, will make this simpler

ncdc · 2018-06-28T14:44:08Z

Any plans to unit test the tracking in backupItem?

skriss · 2018-06-28T16:09:17Z

Yeah, I'll add some tests.

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss · 2018-06-28T17:19:55Z

OK got tests updated

ncdc · 2018-06-28T17:22:36Z

LGTM

skriss requested review from ncdc and nrb June 25, 2018 18:43

skriss changed the title ~~don't snapshot volumes that have been backed up with restic~~ [WIP] don't snapshot volumes that have been backed up with restic Jun 25, 2018

skriss force-pushed the no-pv-snapshot-if-restic-backup branch 2 times, most recently from 445dcc3 to 3bbe0e6 Compare June 25, 2018 20:58

skriss changed the title ~~[WIP] don't snapshot volumes that have been backed up with restic~~ don't snapshot volumes that have been backed up with restic Jun 25, 2018

nrb reviewed Jun 26, 2018

View reviewed changes

ncdc reviewed Jun 27, 2018

View reviewed changes

skriss force-pushed the no-pv-snapshot-if-restic-backup branch from 3bbe0e6 to f3b88df Compare June 27, 2018 21:18

ncdc reviewed Jun 27, 2018

View reviewed changes

skriss force-pushed the no-pv-snapshot-if-restic-backup branch from f3b88df to af30892 Compare June 27, 2018 21:53

nrb reviewed Jun 27, 2018

View reviewed changes

skriss force-pushed the no-pv-snapshot-if-restic-backup branch from af30892 to 93dbcd9 Compare June 28, 2018 00:00

ncdc reviewed Jun 28, 2018

View reviewed changes

don't snapshot volumes that have been backed up with restic

11c176c

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss force-pushed the no-pv-snapshot-if-restic-backup branch from 93dbcd9 to 11c176c Compare June 28, 2018 17:19

ncdc merged commit eaeb9d6 into vmware-tanzu:master Jun 28, 2018

skriss deleted the no-pv-snapshot-if-restic-backup branch June 28, 2018 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't snapshot volumes that have been backed up with restic #608

don't snapshot volumes that have been backed up with restic #608

skriss commented Jun 25, 2018

nrb Jun 26, 2018

skriss Jun 27, 2018

ncdc Jun 27, 2018

skriss Jun 27, 2018

ncdc Jun 27, 2018

skriss Jun 27, 2018

nrb Jun 27, 2018

skriss Jun 28, 2018

ncdc commented Jun 28, 2018

ncdc Jun 28, 2018

skriss Jun 28, 2018

ncdc commented Jun 28, 2018

skriss commented Jun 28, 2018

skriss commented Jun 28, 2018

ncdc commented Jun 28, 2018

don't snapshot volumes that have been backed up with restic #608

don't snapshot volumes that have been backed up with restic #608

Conversation

skriss commented Jun 25, 2018

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

ncdc commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Jun 28, 2018

skriss commented Jun 28, 2018

skriss commented Jun 28, 2018

ncdc commented Jun 28, 2018