Linstor: Fix migrate primary storage by rp- · Pull Request #9528 · apache/cloudstack

rp- · 2024-08-14T11:20:05Z

Description

This PR fixes migrate primary storage for Linstor primary storage.
Always use RAW as copy sourceFormat for Linstor devices, as all images are stored as RAW on Linstor, but we have to pretend to use qcow2 to be able to use qcow2 as snapshot format. Also use .getPath() for resource names, as they are altered after a primary storage migration.
Fixes: #9176 and #9522

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 with migrating VM volumes between Linstor and NFS primary storage

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

Migrated from Linstor to NFS and back to NFS, than tried various volume actions like snaphots and revert.

rp- · 2024-08-14T11:26:39Z

If it helps, I can make the:

commits into its own PR.

codecov · 2024-08-14T11:32:53Z

Codecov Report

Attention: Patch coverage is 0% with 48 lines in your changes missing coverage. Please review.

Project coverage is 15.08%. Comparing base (eaab991) to head (ebcf66a).
Report is 25 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
.../cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java	0.00%	14 Missing ⚠️
...cloudstack/storage/datastore/util/LinstorUtil.java	0.00%	11 Missing ⚠️
.../hypervisor/kvm/storage/LinstorStorageAdaptor.java	0.00%	7 Missing ⚠️
...tore/driver/LinstorPrimaryDataStoreDriverImpl.java	0.00%	7 Missing ⚠️
...ud/hypervisor/kvm/storage/KVMStorageProcessor.java	0.00%	5 Missing ⚠️
.../hypervisor/kvm/storage/LibvirtStorageAdaptor.java	0.00%	2 Missing ⚠️
...ck/storage/snapshot/LinstorVMSnapshotStrategy.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##               4.19    #9528     +/-   ##
===========================================
  Coverage     15.08%   15.08%             
- Complexity    11180    11185      +5     
===========================================
  Files          5406     5406             
  Lines        472915   472942     +27     
  Branches      61731    58606   -3125     
===========================================
+ Hits          71326    71350     +24     
- Misses       393647   393648      +1     
- Partials       7942     7944      +2

Flag	Coverage Δ
uitests	`4.30% <ø> (ø)`
unittests	`15.80% <0.00%> (+<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.

github-actions · 2024-08-20T10:54:35Z

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

github-actions · 2024-08-20T11:03:51Z

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

Always use RAW as copy sourceFormat for Linstor devices, as all images are stored as RAW on Linstor, but we have to pretend to use qcow2 to be able to use qcow2 as snapshot format. Also use .getPath() for resource names, as they are altered after a primary storage migration.

DaanHoogland

clgtm, mostly Linstore code but might have some effect on other implementations!

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java

sureshanaparti · 2024-08-27T08:49:56Z

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java

-    private KVMStoragePool pool;
+    private final String name;
+    private final KVMStoragePool pool;
+    private String dispName;


Suggested change

private String dispName;

private String displayName;

sureshanaparti · 2024-08-27T08:52:11Z

...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

+        /* Linstor images are always stored as RAW, but Linstor uses qcow2 in DB,
+           to support snapshots(backuped) as qcow2 files. */
+        PhysicalDiskFormat sourceFormat = srcPool.getType() != StoragePoolType.Linstor ?
+                disk.getFormat() : PhysicalDiskFormat.RAW;


can set format RAW in KVMPhysicalDisk for Linstor disks?

You mean, setting it in KVMPhysicalDisk.setFormat() or KVMPhysicalDisk.getFormat()?

@sureshanaparti ?

@sureshanaparti ping

@rp- sorry, missed this. if the format is set while creating the linstor disk (KVMPhysicalDisk), you get it with disk.getFormat()

check in LinstorStorageAdaptor.java - getPhysicalDisk() & createPhysicalDisk() - where KVMPhysicalDisk obj is created and RAW format is set (so, I think, no need to set the format RAW with StoragePoolType.Linstor pool type check again).

Sadly it is, because the LinstorStorageAdaptor isn't used for migrating from another PrimaryStorage.
And so it will use the format sent from the management server and this doesn't match what is expected from the qemu-img convert command.

If you want I can create the migration error and show you the exception/error log.

ok, got it. maybe, you can revisit that later.

yadvr · 2024-09-03T11:01:47Z

@blueorangutan package

blueorangutan · 2024-09-03T11:04:05Z

@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-09-03T11:08:47Z

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

blueorangutan · 2024-09-05T10:55:31Z

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

DaanHoogland · 2024-09-05T12:37:20Z

@blueorangutan test

blueorangutan · 2024-09-05T12:38:04Z

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

DaanHoogland · 2024-09-05T18:52:25Z

@slavkap @rg9975 @harikrishna-patnala can you review?

blueorangutan · 2024-09-06T01:47:01Z

[SF] Trillian test result (tid-11375)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 44760 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9528-t11375-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

slavkap

The code LGTM, but I haven't tested it

DaanHoogland · 2024-09-09T06:56:11Z

The code LGTM, but I haven't tested it

Are you alright with merging as is @slavkap ?

slavkap · 2024-09-09T06:58:26Z

@DaanHoogland, yes all looks good

boring-cyborg bot added component:kvm component:primary-storage component:storage labels Aug 14, 2024

rp- self-assigned this Aug 14, 2024

rp- mentioned this pull request Aug 14, 2024

[linstor] Fix instance snapshot revert and delete on linstor storage #9527

Closed

12 tasks

JoaoJandre added this to the 4.19.2.0 milestone Aug 19, 2024

JoaoJandre linked an issue Aug 19, 2024 that may be closed by this pull request

[linstor] Fail to revert or delete instance snapshot in case of migrated volumes #9522

Closed

github-actions bot added the status:has-conflicts label Aug 20, 2024

rp- force-pushed the 9176-instances-volumes-stored-on-linstor-are-of-format-qcow2-instead-of-raw branch from b60e159 to 064a8d7 Compare August 21, 2024 09:44

github-actions bot removed the status:has-conflicts label Aug 21, 2024

DaanHoogland approved these changes Aug 22, 2024

View reviewed changes

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java Outdated Show resolved Hide resolved

DaanHoogland added the status:needs-testing label Aug 22, 2024

rp- added 2 commits August 22, 2024 17:12

kvm: add more volume info to KVMPhysicalDisk

a3e320d

linstor: use added KVMPhysicalDisk info for Aux properties

ebcf66a

rp- force-pushed the 9176-instances-volumes-stored-on-linstor-are-of-format-qcow2-instead-of-raw branch from 064a8d7 to ebcf66a Compare August 22, 2024 15:13

rp- requested review from sureshanaparti and yadvr August 27, 2024 08:46

sureshanaparti reviewed Aug 27, 2024

View reviewed changes

yadvr approved these changes Sep 3, 2024

View reviewed changes

sureshanaparti approved these changes Sep 5, 2024

View reviewed changes

slavkap approved these changes Sep 9, 2024

View reviewed changes

DaanHoogland merged commit 3f5a77e into apache:4.19 Sep 9, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Sep 10, 2024

Linstor: Fix migrate primary storage (apache#9528)

983540e

DaanHoogland mentioned this pull request Dec 10, 2024

Instances volumes stored on linstor are of format qcow2 instead of raw #9176

Closed

DaanHoogland removed the status:needs-testing label Jan 31, 2025

rp- deleted the 9176-instances-volumes-stored-on-linstor-are-of-format-qcow2-instead-of-raw branch February 13, 2025 08:55

rp- added a commit to LINBIT/cloudstack that referenced this pull request Feb 24, 2026

Linstor: Fix migrate primary storage (apache#9528)

18f6676

Conversation

rp- commented Aug 14, 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?

Uh oh!

rp- commented Aug 14, 2024

Uh oh!

codecov bot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Aug 20, 2024

Uh oh!

github-actions bot commented Aug 20, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

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!

sureshanaparti Sep 4, 2024 • 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!

sureshanaparti Sep 4, 2024 • 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!

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!

yadvr commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 3, 2024

Uh oh!

blueorangutan commented Sep 5, 2024

Uh oh!

DaanHoogland commented Sep 5, 2024

Uh oh!

blueorangutan commented Sep 5, 2024

Uh oh!

DaanHoogland commented Sep 5, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

slavkap left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Sep 9, 2024

Uh oh!

slavkap commented Sep 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Aug 14, 2024 •

edited

Loading

sureshanaparti Sep 4, 2024 •

edited

Loading

sureshanaparti Sep 4, 2024 •

edited

Loading