remove SnapshotService, replace with direct BlockStore usage

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

pkg/backup/backup.go

@@ -55,7 +55,7 @@ type kubernetesBackupper struct {
 	discoveryHelper        discovery.Helper
 	podCommandExecutor     podexec.PodCommandExecutor
 	groupBackupperFactory  groupBackupperFactory
-	snapshotService        cloudprovider.SnapshotService
+	blockStore             cloudprovider.BlockStore
 	resticBackupperFactory restic.BackupperFactory
 	resticTimeout          time.Duration
 }
@@ -93,7 +93,7 @@ func NewKubernetesBackupper(
 	discoveryHelper discovery.Helper,
 	dynamicFactory client.DynamicFactory,
 	podCommandExecutor podexec.PodCommandExecutor,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupperFactory restic.BackupperFactory,
 	resticTimeout time.Duration,
 ) (Backupper, error) {
@@ -102,7 +102,7 @@ func NewKubernetesBackupper(
 		dynamicFactory:         dynamicFactory,
 		podCommandExecutor:     podCommandExecutor,
 		groupBackupperFactory:  &defaultGroupBackupperFactory{},
-		snapshotService:        snapshotService,
+		blockStore:             blockStore,
 		resticBackupperFactory: resticBackupperFactory,
 		resticTimeout:          resticTimeout,
 	}, nil
@@ -276,7 +276,7 @@ func (kb *kubernetesBackupper) Backup(logger logrus.FieldLogger, backup *api.Bac
 		kb.podCommandExecutor,
 		tw,
 		resourceHooks,
-		kb.snapshotService,
+		kb.blockStore,
 		resticBackupper,
 		newPVCSnapshotTracker(),
 	)

pkg/backup/backup_test.go

@@ -652,7 +652,7 @@ func (f *mockGroupBackupperFactory) newGroupBackupper(
 	podCommandExecutor podexec.PodCommandExecutor,
 	tarWriter tarWriter,
 	resourceHooks []resourceHook,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) groupBackupper {
@@ -669,7 +669,7 @@ func (f *mockGroupBackupperFactory) newGroupBackupper(
 		podCommandExecutor,
 		tarWriter,
 		resourceHooks,
-		snapshotService,
+		blockStore,
 		resticBackupper,
 		resticSnapshotTracker,
 	)

pkg/backup/group_backupper.go

@@ -49,7 +49,7 @@ type groupBackupperFactory interface {
 		podCommandExecutor podexec.PodCommandExecutor,
 		tarWriter tarWriter,
 		resourceHooks []resourceHook,
-		snapshotService cloudprovider.SnapshotService,
+		blockStore cloudprovider.BlockStore,
 		resticBackupper restic.Backupper,
 		resticSnapshotTracker *pvcSnapshotTracker,
 	) groupBackupper
@@ -69,7 +69,7 @@ func (f *defaultGroupBackupperFactory) newGroupBackupper(
 	podCommandExecutor podexec.PodCommandExecutor,
 	tarWriter tarWriter,
 	resourceHooks []resourceHook,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) groupBackupper {
@@ -86,7 +86,7 @@ func (f *defaultGroupBackupperFactory) newGroupBackupper(
 		podCommandExecutor:       podCommandExecutor,
 		tarWriter:                tarWriter,
 		resourceHooks:            resourceHooks,
-		snapshotService:          snapshotService,
+		blockStore:               blockStore,
 		resticBackupper:          resticBackupper,
 		resticSnapshotTracker:    resticSnapshotTracker,
 		resourceBackupperFactory: &defaultResourceBackupperFactory{},
@@ -109,7 +109,7 @@ type defaultGroupBackupper struct {
 	podCommandExecutor       podexec.PodCommandExecutor
 	tarWriter                tarWriter
 	resourceHooks            []resourceHook
-	snapshotService          cloudprovider.SnapshotService
+	blockStore               cloudprovider.BlockStore
 	resticBackupper          restic.Backupper
 	resticSnapshotTracker    *pvcSnapshotTracker
 	resourceBackupperFactory resourceBackupperFactory
@@ -133,7 +133,7 @@ func (gb *defaultGroupBackupper) backupGroup(group *metav1.APIResourceList) erro
 			gb.podCommandExecutor,
 			gb.tarWriter,
 			gb.resourceHooks,
-			gb.snapshotService,
+			gb.blockStore,
 			gb.resticBackupper,
 			gb.resticSnapshotTracker,
 		)

pkg/backup/group_backupper_test.go

@@ -161,7 +161,7 @@ func (rbf *mockResourceBackupperFactory) newResourceBackupper(
 	podCommandExecutor podexec.PodCommandExecutor,
 	tarWriter tarWriter,
 	resourceHooks []resourceHook,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) resourceBackupper {
@@ -178,7 +178,7 @@ func (rbf *mockResourceBackupperFactory) newResourceBackupper(
 		podCommandExecutor,
 		tarWriter,
 		resourceHooks,
-		snapshotService,
+		blockStore,
 		resticBackupper,
 		resticSnapshotTracker,
 	)

pkg/backup/item_backupper.go

@@ -54,7 +54,7 @@ type itemBackupperFactory interface {
 		resourceHooks []resourceHook,
 		dynamicFactory client.DynamicFactory,
 		discoveryHelper discovery.Helper,
-		snapshotService cloudprovider.SnapshotService,
+		blockStore cloudprovider.BlockStore,
 		resticBackupper restic.Backupper,
 		resticSnapshotTracker *pvcSnapshotTracker,
 	) ItemBackupper
@@ -72,7 +72,7 @@ func (f *defaultItemBackupperFactory) newItemBackupper(
 	resourceHooks []resourceHook,
 	dynamicFactory client.DynamicFactory,
 	discoveryHelper discovery.Helper,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) ItemBackupper {
@@ -86,7 +86,7 @@ func (f *defaultItemBackupperFactory) newItemBackupper(
 		resourceHooks:   resourceHooks,
 		dynamicFactory:  dynamicFactory,
 		discoveryHelper: discoveryHelper,
-		snapshotService: snapshotService,
+		blockStore:      blockStore,
 		itemHookHandler: &defaultItemHookHandler{
 			podCommandExecutor: podCommandExecutor,
 		},
@@ -114,7 +114,7 @@ type defaultItemBackupper struct {
 	resourceHooks         []resourceHook
 	dynamicFactory        client.DynamicFactory
 	discoveryHelper       discovery.Helper
-	snapshotService       cloudprovider.SnapshotService
+	blockStore            cloudprovider.BlockStore
 	resticBackupper       restic.Backupper
 	resticSnapshotTracker *pvcSnapshotTracker
 
@@ -219,7 +219,7 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 	obj = updatedObj
 
 	if groupResource == kuberesource.PersistentVolumes {
-		if ib.snapshotService == nil {
+		if ib.blockStore == nil {
 			log.Debug("Skipping Persistent Volume snapshot because they're not enabled.")
 		} else if err := ib.takePVSnapshot(obj, ib.backup, log); err != nil {
 			backupErrs = append(backupErrs, err)
@@ -399,7 +399,7 @@ func (ib *defaultItemBackupper) takePVSnapshot(obj runtime.Unstructured, backup
 		log.Infof("label %q is not present on PersistentVolume", zoneLabel)
 	}
 
-	volumeID, err := ib.snapshotService.GetVolumeID(obj)
+	volumeID, err := ib.blockStore.GetVolumeID(obj)
 	if err != nil {
 		return errors.Wrapf(err, "error getting volume ID for PersistentVolume")
 	}
@@ -416,14 +416,14 @@ func (ib *defaultItemBackupper) takePVSnapshot(obj runtime.Unstructured, backup
 	}
 
 	log.Info("Snapshotting PersistentVolume")
-	snapshotID, err := ib.snapshotService.CreateSnapshot(volumeID, pvFailureDomainZone, tags)
+	snapshotID, err := ib.blockStore.CreateSnapshot(volumeID, pvFailureDomainZone, tags)
 	if err != nil {
 		// log+error on purpose - log goes to the per-backup log file, error goes to the backup
 		log.WithError(err).Error("error creating snapshot")
 		return errors.WithMessage(err, "error creating snapshot")
 	}
 
-	volumeType, iops, err := ib.snapshotService.GetVolumeInfo(volumeID, pvFailureDomainZone)
+	volumeType, iops, err := ib.blockStore.GetVolumeInfo(volumeID, pvFailureDomainZone)
 	if err != nil {
 		log.WithError(err).Error("error getting volume info")
 		return errors.WithMessage(err, "error getting volume info")

pkg/backup/item_backupper_test.go

@@ -277,7 +277,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 			additionalItemError: errors.New("foo"),
 		},
 		{
-			name: "takePVSnapshot is not invoked for PVs when snapshotService == nil",
+			name: "takePVSnapshot is not invoked for PVs when blockStore == nil",
 			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
 			item:                  `{"apiVersion": "v1", "kind": "PersistentVolume", "metadata": {"name": "mypv", "labels": {"failure-domain.beta.kubernetes.io/zone": "us-east-1c"}}, "spec": {"awsElasticBlockStore": {"volumeID": "aws://us-east-1c/vol-abc123"}}}`,
 			expectError:           false,
@@ -286,7 +286,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 			groupResource:         "persistentvolumes",
 		},
 		{
-			name: "takePVSnapshot is invoked for PVs when snapshotService != nil",
+			name: "takePVSnapshot is invoked for PVs when blockStore != nil",
 			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
 			item:                  `{"apiVersion": "v1", "kind": "PersistentVolume", "metadata": {"name": "mypv", "labels": {"failure-domain.beta.kubernetes.io/zone": "us-east-1c"}}, "spec": {"awsElasticBlockStore": {"volumeID": "aws://us-east-1c/vol-abc123"}}}`,
 			expectError:           false,
@@ -305,7 +305,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 			expectExcluded:        false,
 			expectedTarHeaderName: "resources/persistentvolumes/cluster/mypv.json",
 			groupResource:         "persistentvolumes",
-			// empty snapshottableVolumes causes a snapshotService to be created, but no
+			// empty snapshottableVolumes causes a blockStore to be created, but no
 			// snapshots are expected to be taken.
 			snapshottableVolumes: map[string]api.VolumeBackupInfo{},
 			trackedPVCs:          sets.NewString(key("pvc-ns", "pvc"), key("another-pvc-ns", "another-pvc")),
@@ -419,14 +419,14 @@ func TestBackupItemNoSkips(t *testing.T) {
 				newPVCSnapshotTracker(),
 			).(*defaultItemBackupper)
 
-			var snapshotService *arktest.FakeSnapshotService
+			var blockStore *arktest.FakeBlockStore
 			if test.snapshottableVolumes != nil {
-				snapshotService = &arktest.FakeSnapshotService{
+				blockStore = &arktest.FakeBlockStore{
 					SnapshottableVolumes: test.snapshottableVolumes,
 					VolumeID:             "vol-abc123",
 					Error:                test.snapshotError,
 				}
-				b.snapshotService = snapshotService
+				b.blockStore = blockStore
 			}
 
 			if test.trackedPVCs != nil {
@@ -514,7 +514,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 			}
 
 			if test.snapshottableVolumes != nil {
-				require.Equal(t, len(test.snapshottableVolumes), len(snapshotService.SnapshotsTaken))
+				require.Equal(t, len(test.snapshottableVolumes), len(blockStore.SnapshotsTaken))
 
 				var expectedBackups []api.VolumeBackupInfo
 				for _, vbi := range test.snapshottableVolumes {
@@ -719,12 +719,12 @@ func TestTakePVSnapshot(t *testing.T) {
 				},
 			}
 
-			snapshotService := &arktest.FakeSnapshotService{
+			blockStore := &arktest.FakeBlockStore{
 				SnapshottableVolumes: test.volumeInfo,
 				VolumeID:             test.expectedVolumeID,
 			}
 
-			ib := &defaultItemBackupper{snapshotService: snapshotService}
+			ib := &defaultItemBackupper{blockStore: blockStore}
 
 			pv, err := arktest.GetAsMap(test.pv)
 			if err != nil {
@@ -754,12 +754,12 @@ func TestTakePVSnapshot(t *testing.T) {
 			}
 
 			// we should have one snapshot taken exactly
-			require.Equal(t, test.expectedSnapshotsTaken, snapshotService.SnapshotsTaken.Len())
+			require.Equal(t, test.expectedSnapshotsTaken, blockStore.SnapshotsTaken.Len())
 
 			if test.expectedSnapshotsTaken > 0 {
-				// the snapshotID should be the one in the entry in snapshotService.SnapshottableVolumes
+				// the snapshotID should be the one in the entry in blockStore.SnapshottableVolumes
 				// for the volume we ran the test for
-				snapshotID, _ := snapshotService.SnapshotsTaken.PopAny()
+				snapshotID, _ := blockStore.SnapshotsTaken.PopAny()
 
 				expectedVolumeBackups["mypv"] = &v1.VolumeBackupInfo{
 					SnapshotID:       snapshotID,

pkg/backup/resource_backupper.go

@@ -51,7 +51,7 @@ type resourceBackupperFactory interface {
 		podCommandExecutor podexec.PodCommandExecutor,
 		tarWriter tarWriter,
 		resourceHooks []resourceHook,
-		snapshotService cloudprovider.SnapshotService,
+		blockStore cloudprovider.BlockStore,
 		resticBackupper restic.Backupper,
 		resticSnapshotTracker *pvcSnapshotTracker,
 	) resourceBackupper
@@ -72,7 +72,7 @@ func (f *defaultResourceBackupperFactory) newResourceBackupper(
 	podCommandExecutor podexec.PodCommandExecutor,
 	tarWriter tarWriter,
 	resourceHooks []resourceHook,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) resourceBackupper {
@@ -89,7 +89,7 @@ func (f *defaultResourceBackupperFactory) newResourceBackupper(
 		podCommandExecutor:    podCommandExecutor,
 		tarWriter:             tarWriter,
 		resourceHooks:         resourceHooks,
-		snapshotService:       snapshotService,
+		blockStore:            blockStore,
 		resticBackupper:       resticBackupper,
 		resticSnapshotTracker: resticSnapshotTracker,
 		itemBackupperFactory:  &defaultItemBackupperFactory{},
@@ -113,7 +113,7 @@ type defaultResourceBackupper struct {
 	podCommandExecutor    podexec.PodCommandExecutor
 	tarWriter             tarWriter
 	resourceHooks         []resourceHook
-	snapshotService       cloudprovider.SnapshotService
+	blockStore            cloudprovider.BlockStore
 	resticBackupper       restic.Backupper
 	resticSnapshotTracker *pvcSnapshotTracker
 	itemBackupperFactory  itemBackupperFactory
@@ -189,7 +189,7 @@ func (rb *defaultResourceBackupper) backupResource(
 		rb.resourceHooks,
 		rb.dynamicFactory,
 		rb.discoveryHelper,
-		rb.snapshotService,
+		rb.blockStore,
 		rb.resticBackupper,
 		rb.resticSnapshotTracker,
 	)

pkg/backup/resource_backupper_test.go

@@ -544,7 +544,6 @@ func TestBackupResourceOnlyIncludesSpecifiedNamespaces(t *testing.T) {
 		dynamicFactory:  dynamicFactory,
 		discoveryHelper: discoveryHelper,
 		itemHookHandler: itemHookHandler,
-		snapshotService: nil,
 	}
 
 	itemBackupperFactory.On("newItemBackupper",
@@ -690,7 +689,7 @@ func (ibf *mockItemBackupperFactory) newItemBackupper(
 	resourceHooks []resourceHook,
 	dynamicFactory client.DynamicFactory,
 	discoveryHelper discovery.Helper,
-	snapshotService cloudprovider.SnapshotService,
+	blockStore cloudprovider.BlockStore,
 	resticBackupper restic.Backupper,
 	resticSnapshotTracker *pvcSnapshotTracker,
 ) ItemBackupper {
@@ -705,7 +704,7 @@ func (ibf *mockItemBackupperFactory) newItemBackupper(
 		resourceHooks,
 		dynamicFactory,
 		discoveryHelper,
-		snapshotService,
+		blockStore,
 		resticBackupper,
 		resticSnapshotTracker,
 	)

pkg/cloudprovider/aws/block_store.go

@@ -141,15 +141,6 @@ func (b *blockStore) GetVolumeInfo(volumeID, volumeAZ string) (string, *int64, e
 	return volumeType, iops, nil
 }
 
-func (b *blockStore) IsVolumeReady(volumeID, volumeAZ string) (ready bool, err error) {
-	volumeInfo, err := b.describeVolume(volumeID)
-	if err != nil {
-		return false, err
-	}
-
-	return *volumeInfo.State == ec2.VolumeStateAvailable, nil
-}
-
 func (b *blockStore) describeVolume(volumeID string) (*ec2.Volume, error) {
 	req := &ec2.DescribeVolumesInput{
 		VolumeIds: []*string{&volumeID},

pkg/cloudprovider/azure/block_store.go

@@ -183,19 +183,6 @@ func (b *blockStore) GetVolumeInfo(volumeID, volumeAZ string) (string, *int64, e
 	return string(res.AccountType), nil, nil
 }
 
-func (b *blockStore) IsVolumeReady(volumeID, volumeAZ string) (ready bool, err error) {
-	res, err := b.disks.Get(b.resourceGroup, volumeID)
-	if err != nil {
-		return false, errors.WithStack(err)
-	}
-
-	if res.ProvisioningState == nil {
-		return false, errors.New("nil ProvisioningState returned from Get call")
-	}
-
-	return *res.ProvisioningState == "Succeeded", nil
-}
-
 func (b *blockStore) CreateSnapshot(volumeID, volumeAZ string, tags map[string]string) (string, error) {
 	// Lookup disk info for its Location
 	diskInfo, err := b.disks.Get(b.resourceGroup, volumeID)

pkg/cloudprovider/gcp/block_store.go

@@ -137,16 +137,6 @@ func (b *blockStore) GetVolumeInfo(volumeID, volumeAZ string) (string, *int64, e
 	return res.Type, nil, nil
 }
 
-func (b *blockStore) IsVolumeReady(volumeID, volumeAZ string) (ready bool, err error) {
-	disk, err := b.gce.Disks.Get(b.project, volumeAZ, volumeID).Do()
-	if err != nil {
-		return false, errors.WithStack(err)
-	}
-
-	// TODO can we consider a disk ready while it's in the RESTORING state?
-	return disk.Status == "READY", nil
-}
-
 func (b *blockStore) CreateSnapshot(volumeID, volumeAZ string, tags map[string]string) (string, error) {
 	// snapshot names must adhere to RFC1035 and be 1-63 characters
 	// long