From fa92820448e583c7fd722dc20270544e0c3eca53 Mon Sep 17 00:00:00 2001
From: Alexander Wels <awels@redhat.com>
Date: Thu, 6 Jul 2023 15:27:39 -0500
Subject: [PATCH] Ensure volume is removed before returning success (#90)

When removing a volume from a VM, ensure that the volume is
actually removed from the VM before returning nil from the
ControllerUnpublishVolume function.

Added EnsureVolumeRemoved function to virtClient

Signed-off-by: Alexander Wels <awels@redhat.com>
---
 pkg/kubevirt/client.go         | 18 ++++++++++++++++++
 pkg/service/controller.go      |  6 ++++++
 pkg/service/controller_test.go |  4 ++++
 sanity/sanity_test.go          |  6 +++++-
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/pkg/kubevirt/client.go b/pkg/kubevirt/client.go
index c410b6f0..ba5d3bba 100644
--- a/pkg/kubevirt/client.go
+++ b/pkg/kubevirt/client.go
@@ -29,6 +29,7 @@ type Client interface {
 	AddVolumeToVM(namespace string, vmName string, hotPlugRequest *kubevirtv1.AddVolumeOptions) error
 	RemoveVolumeFromVM(namespace string, vmName string, hotPlugRequest *kubevirtv1.RemoveVolumeOptions) error
 	EnsureVolumeAvailable(namespace, vmName, volumeName string, timeout time.Duration) error
+	EnsureVolumeRemoved(namespace, vmName, volumeName string, timeout time.Duration) error
 }
 
 type client struct {
@@ -81,6 +82,23 @@ func (c *client) EnsureVolumeAvailable(namespace, vmName, volumeName string, tim
 	})
 }
 
+// EnsureVolumeAvailable checks to make sure the volume is available in the node before returning, checks for 2 minutes
+func (c *client) EnsureVolumeRemoved(namespace, vmName, volumeName string, timeout time.Duration) error {
+	return wait.PollImmediate(time.Second, timeout, func() (done bool, err error) {
+		vmi, err := c.virtClient.VirtualMachineInstance(namespace).Get(context.TODO(), vmName, &metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		for _, volume := range vmi.Status.VolumeStatus {
+			if volume.Name == volumeName {
+				return false, nil
+			}
+		}
+		// Have not found the hotplugged volume
+		return true, nil
+	})
+}
+
 // ListVirtualMachines fetches a list of VMIs from the passed in namespace
 func (c *client) ListVirtualMachines(namespace string) ([]kubevirtv1.VirtualMachineInstance, error) {
 	list, err := c.virtClient.VirtualMachineInstance(namespace).List(context.TODO(), &metav1.ListOptions{})
diff --git a/pkg/service/controller.go b/pkg/service/controller.go
index 015f8db1..57b9f968 100644
--- a/pkg/service/controller.go
+++ b/pkg/service/controller.go
@@ -341,6 +341,12 @@ func (c *ControllerService) ControllerUnpublishVolume(ctx context.Context, req *
 			return nil, err
 		}
 	}
+	err = c.virtClient.EnsureVolumeRemoved(c.infraClusterNamespace, vmName, dvName, time.Minute*2)
+	if err != nil {
+		klog.Errorf("volume %s failed to be removed in time from VM %s, %v", dvName, vmName, err)
+		return nil, err
+	}
+
 	return &csi.ControllerUnpublishVolumeResponse{}, nil
 }
 
diff --git a/pkg/service/controller_test.go b/pkg/service/controller_test.go
index d6b7d687..8a8a3e17 100644
--- a/pkg/service/controller_test.go
+++ b/pkg/service/controller_test.go
@@ -363,3 +363,7 @@ func (c *ControllerClientMock) RemoveVolumeFromVM(namespace string, vmName strin
 func (c *ControllerClientMock) EnsureVolumeAvailable(namespace, vmName, volumeName string, timeout time.Duration) error {
 	return nil
 }
+
+func (c *ControllerClientMock) EnsureVolumeRemoved(namespace, vmName, volumeName string, timeout time.Duration) error {
+	return nil
+}
diff --git a/sanity/sanity_test.go b/sanity/sanity_test.go
index 2508cfb4..756d2a52 100644
--- a/sanity/sanity_test.go
+++ b/sanity/sanity_test.go
@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
 You may obtain a copy of the License at
 
-    http://www.apache.org/licenses/LICENSE-2.0
+	http://www.apache.org/licenses/LICENSE-2.0
 
 Unless required by applicable law or agreed to in writing, software
 distributed under the License is distributed on an "AS IS" BASIS,
@@ -166,6 +166,10 @@ func (k *fakeKubeVirtClient) EnsureVolumeAvailable(namespace, vmName, volumeName
 	return nil
 }
 
+func (k *fakeKubeVirtClient) EnsureVolumeRemoved(namespace, vmName, volumeName string, timeout time.Duration) error {
+	return nil
+}
+
 func (k *fakeKubeVirtClient) List() ([]byte, error) {
 	ds := make([]device, 0)
 	for _, value := range k.hotpluggedMap {