sidecar controller update, fix potential snapshot leaking

resolve comments

pkg/sidecar-controller/content_create_test.go

@@ -25,12 +25,13 @@ func TestSyncContent(t *testing.T) {
 	var tests []controllerTest
 
 	tests = append(tests, controllerTest{
-		name:             "Basic content create ready to use",
+		name:             "Basic content update ready to use",
-		initialContents:  newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "", retainPolicy, nil, &defaultSize, &False, true),
+		initialContents:  newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
-		expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "", retainPolicy, nil, &defaultSize, &True, true),
+		expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
 		expectedEvents:   noevents,
 		expectedCreateCalls: []createCall{
 			{
+				volumeHandle: "volume-handle-1-1",
 				snapshotName: "snapshot-snapuid1-1",
 				driverName:   mockDriverName,
 				snapshotId:   "snapuid1-1",

pkg/sidecar-controller/framework_test.go

@@ -553,10 +553,6 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa
 		Spec: crdv1.VolumeSnapshotContentSpec{
 			Driver:         mockDriverName,
 			DeletionPolicy: deletionPolicy,
-			Source: crdv1.VolumeSnapshotContentSource{
-				SnapshotHandle: &snapshotHandle,
-				VolumeHandle:   &volumeHandle,
-			},
 		},
 		Status: &crdv1.VolumeSnapshotContentStatus{
 			CreationTime: creationTime,

pkg/sidecar-controller/snapshot_controller.go

@@ -23,7 +23,7 @@ import (
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/utils"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/util/goroutinemap"
@@ -34,20 +34,20 @@ import (
 // Design:
 //
 // This is the sidecar controller that is responsible for creating and deleting a
-// snapshot on the storage infrastructure through a csi volume driver. It watches
+// snapshot on the storage system through a csi volume driver. It watches
-// the VolumeSnapshotContent object which is either created/deleted by the
+// VolumeSnapshotContent objects which have been either created/deleted by the
 // common snapshot controller in the case of dynamic provisioning or by the admin
 // in the case of pre-provisioned snapshots.
 
-// The snapshot creation through csi volume driver should return a snapshot after
+// The snapshot creation through csi driver should return a snapshot after
-// it is created successfully (however, the snapshot might not be ready to use yet if
+// it is created successfully(however, the snapshot might not be ready to use yet
-// there is an uploading phase). The creationTime will be updated accordingly
+// if there is an uploading phase). The creationTime will be updated accordingly
 // on the status of VolumeSnapshotContent.
 // After that, the sidecar controller will keep checking the snapshot status
 // through csi snapshot calls. When the snapshot is ready to use, the sidecar
 // controller set the status "ReadyToUse" to true on the VolumeSnapshotContent object
-// to indicate the snapshot is ready to use. If the creation failed for any reason,
+// to indicate the snapshot is ready to be used to restore a volume.
-// the Error status is set accordingly.
+// If the creation failed for any reason, the Error status is set accordingly.
 
 const controllerUpdateFailMsg = "snapshot controller failed to update"
 
@@ -57,73 +57,50 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 
 	var err error
 	if ctrl.shouldDelete(content) {
-		switch content.Spec.DeletionPolicy {
-		case crdv1.VolumeSnapshotContentRetain:
-			klog.V(4).Infof("VolumeSnapshotContent[%s]: policy is Retain. Keep physical snapshot and remove content finalizer", content.Name)
-			// It is a deletion candidate if DeletionTimestamp is not nil and
-			// VolumeSnapshotContentFinalizer is set.
-			if utils.IsContentDeletionCandidate(content) {
-				// Volume snapshot content is a deletion candidate.
-				// Remove the content finalizer.
-				klog.V(5).Infof("syncContent: Content [%s] is a deletion candidate. Remove finalizer.", content.Name)
-				return ctrl.removeContentFinalizer(content)
-			}
-
-		case crdv1.VolumeSnapshotContentDelete:
-			klog.V(4).Infof("VolumeSnapshotContent[%s]: policy is Delete. Delete physical snapshot", content.Name)
-			err = ctrl.deleteCSISnapshot(content)
-			if err != nil {
-				return err
-			}
-			klog.V(5).Infof("syncContent: check if we should remove Finalizer for VolumeSnapshotContent[%s]", content.Name)
-			// It is a deletion candidate if DeletionTimestamp is not nil and
-			// VolumeSnapshotContentFinalizer is set.
-			if utils.IsContentDeletionCandidate(content) {
-				// Volume snapshot content is a deletion candidate.
-				// Remove the content finalizer.
-				klog.V(5).Infof("syncContent: Content [%s] is a deletion candidate. Remove finalizer.", content.Name)
-				return ctrl.removeContentFinalizer(content)
-			}
-
-		default:
-			// Unknown VolumeSnapshotDeletionPolicy
-			ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotUnknownDeletionPolicy", "Volume Snapshot Content has unrecognized deletion policy")
-		}
 		klog.V(4).Infof("VolumeSnapshotContent[%s]: the policy is %s", content.Name, content.Spec.DeletionPolicy)
+		if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete &&
+			content.Status != nil && content.Status.SnapshotHandle != nil {
+			// issue a CSI deletion call if the snapshot has not been deleted yet from
+			// underlying storage system. Note that the deletion snapshot operation will
+			// update content SnapshotHandle to nil upon a successful deletion. At this
+			// point, the finalizer on content should NOT be removed to avoid leaking.
+			return ctrl.deleteCSISnapshot(content)
+		} else {
+			// otherwise, either the snapshot has been deleted from the underlying
+			// storage system, or the deletion policy is Retain, remove the finalizer
+			// if there is one so that API server could delete the object if there is
+			// no other finalizer.
+			return ctrl.removeContentFinalizer(content)
+		}
 	} else {
-		var err error
-		klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name)
 		if content.Spec.Source.VolumeHandle != nil && content.Status == nil {
+			klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name)
 			if err = ctrl.createSnapshot(content); err != nil {
 				ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err))
 				return err
 			}
 		} else {
-
 			if err = ctrl.checkandUpdateContentStatus(content); err != nil {
 				ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentStatusUpdateFailed", fmt.Sprintf("Failed to update snapshot content status with error %v", err))
 				return err
 			}
 		}
-
-		return nil
 	}
-
 	return nil
 }
 
 // deleteCSISnapshot starts delete action.
 func (ctrl *csiSnapshotSideCarController) deleteCSISnapshot(content *crdv1.VolumeSnapshotContent) error {
 	operationName := fmt.Sprintf("delete-%s", content.Name)
-	klog.V(5).Infof("Snapshotter is about to delete volume snapshot content and the operation named %s", operationName)
+	klog.V(5).Infof("schedule to delete snapshot, operation named %s", operationName)
 	ctrl.scheduleOperation(operationName, func() error {
 		return ctrl.deleteCSISnapshotOperation(content)
 	})
 	return nil
 }
 
-// scheduleOperation starts given asynchronous operation on given volume. It
+// scheduleOperation starts given asynchronous operation on given snapshot. It
-// makes sure the operation is already not running.
+// makes sure there is no running operation with the same operationName
 func (ctrl *csiSnapshotSideCarController) scheduleOperation(operationName string, operation func() error) {
 	klog.V(5).Infof("scheduleOperation[%s]", operationName)
 
@@ -199,17 +176,17 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont
 	if content.Status != nil && content.Status.Error != nil && *content.Status.Error.Message == message {
 		klog.V(4).Infof("updateContentStatusWithEvent[%s]: the same error %v is already set", content.Name, content.Status.Error)
 		return nil
-	} else if content.Status == nil {
-		content.Status = &crdv1.VolumeSnapshotContentStatus{}
 	}
 	contentClone := content.DeepCopy()
-	statusError := &crdv1.VolumeSnapshotError{
+	if contentClone.Status == nil {
+		contentClone.Status = &crdv1.VolumeSnapshotContentStatus{}
+	}
+	contentClone.Status.Error = &crdv1.VolumeSnapshotError{
 		Time: &metav1.Time{
 			Time: time.Now(),
 		},
 		Message: &message,
 	}
-	contentClone.Status.Error = statusError
 	ready := false
 	contentClone.Status.ReadyToUse = &ready
 	newContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().UpdateStatus(contentClone)
@@ -233,7 +210,7 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont
 
 func (ctrl *csiSnapshotSideCarController) getCSISnapshotInput(content *crdv1.VolumeSnapshotContent) (*crdv1.VolumeSnapshotClass, map[string]string, error) {
 	className := content.Spec.VolumeSnapshotClassName
-	klog.V(5).Infof("getCSISnapshotInput for content [%s]: VolumeSnapshotClassName [%s]", content.Name, *className)
+	klog.V(5).Infof("getCSISnapshotInput for content [%s]", content.Name)
 	var class *crdv1.VolumeSnapshotClass
 	var err error
 	if className != nil {
@@ -358,6 +335,7 @@ func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *cr
 
 	_, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content)
 	if err != nil {
+		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to get snapshot class or credentials")
 		return fmt.Errorf("failed to get input parameters to delete snapshot for content %s: %q", content.Name, err)
 	}
 
@@ -366,10 +344,42 @@ func (ctrl *csiSnapshotSideCarController) deleteCSISnapshotOperation(content *cr
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to delete snapshot")
 		return fmt.Errorf("failed to delete snapshot %#v, err: %v", content.Name, err)
 	}
-
+	// the snapshot has been deleted from the underlying storage system, update
+	// content status to remove snapshot handle etc.
+	newContent, err := ctrl.clearVolumeContentStatus(content.Name)
+	if err != nil {
+		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotDeleteError", "Failed to clear content status")
+		return err
+	}
+	// update local cache
+	ctrl.updateContentInCacheStore(newContent)
 	return nil
 }
 
+// clearVolumeContentStatus resets all fields to nil related to a snapshot in
+// content.Status. On success, the latest version of the content object will be
+// returned.
+func (ctrl *csiSnapshotSideCarController) clearVolumeContentStatus(
+	contentName string) (*crdv1.VolumeSnapshotContent, error) {
+	klog.V(5).Infof("cleanVolumeSnapshotStatus content [%s]", contentName)
+	// get the latest version from API server
+	content, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Get(contentName, metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("error get snapshot content %s from api server: %v", contentName, err)
+	}
+	if content.Status != nil {
+		content.Status.SnapshotHandle = nil
+		content.Status.ReadyToUse = nil
+		content.Status.CreationTime = nil
+		content.Status.RestoreSize = nil
+	}
+	newContent, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().UpdateStatus(content)
+	if err != nil {
+		return nil, newControllerUpdateError(contentName, err.Error())
+	}
+	return newContent, nil
+}
+
 func (ctrl *csiSnapshotSideCarController) updateSnapshotContentStatus(
 	content *crdv1.VolumeSnapshotContent,
 	snapshotHandle string,
@@ -497,8 +507,13 @@ func (ctrl *csiSnapshotSideCarController) GetCredentialsFromAnnotation(content *
 	return snapshotterCredentials, nil
 }
 
-// removeContentFinalizer removes a Finalizer for VolumeSnapshotContent.
+// removeContentFinalizer removes the VolumeSnapshotContentFinalizer from a
+// content if there exists one.
 func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
+	if !slice.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) {
+		// the finalizer does not exit, return directly
+		return nil
+	}
 	contentClone := content.DeepCopy()
 	contentClone.ObjectMeta.Finalizers = slice.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil)
 
@@ -507,12 +522,11 @@ func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.V
 		return newControllerUpdateError(content.Name, err.Error())
 	}
 
+	klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name)
 	_, err = ctrl.storeContentUpdate(contentClone)
 	if err != nil {
 		klog.Errorf("failed to update content store %v", err)
 	}
-
-	klog.V(5).Infof("Removed protection finalizer from volume snapshot content %s", content.Name)
 	return nil
 }
 
@@ -524,7 +538,7 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap
 	if content.ObjectMeta.DeletionTimestamp == nil {
 		return false
 	}
-	// 1) shouldDelete returns true if content is not bound
+	// 1) shouldDelete returns true if a content is not bound
 	// (VolumeSnapshotRef.UID == "") for pre-provisioned snapshot
 	if content.Spec.Source.SnapshotHandle != nil && content.Spec.VolumeSnapshotRef.UID == "" {
 		return true

pkg/sidecar-controller/snapshot_delete_test.go

@@ -25,7 +25,7 @@ import (
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/utils"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -140,7 +140,7 @@ func TestDeleteSync(t *testing.T) {
 		{
 			name:             "1-1 - content non-nil DeletionTimestamp with delete policy will delete snapshot",
 			initialContents:  newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
-			expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
+			expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "", classGold, "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
 			expectedEvents:   noevents,
 			errors:           noerrors,
 			initialSecrets:   []*v1.Secret{secret()},
@@ -183,9 +183,9 @@ func TestDeleteSync(t *testing.T) {
 			test:                testSyncContent,
 		},
 		{
-			name:             "1-3 - delete snapshot error should result in an event",
+			name:             "1-3 - delete snapshot error should result in an event, bound finalizer should remain",
 			initialContents:  newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, true, &timeNowMetav1),
-			expectedContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1),
+			expectedContents: newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletePolicy, nil, nil, true, &timeNowMetav1),
 			errors:           noerrors,
 			expectedCreateCalls: []createCall{
 				{
@@ -204,235 +204,124 @@ func TestDeleteSync(t *testing.T) {
 			expectedListCalls:   []listCall{{"sid1-3", true, time.Now(), 1, nil}},
 			test:                testSyncContent,
 		},
-		/*{
-			name:            "1-4 - create snapshot error should result in an event",
-			initialContents: newContentArrayWithDeletionTimestamp("content1-4", "snapuid1-4", "snap1-4", "sid1-4", classGold, "", "snap1-4-volumehandle", deletePolicy, nil, nil, true, nil),
-			//expectedContents: newContentArrayWithDeletionTimestamp("content1-4", "snapuid1-4", "snap1-4", "sid1-4", classGold, "", "snap1-4-volumehandle", deletePolicy, nil, nil, true, nil),
-			errors: []reactorError{},
-			expectedCreateCalls: []createCall{
-				{
-					snapshotName: "snapshot-snapuid1-4",
-					volumeHandle: "snap1-4-volumehandle",
-					parameters:   map[string]string{"param1": "value1"},
-					err:          fmt.Errorf("Create failed"),
-				},
-			},
-			//expectedDeleteCalls: []deleteCall{{"sid1-4", nil, nil}},
-			expectedListCalls: []listCall{{"sid1-4", true, time.Now(), 1, nil}},
-			expectedEvents:    []string{"Warning SnapshotContentCheckandUpdateFailed Failed to check and update snapshot content: Create failed"},
-			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),
-			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, "", "volumeHandle", deletionPolicy, nil, nil, true),
-			expectedContents:    nocontents,
-			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 - content non-nil DeletionTimestamp with delete policy will delete snapshot",
-			initialContents:   newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-1", "snap1-1", "sid1-3", classGold, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
-			expectedContents:  newContentArrayWithDeletionTimestamp("content1-3", "snapuid1-1", "snap1-1", "sid1-3", classGold, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
-			initialSnapshots:  newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil),
-			expectedSnapshots: newSnapshotArray("snap1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil),
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			initialSecrets:    []*v1.Secret{secret()},
-			expectedCreateCalls: []createCall{
-				{
-					snapshotName: "snapshot-snapuid1-1",
-					volumeHandle: "snap1-1-volumehandle",
-					parameters:   map[string]string{"param1": "value1"},
-					driverName:   mockDriverName,
-					size:         defaultSize,
-					snapshotId:   "snapuid1-1-deleted",
-					creationTime: timeNow,
-					readyToUse:   true,
-				},
-			},
-			expectedListCalls:   []listCall{{"sid1-1", true, time.Now(), 1, nil}},
-			expectedDeleteCalls: []deleteCall{{"sid1-1", nil, 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, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, true),
-			expectedContents:  newContentArray("content1-3", "sid1-3", "snap1-3", "sid1-3", validSecretClass, "", "snap1-3-volumehandle", deletionPolicy, nil, nil, false),
-			initialSnapshots:  newSnapshotArray("snapshot-snapuid1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil),
-			expectedSnapshots: newSnapshotArray("snapshot-snapuid1-3", "snapuid1-3", "claim1-3", "", validSecretClass, "", &False, nil, nil, nil),
-			expectedEvents:    noevents,
-			errors:            noerrors,
-			initialSecrets:    []*v1.Secret{secret()},
-			expectedCreateCalls: []createCall{
-				{
-					snapshotName: "snap1-3",
-					volumeHandle: "snap1-3-volumehandle",
-					parameters:   map[string]string{"param1": "value1"},
-					driverName:   mockDriverName,
-					size:         defaultSize,
-					snapshotId:   "sid1-3",
-					creationTime: timeNow,
-				},
-			},
-			expectedListCalls:   []listCall{{"sid1-3", true, time.Now(), 1, nil}},
-			expectedDeleteCalls: []deleteCall{{"sid1-3", map[string]string{"param1": "value1"}, nil}},
-			test:                testSyncContent,
-		},
-		*/
 		{
-			name:             "1-4 - fail delete with snapshot class that has invalid secret parameter",
+			name:             "1-4 - fail to delete with a snapshot class which has invalid secret parameter, bound finalizer should remain",
-			initialContents:  newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-1-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
+			initialContents:  newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
-			expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-1-volumehandle", deletionPolicy, nil, nil, false, &timeNowMetav1),
+			expectedContents: newContentArrayWithDeletionTimestamp("content1-1", "snapuid1-1", "snap1-1", "sid1-1", "invalid", "", "snap1-4-volumehandle", deletionPolicy, nil, nil, true, &timeNowMetav1),
 			expectedEvents:   noevents,
 			errors:           noerrors,
 			test:             testSyncContent,
 		},
 		{
-			name:                "1-5 - csi driver delete snapshot returns error",
+			name:                "1-5 - csi driver delete snapshot returns error, bound finalizer should remain",
-			initialContents:     newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1),
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1),
-			expectedContents:    newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "", deletionPolicy, nil, &defaultSize, false, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1),
 			expectedListCalls:   []listCall{{"sid1-5", true, time.Now(), 1000, nil}},
 			expectedDeleteCalls: []deleteCall{{"sid1-5", nil, errors.New("mock csi driver delete error")}},
 			expectedEvents:      []string{"Warning SnapshotDeleteError"},
 			errors:              noerrors,
 			test:                testSyncContent,
 		},
-		/*{
-			name:            "1-6 - api server delete content returns error",
-			initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "", "", deletionPolicy, nil, nil, true),
-			//expectedContents:    newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "", "", deletionPolicy, nil, nil, true),
-			expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}},
-			expectedListCalls:   []listCall{{"sid1-6", false, time.Now(), 0, nil}},
-			expectedEvents:      []string{"Warning SnapshotContentObjectDeleteError"},
-			errors: []reactorError{
-				// Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshotContents().Delete call.
-				// All other calls will succeed.
-				{"delete", "volumesnapshotcontents", errors.New("mock delete error")},
-			},
-			test: testSyncContent,
-		},*/
-		/*
-			{
-				// delete success - snapshot that the content was pointing to was deleted, and another
-				// with the same name created.
-				name:                "1-7 - prebound content is deleted while the snapshot exists",
-				initialContents:     newContentArray("content1-7", "sid1-7", "snap1-7", "sid1-7", emptySecretClass, "", "", deletionPolicy, nil, nil, true),
-				expectedContents:    nocontents,
-				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
+			// delete success(?) - content is deleted before deleteSnapshot starts
-			name:                "1-8 - content is deleted before deleting",
+			name:                "1-6 - content is deleted before deleting",
-			initialContents:     newContentArray("content1-8", "sid1-8", "snap1-8", "sid1-8", classGold, "", "", deletionPolicy, nil, nil, true),
+			initialContents:     newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "sid1-6", "", deletionPolicy, nil, nil, true),
 			expectedContents:    nocontents,
-			expectedListCalls:   []listCall{{"sid1-8", false, time.Now(), 0, nil}},
+			expectedListCalls:   []listCall{{"sid1-6", false, time.Now(), 0, nil}},
-			expectedDeleteCalls: []deleteCall{{"sid1-8", map[string]string{"foo": "bar"}, nil}},
+			expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}},
 			expectedEvents:      noevents,
 			errors:              noerrors,
 			test: wrapTestWithInjectedOperation(testSyncContent, func(ctrl *csiSnapshotSideCarController, reactor *snapshotReactor) {
-				// Delete the volume before delete operation starts
+				// Delete the content before delete operation starts
 				reactor.lock.Lock()
-				delete(reactor.contents, "content1-8")
+				delete(reactor.contents, "content1-6")
 				reactor.lock.Unlock()
 			}),
 		},
-		/*{
-			name:              "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
-			expectedContents:  newContentArrayWithDeletionTimestamp("content1-9", "snapuid1-9", "snap1-9", "sid1-9", classGold, "", "snap1-9-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1),
-			initialContents:   newContentArrayWithDeletionTimestamp("content1-9", "snapuid1-9", "snap1-9", "sid1-9", classGold, "", "snap1-9-volumehandle", retainPolicy, nil, nil, false, &timeNowMetav1),
-			expectedEvents:    noevents,
-			expectedListCalls: []listCall{{"sid1-9", true, time.Now(), 0, nil}},
-			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),
-				expectedSnapshots: newSnapshotArray("snap1-10", "snapuid1-10", "claim1-10", "", validSecretClass, "content1-10", &False, nil, nil, 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",
+			name:              "1-7 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is not specified",
-			initialContents:  newContentArrayWithReadyToUse("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, &defaultSize, &True, true),
+			initialContents:   newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true),
-			expectedContents: newContentArrayWithReadyToUse("content1-11", "", "snap1-11", "sid1-11", validSecretClass, "", "", deletePolicy, nil, &defaultSize, &True, true),
+			expectedContents:  newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true),
-			expectedEvents:   noevents,
+			expectedEvents:    noevents,
-			expectedCreateCalls: []createCall{
+			expectedListCalls: []listCall{{"sid1-7", true, time.Now(), 1000, nil}},
-				{
-					snapshotName: "snap1-11",
-					volumeHandle: "snap1-11",
-					parameters:   map[string]string{"param1": "value1"},
-					driverName:   mockDriverName,
-					size:         defaultSize,
-					snapshotId:   "snapuid1-1-deleted",
-					creationTime: timeNow,
-					readyToUse:   true,
-				},
-			},
-			expectedListCalls: []listCall{{"sid1-11", true, time.Now(), 1000, nil}},
 			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",
+			name:              "1-8 - 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:   newContentArrayWithReadyToUse("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, &defaultSize, &True, true),
+			initialContents:   newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true),
-			expectedContents:  newContentArrayWithReadyToUse("content1-12", "sid1-12", "snap1-11", "sid1-11", validSecretClass, "", "", retainPolicy, nil, &defaultSize, &True, true),
+			expectedContents:  newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true),
-			expectedEvents:    noevents,
-			expectedListCalls: []listCall{{"sid1-11", true, time.Now(), 0, nil}},
-			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),
 			expectedEvents:    noevents,
+			expectedListCalls: []listCall{{"sid1-8", true, time.Now(), 0, nil}},
 			errors:            noerrors,
 			test:              testSyncContent,
 		},
 		{
-			name:              "1-14 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
+			name:                "1-9 - continue deletion with snapshot class that has nonexistent secret, bound finalizer removed",
-			initialContents:   newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true),
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
-			expectedContents:  newContentArray("content1-14", "snapuid1-14", "snap1-14", "sid1-14", validSecretClass, "", "", retainPolicy, nil, nil, true),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
-			expectedEvents:    noevents,
-			initialSecrets:    []*v1.Secret{secret()},
-			errors:            noerrors,
-			test:              testSyncContent,
-		},*/
-		{
-			name:                "1-16 - continue delete with snapshot class that has nonexistent secret",
-			initialContents:     newContentArrayWithDeletionTimestamp("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
-			expectedContents:    newContentArrayWithDeletionTimestamp("content1-16", "sid1-16", "snap1-16", "sid1-16", emptySecretClass, "", "", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
 			expectedEvents:      noevents,
-			expectedListCalls:   []listCall{{"sid1-16", true, time.Now(), 0, nil}},
+			expectedListCalls:   []listCall{{"sid1-9", true, time.Now(), 0, nil}},
 			errors:              noerrors,
 			initialSecrets:      []*v1.Secret{}, // secret does not exist
-			expectedDeleteCalls: []deleteCall{{"sid1-16", nil, nil}},
+			expectedDeleteCalls: []deleteCall{{"sid1-9", nil, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:              "1-10 - (dynamic)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
+			initialContents:   newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:  newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
+			expectedEvents:    noevents,
+			expectedListCalls: []listCall{{"sid1-10", true, time.Now(), 0, nil}},
+			errors:            noerrors,
+			initialSecrets:    []*v1.Secret{},
+			test:              testSyncContent,
+		},
+		{
+			name:                "1-11 - (dynamic)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "sid1-11", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-11", "sid1-11", "snap1-11", "", emptySecretClass, "", "snap1-11-volumehandle", deletePolicy, nil, nil, false, &timeNowMetav1),
+			expectedEvents:      noevents,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-11", nil, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:              "1-12 - (pre-provision)deletion of content with retain policy should not trigger CSI call, not update status, but remove bound finalizer",
+			initialContents:   newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:  newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
+			expectedEvents:    noevents,
+			expectedListCalls: []listCall{{"sid1-12", true, time.Now(), 0, nil}},
+			errors:            noerrors,
+			initialSecrets:    []*v1.Secret{},
+			test:              testSyncContent,
+		},
+		{
+			name:                "1-13 - (pre-provision)deletion of content with deletion policy should trigger CSI call, update status, and remove bound finalizer removed.",
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "sid1-13", emptySecretClass, "sid1-13", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-13", "sid1-13", "snap1-13", "", emptySecretClass, "sid1-13", "", deletePolicy, nil, nil, false, &timeNowMetav1),
+			expectedEvents:      noevents,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-13", nil, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:                "1-14 - (pre-provision)deletion of content with deletion policy and no snapshotclass should trigger CSI call, update status, and remove bound finalizer removed.",
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "sid1-14", "", "sid1-14", "", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-14", "sid1-14", "snap1-14", "", "", "sid1-14", "", deletePolicy, nil, nil, false, &timeNowMetav1),
+			expectedEvents:      noevents,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-14", nil, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:                "1-15 - (dynamic)deletion of content with no snapshotclass should produce error",
+			initialContents:     newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedContents:    newContentArrayWithDeletionTimestamp("content1-15", "sid1-15", "snap1-15", "sid1-15", "", "", "snap1-15-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
+			expectedEvents:      []string{"Warning SnapshotDeleteError"},
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
 			test:                testSyncContent,
 		},
 	}

pkg/utils/util.go

@@ -20,8 +20,12 @@ import (
 	"fmt"
 	"strings"
 
+	"os"
+	"strconv"
+	"time"
+
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1beta1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -30,9 +34,6 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/util/slice"
-	"os"
-	"strconv"
-	"time"
 )
 
 var (
@@ -318,13 +319,6 @@ func NoResyncPeriodFunc() time.Duration {
 	return 0
 }
 
-// isContentDeletionCandidate checks if a volume snapshot content is a deletion candidate.
-// It is a deletion candidate if DeletionTimestamp is not nil and
-// VolumeSnapshotContentFinalizer is set.
-func IsContentDeletionCandidate(content *crdv1.VolumeSnapshotContent) bool {
-	return content.ObjectMeta.DeletionTimestamp != nil && slice.ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil)
-}
-
 // needToAddContentFinalizer checks if a Finalizer needs to be added for the volume snapshot content.
 func NeedToAddContentFinalizer(content *crdv1.VolumeSnapshotContent) bool {
 	return content.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil)