Merge pull request #972 from RaunakShah/deletegroupsnapshot-part-4

Add finalizer to prevent deletion of individual volume snapshots

pkg/common-controller/framework_test.go

@@ -1271,7 +1271,7 @@ func testAddSingleSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *
 }
 
 func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
-	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true)
+	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true, false)
 }
 
 func testUpdateSnapshotClass(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {

pkg/common-controller/groupsnapshot_controller_helper.go

@@ -1114,6 +1114,9 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 	for _, snapshotRef := range groupSnapshot.Status.VolumeSnapshotRefList {
 		snapshot, err := ctrl.snapshotLister.VolumeSnapshots(snapshotRef.Namespace).Get(snapshotRef.Name)
 		if err != nil {
+			if apierrs.IsNotFound(err) {
+				continue
+			}
 			return err
 		}
 		if ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) {

pkg/common-controller/snapshot_controller.go

@@ -286,21 +286,6 @@ func (ctrl *csiSnapshotCommonController) processSnapshotWithDeletionTimestamp(sn
 		content = nil
 	}
 
-	// Block deletion if this snapshot belongs to a group snapshot.
-	if snapshot.Status != nil && snapshot.Status.VolumeGroupSnapshotName != nil {
-		groupSnapshot, err := ctrl.groupSnapshotLister.VolumeGroupSnapshots(snapshot.Namespace).Get(*snapshot.Status.VolumeGroupSnapshotName)
-		if err == nil {
-			msg := fmt.Sprintf("deletion of the individual volume snapshot %s is not allowed as it belongs to group snapshot %s. Deleting the group snapshot will trigger the deletion of all the individual volume snapshots that are part of the group.", utils.SnapshotKey(snapshot), utils.GroupSnapshotKey(groupSnapshot))
-			klog.Error(msg)
-			ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeleteError", msg)
-			return fmt.Errorf(msg)
-		}
-		if !apierrs.IsNotFound(err) {
-			klog.Errorf("failed to delete snapshot %s: %v", utils.SnapshotKey(snapshot), err)
-			return err
-		}
-	}
-
 	klog.V(5).Infof("processSnapshotWithDeletionTimestamp[%s]: delete snapshot content and remove finalizer from snapshot if needed", utils.SnapshotKey(snapshot))
 
 	return ctrl.checkandRemoveSnapshotFinalizersAndCheckandDeleteContent(snapshot, content, deleteContent)
@@ -323,6 +308,26 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 		return nil
 	}
 
+	removeGroupFinalizer := false
+	// Block deletion if this snapshot belongs to a group snapshot.
+	if snapshot.Status != nil && snapshot.Status.VolumeGroupSnapshotName != nil {
+		groupSnapshot, err := ctrl.groupSnapshotLister.VolumeGroupSnapshots(snapshot.Namespace).Get(*snapshot.Status.VolumeGroupSnapshotName)
+		if err == nil {
+			msg := fmt.Sprintf("deletion of the individual volume snapshot %s is not allowed as it belongs to group snapshot %s. Deleting the group snapshot will trigger the deletion of all the individual volume snapshots that are part of the group.", utils.SnapshotKey(snapshot), utils.GroupSnapshotKey(groupSnapshot))
+			klog.Error(msg)
+			ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeletePending", msg)
+			return fmt.Errorf(msg)
+		}
+		if !apierrs.IsNotFound(err) {
+			klog.Errorf("failed to delete snapshot %s: %v", utils.SnapshotKey(snapshot), err)
+			return err
+		}
+		// group snapshot API object was deleted.
+		// The VolumeSnapshotInGroupFinalizer can be removed from this snapshot
+		// to trigger deletion.
+		removeGroupFinalizer = true
+	}
+
 	// regardless of the deletion policy, set the VolumeSnapshotBeingDeleted on
 	// content object, this is to allow snapshotter sidecar controller to conduct
 	// a delete operation whenever the content has deletion timestamp set.
@@ -349,7 +354,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 	}
 
 	klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Remove Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot))
-	// remove finalizers on the VolumeSnapshot object, there are two finalizers:
+	// remove finalizers on the VolumeSnapshot object, there are three finalizers:
 	// 1. VolumeSnapshotAsSourceFinalizer, once reached here, the snapshot is not
 	//    in use to restore PVC, and the finalizer will be removed directly.
 	// 2. VolumeSnapshotBoundFinalizer:
@@ -359,8 +364,10 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec
 	//       by snapshot sidecar controller.
 	//    c. If deletion will not cascade to the content, remove the finalizer on
 	//       the snapshot such that it can be removed from API server.
+	// 3. VolumeSnapshotInGroupFinalizer, if the snapshot was part of a group snapshot,
+	//    then the group snapshot has been deleted, so remove the finalizer.
 	removeBoundFinalizer := !(content != nil && deleteContent)
-	return ctrl.removeSnapshotFinalizer(snapshot, true, removeBoundFinalizer)
+	return ctrl.removeSnapshotFinalizer(snapshot, true, removeBoundFinalizer, removeGroupFinalizer)
 }
 
 // checkandAddSnapshotFinalizers checks and adds snapshot finailzers when needed
@@ -1542,8 +1549,8 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 }
 
 // removeSnapshotFinalizer removes a Finalizer for VolumeSnapshot.
-func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, removeSourceFinalizer bool, removeBoundFinalizer bool) error {
+func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, removeSourceFinalizer bool, removeBoundFinalizer bool, removeGroupFinalizer bool) error {
-	if !removeSourceFinalizer && !removeBoundFinalizer {
+	if !removeSourceFinalizer && !removeBoundFinalizer && !removeGroupFinalizer {
 		return nil
 	}
 
@@ -1571,6 +1578,9 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1
 	if removeBoundFinalizer {
 		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
 	}
+	if removeGroupFinalizer {
+		snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotInGroupFinalizer)
+	}
 	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
 	if err != nil {
 		return newControllerUpdateError(snapshot.Name, err.Error())

pkg/sidecar-controller/groupsnapshot_helper.go

@@ -461,9 +461,10 @@ func (ctrl *csiSnapshotSideCarController) createGroupSnapshotWrapper(groupSnapsh
 		label["volumeGroupSnapshotName"] = groupSnapshotContent.Spec.VolumeGroupSnapshotRef.Name
 		volumeSnapshot := &crdv1.VolumeSnapshot{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      volumeSnapshotName,
+				Name:       volumeSnapshotName,
-				Namespace: volumeSnapshotNamespace,
+				Namespace:  volumeSnapshotNamespace,
-				Labels:    label,
+				Labels:     label,
+				Finalizers: []string{utils.VolumeSnapshotInGroupFinalizer},
 			},
 			Spec: crdv1.VolumeSnapshotSpec{
 				Source: crdv1.VolumeSnapshotSource{

pkg/utils/util.go

@@ -74,6 +74,8 @@ const (
 	VolumeSnapshotBoundFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-bound-protection"
 	// Name of finalizer on VolumeSnapshot that is used as a source to create a PVC
 	VolumeSnapshotAsSourceFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection"
+	// Name of finalizer on VolumeSnapshot that is a part of a VolumeGroupSnapshot
+	VolumeSnapshotInGroupFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-in-group-protection"
 	// Name of finalizer on PVCs that is being used as a source to create VolumeSnapshots
 	PVCFinalizer = "snapshot.storage.kubernetes.io/pvc-as-source-protection"
 	// Name of finalizer on VolumeGroupSnapshotContents that are bound by VolumeGroupSnapshots