support restoring/deleting legacy backups with .status.volumeBackups

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

pkg/controller/backup_controller_test.go

@@ -398,21 +398,21 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
 	}{
 		{
 			name:            "location name does not correspond to any existing location",
-			backup:          arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations([]string{"random-name"}),
+			backup:          arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("random-name"),
 			locations:       arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList),
 			expectedErrors:  "error getting volume snapshot location named random-name: volumesnapshotlocation.ark.heptio.com \"random-name\" not found",
 			expectedSuccess: false,
 		},
 		{
 			name:                                "duplicate locationName per provider: should filter out dups",
-			backup:                              arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames),
+			backup:                              arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames...),
 			locations:                           arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList),
 			expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0]},
 			expectedSuccess:                     true,
 		},
 		{
 			name:            "multiple location names per provider",
-			backup:          arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(multipleLocationNames),
+			backup:          arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(multipleLocationNames...),
 			locations:       arktest.NewTestVolumeSnapshotLocation().WithName(multipleLocationNames[0]).WithProviderConfig(multipleLocationList),
 			expectedErrors:  "more than one VolumeSnapshotLocation name specified for provider aws: aws-us-east-1; unexpected name was aws-us-west-1",
 			expectedSuccess: false,
@@ -431,7 +431,7 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
 		},
 		{
 			name:                                "multiple location names for a provider, default location name for another provider",
-			backup:                              arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames),
+			backup:                              arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames...),
 			locations:                           arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList),
 			defaultLocations:                    defaultLocationsFake,
 			expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0], defaultLocationsFake["fake-provider"].Name},

pkg/controller/backup_deletion_controller.go

@@ -245,25 +245,47 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e
 
 	if backupStore != nil {
 		log.Info("Removing PV snapshots")
-		if snapshots, err := backupStore.GetBackupVolumeSnapshots(backup.Name); err != nil {
-			errs = append(errs, errors.Wrap(err, "error getting backup's volume snapshots").Error())
-		} else {
-			blockStores := make(map[string]cloudprovider.BlockStore)
-
-			for _, snapshot := range snapshots {
-				log.WithField("providerSnapshotID", snapshot.Status.ProviderSnapshotID).Info("Removing snapshot associated with backup")
-
-				blockStore, ok := blockStores[snapshot.Spec.Location]
-				if !ok {
-					if blockStore, err = blockStoreForSnapshotLocation(backup.Namespace, snapshot.Spec.Location, c.snapshotLocationLister, pluginManager); err != nil {
-						errs = append(errs, err.Error())
-						continue
+		if len(backup.Status.VolumeBackups) > 0 {
+			// pre-v0.10 backup
+			locations, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).List(labels.Everything())
+			if err != nil {
+				errs = append(errs, errors.Wrap(err, "error listing volume snapshot locations").Error())
+			} else if len(locations) != 1 {
+				errs = append(errs, errors.Errorf("unable to delete pre-v0.10 volume snapshots because exactly one volume snapshot location must exist, got %d", len(locations)).Error())
+			} else {
+				blockStore, err := blockStoreForSnapshotLocation(backup.Namespace, locations[0].Name, c.snapshotLocationLister, pluginManager)
+				if err != nil {
+					errs = append(errs, err.Error())
+				} else {
+					for _, snapshot := range backup.Status.VolumeBackups {
+						if err := blockStore.DeleteSnapshot(snapshot.SnapshotID); err != nil {
+							errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", snapshot.SnapshotID).Error())
+						}
 					}
-					blockStores[snapshot.Spec.Location] = blockStore
 				}
+			}
+		} else {
+			// v0.10+ backup
+			if snapshots, err := backupStore.GetBackupVolumeSnapshots(backup.Name); err != nil {
+				errs = append(errs, errors.Wrap(err, "error getting backup's volume snapshots").Error())
+			} else {
+				blockStores := make(map[string]cloudprovider.BlockStore)
 
-				if err := blockStore.DeleteSnapshot(snapshot.Status.ProviderSnapshotID); err != nil {
-					errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", snapshot.Status.ProviderSnapshotID).Error())
+				for _, snapshot := range snapshots {
+					log.WithField("providerSnapshotID", snapshot.Status.ProviderSnapshotID).Info("Removing snapshot associated with backup")
+
+					blockStore, ok := blockStores[snapshot.Spec.Location]
+					if !ok {
+						if blockStore, err = blockStoreForSnapshotLocation(backup.Namespace, snapshot.Spec.Location, c.snapshotLocationLister, pluginManager); err != nil {
+							errs = append(errs, err.Error())
+							continue
+						}
+						blockStores[snapshot.Spec.Location] = blockStore
+					}
+
+					if err := blockStore.DeleteSnapshot(snapshot.Status.ProviderSnapshotID); err != nil {
+						errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", snapshot.Status.ProviderSnapshotID).Error())
+					}
 				}
 			}
 		}

pkg/controller/backup_deletion_controller_test.go

@@ -324,6 +324,139 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) {
 		assert.Equal(t, expectedActions, td.client.Actions())
 	})
 
+	t.Run("pre-v0.10 backup with snapshots, no errors", func(t *testing.T) {
+		backup := arktest.NewTestBackup().WithName("foo").Backup
+		backup.UID = "uid"
+		backup.Spec.StorageLocation = "primary"
+		backup.Status.VolumeBackups = map[string]*v1.VolumeBackupInfo{
+			"pv-1": {
+				SnapshotID: "snap-1",
+			},
+		}
+
+		restore1 := arktest.NewTestRestore("heptio-ark", "restore-1", v1.RestorePhaseCompleted).WithBackup("foo").Restore
+		restore2 := arktest.NewTestRestore("heptio-ark", "restore-2", v1.RestorePhaseCompleted).WithBackup("foo").Restore
+		restore3 := arktest.NewTestRestore("heptio-ark", "restore-3", v1.RestorePhaseCompleted).WithBackup("some-other-backup").Restore
+
+		td := setupBackupDeletionControllerTest(backup, restore1, restore2, restore3)
+
+		td.sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore1)
+		td.sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore2)
+		td.sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(restore3)
+
+		location := &v1.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: backup.Namespace,
+				Name:      backup.Spec.StorageLocation,
+			},
+			Spec: v1.BackupStorageLocationSpec{
+				Provider: "objStoreProvider",
+				StorageType: v1.StorageType{
+					ObjectStorage: &v1.ObjectStorageLocation{
+						Bucket: "bucket",
+					},
+				},
+			},
+		}
+		require.NoError(t, td.sharedInformers.Ark().V1().BackupStorageLocations().Informer().GetStore().Add(location))
+
+		snapshotLocation := &v1.VolumeSnapshotLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: backup.Namespace,
+				Name:      "vsl-1",
+			},
+			Spec: v1.VolumeSnapshotLocationSpec{
+				Provider: "provider-1",
+			},
+		}
+		require.NoError(t, td.sharedInformers.Ark().V1().VolumeSnapshotLocations().Informer().GetStore().Add(snapshotLocation))
+
+		// Clear out req labels to make sure the controller adds them
+		td.req.Labels = make(map[string]string)
+
+		td.client.PrependReactor("get", "backups", func(action core.Action) (bool, runtime.Object, error) {
+			return true, backup, nil
+		})
+		td.blockStore.SnapshotsTaken.Insert("snap-1")
+
+		td.client.PrependReactor("patch", "deletebackuprequests", func(action core.Action) (bool, runtime.Object, error) {
+			return true, td.req, nil
+		})
+
+		td.client.PrependReactor("patch", "backups", func(action core.Action) (bool, runtime.Object, error) {
+			return true, backup, nil
+		})
+
+		pluginManager := &pluginmocks.Manager{}
+		pluginManager.On("GetBlockStore", "provider-1").Return(td.blockStore, nil)
+		pluginManager.On("CleanupClients")
+		td.controller.newPluginManager = func(logrus.FieldLogger) plugin.Manager { return pluginManager }
+
+		td.backupStore.On("DeleteBackup", td.req.Spec.BackupName).Return(nil)
+		td.backupStore.On("DeleteRestore", "restore-1").Return(nil)
+		td.backupStore.On("DeleteRestore", "restore-2").Return(nil)
+
+		err := td.controller.processRequest(td.req)
+		require.NoError(t, err)
+
+		expectedActions := []core.Action{
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"metadata":{"labels":{"ark.heptio.com/backup-name":"foo"}},"status":{"phase":"InProgress"}}`),
+			),
+			core.NewGetAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"metadata":{"labels":{"ark.heptio.com/backup-uid":"uid"}}}`),
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+				[]byte(`{"status":{"phase":"Deleting"}}`),
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("restores"),
+				td.req.Namespace,
+				"restore-1",
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("restores"),
+				td.req.Namespace,
+				"restore-2",
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"status":{"phase":"Processed"}}`),
+			),
+			core.NewDeleteCollectionAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				pkgbackup.NewDeleteBackupRequestListOptions(td.req.Spec.BackupName, "uid"),
+			),
+		}
+
+		arktest.CompareActions(t, expectedActions, td.client.Actions())
+
+		// Make sure snapshot was deleted
+		assert.Equal(t, 0, td.blockStore.SnapshotsTaken.Len())
+	})
+
 	t.Run("full delete, no errors", func(t *testing.T) {
 		backup := arktest.NewTestBackup().WithName("foo").Backup
 		backup.UID = "uid"

pkg/controller/restore_controller.go

@@ -343,6 +343,26 @@ func (c *restoreController) validateAndComplete(restore *api.Restore, pluginMana
 		return backupInfo{}
 	}
 
+	// Ensure that we have either .status.volumeBackups (for pre-v0.10 backups) OR a
+	// volumesnapshots.json.gz file in obj storage (for v0.10+ backups), but not both.
+	// If we have .status.volumeBackups, ensure that there's only one volume snapshot
+	// location configured.
+	if info.backup.Status.VolumeBackups != nil {
+		snapshots, err := info.backupStore.GetBackupVolumeSnapshots(info.backup.Name)
+		if err != nil {
+			restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, errors.Wrap(err, "Error checking for volumesnapshots file").Error())
+		} else if len(snapshots) > 0 {
+			restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, "Backup must not have both .status.volumeBackups and a volumesnapshots.json.gz file in object storage")
+		} else {
+			locations, err := c.snapshotLocationLister.VolumeSnapshotLocations(restore.Namespace).List(labels.Everything())
+			if err != nil {
+				restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, errors.Wrap(err, "Error listing volume snapshot locations").Error())
+			} else if len(locations) > 1 {
+				restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, "Cannot restore backup with .status.volumeBackups when more than one volume snapshot location exists")
+			}
+		}
+	}
+
 	// Fill in the ScheduleName so it's easier to consume for metrics.
 	if restore.Spec.ScheduleName == "" {
 		restore.Spec.ScheduleName = info.backup.GetLabels()["ark-schedule"]

pkg/controller/restore_controller_test.go

@@ -626,6 +626,114 @@ func TestProcessRestore(t *testing.T) {
 	}
 }
 
+func TestValidateAndComplete(t *testing.T) {
+	tests := []struct {
+		name              string
+		storageLocation   *api.BackupStorageLocation
+		snapshotLocations []*api.VolumeSnapshotLocation
+		backup            *api.Backup
+		volumeSnapshots   []*volume.Snapshot
+		restore           *api.Restore
+		expectedErrs      []string
+	}{
+		{
+			name:            "backup with .status.volumeBackups and no volumesnapshots.json file does not error",
+			storageLocation: arktest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			backup:          arktest.NewTestBackup().WithName("backup-1").WithStorageLocation("loc-1").WithSnapshot("pv-1", "snap-1").Backup,
+			volumeSnapshots: nil,
+			restore:         arktest.NewDefaultTestRestore().WithBackup("backup-1").Restore,
+			expectedErrs:    nil,
+		},
+		{
+			name:            "backup with no .status.volumeBackups and volumesnapshots.json file does not error",
+			storageLocation: arktest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			backup:          arktest.NewTestBackup().WithName("backup-1").WithStorageLocation("loc-1").Backup,
+			volumeSnapshots: []*volume.Snapshot{{}},
+			restore:         arktest.NewDefaultTestRestore().WithBackup("backup-1").Restore,
+			expectedErrs:    nil,
+		},
+		{
+			name:            "backup with both .status.volumeBackups and volumesnapshots.json file errors",
+			storageLocation: arktest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			backup:          arktest.NewTestBackup().WithName("backup-1").WithStorageLocation("loc-1").WithSnapshot("pv-1", "snap-1").Backup,
+			volumeSnapshots: []*volume.Snapshot{{}},
+			restore:         arktest.NewDefaultTestRestore().WithBackup("backup-1").Restore,
+			expectedErrs:    []string{"Backup must not have both .status.volumeBackups and a volumesnapshots.json.gz file in object storage"},
+		},
+		{
+			name:            "backup with .status.volumeBackups, and >1 volume snapshot locations exist, errors",
+			storageLocation: arktest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			snapshotLocations: []*api.VolumeSnapshotLocation{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: api.DefaultNamespace,
+						Name:      "vsl-1",
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: api.DefaultNamespace,
+						Name:      "vsl-2",
+					},
+				},
+			},
+			backup:       arktest.NewTestBackup().WithName("backup-1").WithStorageLocation("loc-1").WithSnapshot("pv-1", "snap-1").Backup,
+			restore:      arktest.NewDefaultTestRestore().WithBackup("backup-1").Restore,
+			expectedErrs: []string{"Cannot restore backup with .status.volumeBackups when more than one volume snapshot location exists"},
+		},
+		{
+			name:            "backup with .status.volumeBackups, and 1 volume snapshot location exists, does not error",
+			storageLocation: arktest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			snapshotLocations: []*api.VolumeSnapshotLocation{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: api.DefaultNamespace,
+						Name:      "vsl-1",
+					},
+				},
+			},
+			backup:       arktest.NewTestBackup().WithName("backup-1").WithStorageLocation("loc-1").WithSnapshot("pv-1", "snap-1").Backup,
+			restore:      arktest.NewDefaultTestRestore().WithBackup("backup-1").Restore,
+			expectedErrs: nil,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var (
+				clientset       = fake.NewSimpleClientset()
+				sharedInformers = informers.NewSharedInformerFactory(clientset, 0)
+				logger          = arktest.NewLogger()
+				backupStore     = &persistencemocks.BackupStore{}
+				controller      = &restoreController{
+					genericController: &genericController{
+						logger: logger,
+					},
+					namespace:              api.DefaultNamespace,
+					backupLister:           sharedInformers.Ark().V1().Backups().Lister(),
+					backupLocationLister:   sharedInformers.Ark().V1().BackupStorageLocations().Lister(),
+					snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(),
+					newBackupStore: func(*api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) {
+						return backupStore, nil
+					},
+				}
+			)
+
+			require.NoError(t, sharedInformers.Ark().V1().BackupStorageLocations().Informer().GetStore().Add(tc.storageLocation))
+			require.NoError(t, sharedInformers.Ark().V1().Backups().Informer().GetStore().Add(tc.backup))
+			for _, loc := range tc.snapshotLocations {
+				require.NoError(t, sharedInformers.Ark().V1().VolumeSnapshotLocations().Informer().GetStore().Add(loc))
+			}
+			backupStore.On("GetBackupVolumeSnapshots", tc.backup.Name).Return(tc.volumeSnapshots, nil)
+
+			controller.validateAndComplete(tc.restore, nil)
+
+			assert.Equal(t, tc.expectedErrs, tc.restore.Status.ValidationErrors)
+		})
+	}
+
+}
+
 func TestvalidateAndCompleteWhenScheduleNameSpecified(t *testing.T) {
 	var (
 		client          = fake.NewSimpleClientset()