Skip to content

VolumeStatsTask NPE 방지 #836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dhslove wants to merge 1 commit into
base: ablestack-europa
Choose a base branch
from
Open

VolumeStatsTask NPE 방지 #836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions server/src/main/java/com/cloud/server/StatsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1929,13 +1929,29 @@ protected void runInContext() {
continue;
}
List<VolumeVO> 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<String, VolumeStatsEntry> volumeStatsByUuid;
if (pool.getScope() == ScopeType.ZONE) {
Expand All @@ -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<String, VolumeStatsEntry> entry : volumeStatsByUuid.entrySet()) {
Expand Down Expand Up @@ -2486,4 +2507,4 @@ public ConfigKey<?>[] getConfigKeys() {
public double getImageStoreCapacityThreshold() {
return CapacityManager.SecondaryStorageCapacityThreshold.value();
}
}
}
12 changes: 10 additions & 2 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2431,8 +2431,16 @@ private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOf

@Override
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, int timeout) {
List<HostVO> 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<HostVO> 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<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();

for (HostVO neighbor : neighbors) {
Expand All @@ -2451,7 +2459,7 @@ public HashMap<String, VolumeStatsEntry> 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
Expand Down
73 changes: 73 additions & 0 deletions server/src/test/java/com/cloud/server/StatsCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
Loading