Address review comments

Author: Pearl1594

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;
@@ -106,25 +107,37 @@ public boolean deleteStoragePool(String uuid) {
     }
 
     private boolean undefineInactiveClvmPool(Connect conn, String uuid) throws LibvirtException {
-        StoragePool sp;
-        try {
-            sp = conn.storagePoolLookupByUUIDString(uuid);
-        } catch (LibvirtException e) {
+        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;
         }
 
-        if (sp.isActive() == 1) {
-            sp.destroy();
-        }
-        if (sp.isPersistent() == 1) {
-            sp.undefine();
+        try {
+            if (sp.isActive() == 1) {
+                sp.destroy();
+            }
+            if (sp.isPersistent() == 1) {
+                sp.undefine();
+            }
+        } finally {
+            sp.free();
         }
-        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);

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 @@ public void testCreateCLVMStoragePool_Success() throws Exception {
             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();