Consider volumes in status also while removing finalizers from PVC

Author: skogta

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go

@@ -293,12 +293,16 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 	if instance.DeletionTimestamp != nil && vm == nil {
 		log.Infof("Instance %s is being deleted and VM object is also deleted from VC", request.NamespacedName.String())
 
-		// For every PVC mentioned in instance.Spec, remove finalizer from its PVC.
-		for _, volume := range instance.Spec.Volumes {
-			err := removePvcFinalizer(ctx, r.client, k8sClient, volume.PersistentVolumeClaim.ClaimName, instance.Namespace,
+		pvcsInSpecAndStatus := getPvcsFromSpecAndStatus(ctx, instance)
+
+		// For every PVC mentioned in instance.Spec and in instance.Status, remove finalizer from it.
+		// It is important to remove the finalizer from PVCs in instance.Status also as it is possible
+		// that someone removes the PVC from spec after trriggering deletion of VM.
+		for pvcName := range pvcsInSpecAndStatus {
+			err := removePvcFinalizer(ctx, r.client, k8sClient, pvcName, instance.Namespace,
 				instance.Spec.InstanceUUID)
 			if err != nil {
-				log.Errorf("failed to remove finalizer from PVC %s. Err: %s", volume.PersistentVolumeClaim.ClaimName,
+				log.Errorf("failed to remove finalizer from PVC %s. Err: %s", pvcName,
 					err)
 				return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, err)
 			}
@@ -307,7 +311,7 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 		patchErr := removeFinalizerFromCRDInstance(batchAttachCtx, instance, r.client)
 		if patchErr != nil {
 			log.Errorf("failed to update CnsNodeVMBatchAttachment %s. Err: +%v", instance.Name, patchErr)
-			return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, err)
+			return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, patchErr)
 		}
 		log.Infof("Successfully removed finalizer %s from instance %s",
 			cnsoperatortypes.CNSFinalizer, request.NamespacedName.String())

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go

@@ -454,6 +454,25 @@ func getVolumeMetadataMaps(ctx context.Context,
 	return
 }
 
+// getAllPvcsAttachedToVM returns PVC name to volumeID map for all the FCDs which are attached to the VM.
+func getAllPvcsAttachedToVM(ctx context.Context,
+	attachedFCDList map[string]FCDBackingDetails) map[string]string {
+	log := logger.GetLogger(ctx)
+	pvcsAttachedToVM := make(map[string]string)
+
+	for volumeID := range attachedFCDList {
+		pvcName, _, found := commonco.ContainerOrchestratorUtility.GetPVCNameFromCSIVolumeID(volumeID)
+		if !found {
+			// Do not fail if volumeID is not found to avoid cases where cache is not refreshed.
+			// Clean up routine in syncer will take care of the cases where finalizer was not removed from such PVCs.
+			log.Warnf("failed to find PVC name for volumeID %s", volumeID)
+			continue
+		}
+		pvcsAttachedToVM[pvcName] = volumeID
+	}
+	return pvcsAttachedToVM
+}
+
 // getPvcsInSpec returns map of PVCs and their volumeIDs.
 func getPvcsInSpec(ctx context.Context, instance *v1alpha1.CnsNodeVMBatchAttachment,
 	k8sClient kubernetes.Interface) (map[string]string, error) {
@@ -480,7 +499,6 @@ func getPvcsInSpec(ctx context.Context, instance *v1alpha1.CnsNodeVMBatchAttachm
 	}
 
 	return pvcsInSpec, nil
-
 }
 
 // listAttachedFcdsForVM returns list of FCDs (present in the K8s cluster)
@@ -602,6 +620,21 @@ func constructBatchAttachRequest(ctx context.Context,
 	return pvcsInSpec, volumeIdsInSpec, batchAttachRequest, nil
 }
 
+// getPvcsFromSpecAndStatus returns all the PVCs in spec as well as in status of the given instance.
+func getPvcsFromSpecAndStatus(ctx context.Context,
+	instance *v1alpha1.CnsNodeVMBatchAttachment) map[string]bool {
+
+	listOfPvcsToRemoveFinalzer := make(map[string]bool, 0)
+
+	for _, volume := range instance.Spec.Volumes {
+		listOfPvcsToRemoveFinalzer[volume.PersistentVolumeClaim.ClaimName] = true
+	}
+	for _, volume := range instance.Status.VolumeStatus {
+		listOfPvcsToRemoveFinalzer[volume.PersistentVolumeClaim.ClaimName] = true
+	}
+	return listOfPvcsToRemoveFinalzer
+}
+
 // isPvcEncrypted returns true if annotation csi.vsphere.encryption-class
 // is present on the PVC.
 func isPvcEncrypted(pvcAnnotations map[string]string) bool {
@@ -645,17 +678,6 @@ func getVolumesToAttachAndDetach(ctx context.Context, instance *v1alpha1.CnsNode
 	pvcsToAttach := make(map[string]string, 0)
 	pvcsToDetach := make(map[string]string, 0)
 
-	if instance.DeletionTimestamp != nil {
-		log.Debugf("Instance %s is being deleted, adding all volumes in spec to volumesToDetach list.", instance.Name)
-		volumesToDetach, err := getPvcsInSpec(ctx, instance, k8sClient)
-		if err != nil {
-			log.Errorf("failed to get volumes to detach from instance spec. Err: %s", err)
-			return pvcsToAttach, volumesToDetach, err
-		}
-		log.Debugf("Volumes to detach list %+v for instance %s", volumesToDetach, instance.Name)
-		return pvcsToAttach, volumesToDetach, nil
-	}
-
 	// Query vCenter to find the list of FCDs which are attached to the VM.
 	attachedFcdList, err := listAttachedFcdsForVM(ctx, vm)
 	if err != nil {
@@ -664,6 +686,14 @@ func getVolumesToAttachAndDetach(ctx context.Context, instance *v1alpha1.CnsNode
 	}
 	log.Infof("List of attached FCDs %+v to VM %s", attachedFcdList, instance.Spec.InstanceUUID)
 
+	if instance.DeletionTimestamp != nil {
+		log.Debugf("Instance %s is being deleted, adding all volumes attached to the VM to volumesToDetach list.",
+			instance.Name)
+		volumesToDetach := getAllPvcsAttachedToVM(ctx, attachedFcdList)
+		log.Debugf("Volumes to detach list %+v for instance %s", volumesToDetach, instance.Name)
+		return pvcsToAttach, volumesToDetach, nil
+	}
+
 	// Get all PVCs and their corresponding volumeID mapping from instance spec.
 	volumeIdsInSpec, volumeNamesInSpec, pvcNameToVolumeIDInSpec, err := getVolumeMetadataMaps(ctx, instance)
 	if err != nil {
@@ -923,7 +953,7 @@ func removePvcFinalizer(ctx context.Context, client client.Client,
 		return nil
 	}
 
-	log.Infof("VM %s was the last attached VM for the PVC %s. Finalizer %s can be safely removed fromt the PVC",
+	log.Infof("VM %s was the last attached VM for the PVC %s. Finalizer %s can be safely removed from the PVC",
 		vmInstanceUUID, pvcName, cnsoperatortypes.CNSPvcFinalizer)
 
 	if !controllerutil.ContainsFinalizer(pvc, cnsoperatortypes.CNSPvcFinalizer) {