formatting and logging changes

Author: RaunakShah

pkg/common-controller/groupsnapshot_controller_helper.go

@@ -289,14 +289,6 @@ func (ctrl *csiSnapshotCommonController) syncGroupSnapshot(groupSnapshot *crdv1a
 
 	klog.V(5).Infof("syncGroupSnapshot [%s]: check if we should remove finalizer on group snapshot PVC source and remove it if we can", utils.GroupSnapshotKey(groupSnapshot))
 
-	/*
-		TODO:
-		- Check and remove finalizer if needed.
-		- Check and set invalid group snapshot label, if needed.
-		- Process if deletion timestamp is set.
-		- Check and add group snapshot finalizers.
-	*/
-	// START
 	// Proceed with snapshot deletion and remove finalizers when needed
 	if groupSnapshot.ObjectMeta.DeletionTimestamp != nil {
 		return ctrl.processGroupSnapshotWithDeletionTimestamp(groupSnapshot)
@@ -311,15 +303,13 @@ func (ctrl *csiSnapshotCommonController) syncGroupSnapshot(groupSnapshot *crdv1a
 		return err
 	}
 
-	klog.V(5).Infof("syncGroupSnapshot[%s]: check if we should add finalizers on group snapshot", utils.GroupSnapshotKey(groupSnapshot))
+	klog.V(5).Infof("syncGroupSnapshot: check if we should add finalizers on group snapshot [%s]", utils.GroupSnapshotKey(groupSnapshot))
 	if err := ctrl.checkandAddGroupSnapshotFinalizers(groupSnapshot); err != nil {
-		klog.Errorf("error check and add GroupSnapshot finalizers for group snapshot [%s]: %v", groupSnapshot.Name, err)
+		klog.Errorf("error ccheckandAddGroupSnapshotFinalizers for group snapshot [%s]: %v", utils.GroupSnapshotKey(groupSnapshot), err)
 		ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "GroupSnapshotFinalizerError", fmt.Sprintf("Failed to check and update group snapshot: %s", err.Error()))
 		return err
 	}
 
-	// END
-
 	// Need to build or update groupSnapshot.Status in following cases:
 	// 1) groupSnapshot.Status is nil
 	// 2) groupSnapshot.Status.ReadyToUse is false
@@ -750,7 +740,7 @@ func (ctrl *csiSnapshotCommonController) createGroupSnapshotContent(groupSnapsho
 	klog.Infof("createSnapshotContent: Creating group snapshot content for group snapshot %s through the plugin ...", utils.GroupSnapshotKey(groupSnapshot))
 
 	/*
-		TODO: Add finalizer to group snapshot
+		TODO: Add PVC finalizer
 	*/
 
 	groupSnapshotClass, volumes, contentName, err := ctrl.getCreateGroupSnapshotInput(groupSnapshot)
@@ -872,12 +862,8 @@ func (ctrl *csiSnapshotCommonController) syncGroupSnapshotContent(groupSnapshotC
 		return nil
 	}
 
-	/*
-		TODO: Add finalizer to prevent deletion
-	*/
-
 	if utils.NeedToAddGroupSnapshotContentFinalizer(groupSnapshotContent) {
-		// Content is not being deleted -> it should have the finalizer.
+		// Group Snapshot Content is not being deleted -> it should have the finalizer.
 		klog.V(5).Infof("syncGroupSnapshotContent [%s]: Add Finalizer for VolumeGroupSnapshotContent", groupSnapshotContent.Name)
 		return ctrl.addGroupSnapshotContentFinalizer(groupSnapshotContent)
 	}
@@ -999,7 +985,7 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotContentFinalizer(groupS
 
 // checkandAddSnapshotFinalizers checks and adds snapshot finailzers when needed
 func (ctrl *csiSnapshotCommonController) checkandAddGroupSnapshotFinalizers(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) error {
-	// get the content for this Snapshot
+	// get the group snapshot content for this group snapshot
 	var (
 		groupSnapshotContent *crdv1alpha1.VolumeGroupSnapshotContent
 		err                  error
@@ -1013,29 +999,21 @@ func (ctrl *csiSnapshotCommonController) checkandAddGroupSnapshotFinalizers(grou
 		return err
 	}
 
-	/*
-		TODO: Do we need to add group snapshot as source finalizer?
-	*/
-
-	// note that content could be nil, in this case bound finalizer is not needed
-	addBoundFinalizer := false
-	if groupSnapshotContent != nil {
-		// A bound finalizer is needed ONLY when all following conditions are satisfied:
-		// 1. the VolumeSnapshot is bound to a content
-		// 2. the VolumeSnapshot does not have deletion timestamp set
-		// 3. the matching content has a deletion policy to be Delete
-		// Note that if a matching content is found, it must points back to the snapshot
-		addBoundFinalizer = utils.NeedToAddGroupSnapshotBoundFinalizer(groupSnapshot) && (groupSnapshotContent.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete)
-	}
-	if addBoundFinalizer {
+	// A bound finalizer is needed ONLY when all following conditions are satisfied:
+	// 1. the VolumeGroupSnapshot is bound to a VolumeGroupSnapshotContent
+	// 2. the VolumeGroupSnapshot does not have deletion timestamp set
+	// 3. the matching VolumeGroupSnapshotContent has a deletion policy to be Delete
+	// Note that if a matching VolumeGroupSnapshotContent is found, it must point back to the VolumeGroupSnapshot
+	if groupSnapshotContent != nil && utils.NeedToAddGroupSnapshotBoundFinalizer(groupSnapshot) && (groupSnapshotContent.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete) {
 		// Snapshot is not being deleted -> it should have the finalizer.
 		klog.V(5).Infof("checkandAddGroupSnapshotFinalizers: Add Finalizer for VolumeGroupSnapshot[%s]", utils.GroupSnapshotKey(groupSnapshot))
-		return ctrl.addGroupSnapshotFinalizer(groupSnapshot, addBoundFinalizer)
+		return ctrl.addGroupSnapshotFinalizer(groupSnapshot, true)
+
 	}
 	return nil
 }
 
-// addGroupSnapshotFinalizer adds a Finalizer for VolumeGroupSnapshot.
+// addGroupSnapshotFinalizer adds a Finalizer to a VolumeGroupSnapshot.
 func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot, addBoundFinalizer bool) error {
 	var updatedGroupSnapshot *crdv1alpha1.VolumeGroupSnapshot
 	var err error
@@ -1080,7 +1058,7 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot
 
 // processGroupSnapshotWithDeletionTimestamp processes finalizers and deletes the
 // group snapshot content when appropriate. It has the following steps:
-// 1. Get the group snapshot content which the to-be-deleted VolumeGroupSnapshot
+// 1. Get the VolumeGroupSnapshotContent which the to-be-deleted VolumeGroupSnapshot
 // points to and verifies bi-directional binding.
 // 2. Call checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent()
 // with information obtained from step 1. This function name is very long but the
@@ -1101,7 +1079,7 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 	if groupSnapshotContentName == "" && &groupSnapshot.Spec.Source.VolumeGroupSnapshotContentName == nil {
 		groupSnapshotContentName = utils.GetDynamicSnapshotContentNameForGroupSnapshot(groupSnapshot)
 	}
-	// find a group snapshot content from cache store, note that it's complete legit
+	// find a group snapshot content from cache store, note that it's completely legit
 	// that no group snapshot content has been found from group snapshot content
 	// cache store
 	groupSnapshotContent, err := ctrl.getGroupSnapshotContentFromStore(groupSnapshotContentName)
@@ -1139,18 +1117,24 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAn
 		return nil
 	}
 
-	/* NEEDED?
-	// check if the group snapshot is being used for restore a PVC, if yes, do nothing
-	// and wait until PVC restoration finishes
-	if content != nil && ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) {
-		klog.V(4).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: snapshot is being used to restore a PVC", utils.SnapshotKey(snapshot))
-		ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeletePending", "Snapshot is being used to restore a PVC")
-		// TODO(@xiangqian): should requeue this?
-		return nil
-	}
+	// TODO: In this needed?
+	// check if an individual snapshot belonging to the group snapshot is being
+	// used for restore a PVC
+	// If yes, do nothing and wait until PVC restoration finishes
+	for _, snapshotRef := range groupSnapshot.Status.VolumeSnapshotRefList {
+		snapshot, err := ctrl.snapshotLister.VolumeSnapshots(snapshotRef.Namespace).Get(snapshotRef.Name)
+		if err != nil {
+			return err
+		}
+		if ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) {
+			klog.V(4).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent[%s]: snapshot [%s] belonging to volume group snapshot is being used to restore a PVC", utils.GroupSnapshotKey(groupSnapshot), utils.SnapshotKey(snapshot))
+			ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeletePending", "Snapshot is being used to restore a PVC")
+			// TODO(@xiangqian): should requeue this?
+			return nil
+		}
 
+	}
 
-	*/
 	// 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.
@@ -1167,7 +1151,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemoveGroupSnapshotFinalizersAn
 	// VolumeGroupSnapshot should be deleted. Check and remove finalizers
 	// If group snapshot content exists and has a deletion policy of Delete, set
 	// DeletionTimeStamp on the content;
-	// group snapshot content won't be deleted immediately due to the VolumeGroupSnapshotContentFinalizer
+	// VolumeGroupSnapshotContent won't be deleted immediately due to the VolumeGroupSnapshotContentFinalizer
 	if groupSnapshotContent != nil && deleteGroupSnapshotContent {
 		klog.V(5).Infof("checkandRemoveGroupSnapshotFinalizersAndCheckandDeleteGroupSnapshotContent: set DeletionTimeStamp on group snapshot content [%s].", groupSnapshotContent.Name)
 		err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Delete(context.TODO(), groupSnapshotContent.Name, metav1.DeleteOptions{})
@@ -1228,26 +1212,9 @@ func (ctrl *csiSnapshotCommonController) removeGroupSnapshotFinalizer(groupSnaps
 	if !removeBoundFinalizer {
 		return nil
 	}
-	/* NEEDED?
-	// NOTE(xyang): We have to make sure PVC finalizer is deleted before
-	// the VolumeSnapshot API object is deleted. Once the VolumeSnapshot
-	// API object is deleted, there won't be any VolumeSnapshot update
-	// event that can trigger the PVC finalizer removal any more.
-	// We also can't remove PVC finalizer too early. PVC finalizer should
-	// not be removed if a VolumeSnapshot API object is still using it.
-	// If we are here, it means we are going to remove finalizers from the
-	// VolumeSnapshot API object so that the VolumeSnapshot API object can
-	// be deleted. This means we no longer need to keep the PVC finalizer
-	// for this particular snapshot.
-	if err := ctrl.checkandRemovePVCFinalizer(groupSnapshot, true); err != nil {
-		klog.Errorf("removeSnapshotFinalizer: error check and remove PVC finalizer for snapshot [%s]: %v", groupSnapshot.Name, err)
-		// Log an event and keep the original error from checkandRemovePVCFinalizer
-		ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "ErrorPVCFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot")
-		return newControllerUpdateError(groupSnapshot.Name, err.Error())
-	}
 
+	// TODO: Remove PVC Finalizer
 
-	*/
 	groupSnapshotClone := groupSnapshot.DeepCopy()
 	groupSnapshotClone.ObjectMeta.Finalizers = utils.RemoveString(groupSnapshotClone.ObjectMeta.Finalizers, utils.VolumeGroupSnapshotBoundFinalizer)
 	newGroupSnapshot, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(groupSnapshotClone.Namespace).Update(context.TODO(), groupSnapshotClone, metav1.UpdateOptions{})

pkg/common-controller/snapshot_controller.go

@@ -971,7 +971,7 @@ func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolume
 	return false
 }
 
-// PVCFinalizer checks if the snapshot source finalizer should be removed
+// checkandRemovePVCFinalizer checks if the snapshot source finalizer should be removed
 // and removed it if needed. If skipCurrentSnapshot is true, skip checking if the current
 // snapshot is using the PVC as source.
 func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *crdv1.VolumeSnapshot, skipCurrentSnapshot bool) error {