Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #12617 +/- ##
=============================================
- Coverage 17.90% 3.68% -14.23%
=============================================
Files 5938 454 -5484
Lines 532864 38798 -494066
Branches 65192 7151 -58041
=============================================
- Hits 95392 1428 -93964
+ Misses 426793 37183 -389610
+ Partials 10679 187 -10492
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
5fc1f12 to
9e03f4b
Compare
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16801 |
| UserVmVO vm = userVmDao.findById(vmId); | ||
| String cantHandleLog = String.format("Default VM snapshot cannot handle VM snapshot for [%s]", vm); | ||
|
|
||
| if (isRunningVMVolumeOnCLVMStorage(vm, cantHandleLog)) { |
There was a problem hiding this comment.
@Pearl1594
what's the image format on CLVM ? RAW or QCOW2 ?
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
a08e7a5 to
df61d6f
Compare
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
df61d6f to
43e9384
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12617 +/- ##
============================================
- Coverage 18.02% 17.99% -0.03%
- Complexity 16450 16490 +40
============================================
Files 5968 5976 +8
Lines 537086 538829 +1743
Branches 65961 66239 +278
============================================
+ Hits 96820 96988 +168
- Misses 429346 430895 +1549
- Partials 10920 10946 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
3f900e8 to
c9dd7ed
Compare
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16875 |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16877 |
2. handle attaching volumes to stopped VMs \n 3. Handle lock transfer when VM is started on another host
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
6814a77 to
03a8860
Compare
Pearl1594
force-pushed
the
clvm-enhancements
branch
from
c00447f to
cc924c5
Compare
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17161 |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17162 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a set of management-server, KVM-agent, UI, and scripting changes to improve Clustered LVM (CLVM) behavior and add initial support for a new CLVM_NG storage pool type, focusing on lock handling, migration behavior, snapshots/backup, and secure deletion behavior.
Changes:
- Add
CLVM_NGstorage pool type and URL scheme support across MS/UI and primary storage lifecycle. - Introduce CLVM lock tracking/transfer (lightweight “migration” via lock move) and wire it into volume attach, VM/volume migration flows, and endpoint selection.
- Update KVM snapshot management script and KVM storage code paths for CLVM/CLVM_NG (QCOW2-on-block-device, template backing files, secure zero-fill propagation).
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/src/main/java/com/cloud/utils/UriUtils.java | Adds URI parsing for clvm:// and clvm_ng:// URLs. |
| ui/src/views/infra/AddPrimaryStorage.vue | UI support for selecting CLVM_NG and building CLVM_NG URLs. |
| server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java | Adds mock for ClvmLockManager in tests. |
| server/src/test/java/com/cloud/storage/ClvmLockManagerTest.java | New unit tests for CLVM lock manager behavior. |
| server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml | Registers clvmLockManager bean. |
| server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java | Blocks VM snapshots on CLVM_NG pools (until later phase). |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Removes snapshot-store ref after backup for CLVM/CLVM_NG. |
| server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java | Adds CLVM secure zero-fill config + CLVM lock-transfer logic during attach and expunge. |
| server/src/main/java/com/cloud/storage/StorageManagerImpl.java | Adds scheme param + validates clvm/clvm_ng URL paths. |
| server/src/main/java/com/cloud/storage/ClvmLockManager.java | New manager for tracking and transferring CLVM exclusive locks. |
| scripts/storage/qcow2/managesnapshot.sh | Refactors LVM snapshot handling; adds CLVM_NG QCOW2-on-block logic for backup/revert. |
| pom.xml | Updates Sonar exclusions (currently incorrectly). |
| plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java | Adds clvm_ng scheme mapping to StoragePoolType.CLVM_NG. |
| plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java | Adjusts endpoint selection for snapshot revert with CLVM. |
| plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java | Updates modify-storage-pool tests with CLVM zero-fill detail. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java | Treats CLVM_NG as external-snapshot capable; adds setType. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java | Major CLVM/CLVM_NG handling (virtual pools, VG stats, direct block access, QCOW2-on-LVM, secure zero-fill). |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | CLVM/CLVM_NG template copy, snapshot backup/delete paths, block-disk attach handling. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java | Fixes pool type resolution for CLVM/CLVM_NG; CLVM_NG template/disk creation paths. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePool.java | Adds CLVM_SECURE_ZERO_FILL detail key constant. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java | Enables managesnapshot-based revert for CLVM_NG. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtResizeVolumeCommandWrapper.java | Ensures CLVM_NG uses qemu-img resizing path. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPrepareForMigrationCommandWrapper.java | Activates CLVM volumes shared-mode on destination during migration prep. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPreMigrationCommandWrapper.java | New source-host pre-migration wrapper to switch CLVM volumes to shared mode. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPostMigrationCommandWrapper.java | New destination-host post-migration wrapper to claim exclusive locks. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtModifyStoragePoolCommandWrapper.java | Ensures CLVM secure zero-fill detail has a default. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java | Filters CLVM/CLVM_NG disks from libvirt storage migration list; manages CLVM volume states on success/failure. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java | New agent wrapper to execute lvchange operations for lock transfer. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java | Adds CLVM migration volume-state helpers and CLVM_NG QCOW2 block-format handling. |
| engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeObject.java | Adds destination-host hint support (persisted via volume_details). |
| engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java | Propagates CLVM secure zero-fill setting on host connect. |
| engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java | Adds CLVM-aware endpoint routing based on destination host hint and lock holder. |
| engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java | Adds tests for CLVM snapshot-store-on-secondary behavior. |
| engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/StorageVMSnapshotStrategy.java | Rejects VM snapshots when running VM volumes are on CLVM storage. |
| engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java | Rejects VM snapshots when running VM volumes are on CLVM storage. |
| engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | Allows delete strategy when CLVM snapshots exist on secondary in same zone. |
| engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java | Adds CLVM/CLVM_NG to supported pool types in tests. |
| engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java | Adds CLVM/CLVM_NG handling for migration disk info and pre-migration command. |
| engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java | Generates CLVM(/NG) /dev/<vg>/<lv> dest paths and supports CLVM types. |
| engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java | Changes search criteria operator for state exclusion. |
| engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java | Adds tests for CLVM lock transfer on VM start. |
| engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java | Adds tests for CLVM lock host updates after VM migration. |
| engine/orchestration/src/main/resources/META-INF/cloudstack/core/spring-engine-orchestration-core-context.xml | Registers clvmLockManager bean. |
| engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java | Adds CLVM destination-host hinting, lock updates after migrations, lock transfer at VM start. |
| engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java | Sends pre/post-migration commands; updates CLVM lock host IDs after migration. |
| engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeInfo.java | Adds CLVM_LOCK_HOST_ID constant and destination-host hint API. |
| core/src/main/java/org/apache/cloudstack/storage/command/ClvmLockTransferCommand.java | New agent command for CLVM lock transfer operations. |
| core/src/main/java/com/cloud/agent/api/PreMigrationCommand.java | New agent command sent to source host before migration. |
| core/src/main/java/com/cloud/agent/api/PostMigrationCommand.java | New agent command sent to destination host after migration. |
| core/src/main/java/com/cloud/agent/api/PostMigrationAnswer.java | New answer type for post-migration command. |
| core/src/main/java/com/cloud/agent/api/MigrateCommand.java | Adds destination pool type field to migrate disk info for filtering. |
| api/src/main/java/com/cloud/storage/Storage.java | Introduces StoragePoolType.CLVM_NG. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if this is a CLVM pool | ||
| StoragePoolVO pool = _storagePoolDao.findById(store.getId()); | ||
| if (pool == null || | ||
| (pool.getPoolType() != Storage.StoragePoolType.CLVM || |
There was a problem hiding this comment.
The CLVM pool type check is logically incorrect: (poolType != CLVM || poolType != CLVM_NG) is always true, so this method will always return null and CLVM routing will never be applied. Change this to an AND check (or use a helper like isClvmPoolType) so CLVM/CLVM_NG volumes can actually select the intended endpoint.
| (pool.getPoolType() != Storage.StoragePoolType.CLVM || | |
| (pool.getPoolType() != Storage.StoragePoolType.CLVM && |
| import com.cloud.storage.ClvmLockManager; | ||
| import com.cloud.storage.DataStoreRole; | ||
| import com.cloud.storage.Storage; | ||
| import com.cloud.storage.StorageManager; | ||
| import com.cloud.storage.StoragePool; | ||
| import com.cloud.storage.StoragePoolHostVO; | ||
| import com.cloud.storage.StorageService; | ||
| import com.cloud.storage.VolumeApiServiceImpl; | ||
| import com.cloud.storage.dao.StoragePoolHostDao; |
There was a problem hiding this comment.
cloud-engine-storage-volume does not have a dependency on cloud-server, but this class imports com.cloud.storage.ClvmLockManager and com.cloud.storage.VolumeApiServiceImpl (both live under server/). This will fail compilation for the engine module. To fix, avoid referencing server-layer classes here (e.g., move CLVMSecureZeroFill ConfigKey and the CLVM pool-type helper into an api/core module, or query the config via configuration DAOs available in this module).
| private String findAccessibleDeviceNode(String volumeUuid, String vgName, KVMStoragePool pool) { | ||
| String lvPath = "/dev/" + vgName + "/" + volumeUuid; | ||
| File lvDevice = new File(lvPath); | ||
|
|
||
| if (!lvDevice.exists()) { | ||
| lvPath = tryDeviceMapperPath(volumeUuid, vgName, lvDevice); | ||
|
|
||
| if (!lvDevice.exists()) { | ||
| lvPath = handleMissingDeviceNode(volumeUuid, vgName, pool); | ||
| } | ||
| } | ||
|
|
||
| return lvPath; | ||
| } | ||
|
|
||
| private String tryDeviceMapperPath(String volumeUuid, String vgName, File lvDevice) { | ||
| String vgNameEscaped = vgName.replace("-", "--"); | ||
| String volumeUuidEscaped = volumeUuid.replace("-", "--"); | ||
| String mapperPath = "/dev/mapper/" + vgNameEscaped + "-" + volumeUuidEscaped; | ||
| File mapperDevice = new File(mapperPath); | ||
|
|
||
| if (!mapperDevice.exists()) { | ||
| lvDevice = mapperDevice; | ||
| } | ||
|
|
||
| return mapperPath; | ||
| } |
There was a problem hiding this comment.
findAccessibleDeviceNode attempts to fall back to /dev/mapper/... if /dev/<vg>/<lv> doesn’t exist, but the logic in tryDeviceMapperPath is broken: it checks if (!mapperDevice.exists()) (reversed) and assigning to lvDevice has no effect on the caller. As a result, even if the mapper node exists, the method will still think it’s missing and may throw incorrectly. Rework this to check the mapper path existence and return the correct path (or return a File/boolean) so the fallback actually works.
| logger.error("CLVM lock transfer failed for volume {}: {}}", | ||
| volumeUuid, result); | ||
| return new Answer(cmd, false, | ||
| String.format("lvchange %s %s failed: %s", lvchangeOpt, lvPath, result)); | ||
| } | ||
|
|
||
| logger.info("Successfully executed CLVM lock transfer: {} {}} for volume {}}", | ||
| lvchangeOpt, lvPath, volumeUuid); | ||
|
|
||
| return new Answer(cmd, true, | ||
| String.format("Successfully %s CLVM volume %s", operationDesc, volumeUuid)); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.error("Exception during CLVM lock transfer for volume {}: {}}", |
There was a problem hiding this comment.
Log message placeholder has an extra } ({}}), resulting in malformed output. Replace {}} with {} (and similarly fix the other affected log lines in this class).
| logger.error("CLVM lock transfer failed for volume {}: {}}", | |
| volumeUuid, result); | |
| return new Answer(cmd, false, | |
| String.format("lvchange %s %s failed: %s", lvchangeOpt, lvPath, result)); | |
| } | |
| logger.info("Successfully executed CLVM lock transfer: {} {}} for volume {}}", | |
| lvchangeOpt, lvPath, volumeUuid); | |
| return new Answer(cmd, true, | |
| String.format("Successfully %s CLVM volume %s", operationDesc, volumeUuid)); | |
| } catch (Exception e) { | |
| logger.error("Exception during CLVM lock transfer for volume {}: {}}", | |
| logger.error("CLVM lock transfer failed for volume {}: {}", | |
| volumeUuid, result); | |
| return new Answer(cmd, false, | |
| String.format("lvchange %s %s failed: %s", lvchangeOpt, lvPath, result)); | |
| } | |
| logger.info("Successfully executed CLVM lock transfer: {} {} for volume {}", | |
| lvchangeOpt, lvPath, volumeUuid); | |
| return new Answer(cmd, true, | |
| String.format("Successfully %s CLVM volume %s", operationDesc, volumeUuid)); | |
| } catch (Exception e) { | |
| logger.error("Exception during CLVM lock transfer for volume {}: {}", |
| private boolean areBothPoolsClvmType(StoragePoolVO volumePool, StoragePoolVO vmPool) { | ||
| return volumePool.getPoolType() == StoragePoolType.CLVM && | ||
| vmPool.getPoolType() == StoragePoolType.CLVM; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a storage pool is CLVM type. | ||
| * | ||
| * @param pool Storage pool to check | ||
| * @return true if pool is CLVM type | ||
| */ | ||
| private boolean isClvmPool(StoragePoolVO pool) { | ||
| return pool != null && pool.getPoolType() == StoragePoolType.CLVM; | ||
| } |
There was a problem hiding this comment.
The new CLVM attachment/migration helpers only recognize StoragePoolType.CLVM (e.g., areBothPoolsClvmType and isClvmPool). Since this PR introduces CLVM_NG and ClvmLockManager.isClvmPoolType already treats both as CLVM-like, these helpers will silently skip the lightweight lock-transfer path for CLVM_NG volumes. Update the helper predicates to include CLVM_NG (or reuse a shared helper) so behavior is consistent for both pool types.
| // Propagate CLVM secure zero-fill setting to the host | ||
| // Note: This is done during host connection (agent start, MS restart, host reconnection) | ||
| // so the setting is non-dynamic. Changes require host reconnection to take effect. | ||
| if (ClvmLockManager.isClvmPoolType(pool.getPoolType())) { | ||
| Boolean clvmSecureZeroFill = VolumeApiServiceImpl.CLVMSecureZeroFill.valueIn(poolId); | ||
| if (clvmSecureZeroFill != null) { | ||
| detailsMap.put("clvmsecurezerofill", String.valueOf(clvmSecureZeroFill)); | ||
| logger.debug("Added CLVM secure zero-fill setting: {} for storage pool: {}", clvmSecureZeroFill, pool); | ||
| } |
There was a problem hiding this comment.
This code hard-codes the detail key string "clvmsecurezerofill". The same key is also defined as KVMStoragePool.CLVM_SECURE_ZERO_FILL; having two sources of truth risks drift/typos between management server and agent. Consider moving this key to a shared constant accessible from both modules (or otherwise centralizing it) and reference that constant here.
| <sonar.exclusions>engine/schema/src/main/java/org/apache/cloudstack/backup/BackupOfferingDetailsVO.java</sonar.exclusions> | ||
| <sonar.exclusions>api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java</sonar.exclusions> | ||
| <sonar.exclusions>core/src/main/java/com/cloud/agent/api/PostMigrationAnswer.java</sonar.exclusions> | ||
| <sonar.exclusions>core/src/main/java/com/cloud/agent/api/PostMigrationCommand.java</sonar.exclusions> | ||
| <sonar.exclusions>core/src/main/java/com/cloud/agent/api/PreMigrationCommand.java</sonar.exclusions> | ||
| <sonar.exclusions>core/src/main/java/org/apache/cloudstack/storage/command/ClvmLockTransferCommand.java</sonar.exclusions> |
There was a problem hiding this comment.
sonar.exclusions is declared multiple times. In Maven, later property declarations override earlier ones, so only the last exclusion will actually be used. Combine all exclusions into a single sonar.exclusions property (comma-separated) so none of the earlier exclusions are lost.
| import com.cloud.dc.DedicatedResourceVO; | ||
| import com.cloud.dc.dao.DedicatedResourceDao; | ||
| import com.cloud.storage.ClvmLockManager; | ||
| import com.cloud.storage.Storage; | ||
| import com.cloud.storage.VolumeDetailVO; | ||
| import com.cloud.storage.dao.VolumeDetailsDao; |
There was a problem hiding this comment.
This module (cloud-engine-storage) does not depend on cloud-server (see the commented-out dependency in engine/storage/pom.xml), but this file imports com.cloud.storage.ClvmLockManager which is defined under server/. That will break compilation for this module. Consider removing this dependency (e.g., inline the pool-type check here), or move ClvmLockManager (or at least its isClvmPoolType helper) into a module that cloud-engine-storage depends on (api/core/engine).
|
Description
This PR...
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?