From cff272af56d1f1eeeea7023b41f545ddb8e8c497 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Wed, 7 Oct 2020 08:13:22 +0300 Subject: [PATCH 1/8] fix NPE in volumes statistics If volume is migrated between storage pools and "volume.stats.interval" is set to more frequent interval for exmaple 30 sec. NullPointerException is thrown, because the hypervisor is looking for volume on storage pool from which the volume was migrated. The statistics did not work right - before this fix only one cluster is lists if we have more clusters from the same type (eg. KVM), and only one host will return volume stats. Also all volumes are sent to that host, but they could be on another host. And the logic does not work correctly. Also the response was not working correctly for QCOW2 images, because it's looking for volume's uuid, but in the statistics CS keeps the path (because of the migrated volumes which UUIDs are changed) --- .../java/com/cloud/dc/dao/ClusterDaoImpl.java | 1 - .../cloud/api/query/ViewResponseHelper.java | 5 +---- .../java/com/cloud/vm/UserVmManagerImpl.java | 20 ++++++++----------- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java b/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java index b1fce6195ba5..952b095758a5 100644 --- a/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java @@ -81,7 +81,6 @@ public ClusterDaoImpl() { ZoneSearch = createSearchBuilder(); ZoneSearch.and("dataCenterId", ZoneSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ); - ZoneSearch.groupBy(ZoneSearch.entity().getHypervisorType()); ZoneSearch.done(); AvailHyperSearch = createSearchBuilder(); 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 ced81a6e06c4..9ab7204076a7 100644 --- a/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java +++ b/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java @@ -282,10 +282,7 @@ public static List createVolumeResponse(ResponseView view, Volum 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){ diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 558b9809a243..daab0f0c3856 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.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; @@ -1885,18 +1886,13 @@ public HashMap getVirtualMachineStatistics(long hostId, Stri public HashMap getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List volumeLocators, int timeout) { List neighbors = _resourceMgr.listHostsInClusterByStatus(clusterId, Status.Up); StoragePoolVO storagePool = _storagePoolDao.findPoolByUUID(poolUuid); - for (HostVO neighbor : neighbors) { - // apply filters: - // - managed storage - // - local storage - if (storagePool.isManaged() || storagePool.isLocal()) { - - volumeLocators = getVolumesByHost(neighbor, storagePool); + HashMap volumeStatsByUuid = new HashMap<>(); - } + for (HostVO neighbor : neighbors) { + 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()) || (volumeLocators == null || volumeLocators.size() == 0)) { // skip this neighbour if their hypervisor type is not the same as that of the store continue; } @@ -1911,16 +1907,16 @@ public HashMap getVolumeStatistics(long clusterId, Str if (answer instanceof GetVolumeStatsAnswer){ GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; - return volstats.getVolumeStats(); + 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()); 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.getPath() : null).filter(Objects::nonNull)) .collect(Collectors.toList()); } From 2f445dfc0db1dd634f8f9aab7caa6fc0f6f106d4 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Wed, 7 Oct 2020 09:14:45 +0300 Subject: [PATCH 2/8] list all virtual machines by host id --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index daab0f0c3856..d563fb436d38 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1914,7 +1914,7 @@ public HashMap getVolumeStatistics(long clusterId, Str } 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.getState() == Volume.State.Ready ? vol.getPath() : null).filter(Objects::nonNull)) .collect(Collectors.toList()); From 03a783734be2f36c24dbb717debfa0fb1a49e772 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 9 Oct 2020 14:38:03 +0300 Subject: [PATCH 3/8] Return groupBy hypervisor type when listing clusters by zone id --- engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java b/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java index 952b095758a5..b1fce6195ba5 100644 --- a/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/dc/dao/ClusterDaoImpl.java @@ -81,6 +81,7 @@ public ClusterDaoImpl() { ZoneSearch = createSearchBuilder(); ZoneSearch.and("dataCenterId", ZoneSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ); + ZoneSearch.groupBy(ZoneSearch.entity().getHypervisorType()); ZoneSearch.done(); AvailHyperSearch = createSearchBuilder(); From a82f4e3705d16fe47c63f2294101d3a007bf360c Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 9 Oct 2020 14:39:01 +0300 Subject: [PATCH 4/8] Replace listByZoneId in VolumeStatsTask with listClustersByDcId --- .../main/java/com/cloud/server/StatsCollector.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 3937bd99666a..43aecde34cd2 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 @@ protected void runInContext() { 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 @@ protected void runInContext() { 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()) { From 5c5f5f4dbe3320ab0d5fc72f89b80563e8ac2462 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 9 Oct 2020 14:40:44 +0300 Subject: [PATCH 5/8] Removed unused arg and condition if format is OVA when listing volumes --- server/src/main/java/com/cloud/vm/UserVmManager.java | 2 +- .../src/main/java/com/cloud/vm/UserVmManagerImpl.java | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index 1fe3f4d5b189..95e1a4176a19 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 d563fb436d38..07c46ec95e53 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1883,13 +1883,13 @@ public HashMap getVirtualMachineStatistics(long hostId, Stri } @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) { - volumeLocators = getVolumesByHost(neighbor, storagePool); + List volumeLocators = getVolumesByHost(neighbor, storagePool); // - zone wide storage for specific hypervisortypes if ((ScopeType.ZONE.equals(storagePool.getScope()) && storagePool.getHypervisor() != neighbor.getHypervisorType()) || (volumeLocators == null || volumeLocators.size() == 0)) { @@ -1907,7 +1907,9 @@ public HashMap getVolumeStatistics(long clusterId, Str if (answer instanceof GetVolumeStatsAnswer){ GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; - volumeStatsByUuid.putAll(volstats.getVolumeStats()); + if (volstats.getVolumeStats() != null) { + volumeStatsByUuid.putAll(volstats.getVolumeStats()); + } } } return volumeStatsByUuid.size() > 0 ? volumeStatsByUuid : null; @@ -1916,7 +1918,8 @@ public HashMap getVolumeStatistics(long clusterId, Str 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 -> vol.getState() == Volume.State.Ready ? vol.getPath() : null).filter(Objects::nonNull)) + .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()); } From fcb9979f06f412d75f258d6d9d76b1ec8cf711e5 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Mon, 12 Oct 2020 09:00:38 +0300 Subject: [PATCH 6/8] Formating the brackets --- .../src/main/java/com/cloud/api/query/ViewResponseHelper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 9ab7204076a7..d1681683adab 100644 --- a/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java +++ b/server/src/main/java/com/cloud/api/query/ViewResponseHelper.java @@ -282,10 +282,10 @@ public static List createVolumeResponse(ResponseView view, Volum vrDataList.put(vr.getId(), vrData); VolumeStats vs = null; - if (vr.getFormat() == ImageFormat.VHD || vr.getFormat() == ImageFormat.QCOW2){ + 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()); } From 5008846187b7ab0bc81b228ee99e1344afa4b7fe Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Thu, 29 Oct 2020 11:06:31 +0200 Subject: [PATCH 7/8] Won't get volumes if the hypervisor type is not the same of the store --- .../java/com/cloud/vm/UserVmManagerImpl.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 07c46ec95e53..b001ec6b59d7 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1889,26 +1889,29 @@ public HashMap getVolumeStatistics(long clusterId, Str HashMap volumeStatsByUuid = new HashMap<>(); for (HostVO neighbor : neighbors) { - List volumeLocators = getVolumesByHost(neighbor, storagePool); // - zone wide storage for specific hypervisortypes - if ((ScopeType.ZONE.equals(storagePool.getScope()) && storagePool.getHypervisor() != neighbor.getHypervisorType()) || (volumeLocators == null || volumeLocators.size() == 0)) { + 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); + } + + Answer answer = _agentMgr.easySend(neighbor.getId(), cmd); - if (answer instanceof GetVolumeStatsAnswer){ - GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; - if (volstats.getVolumeStats() != null) { - volumeStatsByUuid.putAll(volstats.getVolumeStats()); + if (answer instanceof GetVolumeStatsAnswer){ + GetVolumeStatsAnswer volstats = (GetVolumeStatsAnswer)answer; + if (volstats.getVolumeStats() != null) { + volumeStatsByUuid.putAll(volstats.getVolumeStats()); + } } } } From 93214ffe2b83f97601c910c54821d7fda1c985a8 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Thu, 29 Oct 2020 21:32:26 +0200 Subject: [PATCH 8/8] Add import for CollectionUtils --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index b001ec6b59d7..2a9bb2e13d76 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -97,6 +97,7 @@ 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;