Support custom volume snapshots & restores #215

ncdc · 2017-11-27T19:24:49Z

The main Ark code was hard-coding specific support for AWS, GCE, and Azure volume snapshots and restores, and anything else was considered unsupported. This PR adds GetVolumeID and SetVolumeID to the BlockStore interface, to allow block store plugins to handle volume snapshots and restores.

Fixes #214

Signed-off-by: Andy Goldstein andy.goldstein@gmail.com

cc @stevesloka

nrb

The Get/SetVolumeID functions make sense to include to me. I also agree with the more lenient approach to PV restoration, but there may be edge cases that I'm not aware of.

skriss · 2017-11-28T03:10:37Z

The plugin change definitely makes sense - thanks for catching that.

I think the PV restorer change is OK, although maybe we still want to return a warning if there is a VolumeBackups map but the current PV isn't in it? It's certainly not an expected case -- likely either the snapshot failed during backup, or someone manually deleted it - so probably worth explicitly recording in the log.

stevesloka · 2017-11-28T14:29:17Z

pkg/cloudprovider/aws/block_store.go

+
+	aws["volumeID"] = volumeID
+
+	return pv, nil


pv was never changed here and aws is not returned?

We have to return the full object. aws comes from pv (collections.GetMap(pv.UnstructuredContent(), "spec.awsElasticBlockStore")). When we mutate aws, it's mutating pv.

ncdc · 2017-11-28T14:47:35Z

@skriss

I think the PV restorer change is OK, although maybe we still want to return a warning if there is a VolumeBackups map but the current PV isn't in it? It's certainly not an expected case -- likely either the snapshot failed during backup, or someone manually deleted it - so probably worth explicitly recording in the log.

I'll continue to have it return an error.

skriss · 2017-11-28T19:52:52Z

pkg/cloudprovider/snapshot_service.go

@@ -46,6 +47,9 @@ type SnapshotService interface {

 	// GetVolumeInfo gets the type and IOPS (if applicable) from the cloud API.
 	GetVolumeInfo(volumeID, volumeAZ string) (string, *int64, error)
+
+	GetVolumeID(pv runtime.Unstructured) (string, error)


yeah, opting to do it last

skriss · 2017-11-28T19:53:17Z

pkg/cloudprovider/storage_interfaces.go

+	// GetVolumeID returns the cloud provider specific identifier for the PersistentVolume.
+	GetVolumeID(pv runtime.Unstructured) (string, error)
+
+	SetVolumeID(pv runtime.Unstructured, volumeID string) (runtime.Unstructured, error)


skriss · 2017-11-28T19:54:34Z

pkg/plugin/block_store.go

+
+	resp, err := c.grpcClient.GetVolumeID(context.Background(), req)
+	if err != nil {
+		return "", nil


shouldn't this be returning "", err?

good catch, thx

skriss · 2017-11-28T19:55:06Z

pkg/plugin/block_store.go

+
+	resp, err := c.grpcClient.SetVolumeID(context.Background(), req)
+	if err != nil {
+		return nil, nil


shouldn't this be returning nil, err?

skriss · 2017-11-28T19:59:24Z

pkg/restore/restorers/pv_restorer.go

 	"k8s.io/apimachinery/pkg/runtime"

 	api "github.com/heptio/ark/pkg/apis/ark/v1"
 	"github.com/heptio/ark/pkg/cloudprovider"
+	"github.com/heptio/ark/pkg/util/boolptr"


this pkg is missing from the commit

skriss · 2017-11-28T20:03:07Z

pkg/restore/restorers/pv_restorer.go

-		}
-
-		restoreFromSnapshot = true
+	if sr.snapshotService == nil {


so this is going to return an error if SnapshotVolumes is auto for both the backup and the restore, and there was no snapshotService for the backup (so no backups). I think this is definitely not desired behavior. I think we want to exit prior to this (without error) if backup.status.VolumeBackups is nil/empty, right? (we may have discussed this already this AM).

Blech. That wasn't a case I was thinking about. I'll add that check.

skriss · 2017-11-28T20:04:23Z

pkg/restore/restorers/pv_restorer.go

 	}

-	return obj, warning, nil
+	return updatedObj, nil, nil


Looks like we can get rid of the warning return arg since it's always nil

I'll do that when I rebase

skriss · 2017-11-29T16:57:27Z

pkg/backup/item_backupper_test.go

@@ -455,7 +458,7 @@ func TestTakePVSnapshot(t *testing.T) {
 			name:            "unsupported PV source type",
 			snapshotEnabled: true,
 			pv:              `{"apiVersion": "v1", "kind": "PersistentVolume", "metadata": {"name": "mypv"}, "spec": {"unsupportedPVSource": {}}}`,
-			expectError:     false,
+			expectError:     true,


Why did this change from false to true? Shouldn't it still not return an error since it doesn't contain a supported PV source?

You're right, thanks.

skriss · 2017-11-29T17:04:37Z

Just the one unit test Q, otherwise LGTM

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

The main Ark code was hard-coding specific support for AWS, GCE, and Azure volume snapshots and restores, and anything else was considered unsupported. Add GetVolumeID and SetVolumeID to the BlockStore interface, to allow block store plugins to handle volume snapshots and restores. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss · 2017-11-29T18:33:37Z

LGTM

…er without PVC (vmware-tanzu#215)

…anzu#215) Bumps [kubevirt.io/api](https://github.com/kubevirt/api) from 1.0.0 to 1.1.1. Signed-off-by: Shelly Kagan <skagan@redhat.com>

ncdc added enhancement labels Nov 27, 2017

ncdc added this to the v0.6.0 milestone Nov 27, 2017

ncdc assigned nrb and skriss Nov 27, 2017

nrb reviewed Nov 27, 2017

View reviewed changes

stevesloka reviewed Nov 28, 2017

View reviewed changes

ncdc force-pushed the support-custom-snapshots branch from 136974b to 236d5a5 Compare November 28, 2017 18:51

skriss suggested changes Nov 28, 2017

View reviewed changes

ncdc force-pushed the support-custom-snapshots branch 2 times, most recently from 39ad5c2 to a390a2b Compare November 28, 2017 21:09

skriss reviewed Nov 29, 2017

View reviewed changes

Add boolptr helper package

526b604

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc force-pushed the support-custom-snapshots branch from f8d6afa to e317c94 Compare November 29, 2017 17:25

ncdc changed the title ~~WIP Support custom volume snapshots & restores~~ Support custom volume snapshots & restores Nov 29, 2017

ncdc force-pushed the support-custom-snapshots branch from e317c94 to c700455 Compare November 29, 2017 18:19

skriss approved these changes Nov 29, 2017

View reviewed changes

skriss merged commit c129d1c into vmware-tanzu:master Nov 29, 2017

ncdc deleted the support-custom-snapshots branch December 1, 2017 16:41

github-actions bot pushed a commit to kaovilai/velero that referenced this pull request Oct 28, 2022

OADP-1002: Ignore not found err for temp vsclass in cases of data mov…

0ce2085

…er without PVC (vmware-tanzu#215)

kaovilai pushed a commit to kaovilai/velero that referenced this pull request Feb 6, 2023

OADP-1002: Ignore not found err for temp vsclass in cases of data mov…

157ec0b

…er without PVC (vmware-tanzu#215)

kaovilai pushed a commit to kaovilai/velero that referenced this pull request Mar 16, 2023

OADP-1002: Ignore not found err for temp vsclass in cases of data mov…

7744edd

…er without PVC (vmware-tanzu#215)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom volume snapshots & restores #215

Support custom volume snapshots & restores #215

ncdc commented Nov 27, 2017 •

edited

Loading

nrb left a comment

skriss commented Nov 28, 2017

stevesloka Nov 28, 2017

ncdc Nov 28, 2017

ncdc commented Nov 28, 2017

skriss Nov 28, 2017

ncdc Nov 28, 2017

skriss Nov 28, 2017

skriss Nov 28, 2017

ncdc Nov 28, 2017

skriss Nov 28, 2017

skriss Nov 28, 2017

ncdc Nov 28, 2017

skriss Nov 28, 2017

ncdc Nov 28, 2017

skriss Nov 28, 2017

ncdc Nov 28, 2017

skriss Nov 29, 2017

ncdc Nov 29, 2017

skriss commented Nov 29, 2017

skriss commented Nov 29, 2017

Support custom volume snapshots & restores #215

Support custom volume snapshots & restores #215

Conversation

ncdc commented Nov 27, 2017 • edited Loading

nrb left a comment

Choose a reason for hiding this comment

skriss commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Nov 28, 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

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 Nov 29, 2017

skriss commented Nov 29, 2017

ncdc commented Nov 27, 2017 •

edited

Loading