Copilot commented on code in PR #13468:
URL: https://github.com/apache/cloudstack/pull/13468#discussion_r3481118360
##########
plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java:
##########
@@ -437,16 +437,10 @@ public void revertSnapshot(SnapshotInfo snapshot,
SnapshotInfo snapshotOnPrimary
try {
EndPoint ep = null;
VolumeInfo volumeInfo =
volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
-
- StoragePoolVO storagePool =
primaryStoreDao.findById(volumeInfo.getPoolId());
- if (storagePool != null && storagePool.getPoolType() ==
StoragePoolType.CLVM) {
- ep = epSelector.select(volumeInfo);
+ if (snapshotOnPrimaryStore != null) {
+ ep = epSelector.select(snapshotOnPrimaryStore);
} else {
- if (snapshotOnPrimaryStore != null) {
- ep = epSelector.select(snapshotOnPrimaryStore);
- } else {
- ep = epSelector.select(volumeInfo);
- }
+ ep = epSelector.select(volumeInfo);
}
Review Comment:
For CLVM snapshots, selecting the endpoint based on `snapshotOnPrimaryStore`
can bypass CLVM lock-holder routing in
`DefaultEndPointSelector.select(DataObject)` (it only special-cases
`VolumeInfo`). This can cause `RevertSnapshotCommand` to be sent to an
arbitrary host that does not hold the CLVM lock / does not have the LV active,
leading to revert failures. Route CLVM snapshot reverts via the volume
(lock-holder aware) and only use `snapshotOnPrimaryStore` selection for
non-CLVM pools.
##########
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/volume path match only compares the disk path basename to
`volumeTO.getPath()`, and drops the prior fallback match against
`diskTO.getPath()` and full-path equality. In practice `VolumeObjectTO.path`
can be either a full device path (e.g. "/dev/vg/lv") or just the LV name
(depending on the operation), so this can cause false negatives and re-enable
the expensive VG probing path or mis-detect CLVM volumes. Keep matching
compatible by accepting either full-path equality or suffix (basename) match
against both `volumeTO.getPath()` and `diskTO.getPath()`.
--
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]
