NPE fix while deleting storage pool when pool has detached volumes

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java

@@ -48,7 +48,7 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
 
     List<VolumeVO> findIncludingRemovedByInstanceAndType(long id, Volume.Type vType);
 
-    List<VolumeVO> findByInstanceIdAndPoolId(long instanceId, long poolId);
+    List<VolumeVO> findNonDestroyedVolumesByInstanceIdAndPoolId(long instanceId, long poolId);
 
     List<VolumeVO> findByInstanceIdDestroyed(long vmId);
 
@@ -70,11 +70,11 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
 
     List<VolumeVO> findCreatedByInstance(long id);
 
-    List<VolumeVO> findByPoolId(long poolId);
+    List<VolumeVO> findNonDestroyedVolumesByPoolId(long poolId);
 
     VolumeVO findByPoolIdName(long poolId, String name);
 
-    List<VolumeVO> findByPoolId(long poolId, Volume.Type volumeType);
+    List<VolumeVO> findNonDestroyedVolumesByPoolId(long poolId, Volume.Type volumeType);
 
     List<VolumeVO> findByPoolIdAndState(long poolid, Volume.State state);
 

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java

@@ -135,7 +135,7 @@ public List<VolumeVO> findByInstanceAndDeviceId(long instanceId, long deviceId)
     }
 
     @Override
-    public List<VolumeVO> findByPoolId(long poolId) {
+    public List<VolumeVO> findNonDestroyedVolumesByPoolId(long poolId) {
         SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
         sc.setParameters("poolId", poolId);
         sc.setParameters("notDestroyed", Volume.State.Destroy, Volume.State.Expunged);
@@ -144,7 +144,7 @@ public List<VolumeVO> findByPoolId(long poolId) {
     }
 
     @Override
-    public List<VolumeVO> findByInstanceIdAndPoolId(long instanceId, long poolId) {
+    public List<VolumeVO> findNonDestroyedVolumesByInstanceIdAndPoolId(long instanceId, long poolId) {
         SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
         sc.setParameters("instanceId", instanceId);
         sc.setParameters("poolId", poolId);
@@ -161,7 +161,7 @@ public VolumeVO findByPoolIdName(long poolId, String name) {
     }
 
     @Override
-    public List<VolumeVO> findByPoolId(long poolId, Volume.Type volumeType) {
+    public List<VolumeVO> findNonDestroyedVolumesByPoolId(long poolId, Volume.Type volumeType) {
         SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
         sc.setParameters("poolId", poolId);
         sc.setParameters("notDestroyed", Volume.State.Destroy, Volume.State.Expunged);

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java

@@ -126,7 +126,7 @@ public VolumeInfo getVolume(long id) {
 
     @Override
     public List<VolumeInfo> getVolumes() {
-        List<VolumeVO> volumes = volumeDao.findByPoolId(getId());
+        List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(getId());
         List<VolumeInfo> volumeInfos = new ArrayList<VolumeInfo>();
         for (VolumeVO volume : volumes) {
             volumeInfos.add(VolumeObject.getVolumeObject(this, volume));

plugins/api/vmware-sioc/src/main/java/org/apache/cloudstack/sioc/SiocManagerImpl.java

@@ -123,7 +123,7 @@ public void updateSiocInfo(long zoneId, long storagePoolId, int sharesPerGB, int
 
             int limitIopsTotal = 0;
 
-            List<VolumeVO> volumes = volumeDao.findByPoolId(storagePoolId, null);
+            List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
 
             if (volumes != null && volumes.size() > 0) {
                 Set<Long> instanceIds = new HashSet<>();

plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java

@@ -563,7 +563,7 @@ public long getUsedBytes(StoragePool storagePool) {
     private long getUsedBytes(StoragePool storagePool, long volumeIdToIgnore) {
         long usedSpaceBytes = 0;
 
-        List<VolumeVO> lstVolumes = _volumeDao.findByPoolId(storagePool.getId(), null);
+        List<VolumeVO> lstVolumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId(), null);
 
         if (lstVolumes != null) {
             for (VolumeVO volume : lstVolumes) {

plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java

@@ -247,7 +247,7 @@ private List<String> getStoragePaths(long clusterId, long storagePoolId) {
         List<String> storagePaths = new ArrayList<>();
 
         // If you do not pass in null for the second parameter, you only get back applicable ROOT disks.
-        List<VolumeVO> volumes = _volumeDao.findByPoolId(storagePoolId, null);
+        List<VolumeVO> volumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
 
         if (volumes != null) {
             for (VolumeVO volume : volumes) {
@@ -317,7 +317,7 @@ private List<Map<String, String>> getTargets(long clusterId, long storagePoolId)
         StoragePoolVO storagePool = _storagePoolDao.findById(storagePoolId);
 
         // If you do not pass in null for the second parameter, you only get back applicable ROOT disks.
-        List<VolumeVO> volumes = _volumeDao.findByPoolId(storagePoolId, null);
+        List<VolumeVO> volumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
 
         if (volumes != null) {
             for (VolumeVO volume : volumes) {

plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java

@@ -433,7 +433,7 @@ private long getUsedBytes(StoragePool storagePool, long volumeIdToIgnore) {
     public long getUsedIops(StoragePool storagePool) {
         long usedIops = 0;
 
-        List<VolumeVO> volumes = volumeDao.findByPoolId(storagePool.getId(), null);
+        List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId(), null);
 
         if (volumes != null) {
             for (VolumeVO volume : volumes) {

plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java

@@ -199,7 +199,7 @@ private List<String> getStoragePaths(long clusterId, long storagePoolId) {
         List<String> storagePaths = new ArrayList<>();
 
         // If you do not pass in null for the second parameter, you only get back applicable ROOT disks.
-        List<VolumeVO> volumes = volumeDao.findByPoolId(storagePoolId, null);
+        List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
 
         if (volumes != null) {
             for (VolumeVO volume : volumes) {
@@ -230,7 +230,7 @@ private List<Map<String, String>> getTargets(long clusterId, long storagePoolId)
         StoragePoolVO storagePool = storagePoolDao.findById(storagePoolId);
 
         // If you do not pass in null for the second parameter, you only get back applicable ROOT disks.
-        List<VolumeVO> volumes = volumeDao.findByPoolId(storagePoolId, null);
+        List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
 
         if (volumes != null) {
             for (VolumeVO volume : volumes) {

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java

@@ -1276,7 +1276,7 @@ public Pair<Long, Long> getVolumeStats(StoragePool storagePool, String volumeId)
                 return volumeStats;
             }
         } else {
-            List<VolumeVO> volumes = volumeDao.findByPoolId(storagePool.getId());
+            List<VolumeVO> volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId());
             for (VolumeVO volume : volumes) {
                 if (volume.getPath() != null && volume.getPath().equals(volumeId)) {
                     long size = volume.getSize();

server/src/main/java/com/cloud/resource/ResourceManagerImpl.java

@@ -1026,8 +1026,8 @@ private void addVolumesToList(List<VolumeVO> volumes, List<VolumeVO> volumesToAd
     }
 
     protected void destroyLocalStoragePoolVolumes(long poolId) {
-        List<VolumeVO> rootDisks = volumeDao.findByPoolId(poolId);
-        List<VolumeVO> dataVolumes = volumeDao.findByPoolId(poolId, Volume.Type.DATADISK);
+        List<VolumeVO> rootDisks = volumeDao.findNonDestroyedVolumesByPoolId(poolId);
+        List<VolumeVO> dataVolumes = volumeDao.findNonDestroyedVolumesByPoolId(poolId, Volume.Type.DATADISK);
 
         List<VolumeVO> volumes = new ArrayList<>();
         addVolumesToList(volumes, rootDisks);

server/src/main/java/com/cloud/server/StatsCollector.java

@@ -1646,7 +1646,7 @@ protected void runInContext() {
                 List<StoragePoolVO> pools = _storagePoolDao.listAll();
 
                 for (StoragePoolVO pool : pools) {
-                    List<VolumeVO> volumes = _volsDao.findByPoolId(pool.getId(), null);
+                    List<VolumeVO> volumes = _volsDao.findNonDestroyedVolumesByPoolId(pool.getId(), null);
                     for (VolumeVO volume : volumes) {
                         if (!List.of(ImageFormat.QCOW2, ImageFormat.VHD, ImageFormat.OVA, ImageFormat.RAW).contains(volume.getFormat()) &&
                             !List.of(Storage.StoragePoolType.PowerFlex, Storage.StoragePoolType.FiberChannel).contains(pool.getPoolType())) {

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -1558,17 +1558,21 @@ private boolean deleteDataStoreInternal(StoragePoolVO sPool, boolean forced) {
 
     protected String getStoragePoolNonDestroyedVolumesLog(long storagePoolId) {
         StringBuilder sb = new StringBuilder();
-        List<VolumeVO> nonDestroyedVols = volumeDao.findByPoolId(storagePoolId, null);
+        List<VolumeVO> nonDestroyedVols = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null);
         VMInstanceVO volInstance;
         List<String> logMessageInfo = new ArrayList<>();
 
         sb.append("[");
         for (VolumeVO vol : nonDestroyedVols) {
-            volInstance = _vmInstanceDao.findById(vol.getInstanceId());
-            if (volInstance != null) {
-                logMessageInfo.add(String.format("Volume [%s] (attached to VM [%s])", vol.getUuid(), volInstance.getUuid()));
+            if (vol.getInstanceId() != null) {
+                volInstance = _vmInstanceDao.findById(vol.getInstanceId());
+                if (volInstance != null) {
+                    logMessageInfo.add(String.format("Volume [%s] (attached to VM [%s])", vol.getUuid(), volInstance.getUuid()));
+                } else {
+                    logMessageInfo.add(String.format("Volume [%s] (attached VM with ID [%d] doesn't exists)", vol.getUuid(), vol.getInstanceId()));
+                }
             } else {
-                logMessageInfo.add(String.format("Volume [%s]", vol.getUuid()));
+                logMessageInfo.add(String.format("Volume [%s] (not attached to any VM)", vol.getUuid()));
             }
         }
         sb.append(String.join(", ", logMessageInfo));
@@ -2640,7 +2644,7 @@ private void handleRemoveChildStoragePoolFromDatastoreCluster(Set<String> childD
 
         for (String childDatastoreUUID : childDatastoreUUIDs) {
             StoragePoolVO dataStoreVO = _storagePoolDao.findPoolByUUID(childDatastoreUUID);
-            List<VolumeVO> allVolumes = volumeDao.findByPoolId(dataStoreVO.getId());
+            List<VolumeVO> allVolumes = volumeDao.findNonDestroyedVolumesByPoolId(dataStoreVO.getId());
             allVolumes.removeIf(volumeVO -> volumeVO.getInstanceId() == null);
             allVolumes.removeIf(volumeVO -> volumeVO.getState() != Volume.State.Ready);
             for (VolumeVO volume : allVolumes) {

server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java

@@ -91,7 +91,7 @@ public boolean maintain(DataStore store) {
                 boolean restart = !CollectionUtils.isEmpty(upPools);
 
                 // 2. Get a list of all the ROOT volumes within this storage pool
-                List<VolumeVO> allVolumes = volumeDao.findByPoolId(pool.getId());
+                List<VolumeVO> allVolumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId());
                 // 3. Enqueue to the work queue
                 enqueueMigrationsForVolumes(allVolumes, pool);
                 // 4. Process the queue

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -2261,7 +2261,7 @@ public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, Str
     private List<String> getVolumesByHost(HostVO host, StoragePool pool){
         List<VMInstanceVO> vmsPerHost = _vmInstanceDao.listByHostId(host.getId());
         return vmsPerHost.stream()
-                .flatMap(vm -> _volsDao.findByInstanceIdAndPoolId(vm.getId(),pool.getId()).stream().map(vol ->
+                .flatMap(vm -> _volsDao.findNonDestroyedVolumesByInstanceIdAndPoolId(vm.getId(),pool.getId()).stream().map(vol ->
                 vol.getState() == Volume.State.Ready ? (vol.getFormat() == ImageFormat.OVA ? vol.getChainInfo() : vol.getPath()) : null).filter(Objects::nonNull))
                 .collect(Collectors.toList());
     }

server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java

@@ -198,8 +198,8 @@ public void setup() throws Exception {
 
         rootDisks = Arrays.asList(rootDisk1, rootDisk2);
         dataDisks = Collections.singletonList(dataDisk);
-        when(volumeDao.findByPoolId(poolId)).thenReturn(rootDisks);
-        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(dataDisks);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(poolId)).thenReturn(rootDisks);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(dataDisks);
     }
 
     @After
@@ -564,22 +564,22 @@ public void testDestroyLocalStoragePoolVolumesBothRootDisksAndDataDisks() {
 
     @Test
     public void testDestroyLocalStoragePoolVolumesOnlyRootDisks() {
-        when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(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);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(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);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(poolId)).thenReturn(null);
+        when(volumeDao.findNonDestroyedVolumesByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
         resourceManager.destroyLocalStoragePoolVolumes(poolId);
         verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
     }

server/src/test/java/com/cloud/storage/StorageManagerImplTest.java

@@ -531,15 +531,15 @@ public void testEnableDefaultDatastoreDownloadRedirectionForExistingInstallation
     }
 
     @Test
-    public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumesReturnLog() {
+    public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumes_VMAttachedLogs() {
         Mockito.doReturn(1L).when(storagePoolVOMock).getId();
         Mockito.doReturn(1L).when(volume1VOMock).getInstanceId();
         Mockito.doReturn("786633d1-a942-4374-9d56-322dd4b0d202").when(volume1VOMock).getUuid();
         Mockito.doReturn(1L).when(volume2VOMock).getInstanceId();
         Mockito.doReturn("ffb46333-e983-4c21-b5f0-51c5877a3805").when(volume2VOMock).getUuid();
         Mockito.doReturn("58760044-928f-4c4e-9fef-d0e48423595e").when(vmInstanceVOMock).getUuid();
 
-        Mockito.when(_volumeDao.findByPoolId(storagePoolVOMock.getId(), null)).thenReturn(List.of(volume1VOMock, volume2VOMock));
+        Mockito.when(_volumeDao.findNonDestroyedVolumesByPoolId(storagePoolVOMock.getId(), null)).thenReturn(List.of(volume1VOMock, volume2VOMock));
         Mockito.doReturn(vmInstanceVOMock).when(vmInstanceDao).findById(Mockito.anyLong());
 
         String log = storageManagerImpl.getStoragePoolNonDestroyedVolumesLog(storagePoolVOMock.getId());
@@ -548,6 +548,58 @@ public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumesReturnLog
         Assert.assertEquals(expected, log);
     }
 
+    @Test
+    public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumes_VMLogForOneVolume() {
+        Mockito.doReturn(1L).when(storagePoolVOMock).getId();
+        Mockito.doReturn(null).when(volume1VOMock).getInstanceId();
+        Mockito.doReturn("786633d1-a942-4374-9d56-322dd4b0d202").when(volume1VOMock).getUuid();
+        Mockito.doReturn(1L).when(volume2VOMock).getInstanceId();
+        Mockito.doReturn("ffb46333-e983-4c21-b5f0-51c5877a3805").when(volume2VOMock).getUuid();
+        Mockito.doReturn("58760044-928f-4c4e-9fef-d0e48423595e").when(vmInstanceVOMock).getUuid();
+
+        Mockito.when(_volumeDao.findNonDestroyedVolumesByPoolId(storagePoolVOMock.getId(), null)).thenReturn(List.of(volume1VOMock, volume2VOMock));
+        Mockito.doReturn(vmInstanceVOMock).when(vmInstanceDao).findById(Mockito.anyLong());
+
+        String log = storageManagerImpl.getStoragePoolNonDestroyedVolumesLog(storagePoolVOMock.getId());
+        String expected = String.format("[Volume [%s] (not attached to any VM), Volume [%s] (attached to VM [%s])]", volume1VOMock.getUuid(), volume2VOMock.getUuid(), vmInstanceVOMock.getUuid());
+
+        Assert.assertEquals(expected, log);
+    }
+
+    @Test
+    public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumes_NotAttachedLogs() {
+        Mockito.doReturn(1L).when(storagePoolVOMock).getId();
+        Mockito.doReturn(null).when(volume1VOMock).getInstanceId();
+        Mockito.doReturn("786633d1-a942-4374-9d56-322dd4b0d202").when(volume1VOMock).getUuid();
+        Mockito.doReturn(null).when(volume2VOMock).getInstanceId();
+        Mockito.doReturn("ffb46333-e983-4c21-b5f0-51c5877a3805").when(volume2VOMock).getUuid();
+
+        Mockito.when(_volumeDao.findNonDestroyedVolumesByPoolId(storagePoolVOMock.getId(), null)).thenReturn(List.of(volume1VOMock, volume2VOMock));
+
+        String log = storageManagerImpl.getStoragePoolNonDestroyedVolumesLog(storagePoolVOMock.getId());
+        String expected = String.format("[Volume [%s] (not attached to any VM), Volume [%s] (not attached to any VM)]", volume1VOMock.getUuid(), volume2VOMock.getUuid());
+
+        Assert.assertEquals(expected, log);
+    }
+
+    @Test
+    public void getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumes_VMNotExistsLog() {
+        Mockito.doReturn(1L).when(storagePoolVOMock).getId();
+        Mockito.doReturn(1L).when(volume1VOMock).getInstanceId();
+        Mockito.doReturn("786633d1-a942-4374-9d56-322dd4b0d202").when(volume1VOMock).getUuid();
+        Mockito.doReturn(1L).when(volume2VOMock).getInstanceId();
+        Mockito.doReturn("ffb46333-e983-4c21-b5f0-51c5877a3805").when(volume2VOMock).getUuid();
+
+        Mockito.when(_volumeDao.findNonDestroyedVolumesByPoolId(storagePoolVOMock.getId(), null)).thenReturn(List.of(volume1VOMock, volume2VOMock));
+        Mockito.doReturn(null).when(vmInstanceDao).findById(Mockito.anyLong());
+
+        String log = storageManagerImpl.getStoragePoolNonDestroyedVolumesLog(storagePoolVOMock.getId());
+        String expected = String.format("[Volume [%s] (attached VM with ID [%d] doesn't exists), Volume [%s] (attached VM with ID [%d] doesn't exists)]",
+                volume1VOMock.getUuid(), volume1VOMock.getInstanceId(), volume2VOMock.getUuid(), volume2VOMock.getInstanceId());
+
+        Assert.assertEquals(expected, log);
+    }
+
     private ChangeStoragePoolScopeCmd mockChangeStoragePooolScopeCmd(String newScope) {
         ChangeStoragePoolScopeCmd cmd = new ChangeStoragePoolScopeCmd();
         ReflectionTestUtils.setField(cmd, "id", 1L);