Inserts timer in check detach volume

agent/conf/agent.properties

@@ -291,3 +291,7 @@ iscsi.session.cleanup.enabled=false
 
 # Enable/disable IO driver for Qemu (in case it is not set CloudStack can also detect if its supported by qemu)
 # enable.io.uring=true
+
+# Time (in milliseconds) to wait before assuming the VM was unable to detach a volume after the hypervisor sends the detach command.
+# Default value: 10000
+#wait.detach.device=10000

agent/src/main/java/com/cloud/agent/properties/AgentProperties.java

@@ -54,6 +54,14 @@ public class AgentProperties{
      */
     public static final Property<Boolean> ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM = new Property<Boolean>("enable.manually.setting.cpu.topology.on.kvm.vm", true);
 
+    /**
+     * Time (in milliseconds) to wait before assuming the VM was unable to detach a volume after the hypervisor sends the detach command.<br>
+     * This property is for KVM only.
+     * Data type: Long.<br>
+     * Default value: <code>10000l</code>
+     */
+    public static final Property<Long> WAIT_DETACH_DEVICE = new Property<Long>("wait.detach.device", 10000l);
+
     public static class Property <T>{
         private final String name;
         private final T defaultValue;

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java

@@ -37,6 +37,8 @@
 
 import javax.naming.ConfigurationException;
 
+import com.cloud.agent.properties.AgentProperties;
+import com.cloud.agent.properties.AgentPropertiesFileHandler;
 import org.apache.cloudstack.agent.directdownload.DirectDownloadAnswer;
 import org.apache.cloudstack.agent.directdownload.DirectDownloadCommand;
 import org.apache.cloudstack.agent.directdownload.HttpDirectDownloadCommand;
@@ -154,6 +156,11 @@ public class KVMStorageProcessor implements StorageProcessor {
     private static final String CEPH_AUTH_KEY = "key";
     private static final String CEPH_CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
     private static final String CEPH_DEFAULT_MOUNT_TIMEOUT = "30";
+    /**
+     * Time interval before rechecking virsh commands
+     */
+    private long waitDelayForVirshCommands = 1000l;
+    protected Long waitDetachDevice;
 
     public KVMStorageProcessor(final KVMStoragePoolManager storagePoolMgr, final LibvirtComputingResource resource) {
         this.storagePoolMgr = storagePoolMgr;
@@ -169,6 +176,7 @@ public boolean configure(final String name, final Map<String, Object> params) th
         storageLayer.configure("StorageLayer", params);
 
         String storageScriptsDir = (String)params.get("storage.scripts.dir");
+        waitDetachDevice = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.WAIT_DETACH_DEVICE);
         if (storageScriptsDir == null) {
             storageScriptsDir = getDefaultStorageScriptsDir();
         }
@@ -1169,26 +1177,37 @@ private String getDataStoreUrlFromStore(DataStoreTO store) {
         return store.getUrl();
     }
 
-    protected synchronized String attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final String xml) throws LibvirtException, InternalErrorException {
+    protected synchronized String attachOrDetachDevice(final Connect conn, final boolean attach, final String vmName, final String xml)
+            throws LibvirtException, InternalErrorException {
         Domain dm = null;
         try {
             dm = conn.domainLookupByName(vmName);
 
             if (attach) {
                 s_logger.debug("Attaching device: " + xml);
                 dm.attachDevice(xml);
-            } else {
-                s_logger.debug("Detaching device: " + xml);
-                dm.detachDevice(xml);
-                LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
-                parser.parseDomainXML(dm.getXMLDesc(0));
-                List<DiskDef> disks = parser.getDisks();
-                for (DiskDef diskDef : disks) {
-                    if (StringUtils.contains(xml, diskDef.getDiskPath())) {
-                        throw new InternalErrorException("Could not detach volume. Probably the VM is in boot state at the moment");
-                    }
+                return null;
+            }
+            s_logger.debug(String.format("Detaching device: [%s].", xml));
+            dm.detachDevice(xml);
+            long wait = waitDetachDevice;
+            while (!checkDetachSucess(xml, dm) && wait > 0) {
+                try {
+                    wait -= waitDelayForVirshCommands;
+                    Thread.sleep(waitDelayForVirshCommands);
+                    s_logger.trace(String.format("Trying to detach device [%s] from VM instance with UUID [%s]. " +
+                            "Waiting [%s] milliseconds before assuming the VM was unable to detach the volume.", xml, dm.getUUIDString(), wait));
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
                 }
             }
+            if (wait <= 0) {
+                throw new InternalErrorException(String.format("Could not detach volume after sending the command and waiting for [%s] milliseconds. Probably the VM does " +
+                                "not support the sent detach command or the device is busy at the moment. Try again in a couple of minutes.",
+                        waitDetachDevice));
+            }
+            s_logger.debug(String.format("The detach command was executed successfully. The device [%s] was removed from the VM instance with UUID [%s].",
+                    xml, dm.getUUIDString()));
         } catch (final LibvirtException e) {
             if (attach) {
                 s_logger.warn("Failed to attach device to " + vmName + ": " + e.getMessage());
@@ -1209,6 +1228,20 @@ protected synchronized String attachOrDetachDevice(final Connect conn, final boo
         return null;
     }
 
+    protected boolean checkDetachSucess(String xml, Domain dm) throws LibvirtException {
+        LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+        parser.parseDomainXML(dm.getXMLDesc(0));
+        List<DiskDef> disks = parser.getDisks();
+        for (DiskDef diskDef : disks) {
+            if (StringUtils.contains(xml, diskDef.getDiskPath())) {
+                s_logger.debug(String.format("The hypervisor sent the detach command, but it is still possible to identify the device [%s] in the instance with UUID [%s].",
+                        xml, dm.getUUIDString()));
+                return false;
+            }
+        }
+        return true;
+    }
+
     protected synchronized String attachOrDetachDisk(final Connect conn, final boolean attach, final String vmName, final KVMPhysicalDisk attachingDisk, final int devId, final String serial,
             final Long bytesReadRate, final Long bytesReadRateMax, final Long bytesReadRateMaxLength,
             final Long bytesWriteRate, final Long bytesWriteRateMax, final Long bytesWriteRateMaxLength,
@@ -1243,7 +1276,9 @@ protected synchronized String attachOrDetachDisk(final Connect conn, final boole
                     }
                 }
                 if (diskdef == null) {
-                    throw new InternalErrorException("disk: " + attachingDisk.getPath() + " is not attached before");
+                    s_logger.warn(String.format("Could not find disk [%s] attached to VM instance with UUID [%s]. We will set it as detached in the database to ensure consistency.",
+                            attachingDisk.getPath(), dm.getUUIDString()));
+                    return null;
                 }
             } else {
                 DiskDef.DiskBus busT = DiskDef.DiskBus.VIRTIO;

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java

@@ -18,7 +18,9 @@
  */
 package com.cloud.hypervisor.kvm.storage;
 
+import com.cloud.exception.InternalErrorException;
 import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
 import com.cloud.hypervisor.kvm.resource.wrapper.LibvirtUtilitiesHelper;
 import com.cloud.storage.template.TemplateConstants;
@@ -51,10 +53,12 @@
 import org.mockito.MockitoAnnotations;
 import org.mockito.Spy;
 import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
 @PrepareForTest({ Script.class })
+@PowerMockIgnore({"javax.xml.*", "org.xml.*", "org.w3c.dom.*"})
 @RunWith(PowerMockRunner.class)
 public class KVMStorageProcessorTest {
 
@@ -87,6 +91,9 @@ public class KVMStorageProcessorTest {
     @Mock
     Connect connectMock;
 
+    @Mock
+    LibvirtDomainXMLParser libvirtDomainXMLParserMock;
+
     private static final String directDownloadTemporaryPath = "/var/lib/libvirt/images/dd";
     private static final long templateSize = 80000L;
 
@@ -348,4 +355,83 @@ public void validateDeleteSnapshotFileSuccess () throws IOException {
 
         storageProcessorSpy.deleteSnapshotFile(snapshotObjectToMock);
     }
+
+    private void checkDetachSucessTest(boolean duplicate) throws Exception {
+        List<LibvirtVMDef.DiskDef> disks = createDiskDefs(2, duplicate);
+        PowerMockito.when(domainMock.getXMLDesc(Mockito.anyInt())).thenReturn("test");
+        PowerMockito.whenNew(LibvirtDomainXMLParser.class).withAnyArguments().thenReturn(libvirtDomainXMLParserMock);
+        PowerMockito.when(libvirtDomainXMLParserMock.parseDomainXML(Mockito.anyString())).thenReturn(true);
+        PowerMockito.when(libvirtDomainXMLParserMock.getDisks()).thenReturn(disks);
+    }
+
+    @Test
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void checkDetachSucessTestDetachReturnTrue() throws Exception {
+        checkDetachSucessTest(false);
+        Assert.assertTrue(storageProcessorSpy.checkDetachSucess("path", domainMock));
+    }
+
+    @Test
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void checkDetachSucessTestDetachReturnFalse() throws Exception {
+        checkDetachSucessTest(true);
+        Assert.assertFalse(storageProcessorSpy.checkDetachSucess("path", domainMock));
+    }
+
+    @PrepareForTest(KVMStorageProcessor.class)
+    private String attachOrDetachDeviceTest (boolean attach, String vmName, String xml) throws LibvirtException, InternalErrorException {
+        return storageProcessorSpy.attachOrDetachDevice(connectMock, attach, vmName, xml);
+    }
+
+    @Test (expected = LibvirtException.class)
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestThrowLibvirtException() throws LibvirtException, InternalErrorException {
+        Mockito.when(connectMock.domainLookupByName(Mockito.anyString())).thenThrow(LibvirtException.class);
+        attachOrDetachDeviceTest(true, "vmName", "xml");
+    }
+
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestAttachSuccess() throws LibvirtException, InternalErrorException {
+        Mockito.when(connectMock.domainLookupByName("vmName")).thenReturn(domainMock);
+        String result = attachOrDetachDeviceTest(true, "vmName", "xml");
+        Mockito.verify(domainMock, Mockito.times(1)).attachDevice(Mockito.anyString());
+        Assert.assertNull(result);
+    }
+
+    @Test (expected = LibvirtException.class)
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestAttachThrowLibvirtException() throws LibvirtException, InternalErrorException {
+        Mockito.when(connectMock.domainLookupByName("vmName")).thenReturn(domainMock);
+        Mockito.doThrow(LibvirtException.class).when(domainMock).attachDevice(Mockito.anyString());
+        attachOrDetachDeviceTest(true, "vmName", "xml");
+    }
+
+    @Test (expected = LibvirtException.class)
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestDetachThrowLibvirtException() throws LibvirtException, InternalErrorException {
+        Mockito.when(connectMock.domainLookupByName("vmName")).thenReturn(domainMock);
+        Mockito.doThrow(LibvirtException.class).when(domainMock).detachDevice(Mockito.anyString());
+        attachOrDetachDeviceTest(false, "vmName", "xml");
+    }
+
+    @Test
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestDetachSuccess() throws LibvirtException, InternalErrorException {
+        storageProcessorSpy.waitDetachDevice = 1000l;
+        Mockito.when(connectMock.domainLookupByName("vmName")).thenReturn(domainMock);
+        PowerMockito.doReturn(true).when(storageProcessorSpy).checkDetachSucess(Mockito.anyString(), Mockito.any(Domain.class));
+        String result = attachOrDetachDeviceTest( false, "vmName", "xml");
+        Mockito.verify(domainMock, Mockito.times(1)).detachDevice(Mockito.anyString());
+        Assert.assertNull(result);
+    }
+
+    @Test (expected = InternalErrorException.class)
+    @PrepareForTest(KVMStorageProcessor.class)
+    public void attachOrDetachDeviceTestDetachThrowInternalErrorException() throws LibvirtException, InternalErrorException {
+        storageProcessorSpy.waitDetachDevice = 1000l;
+        Mockito.when(connectMock.domainLookupByName("vmName")).thenReturn(domainMock);
+        PowerMockito.doReturn(false).when(storageProcessorSpy).checkDetachSucess(Mockito.anyString(), Mockito.any(Domain.class));
+        String result = attachOrDetachDeviceTest( false, "vmName", "xml");
+        Mockito.verify(domainMock, Mockito.times(1)).detachDevice(Mockito.anyString());
+    }
 }