Fix datadisks not removed

nvazquez · nvazquez · commit f086beca88b0 · 2023-06-20T12:46:43.000-03:00
diff --git a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
@@ -38,6 +38,7 @@
 
 import com.cloud.exception.StorageConflictException;
 import com.cloud.exception.StorageUnavailableException;
+import com.cloud.storage.Volume;
 import com.cloud.storage.VolumeVO;
 import com.cloud.storage.dao.VolumeDao;
 import org.apache.cloudstack.annotation.AnnotationService;
@@ -1016,10 +1017,24 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
         return true;
     }
 
-    private void destroyLocalStoragePoolVolumes(long poolId) {
-        List<VolumeVO> volumes = volumeDao.findByPoolId(poolId);
-        for (VolumeVO volume : volumes) {
-            volumeDao.updateAndRemoveVolume(volume);
+    private void addVolumesToList(List<VolumeVO> volumes, List<VolumeVO> volumesToAdd) {
+        if (CollectionUtils.isNotEmpty(volumesToAdd)) {
+            volumes.addAll(volumesToAdd);
+        }
+    }
+
+    protected void destroyLocalStoragePoolVolumes(long poolId) {
+        List<VolumeVO> rootDisks = volumeDao.findByPoolId(poolId);
+        List<VolumeVO> dataVolumes = volumeDao.findByPoolId(poolId, Volume.Type.DATADISK);
+
+        List<VolumeVO> volumes = new ArrayList<>();
+        addVolumesToList(volumes, rootDisks);
+        addVolumesToList(volumes, dataVolumes);
+
+        if (CollectionUtils.isNotEmpty(volumes)) {
+            for (VolumeVO volume : volumes) {
+                volumeDao.updateAndRemoveVolume(volume);
+            }
         }
     }
 
diff --git a/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java b/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
@@ -34,9 +34,13 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
 
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
 import org.apache.cloudstack.api.command.admin.host.CancelHostAsDegradedCmd;
 import org.apache.cloudstack.api.command.admin.host.DeclareHostAsDegradedCmd;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -98,6 +102,8 @@ public class ResourceManagerImplTest {
     private VMInstanceDao vmInstanceDao;
     @Mock
     private ConfigurationDao configurationDao;
+    @Mock
+    private VolumeDao volumeDao;
 
     @Spy
     @InjectMocks
@@ -119,6 +125,13 @@ public class ResourceManagerImplTest {
     @Mock
     private GetVncPortCommand getVncPortCommandVm2;
 
+    @Mock
+    private VolumeVO rootDisk1;
+    @Mock
+    private VolumeVO rootDisk2;
+    @Mock
+    private VolumeVO dataDisk;
+
     @Mock
     private Connection sshConnection;
 
@@ -138,6 +151,10 @@ public class ResourceManagerImplTest {
     private static String vm2VncAddress = "10.2.2.2";
     private static int vm2VncPort = 5901;
 
+    private static long poolId = 1L;
+    private List<VolumeVO> rootDisks;
+    private List<VolumeVO> dataDisks;
+
     @Before
     public void setup() throws Exception {
         MockitoAnnotations.initMocks(this);
@@ -179,6 +196,11 @@ public void setup() throws Exception {
                 willReturn(new SSHCmdHelper.SSHCmdResult(0,"",""));
 
         when(configurationDao.getValue(ResourceManager.KvmSshToAgentEnabled.key())).thenReturn("true");
+
+        rootDisks = Arrays.asList(rootDisk1, rootDisk2);
+        dataDisks = Collections.singletonList(dataDisk);
+        when(volumeDao.findByPoolId(poolId)).thenReturn(rootDisks);
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(dataDisks);
     }
 
     @Test
@@ -527,4 +549,33 @@ private HostVO createDummyHost(Status hostStatus) {
         return new HostVO(1L, "host01", Host.Type.Routing, "192.168.1.1", "255.255.255.0", null, null, null, null, null, null, null, null, null, null, UUID.randomUUID().toString(),
                 hostStatus, "1.0", null, null, 1L, null, 0, 0, null, 0, null);
     }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesBothRootDisksAndDataDisks() {
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(rootDisks.size() + dataDisks.size()))
+                .updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesOnlyRootDisks() {
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(rootDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesOnlyDataDisks() {
+        when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, times(dataDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+    }
+
+    @Test
+    public void testDestroyLocalStoragePoolVolumesNoDisks() {
+        when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
+        resourceManager.destroyLocalStoragePoolVolumes(poolId);
+        verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
+    }
 }
