Fix volume detach on hotplug attachment pod delete

pkg/virt-controller/watch/vmi.go

@@ -2144,13 +2144,14 @@ func (c *VMIController) updateVolumeStatus(vmi *virtv1.VirtualMachineInstance, v
 			}
 			attachmentPod := c.findAttachmentPodByVolumeName(volume.Name, attachmentPods)
 			if attachmentPod == nil {
-				status.HotplugVolume.AttachPodName = ""
-				status.HotplugVolume.AttachPodUID = ""
-				// Pod is gone, or hasn't been created yet, check for the PVC associated with the volume to set phase and message
-				phase, reason, message := c.getVolumePhaseMessageReason(&vmi.Spec.Volumes[i], vmi.Namespace)
-				status.Phase = phase
-				status.Message = message
-				status.Reason = reason
+				if !c.volumeReady(status.Phase) {
+					status.HotplugVolume.AttachPodUID = ""
+					// Volume is not hotplugged in VM and Pod is gone, or hasn't been created yet, check for the PVC associated with the volume to set phase and message
+					phase, reason, message := c.getVolumePhaseMessageReason(&vmi.Spec.Volumes[i], vmi.Namespace)
+					status.Phase = phase
+					status.Message = message
+					status.Reason = reason
+				}
 			} else {
 				status.HotplugVolume.AttachPodName = attachmentPod.Name
 				if len(attachmentPod.Status.ContainerStatuses) == 1 && attachmentPod.Status.ContainerStatuses[0].Ready {
@@ -2215,6 +2216,10 @@ func (c *VMIController) updateVolumeStatus(vmi *virtv1.VirtualMachineInstance, v
 	return nil
 }
 
+func (c *VMIController) volumeReady(phase virtv1.VolumePhase) bool {
+	return phase == virtv1.VolumeReady
+}
+
 func (c *VMIController) getFilesystemOverhead(pvc *k8sv1.PersistentVolumeClaim) (cdiv1.Percent, error) {
 	// To avoid conflicts, we only allow having one CDI instance
 	if cdiInstances := len(c.cdiInformer.GetStore().List()); cdiInstances != 1 {
@@ -2239,8 +2244,8 @@ func (c *VMIController) getFilesystemOverhead(pvc *k8sv1.PersistentVolumeClaim)
 }
 
 func (c *VMIController) canMoveToAttachedPhase(currentPhase virtv1.VolumePhase) bool {
-	return currentPhase == "" || currentPhase == virtv1.VolumeBound || currentPhase == virtv1.VolumePending ||
-		currentPhase == virtv1.HotplugVolumeAttachedToNode
+	return (currentPhase == "" || currentPhase == virtv1.VolumeBound || currentPhase == virtv1.VolumePending ||
+		currentPhase == virtv1.HotplugVolumeAttachedToNode)
 }
 
 func (c *VMIController) findAttachmentPodByVolumeName(volumeName string, attachmentPods []*k8sv1.Pod) *k8sv1.Pod {

pkg/virt-launcher/virtwrap/converter/converter.go

@@ -1545,7 +1545,7 @@ func Convert_v1_VirtualMachineInstance_To_api_Domain(vmi *v1.VirtualMachineInsta
 		}
 		volume := volumes[disk.Name]
 		if volume == nil {
-			return fmt.Errorf("No matching volume with name %s found", disk.Name)
+			return fmt.Errorf("no matching volume with name %s found", disk.Name)
 		}
 
 		if _, ok := c.HotplugVolumes[disk.Name]; !ok {

tests/storage/hotplug.go

@@ -57,7 +57,6 @@ import (
 	"kubevirt.io/kubevirt/tests/flags"
 	"kubevirt.io/kubevirt/tests/framework/checks"
 	"kubevirt.io/kubevirt/tests/framework/matcher"
-	. "kubevirt.io/kubevirt/tests/framework/matcher"
 	"kubevirt.io/kubevirt/tests/libdv"
 	"kubevirt.io/kubevirt/tests/libnode"
 	"kubevirt.io/kubevirt/tests/libstorage"
@@ -503,7 +502,7 @@ var _ = SIGDescribe("Hotplug", func() {
 
 		dvBlock, err = virtClient.CdiClient().CdiV1beta1().DataVolumes(testsuite.GetTestNamespace(dvBlock)).Create(context.Background(), dvBlock, metav1.CreateOptions{})
 		Expect(err).ToNot(HaveOccurred())
-		libstorage.EventuallyDV(dvBlock, 240, HaveSucceeded())
+		libstorage.EventuallyDV(dvBlock, 240, matcher.HaveSucceeded())
 		return dvBlock
 	}
 
@@ -1140,7 +1139,7 @@ var _ = SIGDescribe("Hotplug", func() {
 				var err error
 				url := cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskCirros)
 
-				storageClass, foundSC := libstorage.GetRWXFileSystemStorageClass()
+				storageClass, foundSC := libstorage.GetRWOFileSystemStorageClass()
 				if !foundSC {
 					Skip("Skip test when Filesystem storage is not present")
 				}
@@ -1151,7 +1150,6 @@ var _ = SIGDescribe("Hotplug", func() {
 					libdv.WithPVC(
 						libdv.PVCWithStorageClass(storageClass),
 						libdv.PVCWithVolumeSize("256Mi"),
-						libdv.PVCWithReadWriteManyAccessMode(),
 					),
 					libdv.WithForceBindAnnotation(),
 				)
@@ -1160,7 +1158,7 @@ var _ = SIGDescribe("Hotplug", func() {
 				Expect(err).ToNot(HaveOccurred())
 
 				By("waiting for the dv import to pvc to finish")
-				libstorage.EventuallyDV(dv, 180, HaveSucceeded())
+				libstorage.EventuallyDV(dv, 180, matcher.HaveSucceeded())
 
 				By("rename disk image on PVC")
 				pvc, err := virtClient.CoreV1().PersistentVolumeClaims(dv.Namespace).Get(context.Background(), dv.Name, metav1.GetOptions{})
@@ -1191,6 +1189,118 @@ var _ = SIGDescribe("Hotplug", func() {
 		})
 	})
 
+	Context("delete attachment pod several times", func() {
+		var (
+			vm       *v1.VirtualMachine
+			hpvolume *cdiv1.DataVolume
+		)
+
+		BeforeEach(func() {
+			if !libstorage.HasCDI() {
+				Skip("Skip tests when CDI is not present")
+			}
+			_, foundSC := libstorage.GetRWXBlockStorageClass()
+			if !foundSC {
+				Skip("Skip test when block RWX storage is not present")
+			}
+		})
+
+		AfterEach(func() {
+			if vm != nil {
+				err := virtClient.VirtualMachine(vm.Namespace).Delete(context.Background(), vm.Name, &metav1.DeleteOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				vm = nil
+			}
+		})
+
+		deleteAttachmentPod := func(vmi *v1.VirtualMachineInstance) {
+			podName := ""
+			for _, volume := range vmi.Status.VolumeStatus {
+				if volume.HotplugVolume != nil {
+					podName = volume.HotplugVolume.AttachPodName
+					break
+				}
+			}
+			Expect(podName).ToNot(BeEmpty())
+			foreGround := metav1.DeletePropagationForeground
+			err := virtClient.CoreV1().Pods(vmi.Namespace).Delete(context.Background(), podName, metav1.DeleteOptions{
+				GracePeriodSeconds: pointer.Int64(0),
+				PropagationPolicy:  &foreGround,
+			})
+			Expect(err).ToNot(HaveOccurred())
+			Eventually(func() bool {
+				_, err := virtClient.CoreV1().Pods(vmi.Namespace).Get(context.Background(), podName, metav1.GetOptions{})
+				return errors.IsNotFound(err)
+			}, 300*time.Second, 1*time.Second).Should(BeTrue())
+		}
+
+		It("should remain active", func() {
+			checkVolumeName := "checkvolume"
+			volumeMode := corev1.PersistentVolumeBlock
+			addVolumeFunc := addDVVolumeVMI
+			var err error
+			storageClass, _ := libstorage.GetRWXBlockStorageClass()
+
+			blankDv := func() *cdiv1.DataVolume {
+				return libdv.NewDataVolume(
+					libdv.WithNamespace(testsuite.GetTestNamespace(nil)),
+					libdv.WithBlankImageSource(),
+					libdv.WithPVC(
+						libdv.PVCWithStorageClass(storageClass),
+						libdv.PVCWithVolumeSize(cd.BlankVolumeSize),
+						libdv.PVCWithReadWriteManyAccessMode(),
+						libdv.PVCWithVolumeMode(volumeMode),
+					),
+					libdv.WithForceBindAnnotation(),
+				)
+			}
+			vmi := libvmi.NewCirros()
+			vm := tests.NewRandomVirtualMachine(vmi, true)
+			vm, err = virtClient.VirtualMachine(testsuite.GetTestNamespace(vm)).Create(context.Background(), vm)
+			Expect(err).ToNot(HaveOccurred())
+
+			Eventually(func() bool {
+				vm, err := virtClient.VirtualMachine(testsuite.GetTestNamespace(vm)).Get(context.Background(), vm.Name, &metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				return vm.Status.Ready
+			}, 300*time.Second, 1*time.Second).Should(BeTrue())
+			By("creating blank hotplug volumes")
+			hpvolume = blankDv()
+			dv, err := virtClient.CdiClient().CdiV1beta1().DataVolumes(hpvolume.Namespace).Create(context.Background(), hpvolume, metav1.CreateOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			By("waiting for the dv import to pvc to finish")
+			libstorage.EventuallyDV(dv, 180, matcher.HaveSucceeded())
+			vmi, err = virtClient.VirtualMachineInstance(vm.Namespace).Get(context.Background(), vmi.Name, &metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+
+			By("hotplugging the volume check volume")
+			addVolumeFunc(vmi.Name, vmi.Namespace, checkVolumeName, hpvolume.Name, v1.DiskBusSCSI, false, "")
+			vmi, err = virtClient.VirtualMachineInstance(vmi.Namespace).Get(context.Background(), vmi.Name, &metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			verifyVolumeAndDiskVMIAdded(virtClient, vmi, checkVolumeName)
+			verifyVolumeStatus(vmi, v1.VolumeReady, "", checkVolumeName)
+			getVmiConsoleAndLogin(vmi)
+
+			By("verifying the volume is useable and creating some data on it")
+			verifyHotplugAttachedAndUseable(vmi, []string{checkVolumeName})
+			targets := getTargetsFromVolumeStatus(vmi, checkVolumeName)
+			Expect(targets).ToNot(BeEmpty())
+			verifyWriteReadData(vmi, targets[0])
+			vmi, err = virtClient.VirtualMachineInstance(vmi.Namespace).Get(context.Background(), vmi.Name, &metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			By("deleting the attachment pod a few times, try to make the currently attach volume break")
+			for i := 0; i < 10; i++ {
+				deleteAttachmentPod(vmi)
+				vmi, err = virtClient.VirtualMachineInstance(vmi.Namespace).Get(context.Background(), vmi.Name, &metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+			}
+			By("verifying the volume has not been disturbed in the VM")
+			targets = getTargetsFromVolumeStatus(vmi, checkVolumeName)
+			Expect(targets).ToNot(BeEmpty())
+			verifyWriteReadData(vmi, targets[0])
+		})
+	})
+
 	Context("with limit range in namespace", func() {
 		var (
 			sc                         string
@@ -1215,7 +1325,7 @@ var _ = SIGDescribe("Hotplug", func() {
 			vm.Spec.Template.Spec.Domain.Resources.Limits = corev1.ResourceList{}
 			vm.Spec.Template.Spec.Domain.Resources.Limits[corev1.ResourceMemory] = *memLimitQuantity
 			vm.Spec.Template.Spec.Domain.Resources.Limits[corev1.ResourceCPU] = *cpuLimitQuantity
-			vm.Spec.Running = pointer.BoolPtr(true)
+			vm.Spec.Running = pointer.Bool(true)
 			vm, err := virtClient.VirtualMachine(testsuite.GetTestNamespace(vm)).Create(context.Background(), vm)
 			Expect(err).ToNot(HaveOccurred())
 			Eventually(func() bool {