Fixing count for findHostsForMigration by davidjumani · Pull Request #4375 · apache/cloudstack

davidjumani · 2020-10-05T15:57:02Z

Description

In the code, all servers, and their count belonging to the same zone are returned. Then they are filtered for suitability.
This is fine except for the point where VM volumes existing on managed storage pools or hosts on which those storage pools are not suitable. In this case it removes the hosts rather than marking them as not suitable. This messes up the count. Subtracting the non suitable hosts count does not fix this since we're paginating the response so unsuitable hosts and thier count from the current page will be removed but will not fix the total count since hosts on other pages can not be filtered.
Another place is that the source host is removed from the list but the count is not adjusted

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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": [
{},
{},
{}
]
}

davidjumani · 2020-10-05T16:33:12Z

@blueorangutan package

blueorangutan · 2020-10-05T16:33:50Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-05T17:05:36Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2119

davidjumani · 2020-10-05T17:07:28Z

@blueorangutan test

blueorangutan · 2020-10-05T17:08:56Z

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-06T04:01:57Z

Trillian test result (tid-2894)
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

davidjumani · 2020-10-06T04:03:32Z

@blueorangutan package

blueorangutan · 2020-10-06T04:04:58Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-06T04:36:37Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2120

davidjumani · 2020-10-06T04:36:53Z

@blueorangutan test

sureshanaparti · 2020-10-06T05:12:50Z

@davidjumani can you please add the root cause for the count mismatch and the fix to the PR description (and also commit msg).

sureshanaparti · 2020-10-06T05:20:20Z

- // 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();


@davidjumani any reason, why unsuitable hosts are not removed from the hosts list ?

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

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?

@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.

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

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.

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

ok @davidjumani if possible, display the suitable hosts first in the UI.

@sureshanaparti Sorted in the UI via apache/cloudstack-primate@a4732a3

yadvr · 2020-10-12T10:23:34Z

@blueorangutan package

blueorangutan · 2020-10-12T10:24:31Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-12T11:41:25Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2160

DaanHoogland · 2020-10-12T13:15:36Z

@blueorangutan test centos7 kvm-centos7 keepEnv

blueorangutan · 2020-10-12T13:16:31Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland · 2020-10-13T09:14:35Z

@blueorangutan test centos7 kvm-centos7

DaanHoogland · 2020-10-15T09:36:08Z

@blueorangutan test centos7 kvm-centos7 keepEnv

blueorangutan · 2020-10-15T09:37:33Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-15T20:39:20Z

Trillian test result (tid-2964)
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

sureshanaparti

Changes LGTM, not tested though

yadvr · 2020-10-17T20:44:54Z

@blueorangutan package

blueorangutan · 2020-10-17T20:45:55Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-17T21:33:06Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2200

DaanHoogland · 2020-10-20T09:58:40Z

@blueorangutan test

blueorangutan · 2020-10-20T09:59:55Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-21T00:50:44Z

Trillian test result (tid-3025)
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

DaanHoogland · 2020-10-21T07:54:00Z

@davidjumani can you have a quick look at the errors above?

davidjumani · 2020-10-21T08:27:42Z

@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 #4387
Need to look into the vpc issues

davidjumani · 2020-10-23T06:09:08Z

@blueorangutan package

blueorangutan · 2020-10-23T06:10:13Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-10-23T06:41:56Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2268

davidjumani · 2020-10-23T06:42:38Z

@blueorangutan test

blueorangutan · 2020-10-23T06:43:08Z

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-10-23T18:46:00Z

Trillian test result (tid-3062)
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

davidjumani added the component:api label Oct 5, 2020

davidjumani force-pushed the fix-findhostsformigration branch 3 times, most recently from a32685e to 182af0dCompare October 5, 2020 16:19

davidjumani added this to the 4.15.0.0 milestone Oct 5, 2020

davidjumani force-pushed the fix-findhostsformigration branch 3 times, most recently from fd4cc97 to 9668193Compare October 5, 2020 19:36

davidjumani mentioned this pull request Oct 6, 2020
Fixing host count for migratevm apache/cloudstack-primate#788
Merged

davidjumani requested a review from DaanHoogland October 6, 2020 04:37

davidjumani marked this pull request as ready for review October 6, 2020 04:37

sureshanaparti reviewed Oct 6, 2020
View reviewed changes

davidjumani requested a review from yadvr October 6, 2020 06:52

apache deleted a comment from blueorangutan Oct 15, 2020

DaanHoogland approved these changes Oct 16, 2020
View reviewed changes

Changing list name
f5fe0b3

davidjumani requested a review from sureshanaparti October 16, 2020 16:25

sureshanaparti approved these changes Oct 16, 2020
View reviewed changes

apache deleted a comment from blueorangutan Oct 20, 2020

apache deleted a comment from yadvr Oct 20, 2020

DaanHoogland added Severity:Minor type:bug labels Oct 20, 2020

DaanHoogland merged commit 81c524e into apache:masterOct 24, 2020

DaanHoogland deleted the fix-findhostsformigration branch October 24, 2020 11:08

davidjumani mentioned this pull request Nov 24, 2020
Fix hosts for migration count #4500
Merged
12 tasks

Uh oh!

Conversation

davidjumani commented Oct 5, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

Uh oh!

davidjumani commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

davidjumani commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 5, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

davidjumani commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

blueorangutan commented Oct 6, 2020

Uh oh!

davidjumani commented Oct 6, 2020

Uh oh!

sureshanaparti commented Oct 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidjumaniOct 6, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 12, 2020

Uh oh!

blueorangutan commented Oct 12, 2020

Uh oh!

blueorangutan commented Oct 12, 2020

Uh oh!

DaanHoogland commented Oct 12, 2020

Uh oh!

blueorangutan commented Oct 12, 2020

Uh oh!

DaanHoogland commented Oct 13, 2020

Uh oh!

DaanHoogland commented Oct 15, 2020

Uh oh!

blueorangutan commented Oct 15, 2020

Uh oh!

blueorangutan commented Oct 15, 2020

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 17, 2020

Uh oh!

blueorangutan commented Oct 17, 2020

Uh oh!

blueorangutan commented Oct 17, 2020

Uh oh!

DaanHoogland commented Oct 20, 2020

Uh oh!

blueorangutan commented Oct 20, 2020

davidjumani commented Oct 5, 2020•
edited
Loading

davidjumaniOct 6, 2020•
edited
Loading