Fix error check for checkandAddSnapshotFinalizers and add tests

Signed-off-by: Grant Griffiths <grant@portworx.com>

pkg/common-controller/framework_test.go

@@ -173,11 +173,6 @@ func withSnapshotFinalizers(snapshots []*crdv1.VolumeSnapshot, finalizers ...str
 	return snapshots
 }
 
-func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent {
-	content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
-	return content
-}
-
 func withPVCFinalizer(pvc *v1.PersistentVolumeClaim) *v1.PersistentVolumeClaim {
 	pvc.ObjectMeta.Finalizers = append(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer)
 	return pvc
@@ -837,6 +832,18 @@ func withContentAnnotations(contents []*crdv1.VolumeSnapshotContent, annotations
 	return contents
 }
 
+func withContentSpecSnapshotClassName(contents []*crdv1.VolumeSnapshotContent, volumeSnapshotClassName *string) []*crdv1.VolumeSnapshotContent {
+	for i := range contents {
+		contents[i].Spec.VolumeSnapshotClassName = volumeSnapshotClassName
+	}
+	return contents
+}
+
+func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSnapshotContent {
+	content.ObjectMeta.Finalizers = append(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
+	return content
+}
+
 func newContentArray(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
 	deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
 	withFinalizer bool) []*crdv1.VolumeSnapshotContent {

pkg/common-controller/snapshot_controller.go

@@ -173,7 +173,11 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 		}
 	} else {
 		klog.V(5).Infof("syncSnapshot[%s]: check if we should add finalizers on snapshot", utils.SnapshotKey(snapshot))
-		ctrl.checkandAddSnapshotFinalizers(snapshot)
+		if err := ctrl.checkandAddSnapshotFinalizers(snapshot); err != nil {
+			klog.Errorf("error check and add Snapshot finalizers for snapshot [%s]: %v", snapshot.Name, errFinalizer)
+			ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot: %s", err.Error()))
+			return err
+		}
 		// Need to build or update snapshot.Status in following cases:
 		// 1) snapshot.Status is nil
 		// 2) snapshot.Status.ReadyToUse is false
@@ -301,7 +305,7 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot
 	if addSourceFinalizer || addBoundFinalizer {
 		// Snapshot is not being deleted -> it should have the finalizer.
 		klog.V(5).Infof("checkandAddSnapshotFinalizers: Add Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot))
-		ctrl.addSnapshotFinalizer(snapshot, addSourceFinalizer, addBoundFinalizer)
+		return ctrl.addSnapshotFinalizer(snapshot, addSourceFinalizer, addBoundFinalizer)
 	}
 	return nil
 }
@@ -665,26 +669,6 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
 	return nil
 }
 
-// isSnapshotConentBeingUsed checks if snapshot content is bound to snapshot.
-func (ctrl *csiSnapshotCommonController) isSnapshotContentBeingUsed(content *crdv1.VolumeSnapshotContent) bool {
-	if content.Spec.VolumeSnapshotRef.Name != "" && content.Spec.VolumeSnapshotRef.Namespace != "" {
-		snapshotObj, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(content.Spec.VolumeSnapshotRef.Namespace).Get(content.Spec.VolumeSnapshotRef.Name, metav1.GetOptions{})
-		if err != nil {
-			klog.Infof("isSnapshotContentBeingUsed: Cannot get snapshot %s from api server: [%v]. VolumeSnapshot object may be deleted already.", content.Spec.VolumeSnapshotRef.Name, err)
-			return false
-		}
-
-		// Check if the snapshot content is bound to the snapshot
-		if utils.IsSnapshotBound(snapshotObj, content) {
-			klog.Infof("isSnapshotContentBeingUsed: VolumeSnapshot %s is bound to volumeSnapshotContent [%s]", snapshotObj.Name, content.Name)
-			return true
-		}
-	}
-
-	klog.V(5).Infof("isSnapshotContentBeingUsed: Snapshot content %s is not being used", content.Name)
-	return false
-}
-
 // addContentFinalizer adds a Finalizer for VolumeSnapshotContent.
 func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
 	contentClone := content.DeepCopy()
@@ -858,7 +842,7 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c
 	}
 	newContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(contentClone)
 	if err != nil {
-		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", newContent.Name, err)
+		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err)
 		return nil, err
 	}
 

pkg/common-controller/snapshot_create_test.go

@@ -79,13 +79,18 @@ func TestCreateSnapshotSync(t *testing.T) {
 			test:              testSyncSnapshot,
 		},
 		{
-			name:              "6-2 - successful create snapshot with snapshot class silver",
+			name:            "6-2 - successful create snapshot with validSecretClass and initial secret",
-			initialContents:   nocontents,
+			initialContents: nocontents,
-			expectedContents:  newContentArrayNoStatus("snapcontent-snapuid6-2", "snapuid6-2", "snap6-2", "sid6-2", classSilver, "", "pv-handle6-2", deletionPolicy, nil, nil, false, false),
+			expectedContents: withContentAnnotations(newContentArrayNoStatus("snapcontent-snapuid6-2", "snapuid6-2", "snap6-2", "sid6-2", validSecretClass, "", "pv-handle6-2", deletionPolicy, nil, nil, false, false),
-			initialSnapshots:  newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
+				map[string]string{
-			expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "snapcontent-snapuid6-2", &False, nil, nil, nil, false, true, nil),
+					"snapshot.storage.kubernetes.io/deletion-secret-name":      "secret",
+					"snapshot.storage.kubernetes.io/deletion-secret-namespace": "default",
+				}),
+			initialSnapshots:  newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", validSecretClass, "snapcontent-snapuid6-2", &False, nil, nil, nil, false, true, nil),
 			initialClaims:     newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
 			initialVolumes:    newVolumeArray("volume6-2", "pv-uid6-2", "pv-handle6-2", "1Gi", "pvc-uid6-2", "claim6-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:    []*v1.Secret{secret()}, // no initial secret created
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},
@@ -120,12 +125,12 @@ func TestCreateSnapshotSync(t *testing.T) {
 				{"update", "volumesnapshots", errors.New("mock update error")},
 			}, test: testSyncSnapshot,
 		},
-		/*{
+		{
 			name:                  "7-3 - fail to create snapshot without snapshot class ",
 			initialContents:       nocontents,
 			expectedContents:      nocontents,
-			initialSnapshots:      newSnapshotArray("snap7-3", "snapuid7-3", "claim7-3", "", "", "", &False, nil, nil, nil, false, true),
+			initialSnapshots:      newSnapshotArray("snap7-3", "snapuid7-3", "claim7-3", "", "", "", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots:     newSnapshotArray("snap7-3", "snapuid7-3", "claim7-3", "", "", "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-3: \"failed to retrieve snapshot class  from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"\\\\\\\" not found\\\"\""), false, true),
+			expectedSnapshots:     newSnapshotArray("snap7-3", "snapuid7-3", "claim7-3", "", "", "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-3: \"failed to take snapshot snap7-3 without a snapshot class\""), false, true, nil),
 			initialClaims:         newClaimArray("claim7-3", "pvc-uid7-3", "1Gi", "volume7-3", v1.ClaimBound, &classEmpty),
 			initialVolumes:        newVolumeArray("volume7-3", "pv-uid7-3", "pv-handle7-3", "1Gi", "pvc-uid7-3", "claim7-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialStorageClasses: []*storage.StorageClass{diffDriverStorageClass},
@@ -133,7 +138,7 @@ func TestCreateSnapshotSync(t *testing.T) {
 			errors:                noerrors,
 			expectSuccess:         false,
 			test:                  testSyncSnapshot,
-		},*/
+		},
 		{
 			name:              "7-4 - fail create snapshot with no-existing claim",
 			initialContents:   nocontents,
@@ -171,83 +176,87 @@ func TestCreateSnapshotSync(t *testing.T) {
 			expectSuccess:     false,
 			test:              testSyncSnapshot,
 		},
-		/*{
-				name:              "7-8 - fail create snapshot due to cannot update snapshot status",
-				initialContents:   nocontents,
-				expectedContents:  nocontents,
-				initialSnapshots:  newSnapshotArray("snap7-8", "snapuid7-8", "claim7-8", "", classGold, "", &False, nil, nil, nil),
-				expectedSnapshots: newSnapshotArray("snap7-8", "snapuid7-8", "claim7-8", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot: snapshot controller failed to update default/snap7-8 on API server: mock update error")),
-				initialClaims:     newClaimArray("claim7-8", "pvc-uid7-8", "1Gi", "volume7-8", v1.ClaimBound, &classEmpty),
-				initialVolumes:    newVolumeArray("volume7-8", "pv-uid7-8", "pv-handle7-8", "1Gi", "pvc-uid7-8", "claim7-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-				expectedCreateCalls: []createCall{
-								{
-								snapshotName: "snapshot-snapuid7-8",
-								volume:       newVolume("volume7-8", "pv-uid7-8", "pv-handle7-8", "1Gi", "pvc-uid7-8", "claim7-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-								parameters:   map[string]string{"param1": "value1"},
-								// information to return
-								driverName:   mockDriverName,
-								size:         defaultSize,
-								snapshotId:   "sid7-8",
-								creationTime: timeNow,
-								readyToUse:   True,
-						},
-				},
-				/*errors: []reactorError{
-								// Inject error to the forth client.VolumesnapshotV1beta1().VolumeSnapshots().Update call.
-								// All other calls will succeed.
-								{"update", "volumesnapshots", errors.New("mock update error")},
-								{"update", "volumesnapshots", errors.New("mock update error")},
-								{"update", "volumesnapshots", errors.New("mock update error")},
-						},
-						expectedEvents: []string{"Warning SnapshotContentCreationFailed"},
 
-						expectSuccess:     false,
+		{
-						test:           testSyncSnapshot,
+			name:              "7-7 - remove pvc finalizer failed",
-				},
+			initialContents:   newContentArray("snapcontent-snapuid7-7", "snapuid7-7", "snap7-7", "sid7-7", classGold, "", "pv-handle7-7", deletionPolicy, nil, nil, false),
-		        /*{
+			expectedContents:  newContentArray("snapcontent-snapuid7-7", "snapuid7-7", "snap7-7", "sid7-7", classGold, "", "pv-handle7-7", deletionPolicy, nil, nil, false),
-		                // TODO(xiangqian): this test case needs to be revisited the scenario
+			initialSnapshots:  newSnapshotArray("snap7-7", "snapuid7-7", "claim7-7", "", classGold, "snapcontent-snapuid7-7", &True, nil, nil, nil, false, true, nil),
-		                // of VolumeSnapshotContent saving failure. Since there will be no content object
+			expectedSnapshots: newSnapshotArray("snap7-7", "snapuid7-7", "claim7-7", "", classGold, "snapcontent-snapuid7-7", &True, nil, nil, nil, false, true, nil),
-		                // in API server, it could potentially cause leaking issue
+			initialClaims:     newClaimArrayFinalizer("claim7-7", "pvc-uid7-7", "1Gi", "volume7-7", v1.ClaimBound, &classEmpty),
-		                name:              "7-9 - fail create snapshot due to cannot save snapshot content",
+			initialVolumes:    newVolumeArray("volume7-7", "pv-uid7-7", "pv-handle7-7", "1Gi", "pvc-uid7-7", "claim7-7", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-		                initialContents:   nocontents,
+			errors: []reactorError{
-		                expectedContents:  nocontents,
+				{"update", "persistentvolumeclaims", errors.New("mock update error")},
-		                initialSnapshots:  newSnapshotArray("snap7-9", "snapuid7-9", "claim7-9", "", classGold, "", &False, nil, nil, nil),
+				{"update", "persistentvolumeclaims", errors.New("mock update error")},
-		                expectedSnapshots: newSnapshotArray("snap7-9", "snapuid7-9", "claim7-9", "", classGold, "snapcontent-snapuid7-9", &True, metaTimeNowUnix, getSize(defaultSize), nil),
+				{"update", "persistentvolumeclaims", errors.New("mock update error")},
-		                initialClaims:     newClaimArray("claim7-9", "pvc-uid7-9", "1Gi", "volume7-9", v1.ClaimBound, &classEmpty),
+			},
-		                initialVolumes:    newVolumeArray("volume7-9", "pv-uid7-9", "pv-handle7-9", "1Gi", "pvc-uid7-9", "claim7-9", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			expectSuccess: false,
-		                expectedCreateCalls: []createCall{
+			test:          testSyncSnapshot,
-		                        {
+		},
-		                                snapshotName: "snapshot-snapuid7-9",
+		{
-		                                volume:       newVolume("volume7-9", "pv-uid7-9", "pv-handle7-9", "1Gi", "pvc-uid7-9", "claim7-9", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			name:              "7-8 - fail create snapshot due to cannot update snapshot status",
-		                                parameters:   map[string]string{"param1": "value1"},
+			initialContents:   nocontents,
-		                                // information to return
+			expectedContents:  newContentArrayNoStatus("snapcontent-snapuid7-8", "snapuid7-8", "snap7-8", "sid7-8", classGold, "", "pv-handle7-8", deletionPolicy, nil, nil, false, false),
-		                                driverName:   mockDriverName,
+			initialSnapshots:  newSnapshotArray("snap7-8", "snapuid7-8", "claim7-8", "", classGold, "", &False, nil, nil, nil, false, true, nil),
-		                                size:         defaultSize,
+			expectedSnapshots: newSnapshotArray("snap7-8", "snapuid7-8", "claim7-8", "", classGold, "", &False, nil, nil, newVolumeError("Snapshot status update failed, snapshot controller failed to update default/snap7-8 on API server: mock update error"), false, true, nil),
-		                                snapshotId:   "sid7-9",
+			initialClaims:     newClaimArray("claim7-8", "pvc-uid7-8", "1Gi", "volume7-8", v1.ClaimBound, &classEmpty),
-		                                creationTime: timeNow,
+			initialVolumes:    newVolumeArray("volume7-8", "pv-uid7-8", "pv-handle7-8", "1Gi", "pvc-uid7-8", "claim7-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-		                                readyToUse:   True,
+			errors: []reactorError{
-		                        },
+				{"update", "volumesnapshots", errors.New("mock update error")},
-		                },
+				{"update", "volumesnapshots", errors.New("mock update error")},
-		                errors: []reactorError{
+				{"update", "volumesnapshots", errors.New("mock update error")},
-		                        {"create", "volumesnapshotcontents", errors.New("mock create error")},
+			},
-		                        {"create", "volumesnapshotcontents", errors.New("mock create error")},
+			expectedEvents: []string{"Warning SnapshotStatusUpdateFailed"},
-		                        {"create", "volumesnapshotcontents", errors.New("mock create error")},
+			expectSuccess:  false,
-		                },
+			test:           testSyncSnapshot,
-		                expectedEvents: []string{"Warning CreateSnapshotContentFailed"},
+		},
-		                test:           testSyncSnapshot,
+		{
-		        },
+			name:              "7-9 - fail create snapshot due to cannot update snapshot status, and failure cannot be recorded either due to additional status update failure.",
-				{
+			initialContents:   nocontents,
-					name:              "7-10 - fail create snapshot with secret not found",
+			expectedContents:  newContentArrayNoStatus("snapcontent-snapuid7-9", "snapuid7-9", "snap7-9", "sid7-9", classGold, "", "pv-handle7-9", deletionPolicy, nil, nil, false, false),
-					initialContents:   nocontents,
+			initialSnapshots:  newSnapshotArray("snap7-9", "snapuid7-9", "claim7-9", "", classGold, "", &False, nil, nil, nil, false, true, nil),
-					expectedContents:  nocontents,
+			expectedSnapshots: newSnapshotArray("snap7-9", "snapuid7-9", "claim7-9", "", classGold, "", &False, nil, nil, nil, false, true, nil),
-					initialSnapshots:  newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "", validSecretClass, "", &False, nil, nil, nil, false, true),
+			initialClaims:     newClaimArray("claim7-9", "pvc-uid7-9", "1Gi", "volume7-9", v1.ClaimBound, &classEmpty),
-					expectedSnapshots: newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "", validSecretClass, "", &False, nil, nil, newVolumeError("Failed to create snapshot: error getting secret secret in namespace default: cannot find secret secret"), false, true),
+			initialVolumes:    newVolumeArray("volume7-9", "pv-uid7-9", "pv-handle7-9", "1Gi", "pvc-uid7-9", "claim7-9", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-					initialClaims:     newClaimArray("claim7-10", "pvc-uid7-10", "1Gi", "volume7-10", v1.ClaimBound, &classEmpty),
+			errors: []reactorError{
-					initialVolumes:    newVolumeArray("volume7-10", "pv-uid7-10", "pv-handle7-10", "1Gi", "pvc-uid7-10", "claim7-10", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+				{"update", "volumesnapshots", errors.New("mock update error")},
-					initialSecrets:    []*v1.Secret{}, // no initial secret created
+				{"update", "volumesnapshots", errors.New("mock update error")},
-					errors:            noerrors,
+				{"update", "volumesnapshots", errors.New("mock update error")},
-					test:              testSyncSnapshot,
+				{"update", "volumesnapshots", errors.New("mock update error")},
-				},*/
+			},
+			expectSuccess: false,
+			test:          testSyncSnapshot,
+		},
+		{
+			name:              "7-10 - fail create snapshot with invalid secret",
+			initialContents:   nocontents,
+			expectedContents:  nocontents,
+			initialSnapshots:  newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "", invalidSecretClass, "", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap7-10", "snapuid7-10", "claim7-10", "snap7-10", invalidSecretClass, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-10: \"failed to get name and namespace template from params: either name and namespace for Snapshotter secrets specified, Both must be specified\""), false, true, nil),
+			initialClaims:     newClaimArray("claim7-10", "pvc-uid7-10", "1Gi", "volume7-10", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume7-10", "pv-uid7-10", "pv-handle7-10", "1Gi", "pvc-uid7-10", "claim7-10", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialSecrets:    []*v1.Secret{}, // no initial secret created
+			test:              testSyncSnapshot,
+		},
+		{
+			// TODO(xiangqian): this test case needs to be revisited the scenario
+			// of VolumeSnapshotContent saving failure. Since there will be no content object
+			// in API server, it could potentially cause leaking issue
+			name:              "7-11 - fail create snapshot due to cannot save snapshot content",
+			initialContents:   nocontents,
+			expectedContents:  nocontents,
+			initialSnapshots:  newSnapshotArray("snap7-11", "snapuid7-11", "claim7-11", "", classGold, "", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap7-11", "snapuid7-11", "claim7-11", "", classGold, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error snapshot controller failed to update default/snap7-11 on API server: mock create error"), false, true, nil),
+			initialClaims:     newClaimArray("claim7-11", "pvc-uid7-11", "1Gi", "volume7-11", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume7-11", "pv-uid7-11", "pv-handle7-11", "1Gi", "pvc-uid7-11", "claim7-11", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			errors: []reactorError{
+				{"create", "volumesnapshotcontents", errors.New("mock create error")},
+				{"create", "volumesnapshotcontents", errors.New("mock create error")},
+				{"create", "volumesnapshotcontents", errors.New("mock create error")},
+			},
+			expectedEvents: []string{"Warning CreateSnapshotContentFailed"},
+			test:           testSyncSnapshot,
+		},
 	}
 	runSyncTests(t, tests, snapshotClasses)
 }

pkg/common-controller/snapshot_delete_test.go

@@ -35,8 +35,8 @@ var class2Parameters = map[string]string{
 }
 
 var class3Parameters = map[string]string{
-	"param3": "value3",
+	"param3":                               "value3",
-	//utils.SnapshotterSecretNameKey: "name",
+	utils.PrefixedSnapshotterSecretNameKey: "name",
 }
 
 var class4Parameters = map[string]string{
@@ -45,8 +45,8 @@ var class4Parameters = map[string]string{
 }
 
 var class5Parameters = map[string]string{
-	//utils.SnapshotterSecretNameKey:      "secret",
+	utils.PrefixedSnapshotterSecretNameKey:      "secret",
-	//utils.SnapshotterSecretNamespaceKey: "default",
+	utils.PrefixedSnapshotterSecretNamespaceKey: "default",
 }
 
 var timeNowMetav1 = metav1.Now()
@@ -130,158 +130,43 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
 func TestDeleteSync(t *testing.T) {
 	tests := []controllerTest{
 		{
-			name:              "1-1 - content with empty snapshot class is deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified",
+			name:              "1-1 - noop: content will not be deleted if it is bound to a snapshot correctly, snapshot uid is not specified",
-			initialContents:   newContentArray("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "", deletionPolicy, nil, nil, true),
+			initialContents:   newContentArray("content1-1", "", "snap1-1", "sid1-1", validSecretClass, "", "", deletePolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "", deletionPolicy, nil, nil, true),
+			expectedContents:  newContentArray("content1-1", "", "snap1-1", "sid1-1", validSecretClass, "", "", deletePolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
+			initialSnapshots:  newSnapshotArray("snap1-1", "snapuid1-1", "claim1-1", "", validSecretClass, "content1-1", &False, nil, nil, nil, false, true, &timeNowMetav1),
-			expectedSnapshots: nosnapshots,
+			expectedSnapshots: newSnapshotArray("snap1-1", "snapuid1-1", "claim1-1", "", validSecretClass, "content1-1", &False, nil, nil, nil, false, true, &timeNowMetav1),
 			expectedEvents:    noevents,
-			errors:            noerrors,
-			//expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
-			test: testSyncContent,
-		},
-		{
-			name:              "2-1 - content with empty snapshot class will not be deleted if it is bound to a non-exist snapshot but it does not have a snapshot uid specified",
-			initialContents:   newContentArray("content2-1", "", "snap2-1", "sid2-1", "", "", "", deletionPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content2-1", "", "snap2-1", "sid2-1", "", "", "", deletionPolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			//expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}},
-			test: testSyncContent,
-		},
-		{
-			name:              "1-2 - successful delete with snapshot class that has empty secret parameter",
-			initialContents:   newContentArray("content1-2", "sid1-2", "snap1-2", "sid1-2", emptySecretClass, "", "", deletionPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-2", "sid1-2", "snap1-2", "sid1-2", emptySecretClass, "", "", deletionPolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			initialSecrets:    []*v1.Secret{emptySecret()},
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			//expectedDeleteCalls: []deleteCall{{"sid1-2", map[string]string{}, nil}},
-			test: testSyncContent,
-		},
-		{
-			name:              "1-3 - successful delete with snapshot class that has valid secret parameter",
-			initialContents:   newContentArray("content1-3", "sid1-3", "snap1-3", "sid1-3", validSecretClass, "", "", deletionPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-3", "sid1-3", "snap1-3", "sid1-3", validSecretClass, "", "", deletionPolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			expectedEvents:    noevents,
-			errors:            noerrors,
 			initialSecrets:    []*v1.Secret{secret()},
-			//expectedDeleteCalls: []deleteCall{{"sid1-3", map[string]string{"foo": "bar"}, nil}},
+			errors:            noerrors,
-			test: testSyncContent,
+			test:              testSyncContent,
 		},
 		{
-			// delete success - snapshot that the content was pointing to was deleted, and another
+			// delete success - content is deleted before doDelete() starts
-			// with the same name created.
+			name:              "1-2 - content is deleted before deleting",
-			name:              "1-7 - prebound content is deleted while the snapshot exists",
+			initialContents:   newContentArray("content1-2", "sid1-2", "snap1-2", "sid1-2", validSecretClass, "", "", deletionPolicy, nil, nil, true),
-			initialContents:   newContentArray("content1-7", "sid1-7", "snap1-7", "sid1-7", emptySecretClass, "", "", deletionPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-7", "sid1-7", "snap1-7", "sid1-7", emptySecretClass, "", "", deletionPolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap1-7", "snapuid1-7-x", "claim1-7", "", validSecretClass, "", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap1-7", "snapuid1-7-x", "claim1-7", "", validSecretClass, "", &False, nil, nil, nil, false, true, nil),
-			initialSecrets:    []*v1.Secret{secret()},
-			//expectedDeleteCalls: []deleteCall{{"sid1-7", map[string]string{"foo": "bar"}, nil}},
-			expectedEvents: noevents,
-			errors:         noerrors,
-			test:           testSyncContent,
-		},
-		{
-			// delete success(?) - content is deleted before doDelete() starts
-			name:              "1-8 - content is deleted before deleting",
-			initialContents:   newContentArray("content1-8", "sid1-8", "snap1-8", "sid1-8", validSecretClass, "", "", deletionPolicy, nil, nil, true),
 			expectedContents:  nocontents,
 			initialSnapshots:  nosnapshots,
 			expectedSnapshots: nosnapshots,
 			initialSecrets:    []*v1.Secret{secret()},
-			//expectedDeleteCalls: []deleteCall{{"sid1-8", map[string]string{"foo": "bar"}, nil}},
+			expectedEvents:    noevents,
-			expectedEvents: noevents,
+			errors:            noerrors,
-			errors:         noerrors,
 			test: wrapTestWithInjectedOperation(testSyncContent, func(ctrl *csiSnapshotCommonController, reactor *snapshotReactor) {
 				// Delete the volume before delete operation starts
 				reactor.lock.Lock()
-				delete(reactor.contents, "content1-8")
+				delete(reactor.contents, "content1-2")
 				reactor.lock.Unlock()
 			}),
 		},
-		/*{
+		{
-			name:              "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
+			name:              "1-3 - will not delete content with retain policy set which is bound to a snapshot incorrectly",
-			initialContents:   newContentArray("content1-9", "snapuid1-9", "snap1-9", "sid1-9", validSecretClass, "", "", deletionPolicy, nil, nil, true),
+			initialContents:   newContentArray("content1-3", "snapuid1-3-x", "snap1-3", "sid1-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-9", "snapuid1-9", "snap1-9", "sid1-9", validSecretClass, "", "", deletionPolicy, nil, nil, true),
+			expectedContents:  newContentArray("content1-3", "snapuid1-3-x", "snap1-3", "sid1-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap1-9", "snapuid1-9", "claim1-9", "", validSecretClass, "content1-9", &False, nil, nil, nil),
+			initialSnapshots:  newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "content1-3", &False, nil, nil, nil, false, true, &timeNowMetav1),
-			expectedSnapshots: newSnapshotArray("snap1-9", "snapuid1-9", "claim1-9", "", validSecretClass, "content1-9", &True, nil, nil, nil),
+			expectedSnapshots: newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "content1-3", &False, nil, nil, nil, false, true, &timeNowMetav1),
 			expectedEvents:    noevents,
 			initialSecrets:    []*v1.Secret{secret()},
 			errors:            noerrors,
 			test:              testSyncContent,
-		},*/
-		{
-			name:              "1-10 - will not delete content with retain policy set which is bound to a snapshot incorrectly",
-			initialContents:   newContentArray("content1-10", "snapuid1-10-x", "snap1-10", "sid1-10", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-10", "snapuid1-10-x", "snap1-10", "sid1-10", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap1-10", "snapuid1-10", "claim1-10", "", validSecretClass, "content1-10", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap1-10", "snapuid1-10", "claim1-10", "", validSecretClass, "content1-10", &False, nil, nil, nil, false, true, nil),
-			expectedEvents:    noevents,
-			initialSecrets:    []*v1.Secret{secret()},
-			errors:            noerrors,
-			test:              testSyncContent,
-		},
-		{
-			name:              "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified",
-			initialContents:   newContentArray("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap1-11", "snapuid1-11", "claim1-11", "", validSecretClass, "content1-11", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap1-11", "snapuid1-11", "claim1-11", "", validSecretClass, "content1-11", &False, nil, nil, nil, false, true, nil),
-			expectedEvents:    noevents,
-			initialSecrets:    []*v1.Secret{secret()},
-			errors:            noerrors,
-			test:              testSyncContent,
-		},
-		{
-			name:              "1-12 - content with retain policy will not be deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified",
-			initialContents:   newContentArray("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			test:              testSyncContent,
-		},
-		{
-			name:              "1-13 - content with empty snapshot class is not deleted when Deletion policy is not set even if it is bound to a non-exist snapshot and also has a snapshot uid specified",
-			initialContents:   newContentArray("content1-13", "sid1-13", "snap1-13", "sid1-13", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-13", "sid1-13", "snap1-13", "sid1-13", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			test:              testSyncContent,
-		},
-		/*{
-			name:              "1-14 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
-			initialContents:   newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap1-14", "snapuid1-14", "claim1-14", "", validSecretClass, "content1-14", &False, nil, nil, nil),
-			expectedSnapshots: newSnapshotArray("snap1-14", "snapuid1-14", "claim1-14", "", validSecretClass, "content1-14", &True, nil, nil, nil),
-			expectedEvents:    noevents,
-			initialSecrets:    []*v1.Secret{secret()},
-			errors:            noerrors,
-			test:              testSyncContent,
-		},*/
-		{
-			name:              "1-16 - continue delete with snapshot class that has nonexistent secret",
-			initialContents:   newContentArray("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, nil, true),
-			initialSnapshots:  nosnapshots,
-			expectedSnapshots: nosnapshots,
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			initialSecrets:    []*v1.Secret{}, // secret does not exist
-			//expectedDeleteCalls: []deleteCall{{"sid1-16", nil, nil}},
-			test: testSyncContent,
 		},
 		{
 			name:             "3-1 - content will be deleted if snapshot deletion timestamp is set",
@@ -314,6 +199,23 @@ func TestDeleteSync(t *testing.T) {
 			expectSuccess: false,
 			test:          testSyncSnapshotError,
 		},
+		{
+			name:            "3-3 - content will not be deleted if retainPolicy is set",
+			initialContents: newContentArray("content3-3", "", "snap3-3", "sid3-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
+			expectedContents: withContentAnnotations(newContentArray("content3-3", "", "snap3-3", "sid3-3", validSecretClass, "", "", retainPolicy, nil, nil, true),
+				map[string]string{
+					"snapshot.storage.kubernetes.io/volumesnapshot-being-deleted": "yes",
+				}),
+			initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &False, nil, nil, nil, false, true, &timeNowMetav1),
+			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &False, nil, nil, nil, false, false, &timeNowMetav1),
+				utils.VolumeSnapshotBoundFinalizer,
+			),
+			initialClaims:  newClaimArray("claim3-3", "pvc-uid3-3", "1Gi", "volume3-3", v1.ClaimBound, &classEmpty),
+			expectedEvents: noevents,
+			initialSecrets: []*v1.Secret{secret()},
+			errors:         noerrors,
+			test:           testSyncSnapshot,
+		},
 	}
 	runSyncTests(t, tests, snapshotClasses)
 }

pkg/common-controller/snapshot_update_test.go

@@ -37,6 +37,8 @@ var volumeErr = &storagev1beta1.VolumeError{
 	Message: "Failed to upload the snapshot",
 }
 
+var emptyString = ""
+
 // Test single call to syncSnapshot and syncContent methods.
 // 1. Fill in the controller with initial data
 // 2. Call the tested function (syncSnapshot/syncContent) via
@@ -56,16 +58,17 @@ func TestSync(t *testing.T) {
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},
-		/*{
+		{
 			name:              "2-2 - snapshot points to a content but content does not point to snapshot(VolumeSnapshotRef does not match)",
 			initialContents:   newContentArray("content2-2", "snapuid2-2-x", "snap2-2", "sid2-2", validSecretClass, "", "", deletionPolicy, nil, nil, false),
 			expectedContents:  newContentArray("content2-2", "snapuid2-2-x", "snap2-2", "sid2-2", validSecretClass, "", "", deletionPolicy, nil, nil, false),
-			initialSnapshots:  newSnapshotArray("snap2-2", "snapuid2-2", "claim2-2", "", validSecretClass, "content2-2", &False, nil, nil, nil),
+			initialSnapshots:  newSnapshotArray("snap2-2", "snapuid2-2", "", "content2-2", validSecretClass, "content2-2", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap2-2", "snapuid2-2", "claim2-2", "", validSecretClass, "content2-2", &False, nil, nil, newVolumeError("Snapshot is bound to a VolumeSnapshotContent which is bound to other Snapshot")),
+			expectedSnapshots: newSnapshotArray("snap2-2", "snapuid2-2", "", "content2-2", validSecretClass, "content2-2", &False, nil, nil, newVolumeError("Snapshot failed to bind VolumeSnapshotContent, Could not bind snapshot snap2-2 and content content2-2, the VolumeSnapshotRef does not match"), false, true, nil),
-			expectedEvents:    []string{"Warning InvalidSnapshotBinding"},
+			initialClaims:     newClaimArray("claim2-2", "pvc-uid2-2", "1Gi", "volume2-2", v1.ClaimBound, &classEmpty),
+			expectedEvents:    []string{"Warning SnapshotBindFailed"},
 			errors:            noerrors,
 			test:              testSyncSnapshotError,
-		},*/
+		},
 		{
 			name:              "2-3 - success bind snapshot and content but not ready, no status changed",
 			initialContents:   newContentArray("content2-3", "snapuid2-3", "snap2-3", "sid2-3", validSecretClass, "", "", deletionPolicy, nil, nil, false),
@@ -75,22 +78,8 @@ func TestSync(t *testing.T) {
 			initialClaims:     newClaimArray("claim2-3", "pvc-uid2-3", "1Gi", "volume2-3", v1.ClaimBound, &classEmpty),
 			initialVolumes:    newVolumeArray("volume2-3", "pv-uid2-3", "pv-handle2-3", "1Gi", "pvc-uid2-3", "claim2-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialSecrets:    []*v1.Secret{secret()},
-			/*expectedCreateCalls: []createCall{
+			errors:            noerrors,
-				{
+			test:              testSyncSnapshot,
-					snapshotName: "snapshot-snapuid2-3",
-					volume:       newVolume("volume2-3", "pv-uid2-3", "pv-handle2-3", "1Gi", "pvc-uid2-3", "claim2-3", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-					parameters:   class5Parameters,
-					secrets:      map[string]string{"foo": "bar"},
-					// information to return
-					driverName:   mockDriverName,
-					size:         defaultSize,
-					snapshotId:   "sid2-3",
-					creationTime: timeNow,
-					readyToUse:   False,
-				},
-			},*/
-			errors: noerrors,
-			test:   testSyncSnapshot,
 		},
 		{
 			// nothing changed
@@ -111,22 +100,8 @@ func TestSync(t *testing.T) {
 			initialClaims:     newClaimArray("claim2-5", "pvc-uid2-5", "1Gi", "volume2-5", v1.ClaimBound, &classEmpty),
 			initialVolumes:    newVolumeArray("volume2-5", "pv-uid2-5", "pv-handle2-5", "1Gi", "pvc-uid2-5", "claim2-5", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialSecrets:    []*v1.Secret{secret()},
-			/*expectedCreateCalls: []createCall{
+			errors:            noerrors,
-				{
+			test:              testSyncSnapshot,
-					snapshotName: "snapshot-snapuid2-5",
-					volume:       newVolume("volume2-5", "pv-uid2-5", "pv-handle2-5", "1Gi", "pvc-uid2-5", "claim2-5", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-					parameters:   class5Parameters,
-					secrets:      map[string]string{"foo": "bar"},
-					// information to return
-					driverName:   mockDriverName,
-					size:         defaultSize,
-					snapshotId:   "sid2-5",
-					creationTime: timeNow,
-					readyToUse:   True,
-				},
-			},*/
-			errors: noerrors,
-			test:   testSyncSnapshot,
 		},
 		{
 			name:              "2-6 - snapshot bound to prebound content correctly, status ready false -> true, ref.UID '' -> 'snapuid2-6'",
@@ -137,69 +112,47 @@ func TestSync(t *testing.T) {
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},
-		/*{
+		{
-			name:              "2-7 - snapshot and content bound, csi driver get status error",
+			name:              "2-8 - snapshot and content bound, apiserver update status error",
-			initialContents:   newContentArrayWithReadyToUse("content2-7", "snapuid2-7", "snap2-7", "sid2-7", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
+			initialContents:   newContentArrayWithReadyToUse("content2-8", "snapuid2-8", "snap2-8", "sid2-8", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
-			expectedContents:  newContentArrayWithReadyToUse("content2-7", "snapuid2-7", "snap2-7", "sid2-7", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
+			expectedContents:  newContentArrayWithReadyToUse("content2-8", "snapuid2-8", "snap2-8", "sid2-8", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
-			initialSnapshots:  newSnapshotArray("snap2-7", "snapuid2-7", "claim2-7", "", validSecretClass, "content2-7", &False, metaTimeNow, nil, nil),
+			initialSnapshots:  newSnapshotArray("snap2-8", "snapuid2-8", "claim2-8", "", validSecretClass, "content2-8", &False, metaTimeNow, nil, nil, false, false, nil),
-			expectedSnapshots: newSnapshotArray("snap2-7", "snapuid2-7", "claim2-7", "", validSecretClass, "content2-7", &False, metaTimeNow, nil, newVolumeError("Failed to check and update snapshot: mock create snapshot error")),
+			expectedSnapshots: newSnapshotArray("snap2-8", "snapuid2-8", "claim2-8", "", validSecretClass, "content2-8", &False, metaTimeNow, nil, nil, false, false, nil),
 			expectedEvents:    []string{"Warning SnapshotCheckandUpdateFailed"},
-			initialClaims:     newClaimArray("claim2-7", "pvc-uid2-7", "1Gi", "volume2-7", v1.ClaimBound, &classEmpty),
+			initialClaims:     newClaimArray("claim2-8", "pvc-uid2-8", "1Gi", "volume2-8", v1.ClaimBound, &classEmpty),
-			initialVolumes:    newVolumeArray("volume2-7", "pv-uid2-7", "pv-handle2-7", "1Gi", "pvc-uid2-7", "claim2-7", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			initialVolumes:    newVolumeArray("volume2-8", "pv-uid2-8", "pv-handle2-8", "1Gi", "pvc-uid2-8", "claim2-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialSecrets:    []*v1.Secret{secret()},
-			expectedCreateCalls: []createCall{
-				{
-					snapshotName: "snapshot-snapuid2-7",
-					volume:       newVolume("volume2-7", "pv-uid2-7", "pv-handle2-7", "1Gi", "pvc-uid2-7", "claim2-7", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-					parameters:   class5Parameters,
-					secrets:      map[string]string{"foo": "bar"},
-					// information to return
-					err: errors.New("mock create snapshot error"),
-				},
-			},
-			errors: noerrors,
-			test:   testSyncSnapshot,
-		},*/
-		/*{
-		name:              "2-8 - snapshot and content bound, apiserver update status error",
-		initialContents:   newContentArrayWithReadyToUse("content2-8", "snapuid2-8", "snap2-8", "sid2-8", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
-		expectedContents:  newContentArrayWithReadyToUse("content2-8", "snapuid2-8", "snap2-8", "sid2-8", validSecretClass, "", "", deletionPolicy, &timeNowStamp, nil, &False, false),
-		initialSnapshots:  newSnapshotArray("snap2-8", "snapuid2-8", "claim2-8", "", validSecretClass, "content2-8", &False, metaTimeNow, nil, nil),
-		expectedSnapshots: newSnapshotArray("snap2-8", "snapuid2-8", "claim2-8", "", validSecretClass, "content2-8", &False, metaTimeNow, nil, newVolumeError("Failed to check and update snapshot: snapshot controller failed to update default/snap2-8 on API server: mock update error")),
-		expectedEvents:    []string{"Warning SnapshotCheckandUpdateFailed"},
-		initialClaims:     newClaimArray("claim2-8", "pvc-uid2-8", "1Gi", "volume2-8", v1.ClaimBound, &classEmpty),
-		initialVolumes:    newVolumeArray("volume2-8", "pv-uid2-8", "pv-handle2-8", "1Gi", "pvc-uid2-8", "claim2-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-		initialSecrets:    []*v1.Secret{secret()},
-		/*expectedCreateCalls: []createCall{
-			{
-				snapshotName: "snapshot-snapuid2-8",
-				volume:       newVolume("volume2-8", "pv-uid2-8", "pv-handle2-8", "1Gi", "pvc-uid2-8", "claim2-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
-				parameters:   class5Parameters,
-				secrets:      map[string]string{"foo": "bar"},
-				// information to return
-				driverName:   mockDriverName,
-				size:         defaultSize,
-				snapshotId:   "sid2-8",
-				creationTime: timeNow,
-				readyToUse:   true,
-			},
-		},*/ /*
 			errors: []reactorError{
 				// Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshots().Update call.
 				// All other calls will succeed.
 				{"update", "volumesnapshots", errors.New("mock update error")},
 			},
-			test: testSyncSnapshot,
+			test: testSyncSnapshotError,
-		},*/
+		},
 		{
 			name:              "2-9 - fail on status update as there is not pvc provided",
-			initialContents:   newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false),
+			initialContents:   nocontents,
-			expectedContents:  newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false),
+			expectedContents:  nocontents,
 			initialSnapshots:  newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, nil, false, true, nil),
-			expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "content2-9", &True, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error snapshot controller failed to update snap2-9 on API server: cannot get claim from snapshot"), false, true, nil),
-			//expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "content2-9", &False, nil, nil, newVolumeError("Failed to check and update snapshot: failed to get input parameters to create snapshot snap2-9: \"failed to retrieve PVC claim2-9 from the lister: \\\"persistentvolumeclaim \\\\\\\"claim2-9\\\\\\\" not found\\\"\"")),
+			errors: []reactorError{
-			errors: noerrors,
+				{"get", "persistentvolumeclaims", errors.New("mock update error")},
-			test:   testSyncSnapshot,
+				{"get", "persistentvolumeclaims", errors.New("mock update error")},
+				{"get", "persistentvolumeclaims", errors.New("mock update error")},
+			}, test: testSyncSnapshot,
+		},
+		{
+			name:              "7-1 - fail to create snapshot with non-existing snapshot class",
+			initialContents:   nocontents,
+			expectedContents:  nocontents,
+			initialSnapshots:  newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap7-1", "snapuid7-1", "claim7-1", "", classNonExisting, "", &False, nil, nil, newVolumeError("Failed to create snapshot content with error failed to get input parameters to create snapshot snap7-1: \"failed to retrieve snapshot class non-existing from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"non-existing\\\\\\\" not found\\\"\""), false, true, nil),
+			initialClaims:     newClaimArray("claim7-1", "pvc-uid7-1", "1Gi", "volume7-1", v1.ClaimBound, &classEmpty),
+			initialVolumes:    newVolumeArray("volume7-1", "pv-uid7-1", "pv-handle7-1", "1Gi", "pvc-uid7-1", "claim7-1", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
+			expectedEvents:    []string{"Warning SnapshotContentCreationFailed"},
+			errors:            noerrors,
+			expectSuccess:     false,
+			test:              testSyncSnapshot,
 		},
 		{
 			name:              "2-10 - do not bind when snapshot and content not match",
@@ -210,6 +163,29 @@ func TestSync(t *testing.T) {
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},
+		{
+			name:              "2-11 - successful bind snapshot content with volume snapshot classname",
+			initialContents:   withContentSpecSnapshotClassName(newContentArray("content2-11", "snapuid2-11", "snap2-11", "sid2-11", validSecretClass, "", "", deletionPolicy, nil, nil, false), nil),
+			expectedContents:  newContentArray("content2-11", "snapuid2-11", "snap2-11", "sid2-11", validSecretClass, "", "", deletionPolicy, nil, nil, false),
+			initialSnapshots:  newSnapshotArray("snap2-11", "snapuid2-11", "", "content2-11", validSecretClass, "content2-11", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap2-11", "snapuid2-11", "", "content2-11", validSecretClass, "content2-11", &True, nil, nil, nil, false, true, nil),
+			initialClaims:     newClaimArray("claim2-11", "pvc-uid2-11", "1Gi", "volume2-11", v1.ClaimBound, &classEmpty),
+			errors:            noerrors,
+			test:              testSyncSnapshot,
+		},
+		{
+			name:              "2-12 - fail bind snapshot content with volume snapshot classname due to API call failed",
+			initialContents:   withContentSpecSnapshotClassName(newContentArray("content2-12", "snapuid2-12", "snap2-12", "sid2-12", validSecretClass, "", "", deletionPolicy, nil, nil, false), nil),
+			expectedContents:  withContentSpecSnapshotClassName(newContentArray("content2-12", "snapuid2-12", "snap2-12", "sid2-12", validSecretClass, "", "", deletionPolicy, nil, nil, false), nil),
+			initialSnapshots:  newSnapshotArray("snap2-12", "snapuid2-12", "", "content2-12", validSecretClass, "content2-12", &False, nil, nil, nil, false, true, nil),
+			expectedSnapshots: newSnapshotArray("snap2-12", "snapuid2-12", "", "content2-12", validSecretClass, "content2-12", &False, nil, nil, newVolumeError("Snapshot failed to bind VolumeSnapshotContent, mock update error"), false, true, nil),
+			initialClaims:     newClaimArray("claim2-12", "pvc-uid2-12", "1Gi", "volume2-12", v1.ClaimBound, &classEmpty),
+			errors: []reactorError{
+				// Inject error to the forth client.VolumesnapshotV1beta1().VolumeSnapshots().Update call.
+				{"update", "volumesnapshotcontents", errors.New("mock update error")},
+			},
+			test: testSyncSnapshot,
+		},
 		{
 			name:              "3-1 - ready snapshot lost reference to VolumeSnapshotContent",
 			initialContents:   nocontents,