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/05 15:57:10 UTC
[GitHub] [cloudstack] davidjumani opened a new pull request #4375: Fixing count for findHostsForMigration
davidjumani opened a new pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375
## Description
Fixes https://github.com/apache/cloudstack/issues/4371
Needs https://github.com/apache/cloudstack/pull/4374 to work
## 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)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## How Has This Been Tested?
Before :
```
{
"count": 4,
"host": [
{},
{},
{}
]
}
```
After :
```
{
"count": 3,
"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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704014144
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708941692
@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] DaanHoogland removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711758300
@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] DaanHoogland commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712737470
@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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-709577871
<b>Trillian test result (tid-2964)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37917 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t2964-kvm-centos7.zip
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_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 461.19 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 172.18 | 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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707068187
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2160
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r504605112
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
- } else {
- if (hasSuitablePoolsForVolume(volume, host, vmProfile)) {
- requiresStorageMotion.put(host, true);
- } else {
- iterator.remove();
- }
+ // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
Review comment:
Will mark them as not suitable based on the `requiresStorageMotion` map
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-703763781
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2119
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505234261
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
Review comment:
It does since I've added an excludes param in the `searchForServers` method and passing the source host ID to it. This excludes the source host in the query, as well as ensures that the allHosts list is never altered. So the count remains consistant but unsuitable hosts are marked 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] DaanHoogland commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711758300
@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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500036739
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
Again I agree, but that's the way it's already being done, so just fixing the count
https://github.com/apache/cloudstack/blob/master/api/src/main/java/org/apache/cloudstack/api/command/admin/host/FindHostsForMigrationCmd.java#L92
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500013210
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
This is so that the total count and hosts returned remain the same. Rather than removing them, they're marked as not suitable
https://github.com/apache/cloudstack/blob/master/api/src/main/java/org/apache/cloudstack/api/command/admin/host/FindHostsForMigrationCmd.java#L94
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708371601
@blueorangutan test matrix
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-713382400
@davidjumani can you have a quick look at the errors above?
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704021916
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2120
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711083156
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2200
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-713222849
<b>Trillian test result (tid-3025)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36556 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t3025-kvm-centos7.zip
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.41 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 295.68 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 169.68 | 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] DaanHoogland commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712737856
@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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r504622538
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
@sureshanaparti Sorted in the UI via https://github.com/apache/cloudstack-primate/pull/788/commits/a4732a334bd4be2df755dbe704e1894b1200bc69
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-714984570
@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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500020751
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
Agreed, but since we're paginating the result, we can not remove the hosts in page 2 when we've only fetched the ones in page 1. So marking it as not compatible so that the total count is not broken
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500032180
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
Pagination is the UI thing, may be some other alternative that can be worked upon. If someone uses the API, doesn't look good to return all hosts, marking suitable & unsuitable.
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707608503
@blueorangutan test centos7 kvm-centos7
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711089814
@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] sureshanaparti commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505228152
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
Review comment:
@davidjumani didn't see any change with respect to hosts count, does these changes fix the count issue?
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704013778
<b>Trillian test result (tid-2894)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37351 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t2894-kvm-centos7.zip
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_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 301.45 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 168.51 | 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] rhtyd removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711089814
@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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711759538
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708966242
<b>Trillian test result (tid-2960)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 1304 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t2960-kvm-centos7.zip
Smoke tests completed. 0 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505231606
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1275,19 +1275,20 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Type hostType = srcHost.getType();
Pair<List<HostVO>, Integer> allHostsPair = null;
List<HostVO> allHosts = null;
+ List<HostVO> hostsWithStorageMigration = null;
Review comment:
Previously the `allHosts` list was altered (if storage migration was required) and then used to find the suitable hosts. Instead, now it's being cloned into `hostsWithStorageMigration` and using the cloned list to find the suitable hosts so that the original list is not altered.
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500015973
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
in actual production, there can be lot of unsuitable hosts. say 60 out of 100 are unsuitable, in this case, list all these and marking unsuitable doesn't sound good. thoughts?
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-703765500
@davidjumani 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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708368314
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2178
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707031956
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712738863
@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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r503916203
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
ok @davidjumani if possible, display the suitable hosts first in the UI.
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-703745938
@davidjumani 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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-709043178
@blueorangutan test centos7 kvm-centos7 keepEnv
----------------------------------------------------------------
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 removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708431678
@blueorangutan test centos7 kvm-centos7 keepEnv
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r506587901
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
plan = new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
}
Review comment:
It's needed here since the data type is different `Pair<List<HostVO>, Integer>` vs `Pair<List<? extends Host>, Integer>`
----------------------------------------------------------------
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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500036739
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
Again I agree, but that's the way it's already being done, so just fixing the count
https://github.com/apache/cloudstack/blob/master/api/src/main/java/org/apache/cloudstack/api/command/admin/host/FindHostsForMigrationCmd.java#L89
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-714983336
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2268
----------------------------------------------------------------
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 removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711759538
----------------------------------------------------------------
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 removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712737470
@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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-703745595
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708432017
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707113955
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-715514222
<b>Trillian test result (tid-3062)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41373 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t3062-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 84 look OK, 1 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` | 321.16 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 296.90 | test_vpc_redundant.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] rhtyd commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r504563714
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
- } else {
- if (hasSuitablePoolsForVolume(volume, host, vmProfile)) {
- requiresStorageMotion.put(host, true);
- } else {
- iterator.remove();
- }
+ // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
Review comment:
My only concern here is that now that we're not removing hosts, what is those hosts which are not removed now as shown as suitable, would that cause any 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] sureshanaparti commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500020419
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
@davidjumani better to update the count result from _searchForServers()_, when unsuitable hosts are removed, instead changing the existing code for suitable hosts, 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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r506587901
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
plan = new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
}
Review comment:
It's needed here since the data type is different `Pair<List<HostVO>, Integer>` vs `Pair<List<? extends Host>, Integer>`. Also the allHostsPair contains the total count of servers whereas the allHosts lists contains only those in the specified page / pagesize
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708372097
@davidjumani a Trillian-Jenkins matrix job (centos6 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505339089
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
plan = new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
}
Review comment:
@davidjumani can you check if `otherHosts` here is not required as there is no change in the `allHosts` & its count, which you can return as it is (as a List), instead of Pair (with count). You can use use the `allHosts` count wherever applicable.
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707032434
@rhtyd 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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-709044467
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704014459
@davidjumani 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] blueorangutan removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708966242
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708352655
@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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-709035908
<b>Trillian test result (tid-2962)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 1305 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4375-t2962-kvm-centos7.zip
Smoke tests completed. 0 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-714985432
@davidjumani 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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r506604784
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1326,10 +1327,9 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
if (s_logger.isDebugEnabled()) {
s_logger.debug("Searching for all hosts in cluster " + cluster + " for migrating VM " + vm);
}
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null, null);
- // Filter out the current host.
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, null, null, cluster, null, keyword, null, null, null,
+ null, srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
plan = new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
}
Review comment:
thanks for confirming that @davidjumani
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708941117
@blueorangutan test centos7 kvm-centos7 keepEnv
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711078159
@rhtyd 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] blueorangutan commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-714936990
@davidjumani 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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708431678
@blueorangutan test centos7 kvm-centos7 keepEnv
----------------------------------------------------------------
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 merged pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704021968
@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] rhtyd commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711078023
@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] DaanHoogland commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712738127
@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] davidjumani commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r504605112
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
- } else {
- if (hasSuitablePoolsForVolume(volume, host, vmProfile)) {
- requiresStorageMotion.put(host, true);
- } else {
- iterator.remove();
- }
+ // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
Review comment:
Changed to use a cloned list `hostsWithStorageMigration` instead, but reutrning the original allHosts list to not mess up the pagination
----------------------------------------------------------------
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 removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708432017
@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] DaanHoogland removed a comment on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-712737856
@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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-714936503
@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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505220901
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1275,19 +1275,20 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Type hostType = srcHost.getType();
Pair<List<HostVO>, Integer> allHostsPair = null;
List<HostVO> allHosts = null;
+ List<HostVO> hostsWithStorageMigration = null;
Review comment:
@davidjumani do you mean hosts for migration with storage? why another list is required 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] sureshanaparti commented on a change in pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r500011471
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1278,33 +1277,24 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Map<Host, Boolean> requiresStorageMotion = new HashMap<Host, Boolean>();
DataCenterDeployment plan = null;
if (canMigrateWithStorage) {
- allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword, null, null, srcHost.getHypervisorType(),
- srcHost.getHypervisorVersion());
+ allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, null, keyword,
+ null, null, srcHost.getHypervisorType(), srcHost.getHypervisorVersion(), srcHost.getId());
allHosts = allHostsPair.first();
- allHosts.remove(srcHost);
for (final VolumeVO volume : volumes) {
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
Long volClusterId = storagePool.getClusterId();
- for (Iterator<HostVO> iterator = allHosts.iterator(); iterator.hasNext();) {
- final Host host = iterator.next();
-
+ for (HostVO host : allHosts) {
if (volClusterId != null) {
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
- if (storagePool.isManaged()) {
- // At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
- // is at the zone level and the source and target storage pool is the same.
- // If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
- // because we need to create a new target volume and copy the contents of the source volume into it before deleting the
- // source volume.
- iterator.remove();
Review comment:
@davidjumani any reason, why unsuitable hosts are not removed from the hosts list ?
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-713401390
@DaanHoogland The host fencing errors are due to `com.google.gson.stream.MalformedJsonException: Unterminated object near assword":"p*****},"resourcestate":"Enabl'}`
Think that's beeen fixed in https://github.com/apache/cloudstack/pull/4387
Need to look into the vpc issues
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-704031486
@davidjumani can you please add the root cause for the count mismatch and the fix to the PR description (and also commit msg).
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-708353277
@davidjumani 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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#discussion_r505333849
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -1275,19 +1275,20 @@ private HypervisorType getHypervisorType(VMInstanceVO vm, StoragePool srcVolumeP
final Type hostType = srcHost.getType();
Pair<List<HostVO>, Integer> allHostsPair = null;
List<HostVO> allHosts = null;
+ List<HostVO> hostsWithStorageMigration = null;
Review comment:
Ok @davidjumani
```suggestion
List<HostVO> hostsForMigrationWithStorage = null;
```
----------------------------------------------------------------
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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-711089878
@rhtyd 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] DaanHoogland commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707113403
@blueorangutan test centos7 kvm-centos7 keepEnv
----------------------------------------------------------------
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] davidjumani commented on pull request #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
davidjumani commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-703764750
@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 #4375: Fixing count for findHostsForMigration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4375:
URL: https://github.com/apache/cloudstack/pull/4375#issuecomment-707609074
@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