CLVM: remove inactive libvirt pool on storage pool delete and stop autostartign CLVM pools

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

@@ -38,6 +38,7 @@
 import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
 import org.joda.time.Duration;
 import org.libvirt.Connect;
+import org.libvirt.Error;
 import org.libvirt.LibvirtException;
 import org.libvirt.StoragePool;
 import org.libvirt.StorageVol;
@@ -89,6 +90,54 @@
         }
     }
 
+    @Override
+    public boolean deleteStoragePool(String uuid) {
+        logger.info("Attempting to remove CLVM/CLVM_NG storage pool {} from libvirt", uuid);
+
+        if (decStoragePoolRefCount(uuid)) {
+            logger.info("deleteStoragePool: CLVM/CLVM_NG storage pool {} still in use", uuid);
+            return true;
+        }
+
+        try {
+            return undefineInactiveClvmPool(LibvirtConnection.getConnection(), uuid);
+        } catch (LibvirtException e) {
+            throw new CloudRuntimeException(e.toString(), e);
+        }
+    }
+
+    private boolean undefineInactiveClvmPool(Connect conn, String uuid) throws LibvirtException {
+        StoragePool sp = lookupClvmPool(conn, uuid);
+        if (sp == null) {
+            logger.warn("CLVM/CLVM_NG storage pool {} doesn't exist in libvirt. Assuming it is already removed", uuid);
+            return true;
+        }
+
+        try {
+            if (sp.isActive() == 1) {
+                sp.destroy();
+            }
+            if (sp.isPersistent() == 1) {
+                sp.undefine();
+            }
+        } finally {
+            sp.free();
+        }
+        logger.info("CLVM/CLVM_NG storage pool {} was successfully removed from libvirt", uuid);
+        return true;
+    }
+
+    private StoragePool lookupClvmPool(Connect conn, String uuid) throws LibvirtException {
+        try {
+            return conn.storagePoolLookupByUUIDString(uuid);
+        } catch (LibvirtException e) {
+            if (e.getError() != null && e.getError().getCode() == Error.ErrorNumber.VIR_ERR_NO_STORAGE_POOL) {
+                return null;
+            }
+            throw e;
+        }
+    }
+
     @Override
     public KVMStoragePool getStoragePool(String uuid, boolean refreshInfo) {
         logger.info("Fetching CLVM/CLVM_NG storage pool {} ", uuid);
@@ -290,7 +339,6 @@
         try {
             StoragePool pool = conn.storagePoolDefineXML(poolDef.toString(), 0);
             logger.info("Created libvirt pool definition for CLVM/CLVM_NG VG: {} (pool will remain inactive)", volgroupName);
-            pool.setAutostart(1);
             return pool;
         } catch (LibvirtException e) {
             logger.warn("Failed to define CLVM/CLVM_NG pool in libvirt: {}", e.getMessage());

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

@@ -40,6 +40,8 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.libvirt.Connect;
+import org.libvirt.Error;
+import org.libvirt.LibvirtException;
 import org.libvirt.StoragePool;
 import org.mockito.Mock;
 import org.mockito.MockedConstruction;
@@ -107,10 +109,96 @@
             clvmStorageAdaptor, mockConn, uuid, host, vgName);
 
         assertNotNull("Storage pool should be created", result);
-        Mockito.verify(mockStoragePool).setAutostart(1);
+        Mockito.verify(mockStoragePool, never()).setAutostart(anyInt());
         Mockito.verify(mockConn).storagePoolDefineXML(anyString(), eq(0));
     }
 
+    private Connect mockLibvirtConnection() throws Exception {
+        Connect mockConn = Mockito.mock(Connect.class);
+        libvirtConnectionMockedStatic.when(LibvirtConnection::getConnection).thenReturn(mockConn);
+        return mockConn;
+    }
+
+    private LibvirtException mockLibvirtException(Error.ErrorNumber errorNumber) {
+        Error error = Mockito.mock(Error.class);
+        when(error.getCode()).thenReturn(errorNumber);
+        LibvirtException exception = Mockito.mock(LibvirtException.class);
+        when(exception.getError()).thenReturn(error);
+        return exception;
+    }
+
+    @Test
+    public void testDeleteStoragePool_InactivePersistentPoolIsUndefinedWithoutDestroy() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        Connect mockConn = mockLibvirtConnection();
+        StoragePool mockStoragePool = Mockito.mock(StoragePool.class);
+        when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
+        when(mockStoragePool.isActive()).thenReturn(0);
+        when(mockStoragePool.isPersistent()).thenReturn(1);
+
+        assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));
+
+        Mockito.verify(mockStoragePool, never()).destroy();
+        Mockito.verify(mockStoragePool).undefine();
+        Mockito.verify(mockStoragePool).free();
+    }
+
+    @Test
+    public void testDeleteStoragePool_ActivePoolIsDestroyedThenUndefined() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        Connect mockConn = mockLibvirtConnection();
+        StoragePool mockStoragePool = Mockito.mock(StoragePool.class);
+        when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
+        when(mockStoragePool.isActive()).thenReturn(1);
+        when(mockStoragePool.isPersistent()).thenReturn(1);
+
+        assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));
+
+        org.mockito.InOrder inOrder = Mockito.inOrder(mockStoragePool);
+        inOrder.verify(mockStoragePool).destroy();
+        inOrder.verify(mockStoragePool).undefine();
+        inOrder.verify(mockStoragePool).free();
+    }
+
+    @Test
+    public void testDeleteStoragePool_MissingPoolIsTreatedAsRemoved() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        Connect mockConn = mockLibvirtConnection();
+        LibvirtException lookupError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_NO_STORAGE_POOL);
+        when(mockConn.storagePoolLookupByUUIDString(uuid)).thenThrow(lookupError);
+
+        assertTrue(clvmStorageAdaptor.deleteStoragePool(uuid));
+    }
+
+    @Test(expected = CloudRuntimeException.class)
+    public void testDeleteStoragePool_OtherLookupErrorIsNotSwallowed() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        Connect mockConn = mockLibvirtConnection();
+        LibvirtException lookupError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_INTERNAL_ERROR);
+        when(mockConn.storagePoolLookupByUUIDString(uuid)).thenThrow(lookupError);
+
+        clvmStorageAdaptor.deleteStoragePool(uuid);
+    }
+
+    @Test
+    public void testDeleteStoragePool_PoolIsFreedWhenUndefineFails() throws Exception {
+        String uuid = UUID.randomUUID().toString();
+        Connect mockConn = mockLibvirtConnection();
+        StoragePool mockStoragePool = Mockito.mock(StoragePool.class);
+        when(mockConn.storagePoolLookupByUUIDString(uuid)).thenReturn(mockStoragePool);
+        when(mockStoragePool.isActive()).thenReturn(0);
+        when(mockStoragePool.isPersistent()).thenReturn(1);
+        LibvirtException undefineError = mockLibvirtException(Error.ErrorNumber.VIR_ERR_INTERNAL_ERROR);
+        Mockito.doThrow(undefineError).when(mockStoragePool).undefine();
+
+        try {
+            clvmStorageAdaptor.deleteStoragePool(uuid);
+            org.junit.Assert.fail("Expected CloudRuntimeException");
+        } catch (CloudRuntimeException expected) {
+            Mockito.verify(mockStoragePool).free();
+        }
+    }
+
     @Test
     public void testCreateCLVMStoragePool_VGNotFound() throws Exception {
         String uuid = UUID.randomUUID().toString();