CloudStack fails to migrate VM with volume when there are datadisks attatched #5410

GabrielBrascher · 2021-09-06T20:58:17Z

Description

This PR fixes issue #5407. In summary, volumes that should not be migrated are still being mapped in VirtualMachineManagerImpl.createStoragePoolMappingsForVolumes.

To give some context: method createStoragePoolMappingsForVolumes is used only when migrating a VM with its volume(s), therefore via the API call migrateVirtualMachineWithVolume.

In such cases, there are two options:
A. User does not provide volumes, just the VM ID, and Host ID
B. User provides VM ID, Host ID, and a volume map, e.g. migrateto[0].volume=71f43cd6-69b0-4d3b-9fbc-67f50963d60b&migrateto[0].pool=a382f181-3d2b-4413-b92d-b8931befa7e1&migrateto[1].volume=88de0173-55c0-4c1c-a269-83d0279eeedf&migrateto[1].pool=95d6e97c-6766-4d67-9a30-c449c15011d1.

However, regardless of case A or B, createStoragePoolMappingsForVolumes iterates all the VM's volumes and adds to be migrated. Therefore, this causes exceptions when migrating VMs with data disk attached whenever the data-disk (shared) should not be migrated.

From my point of view, it does not make sense to add volumes that the user has not mapped to be migrated unless the VM is migrated to (I) different cluster or (II) the volume scope is LOCAL. Both of these cases are already covered by the if statement previous to the removed line in this PR.

if (ScopeType.HOST.equals(currentPool.getScope()) || isStorageCrossClusterMigration(plan.getClusterId(), currentPool)) {
                createVolumeToStoragePoolMappingIfPossible(profile, plan, volumeToPoolObjectMap, volume, currentPool);
}

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

How Has This Been Tested?

Migrate VM with Root disk in local storage.

Tests have been done via the UI as well as the API. Also, it considered on both cases:
A. User does not provide volumes, just the VM ID, and HostID;
B. User provides VM ID, and HostID, and a volume map, e.g. migrateto[0].volume=71f43cd6-69b0-4d3b-9fbc-67f50963d60b&migrateto[0].pool=a382f181-3d2b-4413-b92d-b8931befa7e1&migrateto[1].volume=88de0173-55c0-4c1c-a269-83d0279eeedf&migrateto[1].pool=95d6e97c-6766-4d67-9a30-c449c15011d1.

In both cases, the volume map has been done accordingly with the expected behavior.

VMs that fail to be migrated, have been successfully migrated and only the correct volume was migrated; data disks remained attached to the VM, at their storage pool, and in "Ready" state.

GabrielBrascher · 2021-09-06T21:01:11Z

This has been tested with KVM.

Pinging some of the experts in managed storages (@mike-tutkowski @slavkap) to check if there are any cases I am missing where such volume mapping is correct.

Also, I would appreciate feedback regarding different hypervisors, such as VMware and Xen (@rhtyd @shwstppr @nvazquez @DaanHoogland).

nvazquez

I think this would break at least Vmware offline migrations (https://github.com/apache/cloudstack/blob/main/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java#L1314) cc. @rhtyd @shwstppr @weizhouapache @sureshanaparti @davidjumani - any input on this?

shwstppr · 2021-09-07T07:41:09Z

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

-            } else {
-                volumeToPoolObjectMap.put(volume, currentPool);


@GabrielBrascher as you have seen allVolumes is a confusing name here. It is only the volumes that are not mapped using API. I've not tested but VMware might expect mappings for each of the volumes of the VM.
I feel instead of preventing adding a map here, we should look into the code that does migration and skip volumes that are already on the destination pool. For KVM this could be,
https://github.com/apache/cloudstack/blob/main/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java#L1787
https://github.com/apache/cloudstack/blob/main/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java#L1978-L1980

shwstppr · 2021-09-07T08:37:44Z

@blueorangutan package

blueorangutan · 2021-09-07T08:38:02Z

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

blueorangutan · 2021-09-07T09:15:30Z

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

shwstppr · 2021-09-07T11:53:37Z

Tested offline migration on VMware env and the volume for which mapping was not provided in API call got detached from the VM in vCenter.

Migrated a stopped with a ROOT and a DATA disk within the cluster.
Both volumes were on same primary store before migration - ps1
Migrated with migrateVirtualMachineWithVolume API by passing a mapping for ROOT volume for primary store ps1-1

GabrielBrascher · 2021-09-07T11:58:05Z

Thanks for reviewing and testing @shwstppr @nvazquez!

I will check different approaches as this could impact VMware migrations.
When another approach is ready for review I will ping you.

GabrielBrascher · 2021-09-08T20:45:42Z

@shwstppr @nvazquez PR is ready for review.

GabrielBrascher · 2021-09-08T22:39:35Z

I've found some issues when having Local + NFS attached, with Local + RBD data disk all works fine.

I will check the workflow with the NFS tests feedback.

rohityadavcloud · 2021-09-13T09:10:05Z

@blueorangutan package

blueorangutan · 2021-09-13T09:12:06Z

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

blueorangutan · 2021-09-13T10:39:59Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 1209

nvazquez · 2021-09-14T05:16:36Z

@blueorangutan package

blueorangutan · 2021-09-14T05:17:03Z

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

blueorangutan · 2021-09-14T06:19:34Z

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

GabrielBrascher · 2021-09-14T14:25:20Z

I am running a couple more tests. However, considering that the 4.16 cut will be soon, I might reduce the scope to fix only RBD (datadisk) + Local (Root). And then block migrations with NFS (datadisk) + Local (Root).

sureshanaparti · 2021-09-17T09:10:53Z

@blueorangutan test

blueorangutan · 2021-09-17T09:12:07Z

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

blueorangutan · 2021-10-06T19:10:03Z

@nvazquez a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

sureshanaparti · 2021-10-07T04:34:20Z

@blueorangutan test matrix

blueorangutan · 2021-10-07T04:35:03Z

@sureshanaparti a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2021-10-07T04:44:55Z

Trillian test result (tid-2340)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33114 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2340-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2021-10-07T04:46:16Z

Trillian test result (tid-2339)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33255 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2339-xenserver-71.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2021-10-07T05:51:40Z

Trillian test result (tid-2341)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37299 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2341-vmware-65u2.zip
Smoke tests completed. 90 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_create_pvlan_network	`Error`	0.03	test_pvlan.py

nvazquez · 2021-10-07T12:24:13Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-10-07T12:27:02Z

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

blueorangutan · 2021-10-07T12:31:09Z

Trillian Build Failed (tid-2348)

nvazquez · 2021-10-07T12:45:53Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-10-07T12:47:03Z

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

blueorangutan · 2021-10-07T23:06:43Z

Trillian test result (tid-2349)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36103 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5410-t2349-vmware-67u3.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez

LGTM

GutoVeronezi

LGTM

rohityadavcloud · 2021-10-08T03:30:52Z

@sureshanaparti @vladimirpetrov @borisstoyanov @nvazquez do we need any further manual testing before merging this?

sureshanaparti · 2021-10-08T03:37:40Z

@sureshanaparti @vladimirpetrov @borisstoyanov @nvazquez do we need any further manual testing before merging this?

@rhtyd yes, requires some manual tests for VMware migration cases.

sureshanaparti · 2021-10-08T06:19:30Z

@GabrielBrascher @nvazquez @rhtyd Tested the following VM migration with volume(s) scenarios, with VMware 6.7.0. All passed.

VM With local Root disk
(i) Without volumes mapping (in same cluster)
(ii) With ROOT volume mapping (in same cluster)
(iii) Without volumes mapping (across different clusters, in the same pod)
VM With local Root disk + Shared (NFS) DATA disk
(i) Without volumes mapping (in same cluster)
(ii) With ROOT volume mapping (in same cluster)
(iii) With DATA volume mapping (in same cluster)
(iv) With ROOT and DATA volumes mapping (in same cluster)
(v) Without volumes mapping (across different clusters, in the same pod)
(vi) With ROOT volume mapping (across different clusters, in the same pod)
(vii) With ROOT and DATA volumes mapping (across different clusters, in the same pod)

Noticed issue (#5558) with storage type/offering for the volume, not related to this PR.

GabrielBrascher added this to the 4.16.0.0 milestone Sep 6, 2021

GabrielBrascher self-assigned this Sep 6, 2021

GabrielBrascher mentioned this pull request Sep 6, 2021

KVM: Fail to migrate VM with local root disk and datadisk attached #5407

Closed

nvazquez reviewed Sep 7, 2021

View reviewed changes

shwstppr reviewed Sep 7, 2021

View reviewed changes

GabrielBrascher marked this pull request as draft September 7, 2021 11:58

GabrielBrascher force-pushed the kvm-local-and-datadisk-migrate branch 4 times, most recently from 2925a11 to 9dfd1ec Compare September 8, 2021 20:38

GabrielBrascher marked this pull request as ready for review September 8, 2021 20:45

nvazquez added component:kvm status:needs-testing labels Sep 14, 2021

apache deleted a comment from blueorangutan Oct 6, 2021

sureshanaparti removed the status:needs-testing label Oct 7, 2021

apache deleted a comment from blueorangutan Oct 7, 2021

nvazquez approved these changes Oct 8, 2021

View reviewed changes

rohityadavcloud requested review from GutoVeronezi, borisstoyanov, shwstppr, sureshanaparti and vladimirpetrov October 8, 2021 02:54

GutoVeronezi approved these changes Oct 8, 2021

View reviewed changes

rohityadavcloud added the status:needs-testing label Oct 8, 2021

sureshanaparti removed the status:needs-testing label Oct 8, 2021

sureshanaparti merged commit 404e264 into apache:main Oct 8, 2021

weizhouapache mentioned this pull request Mar 16, 2022

Fix migration of VM with volume on Ubuntu #6116

Merged

12 tasks

CloudStack fails to migrate VM with volume when there are datadisks attatched #5410

CloudStack fails to migrate VM with volume when there are datadisks attatched #5410

Uh oh!

Conversation

GabrielBrascher commented Sep 6, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

GabrielBrascher commented Sep 6, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr Sep 7, 2021 • 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!

shwstppr commented Sep 7, 2021

Uh oh!

blueorangutan commented Sep 7, 2021

Uh oh!

blueorangutan commented Sep 7, 2021

Uh oh!

shwstppr commented Sep 7, 2021

Uh oh!

GabrielBrascher commented Sep 7, 2021

Uh oh!

GabrielBrascher commented Sep 8, 2021

Uh oh!

GabrielBrascher commented Sep 8, 2021

Uh oh!

rohityadavcloud commented Sep 13, 2021

Uh oh!

blueorangutan commented Sep 13, 2021

Uh oh!

blueorangutan commented Sep 13, 2021

Uh oh!

nvazquez commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

blueorangutan commented Sep 14, 2021

Uh oh!

GabrielBrascher commented Sep 14, 2021

Uh oh!

sureshanaparti commented Sep 17, 2021

Uh oh!

blueorangutan commented Sep 17, 2021

Uh oh!

blueorangutan commented Oct 6, 2021

Uh oh!

sureshanaparti commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

nvazquez commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

nvazquez commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

blueorangutan commented Oct 7, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr Sep 7, 2021 •

edited

Loading

sureshanaparti commented Oct 8, 2021 •

edited

Loading