From c8cadcb56e553bdbbc141365061bd3542f43612e Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Mon, 26 Jan 2026 14:01:14 +0530 Subject: [PATCH 1/9] NPE fix while deleting storage pool when pool has detached volumes (#12451) * NPE fix while deleting storage pool when pool has detached volumes * review * unit tests * Added log for volumes not attached to any VMs * update filter, log and test * updated volume dao method names returning non destroyed volumes * build fix --------- Co-authored-by: dahn --- .../java/com/cloud/storage/dao/VolumeDao.java | 6 +- .../com/cloud/storage/dao/VolumeDaoImpl.java | 6 +- .../datastore/PrimaryDataStoreImpl.java | 2 +- .../cloudstack/sioc/SiocManagerImpl.java | 2 +- .../driver/DateraPrimaryDataStoreDriver.java | 2 +- .../provider/DateraHostListener.java | 4 +- .../SolidFirePrimaryDataStoreDriver.java | 2 +- .../provider/SolidFireHostListener.java | 4 +- .../StorPoolPrimaryDataStoreDriver.java | 2 +- .../cloud/resource/ResourceManagerImpl.java | 4 +- .../java/com/cloud/server/StatsCollector.java | 2 +- .../com/cloud/storage/StorageManagerImpl.java | 16 ++++-- .../storage/StoragePoolAutomationImpl.java | 2 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 2 +- .../resource/ResourceManagerImplTest.java | 12 ++-- .../cloud/storage/StorageManagerImplTest.java | 56 ++++++++++++++++++- 16 files changed, 90 insertions(+), 34 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java index 4936af3caab..83f02719518 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java @@ -48,7 +48,7 @@ public interface VolumeDao extends GenericDao, StateDao findIncludingRemovedByInstanceAndType(long id, Volume.Type vType); - List findByInstanceIdAndPoolId(long instanceId, long poolId); + List findNonDestroyedVolumesByInstanceIdAndPoolId(long instanceId, long poolId); List findByInstanceIdDestroyed(long vmId); @@ -70,11 +70,11 @@ public interface VolumeDao extends GenericDao, StateDao findCreatedByInstance(long id); - List findByPoolId(long poolId); + List findNonDestroyedVolumesByPoolId(long poolId); VolumeVO findByPoolIdName(long poolId, String name); - List findByPoolId(long poolId, Volume.Type volumeType); + List findNonDestroyedVolumesByPoolId(long poolId, Volume.Type volumeType); List findByPoolIdAndState(long poolid, Volume.State state); diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java index 5ef64b04664..a72b4a25845 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java @@ -135,7 +135,7 @@ public class VolumeDaoImpl extends GenericDaoBase implements Vol } @Override - public List findByPoolId(long poolId) { + public List findNonDestroyedVolumesByPoolId(long poolId) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("poolId", poolId); sc.setParameters("notDestroyed", Volume.State.Destroy, Volume.State.Expunged); @@ -144,7 +144,7 @@ public class VolumeDaoImpl extends GenericDaoBase implements Vol } @Override - public List findByInstanceIdAndPoolId(long instanceId, long poolId) { + public List findNonDestroyedVolumesByInstanceIdAndPoolId(long instanceId, long poolId) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("instanceId", instanceId); sc.setParameters("poolId", poolId); @@ -161,7 +161,7 @@ public class VolumeDaoImpl extends GenericDaoBase implements Vol } @Override - public List findByPoolId(long poolId, Volume.Type volumeType) { + public List findNonDestroyedVolumesByPoolId(long poolId, Volume.Type volumeType) { SearchCriteria sc = AllFieldsSearch.create(); sc.setParameters("poolId", poolId); sc.setParameters("notDestroyed", Volume.State.Destroy, Volume.State.Expunged); diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java index 6a10c26cc0b..d864bf8cd8c 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java @@ -126,7 +126,7 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore { @Override public List getVolumes() { - List volumes = volumeDao.findByPoolId(getId()); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(getId()); List volumeInfos = new ArrayList(); for (VolumeVO volume : volumes) { volumeInfos.add(VolumeObject.getVolumeObject(this, volume)); diff --git a/plugins/api/vmware-sioc/src/main/java/org/apache/cloudstack/sioc/SiocManagerImpl.java b/plugins/api/vmware-sioc/src/main/java/org/apache/cloudstack/sioc/SiocManagerImpl.java index e93b8df39e9..b01af35725f 100644 --- a/plugins/api/vmware-sioc/src/main/java/org/apache/cloudstack/sioc/SiocManagerImpl.java +++ b/plugins/api/vmware-sioc/src/main/java/org/apache/cloudstack/sioc/SiocManagerImpl.java @@ -123,7 +123,7 @@ public class SiocManagerImpl implements SiocManager { int limitIopsTotal = 0; - List volumes = volumeDao.findByPoolId(storagePoolId, null); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); if (volumes != null && volumes.size() > 0) { Set instanceIds = new HashSet<>(); diff --git a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java index dcf84525748..62393610499 100644 --- a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java +++ b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java @@ -563,7 +563,7 @@ public class DateraPrimaryDataStoreDriver implements PrimaryDataStoreDriver { private long getUsedBytes(StoragePool storagePool, long volumeIdToIgnore) { long usedSpaceBytes = 0; - List lstVolumes = _volumeDao.findByPoolId(storagePool.getId(), null); + List lstVolumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId(), null); if (lstVolumes != null) { for (VolumeVO volume : lstVolumes) { diff --git a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java index a0dc23da486..08bc89737f2 100644 --- a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java +++ b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/provider/DateraHostListener.java @@ -247,7 +247,7 @@ public class DateraHostListener implements HypervisorHostListener { List storagePaths = new ArrayList<>(); // If you do not pass in null for the second parameter, you only get back applicable ROOT disks. - List volumes = _volumeDao.findByPoolId(storagePoolId, null); + List volumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); if (volumes != null) { for (VolumeVO volume : volumes) { @@ -317,7 +317,7 @@ public class DateraHostListener implements HypervisorHostListener { StoragePoolVO storagePool = _storagePoolDao.findById(storagePoolId); // If you do not pass in null for the second parameter, you only get back applicable ROOT disks. - List volumes = _volumeDao.findByPoolId(storagePoolId, null); + List volumes = _volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); if (volumes != null) { for (VolumeVO volume : volumes) { diff --git a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java index 6cc76d99d9e..1e927e20168 100644 --- a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java +++ b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java @@ -433,7 +433,7 @@ public class SolidFirePrimaryDataStoreDriver implements PrimaryDataStoreDriver { public long getUsedIops(StoragePool storagePool) { long usedIops = 0; - List volumes = volumeDao.findByPoolId(storagePool.getId(), null); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId(), null); if (volumes != null) { for (VolumeVO volume : volumes) { diff --git a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java index 052191128f1..c961c926739 100644 --- a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java +++ b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/provider/SolidFireHostListener.java @@ -199,7 +199,7 @@ public class SolidFireHostListener implements HypervisorHostListener { List storagePaths = new ArrayList<>(); // If you do not pass in null for the second parameter, you only get back applicable ROOT disks. - List volumes = volumeDao.findByPoolId(storagePoolId, null); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); if (volumes != null) { for (VolumeVO volume : volumes) { @@ -230,7 +230,7 @@ public class SolidFireHostListener implements HypervisorHostListener { StoragePoolVO storagePool = storagePoolDao.findById(storagePoolId); // If you do not pass in null for the second parameter, you only get back applicable ROOT disks. - List volumes = volumeDao.findByPoolId(storagePoolId, null); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); if (volumes != null) { for (VolumeVO volume : volumes) { diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java index 6ca67cb5923..619beee3ec6 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java @@ -1276,7 +1276,7 @@ public class StorPoolPrimaryDataStoreDriver implements PrimaryDataStoreDriver { return volumeStats; } } else { - List volumes = volumeDao.findByPoolId(storagePool.getId()); + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId()); for (VolumeVO volume : volumes) { if (volume.getPath() != null && volume.getPath().equals(volumeId)) { long size = volume.getSize(); diff --git a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java index e62e89eb0ef..a77ecfcb7fe 100755 --- a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java +++ b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java @@ -1026,8 +1026,8 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager, } protected void destroyLocalStoragePoolVolumes(long poolId) { - List rootDisks = volumeDao.findByPoolId(poolId); - List dataVolumes = volumeDao.findByPoolId(poolId, Volume.Type.DATADISK); + List rootDisks = volumeDao.findNonDestroyedVolumesByPoolId(poolId); + List dataVolumes = volumeDao.findNonDestroyedVolumesByPoolId(poolId, Volume.Type.DATADISK); List volumes = new ArrayList<>(); addVolumesToList(volumes, rootDisks); diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 7e83d452bb9..1e0138f7cf9 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -1646,7 +1646,7 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc List pools = _storagePoolDao.listAll(); for (StoragePoolVO pool : pools) { - List volumes = _volsDao.findByPoolId(pool.getId(), null); + List 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())) { diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 8392c85527d..13b7fbb00c2 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -1558,17 +1558,21 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C protected String getStoragePoolNonDestroyedVolumesLog(long storagePoolId) { StringBuilder sb = new StringBuilder(); - List nonDestroyedVols = volumeDao.findByPoolId(storagePoolId, null); + List nonDestroyedVols = volumeDao.findNonDestroyedVolumesByPoolId(storagePoolId, null); VMInstanceVO volInstance; List 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 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C for (String childDatastoreUUID : childDatastoreUUIDs) { StoragePoolVO dataStoreVO = _storagePoolDao.findPoolByUUID(childDatastoreUUID); - List allVolumes = volumeDao.findByPoolId(dataStoreVO.getId()); + List allVolumes = volumeDao.findNonDestroyedVolumesByPoolId(dataStoreVO.getId()); allVolumes.removeIf(volumeVO -> volumeVO.getInstanceId() == null); allVolumes.removeIf(volumeVO -> volumeVO.getState() != Volume.State.Ready); for (VolumeVO volume : allVolumes) { diff --git a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java index 612582640f4..667af5a876f 100644 --- a/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java +++ b/server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java @@ -91,7 +91,7 @@ public class StoragePoolAutomationImpl implements StoragePoolAutomation { boolean restart = !CollectionUtils.isEmpty(upPools); // 2. Get a list of all the ROOT volumes within this storage pool - List allVolumes = volumeDao.findByPoolId(pool.getId()); + List allVolumes = volumeDao.findNonDestroyedVolumesByPoolId(pool.getId()); // 3. Enqueue to the work queue enqueueMigrationsForVolumes(allVolumes, pool); // 4. Process the queue diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index b00358caaa9..3e045f5a905 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2261,7 +2261,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir private List getVolumesByHost(HostVO host, StoragePool pool){ List 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()); } diff --git a/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java b/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java index 414d41145f7..5b7353bded6 100644 --- a/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java +++ b/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java @@ -198,8 +198,8 @@ public class ResourceManagerImplTest { 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 class ResourceManagerImplTest { @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)); } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 4a28e044d9c..5f02c89339a 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -531,7 +531,7 @@ public class StorageManagerImplTest { } @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(); @@ -539,7 +539,7 @@ public class StorageManagerImplTest { 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 class StorageManagerImplTest { 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); From 4adb7195701f2b28b86456f739ad59ec9f369abf Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Mon, 26 Jan 2026 04:18:12 -0500 Subject: [PATCH 2/9] Allow modification of user vm details if user.vm.readonly.details is empty (#10456) --- .../apache/cloudstack/query/QueryService.java | 2 +- .../framework/config/ConfigKey.java | 28 +++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 828f9d5e064..5181ebe2b76 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -118,7 +118,7 @@ public interface QueryService { ConfigKey UserVMReadOnlyDetails = new ConfigKey<>(String.class, "user.vm.readonly.details", "Advanced", "dataDiskController, rootDiskController", - "List of read-only VM settings/details as comma separated string", true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV, null); + "List of read-only VM settings/details as comma separated string", true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.CSV, null, ""); ConfigKey SortKeyAscending = new ConfigKey<>("Advanced", Boolean.class, "sortkey.algorithm", "true", "Sort algorithm - ascending or descending - to use. For entities that use sort key(template, disk offering, service offering, " + diff --git a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java index 00cf56345c8..27b04ddf893 100644 --- a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java +++ b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java @@ -120,10 +120,18 @@ public class ConfigKey { static ConfigDepotImpl s_depot = null; - static public void init(ConfigDepotImpl depot) { + private String _defaultValueIfEmpty = null; + + public static void init(ConfigDepotImpl depot) { s_depot = depot; } + public ConfigKey(Class type, String name, String category, String defaultValue, String description, boolean isDynamic, Scope scope, T multiplier, + String displayText, String parent, Ternary group, Pair subGroup, Kind kind, String options, String defaultValueIfEmpty) { + this(type, name, category, defaultValue, description, isDynamic, scope, multiplier, displayText, parent, group, subGroup, kind, options); + this._defaultValueIfEmpty = defaultValueIfEmpty; + } + public ConfigKey(String category, Class type, String name, String defaultValue, String description, boolean isDynamic, Scope scope) { this(type, name, category, defaultValue, description, isDynamic, scope, null); } @@ -216,7 +224,19 @@ public class ConfigKey { public T value() { if (_value == null || isDynamic()) { String value = s_depot != null ? s_depot.getConfigStringValue(_name, Scope.Global, null) : null; - _value = valueOf((value == null) ? defaultValue() : value); + + String effective; + if (value != null) { + if (value.isEmpty() && _defaultValueIfEmpty != null) { + effective = _defaultValueIfEmpty; + } else { + effective = value; + } + } else { + effective = _defaultValueIfEmpty != null ? _defaultValueIfEmpty : defaultValue(); + } + + _value = valueOf(effective); } return _value; @@ -231,6 +251,10 @@ public class ConfigKey { if (value == null) { return value(); } + + if (value.isEmpty() && _defaultValueIfEmpty != null) { + return valueOf(_defaultValueIfEmpty); + } return valueOf(value); } From 0958dfc13864315e83ae906bf4f90328ccd1557c Mon Sep 17 00:00:00 2001 From: Artem Sidorenko Date: Mon, 26 Jan 2026 10:21:47 +0100 Subject: [PATCH 3/9] Fix: proper permissions for systemvm template registrations on hardened systems (#12098) Related to https://github.com/apache/cloudstack/issues/10029#issuecomment-2531599607 We have umask 0077, so cloud-install-sys-tmplt is creating by default paths like below ``` $ ls -l /mnt/secondary/template/tmpl/ total 16 drwx------. 3 root root 4096 Nov 19 13:58 1 drwxrwxrwx. 7 root root 4096 Oct 31 09:42 2 drwxrwxrwx. 3 root root 4096 Oct 30 15:59 4 drwxr-xr-x. 2 root root 4096 Oct 31 10:21 5 $ ls -l /mnt/secondary/template/tmpl/1/ total 4 drwx------. 2 root root 4096 Nov 19 13:59 3 $ ls -l /mnt/secondary/template/tmpl/1/3/ total 549848 -rw-------. 1 root root 563032576 Nov 19 13:59 d23a1e19-c563-4f69-85ca-8721cf02082c.qcow2 -rw-------. 1 root root 287 Nov 19 13:59 template.properties ``` This results to the permissions problems later on, when trying to access the image Signed-off-by: Artem Sidorenko --- scripts/storage/secondary/cloud-install-sys-tmplt | 1 + scripts/storage/secondary/setup-sysvm-tmplt | 1 + 2 files changed, 2 insertions(+) diff --git a/scripts/storage/secondary/cloud-install-sys-tmplt b/scripts/storage/secondary/cloud-install-sys-tmplt index ad976c502c6..fc09dc968ff 100755 --- a/scripts/storage/secondary/cloud-install-sys-tmplt +++ b/scripts/storage/secondary/cloud-install-sys-tmplt @@ -44,6 +44,7 @@ failed() { } #set -x +umask 0022 # ensure we have the proper permissions even on hardened deployments mflag= fflag= ext="vhd" diff --git a/scripts/storage/secondary/setup-sysvm-tmplt b/scripts/storage/secondary/setup-sysvm-tmplt index 06f0586fe34..63006cc4e4c 100755 --- a/scripts/storage/secondary/setup-sysvm-tmplt +++ b/scripts/storage/secondary/setup-sysvm-tmplt @@ -19,6 +19,7 @@ # Usage: e.g. failed $? "this is an error" set -x +umask 0022 # ensure we have the proper permissions even on hardened deployments failed() { local returnval=$1 From d010e9fcf29822a312618eee5d7a4c8f0eb69d2e Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Mon, 26 Jan 2026 15:03:30 +0530 Subject: [PATCH 4/9] Notify user if template upgrade is not required (#12483) --- .../cloud/network/router/VirtualNetworkApplianceManagerImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java index e171b68399b..7d0a4f20838 100644 --- a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java +++ b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java @@ -3358,6 +3358,7 @@ Configurable, StateListener Date: Mon, 26 Jan 2026 16:25:55 +0530 Subject: [PATCH 5/9] Add log for null templateVO (#12406) --- .../cloudstack/storage/image/TemplateDataFactoryImpl.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java index c6430bcf9f9..3e1504beb3a 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java @@ -296,6 +296,9 @@ public class TemplateDataFactoryImpl implements TemplateDataFactory { @Override public boolean isTemplateMarkedForDirectDownload(long templateId) { VMTemplateVO templateVO = imageDataDao.findById(templateId); + if (templateVO == null) { + throw new CloudRuntimeException(String.format("Template not found with ID: %s", templateId)); + } return templateVO.isDirectDownload(); } } From 097c3a018bae6faf6cdac2db09c56507b980fa4f Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Mon, 26 Jan 2026 11:56:14 +0100 Subject: [PATCH 6/9] ConfigDrive: use file absolute path instead of canonical path to create ISO (#11623) * ConfigDrive: use file absolute path instead of canonical path to create ISO * el8: add xorrisofs as option --- .../storage/configdrive/ConfigDriveBuilder.java | 4 ++-- .../storage/configdrive/ConfigDriveBuilderTest.java | 12 ++++++------ packaging/el8/cloud.spec | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java b/engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java index 0b81a25b1cd..15febbe972c 100644 --- a/engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java +++ b/engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java @@ -231,9 +231,9 @@ public class ConfigDriveBuilder { throw new CloudRuntimeException("Cannot create ISO for config drive using any know tool. Known paths [/usr/bin/genisoimage, /usr/bin/mkisofs, /usr/local/bin/mkisofs]"); } if (!isoCreator.canExecute()) { - throw new CloudRuntimeException("Cannot create ISO for config drive using: " + isoCreator.getCanonicalPath()); + throw new CloudRuntimeException("Cannot create ISO for config drive using: " + isoCreator.getAbsolutePath()); } - return isoCreator.getCanonicalPath(); + return isoCreator.getAbsolutePath(); } /** diff --git a/engine/storage/configdrive/src/test/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilderTest.java b/engine/storage/configdrive/src/test/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilderTest.java index c04ff0a1601..03ceac84399 100644 --- a/engine/storage/configdrive/src/test/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilderTest.java +++ b/engine/storage/configdrive/src/test/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilderTest.java @@ -435,7 +435,7 @@ public class ConfigDriveBuilderTest { Mockito.verify(genIsoFileMock, Mockito.times(2)).exists(); Mockito.verify(genIsoFileMock).canExecute(); - Mockito.verify(genIsoFileMock).getCanonicalPath(); + Mockito.verify(genIsoFileMock).getAbsolutePath(); } } @@ -475,11 +475,11 @@ public class ConfigDriveBuilderTest { Mockito.verify(genIsoFileMock, Mockito.times(1)).exists(); Mockito.verify(genIsoFileMock, Mockito.times(0)).canExecute(); - Mockito.verify(genIsoFileMock, Mockito.times(0)).getCanonicalPath(); + Mockito.verify(genIsoFileMock, Mockito.times(0)).getAbsolutePath(); Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(2)).exists(); Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(1)).canExecute(); - Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(1)).getCanonicalPath(); + Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(1)).getAbsolutePath(); } } @@ -509,15 +509,15 @@ public class ConfigDriveBuilderTest { Mockito.verify(genIsoFileMock, Mockito.times(1)).exists(); Mockito.verify(genIsoFileMock, Mockito.times(0)).canExecute(); - Mockito.verify(genIsoFileMock, Mockito.times(0)).getCanonicalPath(); + Mockito.verify(genIsoFileMock, Mockito.times(0)).getAbsolutePath(); Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(1)).exists(); Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(0)).canExecute(); - Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(0)).getCanonicalPath(); + Mockito.verify(mkIsoProgramInLinuxFileMock, Mockito.times(0)).getAbsolutePath(); Mockito.verify(mkIsoProgramInMacOsFileMock, Mockito.times(1)).exists(); Mockito.verify(mkIsoProgramInMacOsFileMock, Mockito.times(1)).canExecute(); - Mockito.verify(mkIsoProgramInMacOsFileMock, Mockito.times(1)).getCanonicalPath(); + Mockito.verify(mkIsoProgramInMacOsFileMock, Mockito.times(1)).getAbsolutePath(); } } diff --git a/packaging/el8/cloud.spec b/packaging/el8/cloud.spec index 3d485112266..507a6e64173 100644 --- a/packaging/el8/cloud.spec +++ b/packaging/el8/cloud.spec @@ -76,7 +76,7 @@ Requires: sudo Requires: /sbin/service Requires: /sbin/chkconfig Requires: /usr/bin/ssh-keygen -Requires: (genisoimage or mkisofs) +Requires: (genisoimage or mkisofs or xorrisofs) Requires: ipmitool Requires: %{name}-common = %{_ver} Requires: (iptables-services or iptables) From 36edd92e480d8b738335c6500b2e90c4d3f91fb9 Mon Sep 17 00:00:00 2001 From: Henrique Sato Date: Mon, 26 Jan 2026 07:58:42 -0300 Subject: [PATCH 7/9] Fix snapshot physical size after migration (#12166) --- .../cloudstack/storage/image/SecondaryStorageServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java index 641a2a40dcd..f739fecf9bf 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java @@ -280,7 +280,7 @@ public class SecondaryStorageServiceImpl implements SecondaryStorageService { private void updateDataObject(DataObject srcData, DataObject destData) { if (destData instanceof SnapshotInfo) { SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySourceSnapshot(srcData.getId(), DataStoreRole.Image); - SnapshotDataStoreVO destSnapshotStore = snapshotStoreDao.findByStoreSnapshot(DataStoreRole.Image, srcData.getDataStore().getId(), srcData.getId()); + SnapshotDataStoreVO destSnapshotStore = snapshotStoreDao.findByStoreSnapshot(DataStoreRole.Image, destData.getDataStore().getId(), destData.getId()); if (snapshotStore != null && destSnapshotStore != null) { destSnapshotStore.setPhysicalSize(snapshotStore.getPhysicalSize()); destSnapshotStore.setCreated(snapshotStore.getCreated()); From 44793da58f29e534562b757bbf905071099507c3 Mon Sep 17 00:00:00 2001 From: Edward-x <30854794+YLChen-007@users.noreply.github.com> Date: Mon, 26 Jan 2026 19:22:22 +0800 Subject: [PATCH 8/9] =?UTF-8?q?fix=20Sensitive=20Data=20Exposure=20Through?= =?UTF-8?q?=20Exception=20Logging=20in=20OVM=20Hypervis=E2=80=A6=20(#12032?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix Sensitive Data Exposure Through Exception Logging in OVM Hypervisor Configuration * extra ‘)’ in log. Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> * remove non-descriptive part Co-authored-by: Suresh Kumar Anaparti --------- Co-authored-by: chenyoulong20g@ict.ac.cn Co-authored-by: dahn Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Co-authored-by: Suresh Kumar Anaparti --- .../src/main/java/com/cloud/ovm/hypervisor/OvmResourceBase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hypervisors/ovm/src/main/java/com/cloud/ovm/hypervisor/OvmResourceBase.java b/plugins/hypervisors/ovm/src/main/java/com/cloud/ovm/hypervisor/OvmResourceBase.java index 9d958a9894a..a65e4d778d3 100644 --- a/plugins/hypervisors/ovm/src/main/java/com/cloud/ovm/hypervisor/OvmResourceBase.java +++ b/plugins/hypervisors/ovm/src/main/java/com/cloud/ovm/hypervisor/OvmResourceBase.java @@ -362,7 +362,7 @@ public class OvmResourceBase implements ServerResource, HypervisorResource { sshConnection = SSHCmdHelper.acquireAuthorizedConnection(_ip, _username, _password); if (sshConnection == null) { - throw new CloudRuntimeException(String.format("Cannot connect to ovm host(IP=%1$s, username=%2$s, password=%3$s", _ip, _username, _password)); + throw new CloudRuntimeException(String.format("Cannot connect to ovm host(IP=%1$s, username=%2$s)", _ip, _username)); } if (!SSHCmdHelper.sshExecuteCmd(sshConnection, "sh /usr/bin/configureOvm.sh postSetup")) { From bbc23a74683052228d254ba0e4958f4c19254f90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Mon, 26 Jan 2026 09:14:40 -0300 Subject: [PATCH 9/9] fix install path for systemvm templates when introducing new sec storage (#11605) --- .../cloudstack/storage/image/TemplateServiceImpl.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index 1bb954da410..c18be7c7335 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -1318,9 +1318,10 @@ public class TemplateServiceImpl implements TemplateService { if (_vmTemplateStoreDao.isTemplateMarkedForDirectDownload(tmplt.getId())) { continue; } - tmpltStore = - new TemplateDataStoreVO(storeId, tmplt.getId(), new Date(), 100, Status.DOWNLOADED, null, null, null, - TemplateConstants.DEFAULT_SYSTEM_VM_TEMPLATE_PATH + tmplt.getId() + '/', tmplt.getUrl()); + String templateDirectoryPath = TemplateConstants.DEFAULT_TMPLT_ROOT_DIR + File.separator + TemplateConstants.DEFAULT_TMPLT_FIRST_LEVEL_DIR; + String installPath = templateDirectoryPath + tmplt.getAccountId() + File.separator + tmplt.getId() + File.separator; + tmpltStore = new TemplateDataStoreVO(storeId, tmplt.getId(), new Date(), 100, Status.DOWNLOADED, + null, null, null, installPath, tmplt.getUrl()); tmpltStore.setSize(0L); tmpltStore.setPhysicalSize(0); // no size information for // pre-seeded system vm templates