Copilot commented on code in PR #13468:
URL: https://github.com/apache/cloudstack/pull/13468#discussion_r3551454147
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -7022,7 +7022,7 @@ private static boolean isClvmVolume(DiskDef disk,
VirtualMachineTO vmSpec) {
continue;
}
VolumeObjectTO volumeTO = (VolumeObjectTO)
diskTO.getData();
- if (!diskPath.equals(volumeTO.getPath()) &&
!diskPath.equals(diskTO.getPath())) {
+ if
(!diskPath.substring(diskPath.lastIndexOf(File.separator) +
1).equals(volumeTO.getPath())) {
continue;
}
Review Comment:
The new disk-path matching only compares the basename of the libvirt disk
path to volumeTO.getPath(). This still won’t match CLVM volumes exposed as
/dev/mapper/<vgEscaped>-<lvEscaped> (see
ClvmStorageAdaptor.tryDeviceMapperPath), which means the code will miss the
VirtualMachineTO match and fall back to the expensive vgs/VG attribute
detection (and may mis-detect if that fails). Please handle /dev/mapper paths
by extracting and unescaping the LV name before comparing to volumeTO.getPath().
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java:
##########
@@ -495,6 +502,17 @@ protected Answer copyVolumeBetweenPools(DataObject
srcData, DataObject destData)
}
}
+ private void setClvmLockHostIdIfApplicable(DataObject destData, EndPoint
ep) {
+ if (ep == null || !(destData instanceof VolumeInfo)) {
+ return;
+ }
+ VolumeInfo destVolume = (VolumeInfo) destData;
+ if (ClvmPoolManager.isClvmPoolType(destVolume.getStoragePoolType())) {
+ clvmPoolManager.setClvmLockHostId(destVolume.getId(), ep.getId());
+ logger.debug("Set CLVM lock host {} for migrated volume {}",
ep.getId(), destVolume.getUuid());
+ }
Review Comment:
setClvmLockHostIdIfApplicable() checks CLVM-ness via
destVolume.getStoragePoolType(), which comes from the VolumeVO poolType and may
still reflect the *source* pool type during/just after migration. In that case
the CLVM lock host ID won’t be updated even though the destination datastore is
CLVM. Use the destination datastore’s pool type (destVolume.getDataStore())
instead, similar to updateLockHostForVolume() earlier in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
