vmware: fix migrate vm with volume by shwstppr · Pull Request #5170 · apache/cloudstack

shwstppr · Jun 30, 2021

Description

Recent forward merge of 4.15 branch accidentally brought a bug in VM relocation method for VMware while trying to find datastore for the migrated volume.
This PR fixes it by using either of available target or source host.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

[root@pr5030-t1171-vmware-65u2-marvin marvin]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=./pr5030-t1171-vmware-65u2-advanced-cfg -s -a tags=advanced --hypervisor=VMware tests/smoke/test_vm_life_cycle.py

==== Marvin Init Started ====

=== Marvin Parse Config Successful ===

=== Marvin Setting TestData Successful===

==== Log Folder Path: /marvin/MarvinLogs/Jun_30_2021_10_47_36_DT690J. All logs will be available here ====

=== Marvin Init Logging Successful===

==== Marvin Init Successful ====
=== TestName: test_advZoneVirtualRouter | Status : SUCCESS ===

=== TestName: test_deploy_vm | Status : SUCCESS ===

=== TestName: test_deploy_vm_multiple | Status : SUCCESS ===

=== TestName: test_01_migrate_VM_and_root_volume | Status : SUCCESS ===

=== TestName: test_02_migrate_VM_with_two_data_disks | Status : SUCCESS ===

=== TestName: test_03_migrate_detached_volume | Status : SUCCESS ===

=== TestName: test_01_unmanage_vm_cycle | Status : SUCCESS ===
...

Recent forward merge of 4.15 branch accidentally brought a bug in VM relocation method for VMware while trying to find datastore for the migrated volume. This PR fixes it by using either of available target or source host. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

weizhouapache · Jun 30, 2021

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

                            newVol.setDataStoreUuid(entry.second().getUuid());
                            String newPath = vmMo.getVmdkFileBaseName(disk);
-                            ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost, entry.second().getUuid());
+                            ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost != null ? targetHyperHost : sourceHyperHost, entry.second().getUuid());


@shwstppr might sourceHyperHost be null as well ?

@weizhouapache Method gets sourceHyperHost either from caller or the host which is processing the command,
https://github.com/apache/cloudstack/pull/5170/files#diff-77635c5f7eb9cef514aaf69e2fdc9217619afc9005bf8db9585c176e7401a88bR7329-R7331
Cannot be null if the command is being processed

@shwstppr great, thanks.

no idea if it breaks some features. at least the NPE is fixed by this pr.

Above ^^ is my understanding. Let me know if there should be additional checks @weizhouapache

Change was originally introduced here, d2ab350

@shwstppr if the origin is a merge commit than this should be fixed on the older branch and merged forward.

shwstppr · Jun 30, 2021

@blueorangutan package

blueorangutan · Jun 30, 2021

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

blueorangutan · Jun 30, 2021

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 432

weizhouapache · Jun 30, 2021

@blueorangutan test centos7 vmware-65u2

blueorangutan · Jun 30, 2021

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

DaanHoogland

clgtm but please change base branch to 4.15

DaanHoogland · Jun 30, 2021

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

                            newVol.setDataStoreUuid(entry.second().getUuid());
                            String newPath = vmMo.getVmdkFileBaseName(disk);
-                            ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost, entry.second().getUuid());
+                            ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost != null ? targetHyperHost : sourceHyperHost, entry.second().getUuid());


@shwstppr if the origin is a merge commit than this should be fixed on the older branch and merged forward.

shwstppr · Jun 30, 2021

@DaanHoogland 4.15 has a different code. Change that resulted in error was added in d2ab350 to probably address merge conflicts cc @nvazquez

nvazquez · Jun 30, 2021

Thanks @shwstppr I have resolved conflicts to merge 4.15 into main, did I miss those changes? Most of the conflicts were from this PR: #5097 as the logic differs from 4.15 and main in those files

shwstppr · Jun 30, 2021

@nvazquez no, I don't think anything was missed. Just that line of code that finds datastore for the migrated volume needed small change as targetHyperHost can be null in relocateVirtualMachine method. https://github.com/apache/cloudstack/pull/5097/files#diff-77635c5f7eb9cef514aaf69e2fdc9217619afc9005bf8db9585c176e7401a88bR4808

And since the relocateVirtualMachine method is not there in 4.15 code, changes from this PR are not needed there.

shwstppr · Jul 1, 2021

@blueorangutan test centos7 vmware-65u2

blueorangutan · Jul 1, 2021

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

blueorangutan · Jul 1, 2021

Trillian test result (tid-1185)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33856 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5170-t1185-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 85 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_VPC_nics_after_destroy	`Error`	5.65	test_vpc_router_nics.py
test_02_VPC_default_routes	`Error`	5.62	test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpn	`Failure`	8.39	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	6.33	test_vpc_vpn.py
test_01_vpc_remote_access_vpn	`Failure`	3.16	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	5.31	test_vpc_vpn.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Error`	0.04	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	0.03	test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure	`Error`	0.03	test_host_maintenance.py

blueorangutan · Jul 2, 2021

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 455

shwstppr · Jul 2, 2021

Though migration tests from test_vm_lifecycle.py seem to be running fine, re-running tests to verify intermittent failures

@blueorangutan test centos7 vmware-65u2

blueorangutan · Jul 2, 2021

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

concerns answerred

shwstppr · Jul 4, 2021

@blueorangutan test centos7 vmware-65u2

blueorangutan · Jul 4, 2021

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

blueorangutan · Jul 5, 2021

Trillian test result (tid-1198)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50235 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5170-t1198-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 85 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_ping_in_vr_success	`Failure`	15.34	test_diagnostics.py
test_05_ping_in_cpvm_success	`Failure`	5.14	test_diagnostics.py
test_07_arping_in_vr	`Failure`	5.14	test_diagnostics.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	405.89	test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	617.55	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	563.75	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	597.71	test_vpc_redundant.py

...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

…vmware/resource/VmwareResource.java Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · Jul 5, 2021

@blueorangutan package

blueorangutan · Jul 5, 2021

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

blueorangutan · Jul 5, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 476

shwstppr · Jul 5, 2021

@blueorangutan test centos7 vmware-65u2

blueorangutan · Jul 5, 2021

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) has been kicked to run smoke tests

blueorangutan · Jul 6, 2021

Trillian test result (tid-1206)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35388 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5170-t1206-vmware-65u2.zip
Smoke tests completed. 88 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache

code lgtm

(did not look through the code changes in debug messages).

DaanHoogland

clgtm

weizhouapache reviewed Jun 30, 2021

View reviewed changes

DaanHoogland previously requested changes Jun 30, 2021

View reviewed changes

shwstppr closed this Jul 2, 2021

shwstppr reopened this Jul 2, 2021

shwstppr closed this Jul 5, 2021

shwstppr reopened this Jul 5, 2021

GutoVeronezi reviewed Jul 5, 2021

View reviewed changes

shwstppr and others added 2 commits July 5, 2021 18:14

Update plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/…

ad8e93b

…vmware/resource/VmwareResource.java Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

fix

6d663a6

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr closed this Jul 5, 2021

shwstppr reopened this Jul 5, 2021

shwstppr requested review from DaanHoogland, harikrishna-patnala, weizhouapache and yadvr July 6, 2021 05:57

weizhouapache approved these changes Jul 6, 2021

View reviewed changes

weizhouapache mentioned this pull request Jul 8, 2021

server: Optional destination host when migrate a vm #4378

Merged

5 tasks

shwstppr mentioned this pull request Jul 9, 2021

Remove condition that are prevent resizing for root volumes (vmware) #5186

Merged

12 tasks

DaanHoogland approved these changes Jul 13, 2021

View reviewed changes

shwstppr requested a review from nvazquez July 13, 2021 11:41

yadvr added this to the 4.16.0.0 milestone Jul 15, 2021

yadvr merged commit 757bc2d into apache:main Jul 15, 2021

nvazquez added component:migration component:vmware type:bug labels Oct 14, 2021

Search code, repositories, users, issues, pull requests...

Conversation

shwstppr commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

weizhouapache Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

weizhouapache Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jun 30, 2021

Uh oh!

blueorangutan commented Jun 30, 2021

Uh oh!

blueorangutan commented Jun 30, 2021

Uh oh!

weizhouapache commented Jun 30, 2021

Uh oh!

blueorangutan commented Jun 30, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jun 30, 2021

Uh oh!

nvazquez commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shwstppr commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shwstppr commented Jul 1, 2021

Uh oh!

blueorangutan commented Jul 1, 2021

Uh oh!

blueorangutan commented Jul 1, 2021

Uh oh!

blueorangutan commented Jul 2, 2021

Uh oh!

shwstppr commented Jul 2, 2021

Uh oh!

blueorangutan commented Jul 2, 2021

Uh oh!

shwstppr commented Jul 4, 2021

Uh oh!

blueorangutan commented Jul 4, 2021

Uh oh!

blueorangutan commented Jul 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shwstppr commented Jul 5, 2021

Uh oh!

blueorangutan commented Jul 5, 2021

Uh oh!

blueorangutan commented Jul 5, 2021

shwstppr commented Jun 30, 2021 •

edited

Loading

nvazquez commented Jun 30, 2021 •

edited

Loading

shwstppr commented Jun 30, 2021 •

edited

Loading