Merge pull request #283 from xing-yang/check_pvc_inuse

Check if PVC is being used by a snapshot as source

pkg/common-controller/snapshot_controller.go

@@ -166,7 +166,11 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 		ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot")
 	}
 
-	if snapshot.ObjectMeta.DeletionTimestamp != nil {
+	// Proceed with snapshot deletion only if snapshot is not in the middled of being
+	// created from a PVC with a finalizer. This is to ensure that the PVC finalizer
+	// can be removed even if a delete snapshot request is received before create
+	// snapshot has completed.
+	if snapshot.ObjectMeta.DeletionTimestamp != nil && !ctrl.isPVCwithFinalizerInUseByCurrentSnapshot(snapshot) {
 		err := ctrl.processSnapshotWithDeletionTimestamp(snapshot)
 		if err != nil {
 			return err
@@ -190,6 +194,27 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 	return nil
 }
 
+// Check if PVC has a finalizer and if it is being used by the current snapshot as source.
+func (ctrl *csiSnapshotCommonController) isPVCwithFinalizerInUseByCurrentSnapshot(snapshot *crdv1.VolumeSnapshot) bool {
+	// Get snapshot source which is a PVC
+	pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot)
+	if err != nil {
+		klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already.", snapshot.Name, err)
+		return false
+	}
+
+	// Check if there is a Finalizer on PVC. If not, return false
+	if !slice.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
+		return false
+	}
+
+	if !utils.IsSnapshotReady(snapshot) {
+		klog.V(2).Infof("PVC %s/%s is being used by snapshot %s/%s as source", pvc.Namespace, pvc.Name, snapshot.Namespace, snapshot.Name)
+		return true
+	}
+	return false
+}
+
 // checkContentAndBoundStatus is a helper function that checks the following:
 // - It checks if content exists and returns the content object if it exists and nil otherwise.
 // - It checks the deletionPolicy and returns true if policy is Delete and false otherwise.

pkg/common-controller/snapshot_delete_test.go

@@ -172,8 +172,8 @@ func TestDeleteSync(t *testing.T) {
 			name:             "3-1 - content will be deleted if snapshot deletion timestamp is set",
 			initialContents:  newContentArray("content3-1", "", "snap3-1", "sid3-1", validSecretClass, "", "", deletePolicy, nil, nil, true),
 			expectedContents: nocontents,
-			initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, true, &timeNowMetav1),
-			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &False, nil, nil, nil, false, false, &timeNowMetav1),
+			initialSnapshots: newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, true, &timeNowMetav1),
+			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-1", "snapuid3-1", "claim3-1", "", validSecretClass, "content3-1", &True, nil, nil, nil, false, false, &timeNowMetav1),
 				utils.VolumeSnapshotBoundFinalizer,
 			),
 			initialClaims:  newClaimArray("claim3-1", "pvc-uid3-1", "1Gi", "volume3-1", v1.ClaimBound, &classEmpty),
@@ -186,8 +186,8 @@ func TestDeleteSync(t *testing.T) {
 			name:              "3-2 - content will not be deleted if deletion API call fails",
 			initialContents:   newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true),
 			expectedContents:  newContentArray("content3-2", "", "snap3-2", "sid3-2", validSecretClass, "", "", deletePolicy, nil, nil, true),
-			initialSnapshots:  newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1),
-			expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &False, nil, nil, nil, false, true, &timeNowMetav1),
+			initialSnapshots:  newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1),
+			expectedSnapshots: newSnapshotArray("snap3-2", "snapuid3-2", "claim3-2", "", validSecretClass, "content3-2", &True, nil, nil, nil, false, true, &timeNowMetav1),
 			initialClaims:     newClaimArray("claim3-2", "pvc-uid3-2", "1Gi", "volume3-2", v1.ClaimBound, &classEmpty),
 			expectedEvents:    []string{"Warning SnapshotContentObjectDeleteError"},
 			initialSecrets:    []*v1.Secret{secret()},
@@ -206,8 +206,8 @@ func TestDeleteSync(t *testing.T) {
 				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),
+			initialSnapshots: newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, true, &timeNowMetav1),
+			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap3-3", "snapuid3-3", "claim3-3", "", validSecretClass, "content3-3", &True, nil, nil, nil, false, false, &timeNowMetav1),
 				utils.VolumeSnapshotBoundFinalizer,
 			),
 			initialClaims:  newClaimArray("claim3-3", "pvc-uid3-3", "1Gi", "volume3-3", v1.ClaimBound, &classEmpty),

pkg/common-controller/snapshot_update_test.go

@@ -315,8 +315,8 @@ func TestSync(t *testing.T) {
 		},
 		{
 			name:             "5-5 - snapshot deletion candidate marked for deletion by syncSnapshot",
-			initialSnapshots: newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &False, nil, nil, nil, false, true, &timeNowMetav1),
-			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &False, nil, nil, nil, false, false, &timeNowMetav1),
+			initialSnapshots: newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &True, nil, nil, nil, false, true, &timeNowMetav1),
+			expectedSnapshots: withSnapshotFinalizers(newSnapshotArray("snap5-5", "snapuid5-5", "claim5-5", "", validSecretClass, "content5-5", &True, nil, nil, nil, false, false, &timeNowMetav1),
 				utils.VolumeSnapshotBoundFinalizer,
 			),
 			initialContents:  newContentArray("content5-5", "snapuid5-5", "snap5-5", "sid5-5", validSecretClass, "", "", crdv1.VolumeSnapshotContentRetain, nil, nil, true),