From 875662b365e60621299e0ffb5cd4cab37039fb29 Mon Sep 17 00:00:00 2001 From: Raunak Pradip Shah Date: Wed, 6 Dec 2023 16:16:30 -0800 Subject: [PATCH 1/2] Add finalizer to indicate volume snapshot is part of a group --- .../groupsnapshot_controller_helper.go | 3 ++ pkg/common-controller/snapshot_controller.go | 41 +++++++++++-------- .../groupsnapshot_helper.go | 7 ++-- pkg/utils/util.go | 2 + 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/pkg/common-controller/groupsnapshot_controller_helper.go b/pkg/common-controller/groupsnapshot_controller_helper.go index 543638eb3..92f377dc5 100644 --- a/pkg/common-controller/groupsnapshot_controller_helper.go +++ b/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) { diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index d509fb296..be7d96bb8 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/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,23 @@ 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 { + removeGroupFinalizer = true + 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 + } + } + // 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. @@ -360,7 +362,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec // c. If deletion will not cascade to the content, remove the finalizer on // the snapshot such that it can be removed from API server. 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 +1544,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 { - if !removeSourceFinalizer && !removeBoundFinalizer { +func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, removeSourceFinalizer bool, removeBoundFinalizer bool, removeGroupFinalizer bool) error { + if !removeSourceFinalizer && !removeBoundFinalizer && !removeGroupFinalizer { return nil } @@ -1571,6 +1573,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()) diff --git a/pkg/sidecar-controller/groupsnapshot_helper.go b/pkg/sidecar-controller/groupsnapshot_helper.go index f4a0cc4e8..286eb96af 100644 --- a/pkg/sidecar-controller/groupsnapshot_helper.go +++ b/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, - Namespace: volumeSnapshotNamespace, - Labels: label, + Name: volumeSnapshotName, + Namespace: volumeSnapshotNamespace, + Labels: label, + Finalizers: []string{utils.VolumeSnapshotInGroupFinalizer}, }, Spec: crdv1.VolumeSnapshotSpec{ Source: crdv1.VolumeSnapshotSource{ diff --git a/pkg/utils/util.go b/pkg/utils/util.go index 4a3dcd88c..ec6545715 100644 --- a/pkg/utils/util.go +++ b/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 From 47e71db17f13e0258b069c0c7aaad8a376a17f13 Mon Sep 17 00:00:00 2001 From: Raunak Pradip Shah Date: Thu, 7 Dec 2023 09:20:11 -0800 Subject: [PATCH 2/2] add comments and fix unit test --- pkg/common-controller/framework_test.go | 2 +- pkg/common-controller/snapshot_controller.go | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 35ddd2200..9d1b3d00c 100644 --- a/pkg/common-controller/framework_test.go +++ b/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 { diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index be7d96bb8..051f6a2bb 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -311,7 +311,6 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec removeGroupFinalizer := false // Block deletion if this snapshot belongs to a group snapshot. if snapshot.Status != nil && snapshot.Status.VolumeGroupSnapshotName != nil { - removeGroupFinalizer = true 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)) @@ -323,6 +322,10 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveSnapshotFinalizersAndChec 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 @@ -351,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: @@ -361,6 +364,8 @@ 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, removeGroupFinalizer) }