Consider volumes in status also while removing finalizers from PVC

Author: skogta

pkg/common/unittestcommon/utils.go

@@ -469,6 +469,10 @@ func (c *FakeK8SOrchestrator) GetPVCNameFromCSIVolumeID(volumeID string) (string
 		return "with-used-by-annotation", "test-ns", true
 	}
 
+	if strings.Contains(volumeID, "vol-1") {
+		return "mock-pvc-1", "mock-namespace", true
+	}
+
 	// Simulate a case where the volumeID corresponds to a PVC.
 	return "mock-pvc", "mock-namespace", true
 }

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

@@ -295,18 +295,21 @@ 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, volumeName := range pvcsInSpecAndStatus {
+			err := removePvcFinalizer(ctx, r.client, k8sClient, pvcName, instance.Namespace,
 				instance.Spec.InstanceUUID)
 			if err != nil {
-				updateInstanceVolumeStatus(ctx, instance, volume.Name, volume.PersistentVolumeClaim.ClaimName, "", "", err,
+				updateInstanceVolumeStatus(ctx, instance, volumeName, pvcName, "", "", err,
 					v1alpha1.ConditionDetached, v1alpha1.ReasonDetachFailed)
-				log.Errorf("failed to remove finalizer from PVC %s. Err: %s", volume.PersistentVolumeClaim.ClaimName,
-					err)
+				log.Errorf("failed to remove finalizer from PVC %s. Err: %s", pvcName, err)
 				return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, err)
 			} else {
-				updateInstanceVolumeStatus(ctx, instance, volume.Name, volume.PersistentVolumeClaim.ClaimName, "", "", err,
+				updateInstanceVolumeStatus(ctx, instance, volumeName, pvcName, "", "", err,
 					v1alpha1.ConditionDetached, "")
 			}
 		}

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

@@ -424,33 +424,23 @@ func getVolumeMetadataMaps(ctx context.Context,
 	return
 }
 
-// getPvcsInSpec returns map of PVCs and their volumeIDs.
-func getPvcsInSpec(ctx context.Context, instance *v1alpha1.CnsNodeVMBatchAttachment,
-	k8sClient kubernetes.Interface) (map[string]string, error) {
+// 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)
 
-	pvcsInSpec := make(map[string]string)
-	for _, volume := range instance.Spec.Volumes {
-		volumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(
-			instance.Namespace, volume.PersistentVolumeClaim.ClaimName)
-		if !ok {
-			pvcName := volume.PersistentVolumeClaim.ClaimName
-			_, err := k8sClient.CoreV1().PersistentVolumeClaims(instance.Namespace).Get(ctx,
-				pvcName, metav1.GetOptions{})
-			if err != nil {
-				if apierrors.IsNotFound(err) {
-					log.Infof("PVC %s has already been deleted. No action to be taken", pvcName)
-					continue
-				}
-				return pvcsInSpec, fmt.Errorf("failed to find volumeID for PVC %s", volume.PersistentVolumeClaim.ClaimName)
-			}
-			return pvcsInSpec, fmt.Errorf("failed to find volumeID for PVC %s", volume.PersistentVolumeClaim.ClaimName)
+	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
 		}
-		pvcsInSpec[volume.PersistentVolumeClaim.ClaimName] = volumeId
+		pvcsAttachedToVM[pvcName] = volumeID
 	}
-
-	return pvcsInSpec, nil
-
+	return pvcsAttachedToVM
 }
 
 // listAttachedFcdsForVM returns list of FCDs (present in the K8s cluster)
@@ -504,7 +494,7 @@ func listAttachedFcdsForVM(ctx context.Context,
 				log.Debugf("failed to get diskMode and sharingMode for virtual disk")
 			}
 
-			log.Infof("Adding volume with ID %s to attachedFCDs list", virtualDisk.VDiskId.Id)
+			log.Debugf("Adding volume with ID %s to attachedFCDs list", virtualDisk.VDiskId.Id)
 			attachedFCDs[virtualDisk.VDiskId.Id] = FCDBackingDetails{
 				ControllerKey: controllerKey,
 				UnitNumber:    unitNumber,
@@ -572,6 +562,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]string {
+
+	listOfPvcsToRemoveFinalzer := make(map[string]string, 0)
+
+	for _, volume := range instance.Spec.Volumes {
+		listOfPvcsToRemoveFinalzer[volume.PersistentVolumeClaim.ClaimName] = volume.Name
+	}
+	for _, volume := range instance.Status.VolumeStatus {
+		listOfPvcsToRemoveFinalzer[volume.PersistentVolumeClaim.ClaimName] = volume.Name
+	}
+	return listOfPvcsToRemoveFinalzer
+}
+
 // isPvcEncrypted returns true if annotation csi.vsphere.encryption-class
 // is present on the PVC.
 func isPvcEncrypted(pvcAnnotations map[string]string) bool {
@@ -615,17 +620,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 {
@@ -634,6 +628,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 {
@@ -893,7 +895,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) {

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

@@ -1254,6 +1254,178 @@ func TestIsPvcEncrypted(t *testing.T) {
 	}
 }
 
+func TestGetPvcsFromSpecAndStatus(t *testing.T) {
+	tests := []struct {
+		name     string
+		instance *v1alpha1.CnsNodeVMBatchAttachment
+		want     map[string]string
+	}{
+		{
+			name: "PVCs only in spec",
+			instance: &v1alpha1.CnsNodeVMBatchAttachment{
+				Spec: v1alpha1.CnsNodeVMBatchAttachmentSpec{
+					Volumes: []v1alpha1.VolumeSpec{
+						{
+							Name: "vol-1",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimSpec{
+								ClaimName: "pvc-a",
+							},
+						},
+						{
+							Name: "vol-2",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimSpec{
+								ClaimName: "pvc-b",
+							},
+						},
+					},
+				},
+			},
+			want: map[string]string{
+				"pvc-a": "vol-1",
+				"pvc-b": "vol-2",
+			},
+		},
+		{
+			name: "PVCs only in status",
+			instance: &v1alpha1.CnsNodeVMBatchAttachment{
+				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+					VolumeStatus: []v1alpha1.VolumeStatus{
+						{
+							Name: "vol-1",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: "pvc-x",
+							},
+						},
+						{
+							Name: "vol-2",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: "pvc-y",
+							},
+						},
+					},
+				},
+			},
+			want: map[string]string{
+				"pvc-x": "vol-1",
+				"pvc-y": "vol-2",
+			},
+		},
+		{
+			name: "PVCs in both spec and status",
+			instance: &v1alpha1.CnsNodeVMBatchAttachment{
+				Spec: v1alpha1.CnsNodeVMBatchAttachmentSpec{
+					Volumes: []v1alpha1.VolumeSpec{
+						{
+							Name: "vol-1",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimSpec{
+								ClaimName: "pvc-a",
+							},
+						},
+					},
+				},
+				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+					VolumeStatus: []v1alpha1.VolumeStatus{
+						{
+							Name: "vol-2",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: "pvc-b",
+							},
+						},
+					},
+				},
+			},
+			want: map[string]string{
+				"pvc-a": "vol-1",
+				"pvc-b": "vol-2",
+			},
+		},
+		{
+			name: "Duplicate PVCs across spec and status",
+			instance: &v1alpha1.CnsNodeVMBatchAttachment{
+				Spec: v1alpha1.CnsNodeVMBatchAttachmentSpec{
+					Volumes: []v1alpha1.VolumeSpec{
+						{
+							Name: "vol-1",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimSpec{
+								ClaimName: "pvc-dup",
+							},
+						},
+					},
+				},
+				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+					VolumeStatus: []v1alpha1.VolumeStatus{
+						{
+							Name: "vol-1",
+							PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: "pvc-dup",
+							},
+						},
+					},
+				},
+			},
+			want: map[string]string{
+				"pvc-dup": "vol-1",
+			},
+		},
+		{
+			name:     "Empty spec and status",
+			instance: &v1alpha1.CnsNodeVMBatchAttachment{},
+			want:     map[string]string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := getPvcsFromSpecAndStatus(context.Background(), tt.instance)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("getPvcsFromSpecAndStatus() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestGetAllPvcsAttachedToVMSuccess(t *testing.T) {
+	commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
+
+	attachedFCD := map[string]FCDBackingDetails{
+		"vol-1": {},
+		"vol-2": {},
+	}
+
+	got := getAllPvcsAttachedToVM(context.Background(), attachedFCD)
+	want := map[string]string{
+		"mock-pvc-1": "vol-1",
+		"mock-pvc":   "vol-2",
+	}
+
+	assert.Equal(t, want, got)
+}
+
+func TestGetAllPvcsAttachedToVM_EmptyInput(t *testing.T) {
+
+	commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
+
+	attachedFCD := map[string]FCDBackingDetails{}
+
+	got := getAllPvcsAttachedToVM(context.Background(), attachedFCD)
+	want := map[string]string{}
+
+	assert.Equal(t, want, got)
+}
+
+func TestGetAllPvcsAttachedToVM_PVCNotFound(t *testing.T) {
+
+	commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
+
+	attachedFCD := map[string]FCDBackingDetails{
+		"invalid": {},
+	}
+	got := getAllPvcsAttachedToVM(context.Background(), attachedFCD)
+	want := map[string]string{}
+
+	assert.Equal(t, want, got)
+}
+
 func MockGetVMFromVcenter(ctx context.Context, nodeUUID string,
 	configInfo config.ConfigurationInfo) (*cnsvsphere.VirtualMachine, error) {
 	var vm *cnsvsphere.VirtualMachine