From 6b420f49a93729da8f132ed10b6aac1a6de18d49 Mon Sep 17 00:00:00 2001
From: Pavan Navarathna Devaraj <pavan@kasten.io>
Date: Mon, 15 Apr 2019 11:11:17 -0700
Subject: [PATCH] Minor enhancements to CSI Snapshot (#5396)

### Overview
1. Handle errors that happen while waiting for snapshot creation.
2. Check if the `snapshot.Status.CreationTime` is set when the snapshot is ready.
3. Fix unit test setup always setting the `storageClassName` to the last one in the list.
4. Check if the `provisioner` for `StorageClass` is same as the `snapshotter` in the `VolumeSnapshotClass`
---
 pkg/kube/snapshot/snapshot.go      |  8 +++++---
 pkg/kube/snapshot/snapshot_test.go | 25 +++++++++++++------------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/pkg/kube/snapshot/snapshot.go b/pkg/kube/snapshot/snapshot.go
index 08dcb6bb21..69079a77d8 100644
--- a/pkg/kube/snapshot/snapshot.go
+++ b/pkg/kube/snapshot/snapshot.go
@@ -20,8 +20,6 @@ const (
 	pvcKind      = "PersistentVolumeClaim"
 )
 
-var snapshotAPIGroup = "snapshot.storage.k8s.io"
-
 // Create creates a VolumeSnapshot and returns it or any error happened meanwhile.
 //
 // 'name' is the name of the VolumeSnapshot.
@@ -210,7 +208,11 @@ func WaitOnReadyToUse(ctx context.Context, snapCli snapshotclient.Interface, sna
 		if err != nil {
 			return false, err
 		}
-		return snap.Status.ReadyToUse, nil
+		// Error can be set while waiting for creation
+		if snap.Status.Error != nil {
+			return false, errors.New(snap.Status.Error.Message)
+		}
+		return (snap.Status.ReadyToUse && snap.Status.CreationTime != nil), nil
 	})
 }
 
diff --git a/pkg/kube/snapshot/snapshot_test.go b/pkg/kube/snapshot/snapshot_test.go
index 492d6a4692..0a24343a19 100644
--- a/pkg/kube/snapshot/snapshot_test.go
+++ b/pkg/kube/snapshot/snapshot_test.go
@@ -66,15 +66,23 @@ func (s *SnapshotTestSuite) SetUpSuite(c *C) {
 		c.Logf("Failed to query VolumeSnapshotClass, skipping test. Error: %v", err)
 		c.Fail()
 	}
+	var snapshotterName string
 	if len(vscs.Items) != 0 {
-		s.snapshotClass = &vscs.Items[0].Name
+		vsClass, err := sc.VolumesnapshotV1alpha1().VolumeSnapshotClasses().Get(vscs.Items[0].Name, metav1.GetOptions{})
+		if err != nil {
+			c.Logf("Failed to get VolumeSnapshotClass, skipping test. Error: %v", err)
+			c.Fail()
+		}
+		snapshotterName = vsClass.Snapshotter
+		s.snapshotClass = &vsClass.Name
 	}
 
 	storageClasses, err := cli.StorageV1().StorageClasses().List(metav1.ListOptions{})
 	c.Assert(err, IsNil)
 	for _, class := range storageClasses.Items {
-		if isCSIProvisioner(class.Provisioner) {
+		if class.Provisioner == snapshotterName {
 			s.storageClassCSI = &class.Name
+			break
 		}
 	}
 
@@ -149,6 +157,7 @@ func (s *SnapshotTestSuite) TestVolumeSnapshotCloneFake(c *C) {
 			},
 		},
 	}
+	ctime := metav1.Now()
 	snapshot := &snapshot.VolumeSnapshot{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fakeSnapshotName,
@@ -159,7 +168,8 @@ func (s *SnapshotTestSuite) TestVolumeSnapshotCloneFake(c *C) {
 			VolumeSnapshotClassName: &fakeClass,
 		},
 		Status: snapshot.VolumeSnapshotStatus{
-			ReadyToUse: true,
+			ReadyToUse:   true,
+			CreationTime: &ctime,
 		},
 	}
 
@@ -274,15 +284,6 @@ func (s *SnapshotTestSuite) TestVolumeSnapshot(c *C) {
 
 }
 
-func isCSIProvisioner(provisioner string) bool {
-	for _, part := range strings.Split(provisioner, ".") {
-		if part == "csi" {
-			return true
-		}
-	}
-	return false
-}
-
 func (s *SnapshotTestSuite) cleanupNamespace(c *C, ns string) {
 	pvcs, erra := s.cli.CoreV1().PersistentVolumeClaims(ns).List(metav1.ListOptions{})
 	if erra != nil {