Fix migration from local storage to NFS in KVM by JoaoJandre · Pull Request #8909 · apache/cloudstack

JoaoJandre · 2024-04-12T18:52:35Z

Description

Using KVM as a hypervisor, when performing a live migration of a VM with a volume, if the volume is on local storage and the migration destination is an NFS storage, an error occurs during the migration. This happens because there is a validation to check whether the destination host has direct access to the source storage of the volume being migrated. However, this validation is not necessary, since the host that will perform the migration is the source host itself. Therefore, the destination host only needs access to the destination storage.

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Before this PR, the following live volume migration cases failed in KVM:

Volume	Origin	Destination
ROOT	Local	NFS1
DATA1	Local	NFS1

Volume	Origin	Destination
ROOT	Local	NFS1

With this PR, the above cases now work. Furthermore, the following cases where validated, to make sure that no regressions were introduced:

Volume	Origin	Destination
ROOT	NFS1	NFS2
DATA1	NFS1	NFS2

Volume	Origin	Destination
ROOT	NFS1	Local
DATA1	NFS1	Local

Volume	Origin	Destination
ROOT	NFS1	ISCSI1
DATA1	NFS1	ISCSI1

Volume	Origin	Destination
ROOT	ISCSI1	NFS1
DATA1	ISCSI1	NFS1

codecov-commenter · 2024-04-12T19:02:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.31%. Comparing base (c25264b) to head (0bec7da).
Report is 164 commits behind head on 4.19.

❗ Current head 0bec7da differs from pull request most recent head b8a77d2
Please upload reports for the commit b8a77d2 to get more accurate results.

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #8909 +/- ##
============================================
- Coverage 30.98% 4.31% -26.67% 
============================================
Files 5359 362 -4997 Lines 377017 29176 -347841 Branches 54856 5120 -49736 ============================================
- Hits 116804 1260 -115544 + Misses 244852 27774 -217078 + Partials 15361 142 -15219

Flag	Coverage Δ
simulator-marvin-tests	`?`
uitests	`4.31% <ø> (-0.06%)`	⬇️
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yadvr · 2024-04-12T19:36:23Z

@JoaoJandre is this also applicable fix for 4.19?

JoaoJandre · 2024-04-12T19:42:37Z

@JoaoJandre is this also applicable fix for 4.19?

@rohityadavcloud yes, I'll rebase with 4.19 and target it

sureshanaparti · 2024-04-15T06:56:43Z

@blueorangutan package

blueorangutan · 2024-04-15T06:58:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-04-15T07:13:26Z

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 9251

DaanHoogland · 2024-04-15T08:15:52Z

@JoaoJandre :

09:11:09 [ERROR] /jenkins/workspace/acs-centos7-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.19.1.0-SNAPSHOT/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java:26:15: Unused import - org.mockito.Mockito.never. [UnusedImports]
09:11:09 [ERROR] /jenkins/workspace/acs-centos7-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.19.1.0-SNAPSHOT/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java:51:8: Unused import - org.mockito.verification.VerificationMode. [UnusedImports]
09:11:09 [ERROR] /jenkins/workspace/acs-centos7-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.19.1.0-SNAPSHOT/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java:65:8: Unused import - java.util.LinkedList. [UnusedImports]

JoaoJandre · 2024-04-15T11:42:33Z

@blueorangutan package

blueorangutan · 2024-04-15T11:44:03Z

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-04-15T12:55:47Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9261

JoaoJandre · 2024-04-16T17:21:41Z

@sureshanaparti could we run the CI here?

DaanHoogland · 2024-04-17T07:53:20Z

@blueorangutan test

blueorangutan · 2024-04-17T07:54:04Z

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

blueorangutan · 2024-04-17T08:23:01Z

[SF] Trillian Build Failed (tid-9871)

blueorangutan · 2024-04-17T23:52:21Z

[SF] Trillian test result (tid-9875)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46521 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8909-t9875-kvm-centos7.zip
Smoke tests completed. 127 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_events_resource	`Error`	296.90	test_events_resource.py
test_01_events_resource	`Error`	296.91	test_events_resource.py
test_04_deploy_vm_for_other_user_and_test_vm_operations	`Failure`	84.97	test_network_permissions.py
ContextSuite context=TestNetworkPermissions>:teardown	`Error`	1.43	test_network_permissions.py

JoaoJandre · 2024-05-16T17:24:45Z

@blueorangutan package

blueorangutan · 2024-05-16T17:26:03Z

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

JoaoJandre · 2024-06-11T13:10:12Z

@JoaoJandre thanks for this fix, I'm trying to reproduce the issue and bug fix, what APIs are you using the replicate this situation. I'm questioning how do you migrate a volume with shared storage offering to local storage?

I did all tests using live VM + volume migration on the UI. I was able to migrate volumes from NFS to local and from local to NFS this way. Looking at the API call that the UI made, it was using migrateVirtualMachineWithVolume. Something like:

http://<mgmtserver>/client/api/?hostid=e8050fa0-c834-47c5-9a1e-089595aa2edc&virtualmachineid=27640463-d916-4e9b-b223-4f3c84fc828a&migrateto[0].volume=31df1fe1-53f2-4ff5-9b86-d75f6f2afe85&migrateto[0].pool=49a3f50d-e70c-4963-8e6a-f70e3a9d295d&command=migrateVirtualMachineWithVolume&response=json

borisstoyanov

Thanks @JoaoJandre, we have the option when changing disk offering to migrate to another storage if required, why not fix it there instead?

I think it's more lean approach to execute this operation since in the case you've described you might end up with volume on shared storage while having assigned offering of local storage and vice versa.
Currently migrating cross different types of storage seems not working when changing the offering ( returns 'Index 0 out of bounds for length 0' ), would you consider moving the fix there?

JoaoJandre · 2024-06-13T11:49:05Z

Thanks @JoaoJandre, we have the option when changing disk offering to migrate to another storage if required, why not fix it there instead?
I think it's more lean approach to execute this operation since in the case you've described you might end up with volume on shared storage while having assigned offering of local storage and vice versa. Currently migrating cross different types of storage seems not working ( returns 'Index 0 out of bounds for length 0' ), would you consider moving the fix there?

Hey @borisstoyanov,

I think you misunderstood the PR. The issue is not with the disk offering, but with a validation that makes no sense: the destination host does not need to have access to the original storage, regardless of the disk offering. This PR fixes the current validation to reflect that.

About ending up with a disk offering in a storage with different type, this is current behavior, as introduced in #2425. It is assumed that when the ROOT admin migrates the volume somewhere, they know what they are doing. Same applies to migrating VMs to hosts that do not have a certain tag, the root admin has this power.

I didn't understand your last point, are you saying that this PR broke migration across different types of storage, or that this is a current issue? if so, could you give me the link to the issue?

borisstoyanov

I apologise @JoaoJandre, I thought this was much deeper change, what you suggest is making sense now, and the issues I'm pointing out are not related to your PR.
LGTM

JoaoJandre · 2024-06-13T18:21:03Z

I apologise @JoaoJandre, I thought this was much deeper change, what you suggest is making sense now, and the issues I'm pointing out are not related to your PR. LGTM

No problem @borisstoyanov :)

yadvr · 2024-06-21T10:30:09Z

Re-kicking GH actions

yadvr · 2024-06-21T10:30:26Z

Is this otherwise good to merge @JoaoJandre @sureshanaparti @DaanHoogland ?
@blueorangutan package

blueorangutan · 2024-06-21T10:32:07Z

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-06-21T11:36:59Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10053

JoaoJandre · 2024-06-21T12:14:42Z

Is this otherwise good to merge @JoaoJandre @sureshanaparti @DaanHoogland ? @blueorangutan package

@rohityadavcloud Only need one more +1

blueorangutan · 2024-06-21T12:16:03Z

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with@rohityadavcloud Only need one more +1 SystemVM template(s). I'll keep you posted as I make progress.

blueorangutan · 2024-06-21T13:03:33Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10055

DaanHoogland

code looks good and trusting the testing done

sureshanaparti

clgtm, didn't test

sureshanaparti · 2024-06-21T13:14:50Z

@blueorangutan package

blueorangutan · 2024-06-21T13:16:04Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

DaanHoogland · 2024-06-21T13:29:38Z

@sureshanaparti always good to have one more round of smoke testing, but I think we can merge.

blueorangutan · 2024-06-21T14:19:38Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10058

DaanHoogland · 2024-06-21T14:37:08Z

@blueorangutan test alma9 kvm-alma9

blueorangutan · 2024-06-21T14:38:04Z

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

blueorangutan · 2024-06-22T04:09:12Z

[SF] Trillian test result (tid-10559)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 46931 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8909-t10559-kvm-alma9.zip
Smoke tests completed. 131 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

* fix migration from local to nfs * remove unused imports * remove dead code

JoaoJandre added this to the 4.20.0.0 milestone Apr 12, 2024

boring-cyborgBot added the component:storage label Apr 12, 2024

yadvr requested review from harikrishna-patnala and sureshanaparti April 12, 2024 19:35

fix migration from local to nfs
2f2d741

JoaoJandre force-pushed the fix-nfs-special-case branch from 0bec7da to 2f2d741Compare April 12, 2024 19:46

JoaoJandre changed the base branch from main to 4.19April 12, 2024 19:46

JoaoJandre modified the milestones: 4.20.0.0, 4.19.1.0 Apr 12, 2024

sureshanaparti added the status:needs-testing label Apr 15, 2024

remove unused imports
c6dfe21

JoaoJandre mentioned this pull request Apr 19, 2024
KVM Incremental Snapshots/Backups #8907
Closed

DaanHoogland mentioned this pull request Apr 24, 2024
Wrong mountpoint generated on secondary storage for ISO on live migration #8901
Closed

borisstoyanov reviewed Jun 12, 2024
View reviewed changes

borisstoyanov approved these changes Jun 13, 2024
View reviewed changes

borisstoyanov removed the status:needs-testing label Jun 19, 2024

borisstoyanov removed their assignment Jun 19, 2024

yadvr closed this Jun 21, 2024

yadvr reopened this Jun 21, 2024

sureshanaparti reviewed Jun 21, 2024
View reviewed changes

Comment thread...tion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java

sureshanaparti reviewed Jun 21, 2024
View reviewed changes

Comment thread...tion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java Outdated

remove dead code
b8a77d2

DaanHoogland approved these changes Jun 21, 2024
View reviewed changes

sureshanaparti approved these changes Jun 21, 2024
View reviewed changes

yadvr merged commit 3e30283 into apache:4.19Jun 24, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 2, 2024
Fix migration from local storage to NFS in KVM (apache#8909)
3ec5f1a
* fix migration from local to nfs * remove unused imports * remove dead code

Uh oh!

Conversation

JoaoJandre commented Apr 12, 2024• edited by DaanHoogland 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!

codecov-commenter commented Apr 12, 2024• edited by codecov BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yadvr commented Apr 12, 2024

Uh oh!

JoaoJandre commented Apr 12, 2024

Uh oh!

sureshanaparti commented Apr 15, 2024

Uh oh!

blueorangutan commented Apr 15, 2024

Uh oh!

blueorangutan commented Apr 15, 2024

Uh oh!

DaanHoogland commented Apr 15, 2024

Uh oh!

JoaoJandre commented Apr 15, 2024

Uh oh!

blueorangutan commented Apr 15, 2024

Uh oh!

blueorangutan commented Apr 15, 2024

Uh oh!

JoaoJandre commented Apr 16, 2024

Uh oh!

DaanHoogland commented Apr 17, 2024

Uh oh!

blueorangutan commented Apr 17, 2024

Uh oh!

blueorangutan commented Apr 17, 2024

Uh oh!

blueorangutan commented Apr 17, 2024

Uh oh!

JoaoJandre commented May 16, 2024

Uh oh!

blueorangutan commented May 16, 2024

Uh oh!

JoaoJandre commented Jun 11, 2024

Uh oh!

borisstoyanov left a comment • 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!

JoaoJandre commented Jun 13, 2024

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre commented Jun 13, 2024

Uh oh!

yadvr commented Jun 21, 2024

Uh oh!

yadvr commented Jun 21, 2024

Uh oh!

blueorangutan commented Jun 21, 2024

Uh oh!

blueorangutan commented Jun 21, 2024

Uh oh!

JoaoJandre commented Jun 21, 2024

Uh oh!

blueorangutan commented Jun 21, 2024

Uh oh!

Uh oh!

Uh oh!

blueorangutan commented Jun 21, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre commented Apr 12, 2024•
edited by DaanHoogland
Loading

codecov-commenter commented Apr 12, 2024•
edited by codecov Bot
Loading

borisstoyanov left a comment •
edited
Loading