Add detaching suffix to vm.status.volumes (#1320)

For batchAttachment workflow, CSI now adds `:detaching` 
suffix to volume name when a volume is being detached. 
But it only adds it once it finished a detaching request 
to CNS, which could take up to minutes. This means there 
is a time period that the volume is not in vm.spec.volumes 
but still in batchAttachment.status.volumes, we still 
want to capture those volumes' status inside 
vm.status.volumes.

Author: lubronzhan

controllers/virtualmachine/volumebatch/volumebatch_controller.go

@@ -16,6 +16,7 @@ import (
 	storagev1 "k8s.io/api/storage/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	storagehelpers "k8s.io/component-helpers/storage/volume"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -45,7 +46,10 @@ import (
 const (
 	controllerName = "volumebatch"
 	// In BatchAttachment status, CSI hardcode a volume name entry with :detaching
-	// suffix if it's being detached.
+	// suffix if it's being detached. CSI only adds that after they have finsihed
+	// a CNS detach call, which could take up to minutes. So we still want to add
+	// that suffix ourselves as soon as a volume is removed from vm.spec.volumes.
+	//
 	// Note: The reason why we have this suffix is because there is a case that
 	// a volume's PVC was changed, that means we need to detach current vol,
 	// and attach another one. But since volume name is unique, and there is a
@@ -655,6 +659,7 @@ func (r *Reconciler) getVMVolStatusesFromBatchAttachment(
 	volumeStatuses = append(volumeStatuses,
 		getVolumeStatusWithDetachingVolumeFromBatchAttachment(
 			existingAttachVolStatus,
+			volumeSpecs,
 		)...,
 	)
 
@@ -920,12 +925,23 @@ func getVolumeStatusesWithDetachingVolumeInLegacyAttachment(
 // These volumes that are being detached all has same suffix ':detaching'.
 func getVolumeStatusWithDetachingVolumeFromBatchAttachment(
 	existingAttachVolStatus map[string]cnsv1alpha1.VolumeStatus,
+	volumeSpecs []cnsv1alpha1.VolumeSpec,
 ) []vmopv1.VirtualMachineVolumeStatus {
+
 	volumeStatuses := []vmopv1.VirtualMachineVolumeStatus{}
 
+	attachVolSpecNames := sets.New[string]()
+	for _, volSpecs := range volumeSpecs {
+		attachVolSpecNames.Insert(volSpecs.Name)
+	}
+
 	for _, volStatus := range existingAttachVolStatus {
 		volName := volStatus.Name
-		if strings.HasSuffix(volName, volumeNameDetachSuffix) {
+		if !attachVolSpecNames.Has(volName) {
+			// Append suffix if it's not added by CSI yet.
+			if !strings.HasSuffix(volName, volumeNameDetachSuffix) {
+				volName += volumeNameDetachSuffix
+			}
 			volumeStatuses = append(volumeStatuses,
 				attachmentStatusToVolumeStatus(volName, volStatus),
 			)
@@ -972,7 +988,7 @@ func (r *Reconciler) getVMVolStatusesFromLegacyAttachments(
 ) []vmopv1.VirtualMachineVolumeStatus {
 
 	volumeStatuses := []vmopv1.VirtualMachineVolumeStatus{}
-	// Maintain the mapping of AttachmentName -> Attachment.
+	// Maintain the mapping of LegacyAttachmentName -> Attachment.
 	orphanedAttachmentsMap := make(map[string]cnsv1alpha1.CnsNodeVmAttachment)
 	for _, o := range orphanedAttachments {
 		orphanedAttachmentsMap[o.Name] = o
@@ -1095,7 +1111,7 @@ func categorizeVolumeSpecs(
 		}
 
 		// Only include greenfield volumes that are not tracked by
-		// legacy CnsNodeVmAttachment
+		// legacy CnsNodeVmAttachment or those whose PVCs have been changed.
 		volumeSpecsForBatch = append(volumeSpecsForBatch, vol)
 	}
 	return volumeSpecsForBatch, volumeSpecsForLegacy

controllers/virtualmachine/volumebatch/volumebatch_controller_intg_test.go

@@ -427,13 +427,13 @@ func intgTestsReconcile() {
 				Expect(ctx.Client.Update(ctx, vm)).To(Succeed())
 			})
 
-			By("VM Status.Volumes should still only have entry for volume volume2", func() {
+			By("VM Status.Volumes should still have entries for volume1 and volume2", func() {
 				// I'm not sure if we have a better way to check for this.
 				Eventually(func(g Gomega) {
 					vm = getVirtualMachine(vmKey)
 					g.Expect(vm).ToNot(BeNil())
-					g.Expect(vm.Status.Volumes).To(HaveLen(1))
-				}).Should(Succeed())
+					g.Expect(vm.Status.Volumes).To(HaveLen(2))
+				}, "3s").Should(Succeed())
 			})
 		})
 	})

controllers/virtualmachine/volumebatch/volumebatch_controller_unit_test.go

@@ -866,7 +866,7 @@ FaultMessage: ([]vimtypes.LocalizableMessage) \u003cnil\u003e\\n }\\n },\\n Type
 					Expect(vm.Status.Volumes).To(HaveLen(1))
 					Expect(attachment.Status.VolumeStatus).To(HaveLen(1))
 					attachment.Status.VolumeStatus[0].PersistentVolumeClaim.Error = "failed to attach cns volume"
-					assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 0)
+					assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 0, false)
 				})
 			})
 		})
@@ -941,8 +941,8 @@ FaultMessage: ([]vimtypes.LocalizableMessage) \u003cnil\u003e\\n }\\n },\\n Type
 					By("VM Status.Volumes are stable-sorted by Spec.Volumes order", func() {
 						Expect(vm.Status.Volumes).To(HaveLen(2))
 						Expect(attachment.Status.VolumeStatus).To(HaveLen(2))
-						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 1, 0)
-						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 1)
+						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 1, 0, false)
+						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 1, false)
 					})
 				})
 
@@ -997,8 +997,8 @@ FaultMessage: ([]vimtypes.LocalizableMessage) \u003cnil\u003e\\n }\\n },\\n Type
 							Expect(vm.Status.Volumes[0]).To(Equal(classicDisk1()))
 							Expect(vm.Status.Volumes[1]).To(Equal(classicDisk2()))
 
-							assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 2, 1)
-							assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 3, 0)
+							assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 2, 1, false)
+							assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 3, 0, false)
 						})
 					}
 
@@ -1393,10 +1393,45 @@ FaultMessage: ([]vimtypes.LocalizableMessage) \u003cnil\u003e\\n }\\n },\\n Type
 
 					By("VM Status.Volumes should contain the volume with detaching suffix", func() {
 						Expect(vm.Status.Volumes).To(HaveLen(1))
-						Expect(vm.Status.Volumes[0].Name).To(Equal(volumeName1 + detachingVolumeSuffix))
-						Expect(vm.Status.Volumes[0].Attached).To(BeTrue())
-						Expect(vm.Status.Volumes[0].DiskUUID).To(Equal(dummyDiskUUID))
-						Expect(vm.Status.Volumes[0].Type).To(Equal(vmopv1.VolumeTypeManaged))
+						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 0, false)
+					})
+				})
+			})
+
+			When("Volumes are just being detached and CnsNodeVMBatchAttachment's status has been cleared by CSI yet", func() {
+				BeforeEach(func() {
+					attachment.Status.VolumeStatus = append(attachment.Status.VolumeStatus,
+						cnsv1alpha1.VolumeStatus{
+							Name: volumeName1 + detachingVolumeSuffix,
+							PersistentVolumeClaim: cnsv1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: claimName1,
+								Attached:  false,
+								DiskUUID:  dummyDiskUUID,
+							},
+						},
+						cnsv1alpha1.VolumeStatus{
+							Name: volumeName2,
+							PersistentVolumeClaim: cnsv1alpha1.PersistentVolumeClaimStatus{
+								ClaimName: claimName2,
+								Attached:  true,
+								DiskUUID:  dummyDiskUUID,
+							},
+						},
+					)
+					initObjects = append(initObjects, attachment)
+				})
+				It("returns success and refresh the vm volume status", func() {
+					err := reconciler.ReconcileNormal(volCtx)
+					Expect(err).ToNot(HaveOccurred())
+
+					attachment := getCNSBatchAttachmentForVolumeName(ctx, vm)
+					Expect(attachment).ToNot(BeNil())
+					Expect(attachment.Spec.Volumes).To(HaveLen(0))
+
+					By("VM Status.Volumes should contain the volume with detaching suffix", func() {
+						Expect(vm.Status.Volumes).To(HaveLen(2))
+						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 0, 0, false)
+						assertVMVolStatusFromBatchAttachmentStatus(vm, attachment, 1, 1, true)
 					})
 				})
 			})
@@ -1476,17 +1511,22 @@ func assertVMVolStatusFromBatchAttachmentStatus(
 	vm *vmopv1.VirtualMachine,
 	attachment *cnsv1alpha1.CnsNodeVMBatchAttachment,
 	vmVolStatusIndex,
-	attachmentStatusIndex int) {
+	attachmentStatusIndex int,
+	detachingSuffix bool) {
 
 	GinkgoHelper()
 
-	Expect(len(vm.Status.Volumes) > vmVolStatusIndex).To(BeTrue(), fmt.Sprintf("vm volume status should have len larger than %d", vmVolStatusIndex))
+	Expect(len(vm.Status.Volumes)).To(BeNumerically(">", vmVolStatusIndex), fmt.Sprintf("vm volume status should have len larger than %d", vmVolStatusIndex))
 	vmVolStatus := vm.Status.Volumes[vmVolStatusIndex]
-	Expect(len(attachment.Status.VolumeStatus) > attachmentStatusIndex).To(BeTrue(), fmt.Sprintf("attachment volume status should have len larger than %d", attachmentStatusIndex))
+	Expect(len(attachment.Status.VolumeStatus)).To(BeNumerically(">", attachmentStatusIndex), fmt.Sprintf("attachment volume status should have len larger than %d", attachmentStatusIndex))
 	attachmentVolStatus := attachment.Status.VolumeStatus[attachmentStatusIndex]
 
+	volName := attachmentVolStatus.Name
+	if detachingSuffix {
+		volName = attachmentVolStatus.Name + ":detaching"
+	}
+	Expect(vmVolStatus.Name).To(Equal(volName), "volume name should match")
 	Expect(vmVolStatus.Type).To(Equal(vmopv1.VolumeTypeManaged), "type should match")
-	Expect(vmVolStatus.Name).To(Equal(attachmentVolStatus.Name), "volume name should match")
 	Expect(vmVolStatus.Attached).To(Equal(attachmentVolStatus.PersistentVolumeClaim.Attached), "attached should match")
 	Expect(vmVolStatus.DiskUUID).To(Equal(attachmentVolStatus.PersistentVolumeClaim.DiskUUID), "diskuuid should match")
 	Expect(vmVolStatus.Error).To(Equal(attachmentVolStatus.PersistentVolumeClaim.Error), "error shouuld match")
@@ -1500,9 +1540,9 @@ func assertVMVolStatusFromBatchAttachmentSpec(
 
 	GinkgoHelper()
 
-	Expect(len(vm.Status.Volumes) > vmVolStatusIndex).To(BeTrue(), fmt.Sprintf("vm volume status should have len larger than %d", vmVolStatusIndex))
+	Expect(len(vm.Status.Volumes)).To(BeNumerically(">", vmVolStatusIndex), fmt.Sprintf("vm volume status should have len larger than %d", vmVolStatusIndex))
 	vmVolStatus := vm.Status.Volumes[vmVolStatusIndex]
-	Expect(len(attachment.Spec.Volumes) > attachmentStatusIndex).To(BeTrue(), fmt.Sprintf("attachment volume spec should have len larger than %d", attachmentStatusIndex))
+	Expect(len(attachment.Spec.Volumes)).To(BeNumerically(">", attachmentStatusIndex), fmt.Sprintf("attachment volume spec should have len larger than %d", attachmentStatusIndex))
 	attachmentVolSpec := attachment.Spec.Volumes[attachmentStatusIndex]
 
 	Expect(vmVolStatus.Type).To(Equal(vmopv1.VolumeTypeManaged), "type should match")