diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 344c8e4c9d36..861b85bd3d4a 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -1929,13 +1929,29 @@ protected void runInContext() { continue; } List volumes = _volsDao.findNonDestroyedVolumesByPoolId(pool.getId(), null); + if (CollectionUtils.isEmpty(volumes)) { + continue; + } + boolean unsupportedVolumeFormat = false; 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())) { - logger.warn("Volume stats not implemented for this format type " + volume.getFormat()); + ImageFormat volumeFormat = volume.getFormat(); + Storage.StoragePoolType poolType = pool.getPoolType(); + boolean poolTypeSupportsVolumeStats = poolType != null && List.of(Storage.StoragePoolType.PowerFlex, Storage.StoragePoolType.FiberChannel).contains(poolType); + boolean volumeFormatSupportsVolumeStats = volumeFormat != null && List.of(ImageFormat.QCOW2, ImageFormat.VHD, ImageFormat.OVA, ImageFormat.RAW).contains(volumeFormat); + if (volumeFormat == null && !poolTypeSupportsVolumeStats) { + logger.debug("Skipping volume stats for volume {} on storage pool {} as volume format is unavailable", volume.getUuid(), pool.getUuid()); + unsupportedVolumeFormat = true; + break; + } + if (!volumeFormatSupportsVolumeStats && !poolTypeSupportsVolumeStats) { + logger.warn("Volume stats not implemented for this format type " + volumeFormat); + unsupportedVolumeFormat = true; break; } } + if (unsupportedVolumeFormat) { + continue; + } try { Map volumeStatsByUuid; if (pool.getScope() == ScopeType.ZONE) { @@ -1947,7 +1963,12 @@ protected void runInContext() { } } } else { - volumeStatsByUuid = _userVmMgr.getVolumeStatistics(pool.getClusterId(), pool.getUuid(), pool.getPoolType(), StatsTimeout.value()); + Long clusterId = pool.getClusterId(); + if (clusterId == null) { + logger.debug("Skipping volume stats for storage pool {} as scope {} has no cluster id", pool.getUuid(), pool.getScope()); + continue; + } + volumeStatsByUuid = _userVmMgr.getVolumeStatistics(clusterId, pool.getUuid(), pool.getPoolType(), StatsTimeout.value()); } if (volumeStatsByUuid != null) { for (final Map.Entry entry : volumeStatsByUuid.entrySet()) { @@ -2486,4 +2507,4 @@ public ConfigKey[] getConfigKeys() { public double getImageStoreCapacityThreshold() { return CapacityManager.SecondaryStorageCapacityThreshold.value(); } -} \ No newline at end of file +} diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index ffc343d0e45f..b3c1b03d24d7 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2431,8 +2431,16 @@ private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOf @Override public HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, int timeout) { - List neighbors = _resourceMgr.listHostsInClusterByStatus(clusterId, Status.Up); StoragePoolVO storagePool = _storagePoolDao.findPoolByUUID(poolUuid); + if (storagePool == null) { + logger.debug("Unable to collect volume stats as storage pool {} was not found", poolUuid); + return null; + } + List neighbors = _resourceMgr.listHostsInClusterByStatus(clusterId, Status.Up); + if (CollectionUtils.isEmpty(neighbors)) { + logger.debug("Unable to collect volume stats for storage pool {} as cluster {} has no Up hosts", poolUuid, clusterId); + return null; + } HashMap volumeStatsByUuid = new HashMap<>(); for (HostVO neighbor : neighbors) { @@ -2451,7 +2459,7 @@ public HashMap getVolumeStatistics(long clusterId, Str DataStoreProvider storeProvider = _dataStoreProviderMgr .getDataStoreProvider(storagePool.getStorageProviderName()); - DataStoreDriver storeDriver = storeProvider.getDataStoreDriver(); + DataStoreDriver storeDriver = storeProvider != null ? storeProvider.getDataStoreDriver() : null; if (storeDriver instanceof PrimaryDataStoreDriver && ((PrimaryDataStoreDriver) storeDriver).canProvideVolumeStats()) { // Get volume stats from the pool directly instead of sending cmd to host diff --git a/server/src/test/java/com/cloud/server/StatsCollectorTest.java b/server/src/test/java/com/cloud/server/StatsCollectorTest.java index 3709f732f045..72d4f533000c 100644 --- a/server/src/test/java/com/cloud/server/StatsCollectorTest.java +++ b/server/src/test/java/com/cloud/server/StatsCollectorTest.java @@ -38,6 +38,7 @@ import com.cloud.utils.DateUtil; import com.google.gson.JsonSyntaxException; import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.commons.collections.CollectionUtils; import org.influxdb.InfluxDB; @@ -66,11 +67,16 @@ import com.cloud.agent.api.VmStatsEntry; import com.cloud.hypervisor.Hypervisor; import com.cloud.server.StatsCollector.ExternalStatsProtocol; +import com.cloud.storage.ScopeType; +import com.cloud.storage.Storage; import com.cloud.storage.StorageStats; import com.cloud.storage.VolumeStatsVO; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.dao.VolumeStatsDao; import com.cloud.user.VmDiskStatisticsVO; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmManager; import com.cloud.vm.VmStats; import com.cloud.vm.VmStatsVO; import com.cloud.vm.dao.VmStatsDaoImpl; @@ -115,6 +121,15 @@ public class StatsCollectorTest { @Mock VolumeStatsDao volumeStatsDao = Mockito.mock(VolumeStatsDao.class); + @Mock + PrimaryDataStoreDao storagePoolDao = Mockito.mock(PrimaryDataStoreDao.class); + + @Mock + VolumeDao volumeDao = Mockito.mock(VolumeDao.class); + + @Mock + UserVmManager userVmManager = Mockito.mock(UserVmManager.class); + @Mock private StoragePoolVO mockPool; @@ -131,6 +146,9 @@ public void setUp() throws Exception { closeable = MockitoAnnotations.openMocks(this); statsCollector.vmStatsDao = vmStatsDaoMock; statsCollector.volumeStatsDao = volumeStatsDao; + ReflectionTestUtils.setField(statsCollector, "_storagePoolDao", storagePoolDao); + ReflectionTestUtils.setField(statsCollector, "_volsDao", volumeDao); + ReflectionTestUtils.setField(statsCollector, "_userVmMgr", userVmManager); Field msStatsGsonField = StatsCollector.class.getDeclaredField("msStatsGson"); msStatsGsonField.setAccessible(true); msStatsGson = (Gson) msStatsGsonField.get(null); @@ -622,6 +640,61 @@ public void testPoolNeedsIopsStatsUpdating_NullIops() { Mockito.verify(mockPool, Mockito.never()).setUsedIops(Mockito.anyLong()); } + @Test + public void volumeStatsTaskSkipsEmptyPools() { + configureStoragePool(1L, "pool-uuid", ScopeType.CLUSTER, 1L, Storage.StoragePoolType.NetworkFilesystem); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(new ArrayList<>()); + + statsCollector.new VolumeStatsTask().runInContext(); + + Mockito.verify(userVmManager, Mockito.never()).getVolumeStatistics(Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyInt()); + } + + @Test + public void volumeStatsTaskSkipsNonZonePoolsWithoutClusterId() { + configureStoragePool(1L, "pool-uuid", ScopeType.CLUSTER, null, Storage.StoragePoolType.NetworkFilesystem); + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(Arrays.asList(volume)); + + statsCollector.new VolumeStatsTask().runInContext(); + + Mockito.verify(userVmManager, Mockito.never()).getVolumeStatistics(Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyInt()); + } + + @Test + public void volumeStatsTaskSkipsVolumesWithoutFormat() { + configureStoragePool(1L, "pool-uuid", ScopeType.CLUSTER, 1L, Storage.StoragePoolType.NetworkFilesystem); + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getFormat()).thenReturn(null); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(Arrays.asList(volume)); + + statsCollector.new VolumeStatsTask().runInContext(); + + Mockito.verify(userVmManager, Mockito.never()).getVolumeStatistics(Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyInt()); + } + + @Test + public void volumeStatsTaskSkipsVolumesWithoutPoolType() { + configureStoragePool(1L, "pool-uuid", ScopeType.CLUSTER, 1L, null); + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getFormat()).thenReturn(null); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(Arrays.asList(volume)); + + statsCollector.new VolumeStatsTask().runInContext(); + + Mockito.verify(userVmManager, Mockito.never()).getVolumeStatistics(Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyInt()); + } + + private void configureStoragePool(long poolId, String uuid, ScopeType scope, Long clusterId, Storage.StoragePoolType poolType) { + when(storagePoolDao.listAll()).thenReturn(Arrays.asList(mockPool)); + when(mockPool.getId()).thenReturn(poolId); + when(mockPool.getUuid()).thenReturn(uuid); + when(mockPool.getScope()).thenReturn(scope); + when(mockPool.getClusterId()).thenReturn(clusterId); + when(mockPool.getPoolType()).thenReturn(poolType); + } + @Test public void testGsonDateFormatSerialization() { Date now = new Date();