kvm-storage: provide isVMMigrate information to storage plugins #10093

rp- · 2024-12-12T10:14:23Z

Description

This PR adds information to the storage adapters if the connectPhysicalDisk is running while doing
live VM migration. At least for Linstor this information is very beneficial to only allow two-primaries while
in this environment.
Other storage adapters will currently just ignore this information.

We just found out by a recent incident that this is rather critical for Linstor in the case that
for some reason the CloudStack management server thinks that a host is down or needs to do
VM HA, even tough the Host is still working as expected.

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
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Linstor cluster, basic VM operations

How did you try to break this feature and the system with this change?

codecov · 2024-12-12T10:23:05Z

Codecov Report

Attention: Patch coverage is 13.04348% with 20 lines in your changes missing coverage. Please review.

Project coverage is 15.12%. Comparing base (a2f2e87) to head (00379ac).
Report is 2 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
.../hypervisor/kvm/storage/LinstorStorageAdaptor.java	0.00%	6 Missing ⚠️
.../hypervisor/kvm/storage/KVMStoragePoolManager.java	0.00%	3 Missing ⚠️
...hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java	0.00%	1 Missing ⚠️
...ud/hypervisor/kvm/storage/IscsiAdmStoragePool.java	0.00%	1 Missing ⚠️
.../hypervisor/kvm/storage/LibvirtStorageAdaptor.java	0.00%	1 Missing ⚠️
...pervisor/kvm/storage/ManagedNfsStorageAdaptor.java	0.00%	1 Missing ⚠️
...pervisor/kvm/storage/MultipathSCSIAdapterBase.java	0.00%	1 Missing ⚠️
...loud/hypervisor/kvm/storage/MultipathSCSIPool.java	0.00%	1 Missing ⚠️
.../hypervisor/kvm/storage/ScaleIOStorageAdaptor.java	50.00%	1 Missing ⚠️
...oud/hypervisor/kvm/storage/ScaleIOStoragePool.java	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19   #10093      +/-   ##
============================================
- Coverage     15.13%   15.12%   -0.01%     
+ Complexity    11268    11263       -5     
============================================
  Files          5408     5408              
  Lines        473867   473869       +2     
  Branches      57778    57779       +1     
============================================
- Hits          71700    71683      -17     
- Misses       394165   394187      +22     
+ Partials       8002     7999       -3

Flag	Coverage Δ
uitests	`4.30% <ø> (ø)`
unittests	`15.84% <13.04%> (-0.01%)`	⬇️

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.

DaanHoogland

clgtm

DaanHoogland · 2024-12-12T10:38:12Z

@blueorangutan package

blueorangutan · 2024-12-12T10:40:04Z

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

...age/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java

blueorangutan · 2024-12-12T11:42:17Z

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

rp- · 2024-12-13T08:15:32Z

@blueorangutan package

blueorangutan · 2024-12-13T08:16:07Z

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

slavkap

code LGTM
I don't see that the changes could affect the other storage plugins but I still cannot understand what the change does for Linstor

rp- · 2024-12-13T08:52:37Z

code LGTM I don't see that the changes could affect the other storage plugins but I still cannot understand what the change does for Linstor

In short, it prevents opening a volume on 2 hosts at the same time if it isn't a live migration (were it is required)

slavkap · 2024-12-13T09:06:15Z

Thanks @rp- for the explanation! That makes sense. Btw, for the Host HA, I've added the option for the storage to disconnect the volume from all hosts before the start of a VM on another. It detaches the VM's volumes from the host just in case the host that should be down isn't. It should prevent the same issue as your change

cloudstack/server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java

Line 661 in 91ab67c

primaryStoreDriver.detachVolumeFromAllStorageNodes(volumeVO);

blueorangutan · 2024-12-13T09:25:03Z

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

DaanHoogland · 2024-12-13T09:28:40Z

@blueorangutan test

blueorangutan · 2024-12-13T09:30:04Z

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

blueorangutan · 2024-12-14T00:20:09Z

[SF] Trillian test result (tid-11898)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51068 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10093-t11898-kvm-ol8.zip
Smoke tests completed. 131 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_secure_vm_migration	`Error`	389.58	test_vm_life_cycle.py
test_02_redundant_VPC_default_routes	`Failure`	420.48	test_vpc_redundant.py

DaanHoogland · 2024-12-14T17:00:35Z

@blueorangutan package

blueorangutan · 2024-12-14T17:02:03Z

@DaanHoogland 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-12-14T18:24:56Z

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

DaanHoogland · 2024-12-14T18:43:21Z

@blueorangutan test

blueorangutan · 2024-12-14T18:44:03Z

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

blueorangutan · 2024-12-15T07:28:00Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2024-12-16T08:02:15Z

@JoaoJandre are your concerns met, here?

github-actions · 2024-12-16T09:29:28Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Particular Linstor needs can use this information to only allow dual volume access for live migration and not enable it in general, which can and will lead to data corruption if for some reason 2 VMs get started on 2 different hosts.

rp- · 2024-12-16T09:36:55Z

fixed conflict and squashed the changelog.md into 1 commit

DaanHoogland · 2024-12-16T10:48:43Z

@blueorangutan package

blueorangutan · 2024-12-16T10:50:04Z

@DaanHoogland 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-12-16T11:20:56Z

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

blueorangutan · 2024-12-16T14:33:25Z

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

DaanHoogland · 2024-12-17T07:55:36Z

@blueorangutan test

blueorangutan · 2024-12-17T07:56:03Z

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

blueorangutan · 2024-12-17T22:04:46Z

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

Test	Result	Time (s)	Test File
test_01_secure_vm_migration	`Error`	134.44	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	134.45	test_vm_life_cycle.py

* 4.20: VR: apply iptables rules when add/remove static routes (#10064) Certificate and VM hostname validation improvements (#10051) set ulimit for server according to redhat spec (#10040) kvm-storage: provide isVMMigrate information to storage plugins (#10093) Allow config drive deletion of migrated VM, on host maintenance (#10045) linstor: improve heartbeat check with also asking linstor (#10105) server: simplify role change validation (#9173) UI: create VPC network offering with conserve mode (#10082) server: fix typo removeaccessvpn in VirtualRouterElement (#10086) UI: remove duplicated Instance Name in Public IP details page (#10087) UI: Fixes in the Usage UI (#10000) SAML2: add cookie with HttpOnly too #10013 (#10047) ui: Allow font-awesome icon usage and optimise icon size inconsistency (#9744)

…he#10093) Particular Linstor needs can use this information to only allow dual volume access for live migration and not enable it in general, which can and will lead to data corruption if for some reason 2 VMs get started on 2 different hosts.

rp- added this to the 4.19.2 milestone Dec 12, 2024

rp- requested review from DaanHoogland and slavkap December 12, 2024 10:14

boring-cyborg bot added component:iscsi component:kvm component:primary-storage component:storage labels Dec 12, 2024

DaanHoogland approved these changes Dec 12, 2024

View reviewed changes

JoaoJandre reviewed Dec 12, 2024

View reviewed changes

...age/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java Show resolved Hide resolved

slavkap approved these changes Dec 13, 2024

View reviewed changes

rp- added the 3rd-party:linstor label Dec 13, 2024

github-actions bot added the status:has-conflicts label Dec 16, 2024

rp- force-pushed the linstor-4.19-allow-two-primaries-only-on-live-migration branch from bbc25a1 to 00379ac Compare December 16, 2024 09:36

github-actions bot removed the status:has-conflicts label Dec 16, 2024

DaanHoogland requested review from slavkap and JoaoJandre December 16, 2024 10:45

JoaoJandre approved these changes Dec 16, 2024

View reviewed changes

slavkap approved these changes Dec 17, 2024

View reviewed changes

DaanHoogland merged commit a9587bf into apache:4.19 Dec 18, 2024
25 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvm-storage: provide isVMMigrate information to storage plugins #10093

kvm-storage: provide isVMMigrate information to storage plugins #10093

rp- commented Dec 12, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

DaanHoogland left a comment

DaanHoogland commented Dec 12, 2024

blueorangutan commented Dec 12, 2024

blueorangutan commented Dec 12, 2024

rp- commented Dec 13, 2024

blueorangutan commented Dec 13, 2024

slavkap left a comment

rp- commented Dec 13, 2024

slavkap commented Dec 13, 2024 •

edited

Loading

blueorangutan commented Dec 13, 2024

DaanHoogland commented Dec 13, 2024

blueorangutan commented Dec 13, 2024

blueorangutan commented Dec 14, 2024

DaanHoogland commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

DaanHoogland commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

blueorangutan commented Dec 15, 2024

DaanHoogland commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

rp- commented Dec 16, 2024

DaanHoogland commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

DaanHoogland commented Dec 17, 2024

blueorangutan commented Dec 17, 2024

blueorangutan commented Dec 17, 2024

kvm-storage: provide isVMMigrate information to storage plugins #10093

kvm-storage: provide isVMMigrate information to storage plugins #10093

Conversation

rp- commented Dec 12, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

DaanHoogland left a comment

Choose a reason for hiding this comment

DaanHoogland commented Dec 12, 2024

blueorangutan commented Dec 12, 2024

blueorangutan commented Dec 12, 2024

rp- commented Dec 13, 2024

blueorangutan commented Dec 13, 2024

slavkap left a comment

Choose a reason for hiding this comment

rp- commented Dec 13, 2024

slavkap commented Dec 13, 2024 • edited Loading

blueorangutan commented Dec 13, 2024

DaanHoogland commented Dec 13, 2024

blueorangutan commented Dec 13, 2024

blueorangutan commented Dec 14, 2024

DaanHoogland commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

DaanHoogland commented Dec 14, 2024

blueorangutan commented Dec 14, 2024

blueorangutan commented Dec 15, 2024

DaanHoogland commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

rp- commented Dec 16, 2024

DaanHoogland commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

blueorangutan commented Dec 16, 2024

DaanHoogland commented Dec 17, 2024

blueorangutan commented Dec 17, 2024

blueorangutan commented Dec 17, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

slavkap commented Dec 13, 2024 •

edited

Loading