Merge pull request #1231 from Madhu-1/fix-1230

Dont call DeleteSnapshot when VS belong to VGS

pkg/sidecar-controller/framework_test.go

@@ -667,6 +667,15 @@ func newContentArrayWithReadyToUse(contentName, boundToSnapshotUID, boundToSnaps
 	}
 }
 
+func newContentWithVolumeGroupSnapshotHandle(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, volumeGroupSnapshotHandle, snapshotClassName, desiredSnapshotHandle,
+	volumeHandle string, deletionPolicy crdv1.DeletionPolicy, creationTime, size *int64, withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent {
+	content := newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, creationTime, size, withFinalizer, deletionTime)
+	for _, c := range content {
+		c.Status.VolumeGroupSnapshotHandle = &volumeGroupSnapshotHandle
+	}
+	return content
+}
+
 func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string,
 	deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64,
 	withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent {

pkg/sidecar-controller/snapshot_controller.go

@@ -61,10 +61,11 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 	if ctrl.shouldDelete(content) {
 		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
+			content.Status != nil && content.Status.SnapshotHandle != nil && content.Status.VolumeGroupSnapshotHandle == nil {
+			// issue a CSI deletion call if the snapshot does not belong to volumegroupsnapshot
+			// and it 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.
 			err := ctrl.deleteCSISnapshot(content)
 			if err != nil {
@@ -73,9 +74,9 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 			return false, nil
 		}
 		// 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.
+		// storage system, or it belongs to a volumegroupsnapshot, 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.
 		err := ctrl.removeContentFinalizer(content)
 		if err != nil {
 			return true, err

pkg/sidecar-controller/snapshot_delete_test.go

@@ -354,6 +354,25 @@ func TestDeleteSync(t *testing.T) {
 			expectedDeleteCalls: []deleteCall{{"sid1-15", nil, nil}},
 			test:                testSyncContent,
 		},
+
+		{
+			name:                "1-16 - (dynamic)deletion of content with no VolumeGroupSnapshotHandle should succeed",
+			initialContents:     newContentWithVolumeGroupSnapshotHandle("content1-16", "sid1-16", "snap1-16", "snap1-16", "grp-snap1-16", "", "", "snap1-16-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
+			expectedContents:    newContentWithVolumeGroupSnapshotHandle("content1-16", "sid1-16", "snap1-16", "snap1-16", "grp-snap1-16", "", "", "snap1-16-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
+			expectSuccess:       true,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-16", nil, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:                "1-17 - (dynamic)deletion of content with VolumeGroupSnapshotHandle should succeed with no call to CSI driver",
+			initialContents:     newContentWithVolumeGroupSnapshotHandle("content1-17", "sid1-17", "snap1-17", "snap1-17", "grp-snap1-17", "", "", "snap1-17-volumehandle", deletePolicy, nil, &defaultSize, true, &nonFractionalTime),
+			expectedContents:    newContentWithVolumeGroupSnapshotHandle("content1-17", "sid1-17", "snap1-17", "snap1-17", "grp-snap1-17", "", "", "snap1-17-volumehandle", deletePolicy, nil, &defaultSize, false, &nonFractionalTime),
+			expectSuccess:       true,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{},
+			test:                testSyncContent,
+		},
 	}
 	runSyncContentTests(t, tests, snapshotClasses)
 }