vmware: fix inter-cluster stopped vm and volume migration by shwstppr · Pull Request #4895 · apache/cloudstack

shwstppr · 2021-04-05T11:26:07Z

Description

For inter-cluster migration without shared storage, VMware needs a host to be specified. Fix is to specify an appropriate host in the target cluster during a stopped VM migration. Also, find target datastore using the host in the target cluster.

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?

Fixesapache#4838 For inter-cluster migration without shared storage, VMware needs a host to be specified. Fix is to specify an appropriate host in the target cluster. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

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

shwstppr · 2021-04-06T07:40:28Z

@blueorangutan package

blueorangutan · 2021-04-06T07:41:02Z

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

blueorangutan · 2021-04-06T08:19:12Z

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

shwstppr · 2021-04-06T08:32:52Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-04-06T08:34:02Z

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

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

DaanHoogland

mostly structural change requests so far.

DaanHoogland · 2021-04-06T10:17:41Z

+ List<VolumeVO> volumes = _volumeDao.findByInstanceAndType(vmId, Volume.Type.ROOT);
+ if (CollectionUtils.isNotEmpty(volumes)) {
+ VolumeVO rootVolume = volumes.get(0);
+ if (rootVolume.getPoolId() != null) {
+ StoragePoolVO pool = _storagePoolDao.findById(rootVolume.getPoolId());
+ if (pool != null && pool.getClusterId() != null) {
+ clusterId = pool.getClusterId();
+ }
+ }
+ }


please extract into a separate method. i.e. Long getPoolFromRootVolume(...)

@DaanHoogland done

DaanHoogland · 2021-04-06T10:19:54Z

+ final boolean isInterClusterMigration = srcClusterId != null && destClusterId != null && ! srcClusterId.equals(destClusterId);
+ Host hostInTargetCluster = null;
+ if (isInterClusterMigration) {
+ // Without host vMotion might fail between non-shared storages with error similar to,
+ // https://kb.vmware.com/s/article/1003795
+ // As this is offline migration VM won't be started on this host
+ List<HostVO> hosts = _hostDao.findHypervisorHostInCluster(destClusterId);
+ if (CollectionUtils.isNotEmpty(hosts)) {
+ hostInTargetCluster = hosts.get(0);
+ }
+ if (hostInTargetCluster == null) {
+ throw new CloudRuntimeException("Migration failed, unable to find suitable target host for VM placement while migrating between storage pools of different clusters without shared storages");
+ }
+ }
+ MigrateVmToPoolCommand migrateVmToPoolCommand = new MigrateVmToPoolCommand(vm.getInstanceName(),
+ vols, destination.getUuid(), hostInTargetCluster == null ? null : hostInTargetCluster.getGuid(), true);
+ commands.add(migrateVmToPoolCommand);


please restructure this in smaller methods

@DaanHoogland done

DaanHoogland · 2021-04-06T10:22:14Z

- private Answer migrateAndAnswer(VirtualMachineMO vmMo, String poolUuid, VmwareHypervisorHost hyperHost, Command cmd) throws Exception {
- ManagedObjectReference morDs = getTargetDatastoreMOReference(poolUuid, hyperHost);
-
+ private Answer migrateAndAnswer(VirtualMachineMO vmMo, String poolUuid, VmwareHypervisorHost sourceHyperHost,


please restructure this method (at least extract the new pieces)

@DaanHoogland done

DaanHoogland · 2021-04-06T10:23:01Z

 morDcOfTargetHost = tgtHyperHost.getHyperHostDatacenter();
 if (!morDc.getValue().equalsIgnoreCase(morDcOfTargetHost.getValue())) {
- String msg = "Source host & target host are in different datacentesr";
+ String msg = "Source host & target host are in different datacenter";


DaanHoogland · 2021-04-06T10:23:27Z

+ VmwareHypervisorHost hyperHostInTargetCluster = null;
+ if (cmd.getHostGuidInTargetCluster() != null) {
+ hyperHostInTargetCluster = VmwareHelper.getHostMOFromHostName(getServiceContext(), cmd.getHostGuidInTargetCluster());
+ }
+ VmwareHypervisorHost targetDSHost = hyperHostInTargetCluster != null ? hyperHostInTargetCluster : hyperHost;


this merrits a call to a specific method

@DaanHoogland which specific method? I didn't get you

something like
Suggested change
VmwareHypervisorHosthyperHostInTargetCluster = null;
if (cmd.getHostGuidInTargetCluster() != null) {
hyperHostInTargetCluster = VmwareHelper.getHostMOFromHostName(getServiceContext(), cmd.getHostGuidInTargetCluster());
}
VmwareHypervisorHosttargetDSHost = hyperHostInTargetCluster != null ? hyperHostInTargetCluster : hyperHost;
VmwareHypervisorHosttargetDSHost = getTargetHostFromTargetCluster(cmd);
and
Suggested change
VmwareHypervisorHosthyperHostInTargetCluster = null;
if (cmd.getHostGuidInTargetCluster() != null) {
hyperHostInTargetCluster = VmwareHelper.getHostMOFromHostName(getServiceContext(), cmd.getHostGuidInTargetCluster());
}
VmwareHypervisorHosttargetDSHost = hyperHostInTargetCluster != null ? hyperHostInTargetCluster : hyperHost;
privateVmwareHypervisorHostgetTargetHostFromTargetCluster(MigrateVolumeCmdcmd) {
VmwareHypervisorHosthyperHostInTargetCluster = null;
if (cmd.getHostGuidInTargetCluster() != null) {
hyperHostInTargetCluster = VmwareHelper.getHostMOFromHostName(getServiceContext(), cmd.getHostGuidInTargetCluster());
}
returnhyperHostInTargetCluster != null ? hyperHostInTargetCluster : hyperHost;
}

@DaanHoogland both hyperHostInTargetCluster and targetDSHost are used in the method later multiple times. I used two different variables to prevent using if..else multiple times.

DaanHoogland · 2021-04-06T10:24:42Z

+ String hardwareVersion = null;
+ if (hyperHostInTargetCluster != null) {
+ Integer sourceHardwareVersion = HypervisorHostHelper.getHostHardwareVersion(hyperHost);
+ Integer destinationHardwareVersion = HypervisorHostHelper.getHostHardwareVersion(hyperHostInTargetCluster);
+ if (sourceHardwareVersion != null && destinationHardwareVersion != null && !sourceHardwareVersion.equals(destinationHardwareVersion)) {
+ hardwareVersion = String.valueOf(Math.min(sourceHardwareVersion, destinationHardwareVersion));
+ }
+ }


please extract

@DaanHoogland done

DaanHoogland · 2021-04-06T10:27:44Z

+ ScopeType targetScopeType = destData.getDataStore().getScope().getScopeType();
+ Long hostId = null;
+ String hostGuidInTargetCluster = null;
+ if (ScopeType.CLUSTER.equals(sourceScopeType)) {
+ // Find Volume source cluster and select any Vmware hypervisor host to attach worker VM
+ hostId = findSuitableHostIdForWorkerVmPlacement(sourcePool.getClusterId());
+ if (hostId == null) {
+ throw new CloudRuntimeException("Offline Migration failed, unable to find suitable host for worker VM placement in the cluster of storage pool: " + sourcePool.getName());
+ }
+ if (ScopeType.CLUSTER.equals(targetScopeType) && !sourcePool.getClusterId().equals(targetPool.getClusterId())) {
+ // Without host vMotion might fail between non-shared storages with error similar to,
+ // https://kb.vmware.com/s/article/1003795
+ List<HostVO> hosts = hostDao.findHypervisorHostInCluster(targetPool.getClusterId());
+ if (CollectionUtils.isNotEmpty(hosts)) {
+ hostGuidInTargetCluster = hosts.get(0).getGuid();
+ }
+ if (hostGuidInTargetCluster == null) {
+ throw new CloudRuntimeException("Offline Migration failed, unable to find suitable target host for worker VM placement while migrating between storage pools of different cluster without shared storages");
+ }
+ }
+ } else if (ScopeType.CLUSTER.equals(targetScopeType)) {
+ hostId = findSuitableHostIdForWorkerVmPlacement(targetPool.getClusterId());
+ if (hostId == null) {
+ throw new CloudRuntimeException("Offline Migration failed, unable to find suitable host for worker VM placement in the cluster of storage pool: " + targetPool.getName());
+ }
+ }


please extract getTargetScopeType(..) and possibly more methods from that.

@DaanHoogland done

blueorangutan · 2021-04-06T19:13:39Z

Trillian test result (tid-357)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37216 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4895-t357-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_deploy_kubernetes_cluster	`Failure`	91.50	test_kubernetes_clusters.py
test_02_invalid_upgrade_kubernetes_cluster	`Failure`	82.93	test_kubernetes_clusters.py
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	61.53	test_kubernetes_clusters.py
test_04_deploy_and_scale_kubernetes_cluster	`Failure`	65.72	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	71.91	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	80.17	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	71.87	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	66.15	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	110.82	test_kubernetes_clusters.py
ContextSuite context=TestVAppsVM>:setup	`Error`	43.81	test_vm_life_cycle.py

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

shwstppr · 2021-04-07T06:45:02Z

@blueorangutan package

blueorangutan · 2021-04-07T06:46:03Z

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

blueorangutan · 2021-04-07T07:10:14Z

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

shwstppr · 2021-04-07T11:26:34Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-04-07T11:27:04Z

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

blueorangutan · 2021-04-07T22:34:01Z

Trillian test result (tid-370)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38520 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4895-t370-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_diagnostics.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 83 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_10_traceroute_in_vr	`Failure`	61.23	test_diagnostics.py
test_01_deploy_kubernetes_cluster	`Failure`	96.60	test_kubernetes_clusters.py
test_02_invalid_upgrade_kubernetes_cluster	`Failure`	99.39	test_kubernetes_clusters.py
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	65.70	test_kubernetes_clusters.py
test_04_deploy_and_scale_kubernetes_cluster	`Failure`	64.58	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	58.55	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	54.61	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	66.72	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	82.07	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	121.47	test_kubernetes_clusters.py
ContextSuite context=TestVAppsVM>:setup	`Error`	44.65	test_vm_life_cycle.py
test_01_redundant_vpc_site2site_vpn	`Failure`	373.50	test_vpc_vpn.py

VM can have multiple ROOT volumes and some can be on zone-wide store therefore iterate over all of them till a cluster ID is found. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2021-04-08T05:05:14Z

@blueorangutan package

blueorangutan · 2021-04-08T05:06:02Z

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

blueorangutan · 2021-04-08T05:27:39Z

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

borisstoyanov · 2021-04-08T07:46:48Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-04-08T07:48:02Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

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

shwstppr · 2021-04-08T10:13:57Z

@blueorangutan package

blueorangutan · 2021-04-08T10:15:02Z

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

blueorangutan · 2021-04-08T10:44:20Z

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

borisstoyanov · 2021-04-08T11:51:06Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-04-08T11:52:02Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

blueorangutan · 2021-04-08T21:53:48Z

Trillian test result (tid-385)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35028 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4895-t385-vmware-67u3.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 85 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_deploy_kubernetes_cluster	`Failure`	89.55	test_kubernetes_clusters.py
test_02_invalid_upgrade_kubernetes_cluster	`Failure`	56.36	test_kubernetes_clusters.py
test_03_deploy_and_upgrade_kubernetes_cluster	`Failure`	55.27	test_kubernetes_clusters.py
test_04_deploy_and_scale_kubernetes_cluster	`Failure`	56.29	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	53.22	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	100.16	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	59.30	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	57.29	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	101.95	test_kubernetes_clusters.py
ContextSuite context=TestVAppsVM>:setup	`Error`	45.65	test_vm_life_cycle.py

borisstoyanov

LGTM, tested on VMware with single and successive migration of VM

yadvr · 2021-04-09T07:06:50Z

@nvazquez are you lgtm on the PR? Will hold merging until your confirmation.

nvazquez

Thanks @shwstppr for addressing the comments, LGTM

yadvr · 2021-04-10T07:52:44Z

Merged based on Bobby, Nicolas's review and testing, and smoketests.

vmware: fix inter-cluster stopped vm migration
54ef6e7
Fixesapache#4838 For inter-cluster migration without shared storage, VMware needs a host to be specified. Fix is to specify an appropriate host in the target cluster. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr added this to the 4.15.1.0 milestone Apr 5, 2021

yadvr requested review from DaanHoogland, harikrishna-patnala and nvazquez April 5, 2021 11:49

fix detached volume inter-cluster migration
7cc6b6e
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr changed the title ~~vmware: fix inter-cluster stopped vm migration~~vmware: fix inter-cluster stopped vm and volume migrationApr 6, 2021

shwstppr marked this pull request as ready for review April 6, 2021 07:40

cleanup unused method
ee6aacd
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

DaanHoogland reviewed Apr 6, 2021
View reviewed changes

yadvr assigned nvazquez Apr 6, 2021

shwstppr added 2 commits April 7, 2021 11:54

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

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

shwstppr mentioned this pull request Apr 7, 2021
vmware: fix stopped VM volume migration #4758
Merged
12 tasks

nvazquez reviewed Apr 8, 2021
View reviewed changes

Comment threadplugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java Outdated
Comment threadvmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java
Comment threadvmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java

find vm clusterid with multiple ROOT volumes
9973e57
VM can have multiple ROOT volumes and some can be on zone-wide store therefore iterate over all of them till a cluster ID is found. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

fix successive storage migration
2356894
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

borisstoyanov approved these changes Apr 9, 2021
View reviewed changes

yadvr approved these changes Apr 9, 2021
View reviewed changes

yadvr requested a review from nvazquez April 9, 2021 07:38

nvazquez approved these changes Apr 9, 2021
View reviewed changes

yadvr merged commit fdefee7 into apache:4.15Apr 10, 2021

yadvr mentioned this pull request Apr 10, 2021
VMware: VM operations fail after inter-cluster storage migration #4838
Closed

weizhouapache mentioned this pull request Jul 22, 2021
Migration not working in ACS 4.14 with Basic Network - VMware Local storage #4187
Closed

-VmwareHypervisorHosthyperHostInTargetCluster = null;
-if (cmd.getHostGuidInTargetCluster() != null) {
-hyperHostInTargetCluster = VmwareHelper.getHostMOFromHostName(getServiceContext(), cmd.getHostGuidInTargetCluster());
- }
-VmwareHypervisorHosttargetDSHost = hyperHostInTargetCluster != null ? hyperHostInTargetCluster : hyperHost;
+VmwareHypervisorHosttargetDSHost = getTargetHostFromTargetCluster(cmd);

Uh oh!

Conversation

shwstppr commented Apr 5, 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!

shwstppr commented Apr 6, 2021

Uh oh!

blueorangutan commented Apr 6, 2021

Uh oh!

blueorangutan commented Apr 6, 2021

Uh oh!

shwstppr commented Apr 6, 2021

Uh oh!

blueorangutan commented Apr 6, 2021

Uh oh!

DaanHoogland left a comment

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!

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!

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!

blueorangutan commented Apr 6, 2021

Uh oh!

shwstppr commented Apr 7, 2021

Uh oh!

blueorangutan commented Apr 7, 2021

Uh oh!

blueorangutan commented Apr 7, 2021

Uh oh!

shwstppr commented Apr 7, 2021

Uh oh!

blueorangutan commented Apr 7, 2021

Uh oh!

blueorangutan commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shwstppr commented Apr 8, 2021

Uh oh!

blueorangutan commented Apr 8, 2021

Uh oh!

blueorangutan commented Apr 8, 2021

Uh oh!

borisstoyanov commented Apr 8, 2021

Uh oh!

shwstppr commented Apr 5, 2021•
edited
Loading

yadvr commented Apr 9, 2021•
edited
Loading