From 8afb451c1c60ec106aeecedafdfb491e3ae16cc8 Mon Sep 17 00:00:00 2001 From: slavkap <51903378+slavkap@users.noreply.github.com> Date: Fri, 30 Oct 2020 17:53:05 +0200 Subject: [PATCH] fix NPE in volumes statistics (#4388) --- .../cloud/api/query/ViewResponseHelper.java | 7 +-- .../java/com/cloud/server/StatsCollector.java | 14 ++---- .../main/java/com/cloud/vm/UserVmManager.java | 2 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 45 ++++++++++--------- 4 files changed, 31 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java b/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java index ced81a6e06c..d1681683ada 100644 --- a/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java +++ b/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java @@ -282,13 +282,10 @@ public class ViewResponseHelper { vrDataList.put(vr.getId(), vrData); VolumeStats vs = null; - if (vr.getFormat() == ImageFormat.QCOW2) { - vs = ApiDBUtils.getVolumeStatistics(vrData.getId()); - } - else if (vr.getFormat() == ImageFormat.VHD){ + if (vr.getFormat() == ImageFormat.VHD || vr.getFormat() == ImageFormat.QCOW2) { vs = ApiDBUtils.getVolumeStatistics(vrData.getPath()); } - else if (vr.getFormat() == ImageFormat.OVA){ + else if (vr.getFormat() == ImageFormat.OVA) { if (vrData.getChainInfo() != null) { vs = ApiDBUtils.getVolumeStatistics(vrData.getChainInfo()); } diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 3937bd99666..43aecde34cd 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -928,13 +928,8 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc for (StoragePoolVO pool : pools) { List volumes = _volsDao.findByPoolId(pool.getId(), null); - List volumeLocators = new ArrayList(); for (VolumeVO volume : volumes) { - if (volume.getFormat() == ImageFormat.QCOW2 || volume.getFormat() == ImageFormat.VHD) { - volumeLocators.add(volume.getPath()); - } else if (volume.getFormat() == ImageFormat.OVA) { - volumeLocators.add(volume.getChainInfo()); - } else { + if (volume.getFormat() != ImageFormat.QCOW2 && volume.getFormat() != ImageFormat.VHD && volume.getFormat() != ImageFormat.OVA) { s_logger.warn("Volume stats not implemented for this format type " + volume.getFormat()); break; } @@ -943,15 +938,14 @@ public class StatsCollector extends ManagerBase implements ComponentMethodInterc Map volumeStatsByUuid; if (pool.getScope() == ScopeType.ZONE) { volumeStatsByUuid = new HashMap<>(); - for (final Cluster cluster : _clusterDao.listByZoneId(pool.getDataCenterId())) { - final Map volumeStatsForCluster = _userVmMgr.getVolumeStatistics(cluster.getId(), pool.getUuid(), pool.getPoolType(), - volumeLocators, StatsTimeout.value()); + for (final Cluster cluster : _clusterDao.listClustersByDcId(pool.getDataCenterId())) { + final Map volumeStatsForCluster = _userVmMgr.getVolumeStatistics(cluster.getId(), pool.getUuid(), pool.getPoolType(), StatsTimeout.value()); if (volumeStatsForCluster != null) { volumeStatsByUuid.putAll(volumeStatsForCluster); } } } else { - volumeStatsByUuid = _userVmMgr.getVolumeStatistics(pool.getClusterId(), pool.getUuid(), pool.getPoolType(), volumeLocators, StatsTimeout.value()); + volumeStatsByUuid = _userVmMgr.getVolumeStatistics(pool.getClusterId(), pool.getUuid(), pool.getPoolType(), StatsTimeout.value()); } if (volumeStatsByUuid != null) { for (final Map.Entry entry : volumeStatsByUuid.entrySet()) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index 1fe3f4d5b18..95e1a4176a1 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManager.java +++ b/server/src/main/java/com/cloud/vm/UserVmManager.java @@ -85,7 +85,7 @@ public interface UserVmManager extends UserVmService { HashMap> getVmDiskStatistics(long hostId, String hostName, List vmIds); - HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List volumeLocator, int timout); + HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, int timout); boolean deleteVmGroup(long groupId); diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 558b9809a24..2a9bb2e13d7 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -27,6 +27,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -96,6 +97,7 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; import org.apache.commons.codec.binary.Base64; +import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; @@ -1882,45 +1884,46 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir } @Override - public HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List volumeLocators, int timeout) { + public HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, int timeout) { List neighbors = _resourceMgr.listHostsInClusterByStatus(clusterId, Status.Up); StoragePoolVO storagePool = _storagePoolDao.findPoolByUUID(poolUuid); + HashMap volumeStatsByUuid = new HashMap<>(); + for (HostVO neighbor : neighbors) { - // apply filters: - // - managed storage - // - local storage - if (storagePool.isManaged() || storagePool.isLocal()) { - - volumeLocators = getVolumesByHost(neighbor, storagePool); - - } // - zone wide storage for specific hypervisortypes - if (ScopeType.ZONE.equals(storagePool.getScope()) && storagePool.getHypervisor() != neighbor.getHypervisorType()) { + if ((ScopeType.ZONE.equals(storagePool.getScope()) && storagePool.getHypervisor() != neighbor.getHypervisorType())) { // skip this neighbour if their hypervisor type is not the same as that of the store continue; } - GetVolumeStatsCommand cmd = new GetVolumeStatsCommand(poolType, poolUuid, volumeLocators); + List volumeLocators = getVolumesByHost(neighbor, storagePool); + if (!CollectionUtils.isEmpty(volumeLocators)) { - if (timeout > 0) { - cmd.setWait(timeout/1000); - } + GetVolumeStatsCommand cmd = new GetVolumeStatsCommand(poolType, poolUuid, volumeLocators); - Answer answer = _agentMgr.easySend(neighbor.getId(), cmd); + if (timeout > 0) { + cmd.setWait(timeout/1000); + } - if (answer instanceof GetVolumeStatsAnswer){ - GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; - return volstats.getVolumeStats(); + Answer answer = _agentMgr.easySend(neighbor.getId(), cmd); + + if (answer instanceof GetVolumeStatsAnswer){ + GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; + if (volstats.getVolumeStats() != null) { + volumeStatsByUuid.putAll(volstats.getVolumeStats()); + } + } } } - return null; + return volumeStatsByUuid.size() > 0 ? volumeStatsByUuid : null; } private List getVolumesByHost(HostVO host, StoragePool pool){ - List vmsPerHost = _vmDao.listByHostId(host.getId()); + List vmsPerHost = _vmInstanceDao.listByHostId(host.getId()); return vmsPerHost.stream() - .flatMap(vm -> _volsDao.findByInstanceIdAndPoolId(vm.getId(),pool.getId()).stream().map(vol -> vol.getPath())) + .flatMap(vm -> _volsDao.findByInstanceIdAndPoolId(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()); }