when backing up PVCs, also back up claimed PVs #65

skriss · 2017-08-31T22:40:43Z

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

addresses #15, but doesn't fully fix
Fixes #105

ncdc · 2017-09-05T20:18:40Z

pkg/backup/backup.go

 			return err
 		}
+
+		for _, itm := range items {


ctx.additionalItems = append(ctx.additionalItems, items...)

ncdc · 2017-09-05T20:21:54Z

pkg/backup/backup.go

+
+	// backedUpItems keeps track of items that have been backed up already. The string should be derived
+	// by calling String() on an itemKey instance representing the item.
+	backedUpItems sets.String


I would probably do this as map[itemKey]struct{} - no need to stringify. And/or we could also create type itemKeySet map[itemKey]struct{} and add methods to it to make it more set like.

heh, i had it that way originally (map[itemKey]struct{}) and switched to this to get the sets methods (mainly for clarity). Not having generics makes me sad at times like these.

ncdc · 2017-09-05T20:44:21Z

pkg/backup/backup.go

+
+	// resolve additionalItems to GroupResources
+	for _, itm := range ctx.additionalItems {
+		groupResource, err := resolveGroupResource(kb.discoveryHelper.Mapper(), itm.resource)


If we modify resolveGroupResource to be resolveGroupVersionResource and return a schema.GroupVersionResource, we'll have the version we need, and you won't need the new clientForGroupResource function below. Let me know if you need further clarification.

We still need the APIResource obj to be able to get a client, how would you do that differently?

ncdc · 2017-09-05T20:53:27Z

pkg/backup/backup.go

+	backedUpItems sets.String
+
+	// additionalItems is a list of items returned by Actions that should be additionally backed up.
+	additionalItems []itemKey


Recording here for posterity - might want to consider refactoring the backup code to use a queue + workers, so backing up anything, whether as part of the normal process or "additional" one, would go through a single queue.

ncdc · 2017-09-07T12:49:09Z

pkg/backup/backup.go

+	// additionalItems is a list of items returned by Actions that should be additionally backed up.
+	additionalItems []itemKey
+
+	// resources is a map of GroupResource->APIResource that we've seen so far.


What would you think about adding extra functionality to our discovery helper so that we could more easily get a client from a GVR or GVK? In other words, when we refresh discovery, build up maps that lets us find an APIResource given either a GroupResource or a GroupKind? That would make it much easier to get a client, and we wouldn't have to rely on either iterating through or keeping track of what we've seen.

Yeah, that's probably a better place to put the logic. I'll make the switch

ncdc · 2017-09-07T12:50:17Z

pkg/backup/backup.go

@@ -400,5 +500,12 @@ func (*realItemBackupper) backupItem(ctx *backupContext, item map[string]interfa
 		return err
 	}

+	itm := itemKey{


Can we call itm "item" instead?

ncdc · 2017-09-07T12:51:01Z

pkg/backup/get_pv_action.go

+// Execute finds the name of the PersistentVolume referenced by the provided
+// PersistentVolumeClaim and returns it as an additional item to back up.
+func (a *getPVAction) Execute(pvc map[string]interface{}, backup *api.Backup) ([]itemKey, error) {
+	volumeName, err := collections.GetString(pvc, "spec.volumeName")


Feel like adding any logging in here?

ncdc · 2017-09-07T12:51:32Z

pkg/backup/backup.go

 			return err
 		}
+


Maybe log what the additional items are?

ncdc · 2017-09-07T12:52:14Z

pkg/backup/backup.go

+	return kuberrs.NewAggregate(errs)
+}
+
+func (kb *kubernetesBackupper) backupAdditionalItems(ctx *backupContext) error {


Log that we're starting to back up additional items?

ncdc · 2017-09-07T12:52:43Z

pkg/client/dynamic.go

@@ -78,6 +78,8 @@ type Dynamic interface {
 	List(metav1.ListOptions) (runtime.Object, error)
 	// Watch watches for changes to objects of a given resource.
 	Watch(metav1.ListOptions) (watch.Interface, error)
+
+	Get(name string, opts metav1.GetOptions) (*unstructured.Unstructured, error)


skriss · 2017-09-11T21:29:57Z

@ncdc PTAL at the revisions to this. I moved resource resolution into the discovery helper and built up a map for APIResource fetching there during Refresh().

ncdc · 2017-09-12T10:59:47Z

pkg/discovery/helper.go

@@ -50,9 +51,10 @@ type helper struct {
 	discoveryClient discovery.DiscoveryInterface

 	// lock guards mapper and resources


update comment to include resourcesMap

ncdc · 2017-09-12T11:00:05Z

pkg/discovery/helper.go

+		return schema.GroupVersionResource{}, metav1.APIResource{}, err
+	}
+
+	apiResource, found := h.resourcesMap[gvr]


need to acquire a read lock

ncdc · 2017-10-03T14:57:08Z

pkg/backup/backup.go

@@ -67,7 +67,17 @@ func (ac ActionContext) log(msg string, args ...interface{}) {
 type Action interface {
 	// Execute is invoked on an item being backed up. If an error is returned, the Backup is marked as
 	// failed.
-	Execute(ctx ActionContext, item map[string]interface{}, backup *api.Backup) error
+	Execute(ctx ActionContext, item map[string]interface{}, backup *api.Backup) ([]itemKey, error)


How about instead of returning the keys of additional items to back up, we just let the action call back to the itemBackupper to backup the additional items? i.e. each action implementation holds a reference to an itemBackupper. We'd need to make sure that the itemBackupper could look up the action instead of having that be part of kubernetesBackupper#backupResource. This way, each item can immediately back up transitive dependencies and add them to the backupContext's set of already backed up items.

I like it in theory; a bunch of stuff will have to move around because we'll want to check resource includes/excludes, label-selector, backed up items before backing up the PV and this is all done outside the backupItem method now.

skriss · 2017-10-04T23:22:10Z

got this approach working, have some fine-tuning to do on the design but generally looks good.

skriss · 2017-10-05T18:01:39Z

@ncdc the code here still needs some fine-tuning but would appreciate a review on the design/refactoring to allow actions to trigger backups of additional items directly (just the last commit)

ncdc

Quick initial scan. Would like to review more thoroughly later.

ncdc · 2017-10-05T18:29:02Z

pkg/backup/backup.go

@@ -69,7 +69,17 @@ func (ac ActionContext) log(msg string, args ...interface{}) {
 type Action interface {
 	// Execute is invoked on an item being backed up. If an error is returned, the Backup is marked as
 	// failed.
-	Execute(ctx ActionContext, item map[string]interface{}, backup *api.Backup) error
+	Execute(ctx *backupContext, item map[string]interface{}, backup *api.Backup, backupper itemBackupper) error


backupContext already has the Backup in it, so no need to pass the Backup in separately.

I wonder if we should put the itemBackupper in the backupContext?

good point on 1; maybe on 2.

Does the move to eliminate ActionContext and just use backupContext within the actions seem reasonable? I was going to have to add many of the fields from the latter to the former otherwise, so this seemed like a better choice.

Yeah I'm fine getting rid of the ActionContext and replacing with backupContext

ncdc · 2017-10-05T18:37:31Z

pkg/backup/backup.go

-	delete(item, "status")
-
-	metadata, err := collections.GetMap(item, "metadata")
+func (b *realItemBackupper) backupItem(ctx *backupContext, item map[string]interface{}, groupResource schema.GroupResource) error {


In my #112 I call this ib instead of b

ncdc · 2017-10-05T18:38:36Z

pkg/backup/backup.go

@@ -421,5 +461,7 @@ func (*realItemBackupper) backupItem(ctx *backupContext, item map[string]interfa
 		return errors.WithStack(err)
 	}

+	ctx.backedUpItems[key] = struct{}{}


Move this up to just below where you check if we've already backed it up, so we record it asap

I left it down here because I didn't want to record it as backed up unless it actually succeeded. WDYT?

My gut says that trying a second time if the initial backup failed would likely encounter the same error?

ncdc · 2017-10-05T18:42:30Z

pkg/backup/backup_test.go

-				t.Errorf("headers: expected %d, got %d", e, a)
-			}
+			e, a := 1, len(w.headers)
+			require.Equal(t, e, a, "headers: expected %d, got %d", e, a)


testify will print out expected and actual values. this could be

require.Equal(t, 1, len(w.headers), "headers")

ncdc · 2017-10-05T18:42:41Z

pkg/backup/volume_snapshot_action.go

@@ -65,6 +65,7 @@ func (a *volumeSnapshotAction) Execute(ctx ActionContext, volume map[string]inte
 	// non-nil error means it's a supported PV source but volume ID can't be found
 	if err != nil {
 		return errors.Wrapf(err, "error getting volume ID for backup %q, PersistentVolume %q", backupName, name)
+


ncdc · 2017-10-10T20:42:13Z

pkg/backup/backup_pv_action.go

+	// back it up
+	ctx.log("backing up PV %s\n", volumeName)
+	if err := backupper.backupItem(ctx, pv.UnstructuredContent(), gr); err != nil {
+		ctx.log("error backing up PV: %s\n", err)


Shouldn't we return the error?

yes, a bunch of the logging/error handling in here was messed up so I fixed it.

ncdc · 2017-10-10T20:44:04Z

pkg/discovery/helper.go

@@ -106,6 +121,16 @@ func (h *helper) Refresh() error {

 	sortResources(h.resources)

+	h.resourcesMap = make(map[schema.GroupVersionResource]metav1.APIResource)
+	for _, resourceGroup := range h.resources {
+		gv, _ := schema.ParseGroupVersion(resourceGroup.GroupVersion)


Might as well return if we get an error

ncdc · 2017-10-10T20:46:13Z

pkg/restore/restore.go

 		ret = append(ret, gr)
 		set.Insert(gr.String())
 	}

 	// go through everything we got from discovery and add anything not in "set" to byName
 	var byName []schema.GroupResource
-	for _, resourceGroup := range helper.Resources() {
+	for _, resourceGroup := range resources {


Was there a reason for this change?

good q, not really/may have been a remnant of some rebasing from before. fixed it.

skriss · 2017-10-10T23:39:20Z

Two notes:

if both PVs and PVCs are being backed up, we need to force PVCs to go first so that the relevant PVs are backed up as part of that. I'll probably just hardcode this logic for now.
we need something like Improved backup resource selection #15 (comment) to really make this use-case work as expected. this PR helps with the backend but doesn't address the UX.

ncdc · 2017-10-11T13:36:23Z

pkg/backup/backup.go

 		ctx.infof("Processing resource %s/%s", group.GroupVersion, resource.Name)
 		if err := kb.backupResource(ctx, group, resource); err != nil {
 			errs = append(errs, err)
 		}
 	}

+	for _, resource := range group.APIResources {
+		// do PVs last because if we're also backing up PVCs, we want to backup


This works for now, but do you think longer-term we should have prioritizedResources for backups too?

I'm actually going to want to make sure we have this order: pods, pvcs, pvs (anything else can come whenever). Maybe I'll do prioritized resources as part of my hooks PR. We don't necessarily have to expose it initially, but I think something a bit more flexible than a bool or two is going to be needed.

Yeah, thought the same thing - if you're okay with it, I'll leave this as-is and either you or I can do priorities in a separate PR

ncdc · 2017-10-11T13:37:05Z

pkg/backup/backup.go

+		// do PVs last because if we're also backing up PVCs, we want to backup
+		// PVs within the scope of the PVCs (within the PVC action) to allow
+		// for hooks to run
+		if strings.ToLower(resource.Name) == "persistentvolumes" {


Just to be safe, let's make sure that group.GroupVersion is v1

skriss · 2017-10-11T16:49:31Z

retest this please

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

bump restic timeouts and add diagnose tools

skriss force-pushed the cluster-resource-selection branch from 8fc5d30 to cc21c4d Compare August 31, 2017 22:53

skriss added this to the v0.4.0 milestone Sep 1, 2017

skriss force-pushed the cluster-resource-selection branch from cc21c4d to 1f62125 Compare September 1, 2017 16:36

skriss requested a review from ncdc September 5, 2017 17:04

ncdc reviewed Sep 5, 2017

View reviewed changes

ncdc modified the milestones: v0.4.0, v0.5.0 Sep 7, 2017

ncdc reviewed Sep 7, 2017

View reviewed changes

skriss force-pushed the cluster-resource-selection branch 3 times, most recently from 8214a21 to ba81127 Compare September 11, 2017 21:28

skriss mentioned this pull request Sep 12, 2017

Add --include-resources, --exclude-resources for restore operations #78

Merged

ncdc mentioned this pull request Oct 2, 2017

Improved backup resource selection #15

Closed

ncdc suggested changes Oct 3, 2017

View reviewed changes

skriss mentioned this pull request Oct 4, 2017

Modify backup/Action interface #105

Closed

skriss force-pushed the cluster-resource-selection branch 4 times, most recently from 7bffb35 to 356b098 Compare October 5, 2017 18:00

ncdc reviewed Oct 5, 2017

View reviewed changes

skriss force-pushed the cluster-resource-selection branch from 356b098 to bef57f6 Compare October 10, 2017 19:03

ncdc reviewed Oct 10, 2017

View reviewed changes

skriss force-pushed the cluster-resource-selection branch 2 times, most recently from 4a8bf90 to dff665c Compare October 10, 2017 23:02

ncdc reviewed Oct 11, 2017

View reviewed changes

skriss force-pushed the cluster-resource-selection branch 2 times, most recently from e061a6c to 9b3a6ba Compare October 11, 2017 16:17

ncdc approved these changes Oct 11, 2017

View reviewed changes

when backing up PVCs, also back up claimed PVs

9438a86

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

skriss force-pushed the cluster-resource-selection branch from 9b3a6ba to 9438a86 Compare October 11, 2017 16:55

skriss changed the title ~~[WIP] when backing up PVCs, also back up claimed PVs~~ when backing up PVCs, also back up claimed PVs Oct 11, 2017

ncdc merged commit 2a975a2 into vmware-tanzu:master Oct 11, 2017

skriss deleted the cluster-resource-selection branch October 23, 2017 17:58

skriss mentioned this pull request Nov 1, 2017

Simplify example that uses PVs #170

Closed

dymurray pushed a commit to dymurray/velero that referenced this pull request Aug 25, 2020

Merge pull request vmware-tanzu#65 from alaypatel07/konveyor-1.3.2

1f37027

bump restic timeouts and add diagnose tools

		@@ -50,9 +51,10 @@ type helper struct {
		discoveryClient discovery.DiscoveryInterface

		// lock guards mapper and resources

when backing up PVCs, also back up claimed PVs #65

when backing up PVCs, also back up claimed PVs #65

Conversation

skriss commented Aug 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Sep 11, 2017

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

skriss commented Oct 4, 2017

skriss commented Oct 5, 2017 • edited Loading

ncdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Oct 5, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Oct 10, 2017

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

skriss commented Oct 11, 2017

skriss commented Aug 31, 2017 •

edited

Loading

skriss commented Oct 5, 2017 •

edited

Loading

skriss Oct 5, 2017 •

edited

Loading