You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/10/07 05:36:44 UTC
[GitHub] [cloudstack] slavkap opened a new pull request #4388: fix NPE in volumes statistics
slavkap opened a new pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388
## Description
If volume is migrated between storage pools and "volume.stats.interval"
is set to more frequent interval for example 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).
We have discussed the NPE with @Spaceman1984 in PR [https://github.com/apache/cloudstack/pull/3949](url)
<!--- Describe your changes in detail -->
With this change all clusters will be listed even those with the same type. Host will receive only the active volumes which are on it. And in the logs we will receive a proper information about the volumes
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X ] Bug fix (non-breaking change which fixes an issue)
- [ X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## How Has This Been Tested?
CS versions - 4.14 and master
2 clusters each having 2 hosts
hypervisors KVM
2 NFS primary storages with scope Zone
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712821390
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718966999
Just checked, it seems that the errors are related to Travis running JDK 11 CloudStack 4.13 uses Java 8.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718952150
I'm ok to move it to 4.14, but I'm not sure how those changes are related to VPC. Here is the failure:
` "Ping to VM on Network Tier N from VM in Network Tier A should be successful at least for 2 out of 3 VMs"`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r503850235
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
removed volumeLocators
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718182129
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r501694339
##########
File path: 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());
Review comment:
@slavkap check if this can cause regression, for listing hypervisors for zone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-719408046
> > code LGTM, haven't tested. Please ensure these changes doesn't cause any regression for managed / unmanaged / local storage.
>
> @sureshanaparti please explain your concerns or ensure what you can? I'm am trusting @slavkap to have validated any configuration she has access too. If you know what to do more please do so or ping the people that can. If we leave it at this I'm pretty sure no follow up will happen.
@DaanHoogland the code changes will impact the volume stats for all storage pools. So, it is better if this can be tested with volume(s) on unmanaged, managed and local storage. Otherwise, can cause regression.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514168091
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
may I ask for advice, because I'm not sure how to proceed? I've never changed the branch of a PR, so what is the right action - to edit this PR branch or to checkout to a branch which is on top of 4.13, to cherry-pick the changes (resolve conflicts) and to create new pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502266925
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
yes, I will add to check if it's OVA
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502209015
##########
File path: 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());
Review comment:
@sureshanaparti I have tested it. My setup is 2 KVM cluster with 2 hosts on each. Before the change when listing hypervisors:
> list hypervisors zoneid=307c2b69-86eb-451e-8517-6113c59648c4
> { "count": 1, "hypervisor": [ { "name": "KVM" } ] }
with the change when listing hypervisors:
> list hypervisors zoneid=307c2b69-86eb-451e-8517-6113c59648c4
> { "count": 2, "hypervisor": [ { "name": "KVM" }, { "name": "KVM" } ] }
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718449743
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712737699
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502260877
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
ok @slavkap you have corrected the volume locator for QCOW2 in the response helper, this would be similar change for OVA here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-713381485
@weizhouapache is this lgty?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514105467
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
Thanks @DaanHoogland! I'll fix the conflicts and will push it against 4.13.2.0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712762549
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2214
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718966999
Just checked, it seems that the errors are related to Travis running JDK 11; CloudStack 4.13 uses Java 8.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502203876
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
@sureshanaparti I was just follow the original logic and just added to get only the volumes with status "Ready". I will check for the rest of the formats, but I will be not able to do tests for them
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r501680506
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
@slavkap what is the impact on volume stats for managed / local storage pool ? can this cause regression?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-719443581
actually what I see is that NFS with default logic is only non managed storage. I can test this only with StorPool (which is non managed) and NFS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712821874
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502258419
##########
File path: 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());
Review comment:
@slavkap list hypervisors should return only 1 KVM, that is the reason for group by.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718957391
> I'm ok to move it to 4.14, but I'm not sure how those changes are related to VPC. Here is the failure:
> ` "Ping to VM on Network Tier N from VM in Network Tier A should be successful at least for 2 out of 3 VMs"`
those can be due to a busy system, @slavkap. I don't think these can be related to your changes, but we can re-run if you want to be sure.
note that the tests nine days ago showed different failures.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718964313
Thanks @GabrielBrascher ! @DaanHoogland I saw some of those failures from nine days ago in another PR of mine, but didn't find the zip with the results
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r501701786
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
@slavkap volume path is not the locator for OVA format, update with chain info ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-704810338
similar issue as #3884 #3878
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502205159
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
The logic was correct for managed/local storages, but for unmanaged I've descried in the description what is happening. I have tested with NFS unmanaged storage and StorPool as well, and it should get the volumes only on the current host
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-713208970
<b>Trillian test result (tid-3018)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38770 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4388-t3018-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 310.76 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 297.25 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 175.40 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502266242
##########
File path: 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());
Review comment:
ok than, I wasn't sure that it should return only 1 KVM if we have for example 3 clusters of them. I will implement another method, which gets all clusters in the zone. It's needed for StatsCollector to list all clusters in the zone, otherwise we get only one cluster of a hypervisor type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712736743
@sureshanaparti all your concerns addressed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514197219
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
you can try and rebase the range of your commit on top of 4.13, or you can cherrypinck. when done you should force push and you can then change the base branch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502203876
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
@sureshanaparti I was just follow the original logic and just added to get only the volumes with status "Ready". I will check for the rest of the formats, but I will be not able to do tests for them
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
The logic was correct for managed/local storages, but for unmanaged I've descried in the description what is happening. I have tested with NFS unmanaged storage and StorPool as well, and it should get the volumes only on the current host
##########
File path: 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());
Review comment:
@sureshanaparti I have tested it. My setup is 2 KVM cluster with 2 hosts on each. Before the change when listing hypervisors:
> list hypervisors zoneid=307c2b69-86eb-451e-8517-6113c59648c4
> { "count": 1, "hypervisor": [ { "name": "KVM" } ] }
with the change when listing hypervisors:
> list hypervisors zoneid=307c2b69-86eb-451e-8517-6113c59648c4
> { "count": 2, "hypervisor": [ { "name": "KVM" }, { "name": "KVM" } ] }
##########
File path: 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());
Review comment:
ok than, I wasn't sure that it should return only 1 KVM if we have for example 3 clusters of them. I will implement another method, which gets all clusters in the zone. It's needed for StatsCollector to list all clusters in the zone, otherwise we get only one cluster of a hypervisor type.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
yes, I will add to check if it's OVA
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-717845466
@slavkap can you fix the conflict?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-719398917
> code LGTM, haven't tested. Please ensure these changes doesn't cause any regression for managed / unmanaged / local storage.
@sureshanaparti please explain your concerns or ensure what you can? I'm am trusting @slavkap to have validated any configuration she has access too. If you know what to do more please do so or ping the people that can. If we leave it at this I'm pretty sure no follow up will happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718909929
<b>Trillian test result (tid-3104)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32733 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4388-t3104-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_04_rvpc_privategw_static_routes | `Failure` | 241.03 | test_privategw_acl.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718975979
There was a missing import in one of the classes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718945283
Considering that the milestone is 4.15 and this PR was manually tested at 4.14, maybe the best would be moving this to 4.14 or 4.15.
What do you think @slavkap @DaanHoogland?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718450798
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502267873
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
@slavkap For non-managed & shared pools, the `volumeLocators` passed to the method is directly used earlier, instead from the host(s). Now, `volumeLocators` in the method arg, is not used anymore. You can remove the arg if not required. Please check and update accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718182474
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-712736927
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514201827
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
thanks again @DaanHoogland !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514068122
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
@slavkap move this condition check `(ScopeType.ZONE.equals(storagePool.getScope()) && storagePool.getHypervisor() != neighbor.getHypervisorType()` before getting volume locators.
and use CollectionUtils for volumeLocators
```suggestion
if (CollectionUtils.isEmpty(volumeLocators)) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-719427756
> > > code LGTM, haven't tested. Please ensure these changes doesn't cause any regression for managed / unmanaged / local storage.
> >
> >
> > @sureshanaparti please explain your concerns or ensure what you can? I'm am trusting @slavkap to have validated any configuration she has access too. If you know what to do more please do so or ping the people that can. If we leave it at this I'm pretty sure no follow up will happen.
>
> @DaanHoogland the code changes will impact the volume stats for all storage pools. So, it is better if this can be tested with volume(s) on unmanaged, managed and local storage. Otherwise, can cause regression.
I understood that, @sureshanaparti. But by who and how should this be tested to appease your concerns.
@slavkap, can you explain more about your testing. You claim you have tested with zone wide NFS storage.
- Is that all?
- have you tested with more types of environement? Or have you @GabrielBrascher ?
- is there any reason to think we don't need to test local - or managed storage , @slavkap ?
tnx all
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r502258419
##########
File path: 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());
Review comment:
@slavkap list hypervisors should return only 1 KVM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r501680506
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
@slavkap what is the impact on volume stats for managed / local storage pool ? can this cause regression?
##########
File path: server/src/main/java/com/cloud/api/query/ViewResponseHelper.java
##########
@@ -287,10 +287,7 @@
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){
Review comment:
```suggestion
if (vr.getFormat() == ImageFormat.VHD || vr.getFormat() == ImageFormat.QCOW2) {
```
##########
File path: 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());
Review comment:
@slavkap check if this can cause regression, for listing hypervisors for zone.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
@slavkap volume path is not the locator for OVA format, update with chain info ?
##########
File path: 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());
Review comment:
@slavkap list hypervisors should return only 1 KVM.
##########
File path: 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());
Review comment:
@slavkap list hypervisors should return only 1 KVM, that is the reason for group by.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1956,16 +1952,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
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<String> getVolumesByHost(HostVO host, StoragePool pool){
- List<UserVmVO> vmsPerHost = _vmDao.listByHostId(host.getId());
+ List<VMInstanceVO> 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.getPath() : null).filter(Objects::nonNull))
Review comment:
ok @slavkap you have corrected the volume locator for QCOW2 in the response helper, this would be similar change for OVA here.
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1930,18 +1931,13 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
List<HostVO> 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()) {
Review comment:
@slavkap For non-managed & shared pools, the `volumeLocators` passed to the method is directly used earlier, instead from the host(s). Now, `volumeLocators` in the method arg, is not used anymore. You can remove the arg if not required. Please check and update accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718199784
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2304
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514102578
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
@slavkap yes, we will then merge forward, no problem (except maybe a conflict to resolve)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718939851
@slavkap there are travis errors can you look at that and provide test report/results, please?
I'm not sure why (yet) but the 4.13 integrations fail all around.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r501681271
##########
File path: server/src/main/java/com/cloud/api/query/ViewResponseHelper.java
##########
@@ -287,10 +287,7 @@
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){
Review comment:
```suggestion
if (vr.getFormat() == ImageFormat.VHD || vr.getFormat() == ImageFormat.QCOW2) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718980588
Thank you all for the time you spent!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718961803
I just re-kicked Travis checks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on a change in pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#discussion_r514100750
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -1954,21 +1955,16 @@ private boolean upgradeRunningVirtualMachine(Long vmId, Long newServiceOfferingI
}
@Override
- public HashMap<String, VolumeStatsEntry> getVolumeStatistics(long clusterId, String poolUuid, StoragePoolType poolType, List<String> volumeLocators, int timeout) {
+ 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);
- for (HostVO neighbor : neighbors) {
- // apply filters:
- // - managed storage
- // - local storage
- if (storagePool.isManaged() || storagePool.isLocal()) {
+ HashMap<String, VolumeStatsEntry> volumeStatsByUuid = new HashMap<>();
- volumeLocators = getVolumesByHost(neighbor, storagePool);
-
- }
+ for (HostVO neighbor : neighbors) {
+ List<String> 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)) {
Review comment:
Thanks @sureshanaparti ! I'll do the change suggestion, but may I ask is it appropriate if I push this PR in earlier version of CS for example 4.13.2.0?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-718979445
Nice! Travis is also running with jdk 8 now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] slavkap commented on pull request #4388: fix NPE in volumes statistics
Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4388:
URL: https://github.com/apache/cloudstack/pull/4388#issuecomment-719435059
@DaanHoogland @sureshanaparti I have tested it only with non managed storage on StorPool and NFS (on master and 4.14) with 2 clusters each having 2 KVM hypervisors. I'm setting now a dev environment with 4.13 and will test with managed (on NFS) and non managed (on NFS and StorPool) storage. About local storage - how should it be set managed/unmanaged? If there is somebody that could test this with OVA files will be appreciated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org