refactor

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java

@@ -30,6 +30,7 @@ import com.cloud.exception.StorageAccessException;
 import com.cloud.host.Host;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.offering.DiskOffering;
+import com.cloud.storage.Storage.StoragePoolType;
 import com.cloud.storage.Volume;
 import com.cloud.user.Account;
 import com.cloud.utils.Pair;
@@ -123,4 +124,71 @@ public interface VolumeService {
     void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host);
 
     void validateChangeDiskOfferingEncryptionType(long existingDiskOfferingId, long newDiskOfferingId);
+
+    /**
+     * Transfers exclusive lock for a volume on cluster-based storage (e.g., CLVM/CLVM_NG) from one host to another.
+     * This is used for storage that requires host-level lock management for volumes on shared storage pools.
+     * For non-CLVM pool types, this method returns false without taking action.
+     *
+     * @param volume The volume to transfer lock for
+     * @param sourceHostId Host currently holding the exclusive lock
+     * @param destHostId Host to receive the exclusive lock
+     * @return true if lock transfer succeeded or was not needed, false if it failed
+     */
+    boolean transferVolumeLock(VolumeInfo volume, Long sourceHostId, Long destHostId);
+
+    /**
+     * Finds which host currently has the exclusive lock on a CLVM volume.
+     * Checks in order: explicit lock tracking, attached VM's host, or first available cluster host.
+     *
+     * @param volume The CLVM volume
+     * @return Host ID that has the exclusive lock, or null if cannot be determined
+     */
+    Long findVolumeLockHost(VolumeInfo volume);
+
+    /**
+     * Performs lightweight CLVM lock migration for a volume to a target host.
+     * This transfers the LVM exclusive lock without copying data (CLVM volumes are on shared cluster storage).
+     * If the volume already has the lock on the destination host, no action is taken.
+     *
+     * @param volume The volume to migrate lock for
+     * @param destHostId Destination host ID
+     * @return Updated VolumeInfo after lock migration
+     */
+    VolumeInfo performLockMigration(VolumeInfo volume, Long destHostId);
+
+    /**
+     * Checks if both storage pools are CLVM type (CLVM or CLVM_NG).
+     *
+     * @param volumePoolType Storage pool type for the volume
+     * @param vmPoolType Storage pool type for the VM
+     * @return true if both pools are CLVM type (CLVM or CLVM_NG)
+     */
+    boolean areBothPoolsClvmType(StoragePoolType volumePoolType, StoragePoolType vmPoolType);
+
+    /**
+     * Determines if CLVM lock transfer is required when a volume is already on the correct storage pool.
+     *
+     * @param volumeToAttach The volume being attached
+     * @param volumePoolType Storage pool type for the volume
+     * @param vmPoolType Storage pool type for the VM's existing volume
+     * @param volumePoolId Storage pool ID for the volume
+     * @param vmPoolId Storage pool ID for the VM's existing volume
+     * @param vmHostId VM's current host ID (or last host ID if stopped)
+     * @return true if CLVM lock transfer is needed
+     */
+    boolean isLockTransferRequired(VolumeInfo volumeToAttach, StoragePoolType volumePoolType, StoragePoolType vmPoolType, 
+                                   Long volumePoolId, Long vmPoolId, Long vmHostId);
+
+    /**
+     * Determines if lightweight CLVM migration is needed instead of full data copy.
+     *
+     * @param volumePoolType Storage pool type for the volume
+     * @param vmPoolType Storage pool type for the VM
+     * @param volumePoolPath Storage pool path for the volume
+     * @param vmPoolPath Storage pool path for the VM
+     * @return true if lightweight migration should be used
+     */
+    boolean isLightweightMigrationNeeded(StoragePoolType volumePoolType, StoragePoolType vmPoolType, 
+                                         String volumePoolPath, String vmPoolPath);
 }

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java

@@ -32,6 +32,8 @@ import java.util.concurrent.ExecutionException;
 
 import javax.inject.Inject;
 
+import com.cloud.storage.ClvmLockManager;
+import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.dao.VMInstanceDao;
 import org.apache.cloudstack.annotation.AnnotationService;
 import org.apache.cloudstack.annotation.dao.AnnotationDao;
@@ -221,6 +223,8 @@ public class VolumeServiceImpl implements VolumeService {
     private PassphraseDao passphraseDao;
     @Inject
     protected DiskOfferingDao diskOfferingDao;
+    @Inject
+    ClvmLockManager clvmLockManager;
 
     public VolumeServiceImpl() {
     }
@@ -2963,4 +2967,166 @@ public class VolumeServiceImpl implements VolumeService {
     protected String buildVolumePath(long accountId, long volumeId) {
         return String.format("%s/%s/%s", TemplateConstants.DEFAULT_VOLUME_ROOT_DIR, accountId, volumeId);
     }
+
+    @Override
+    public boolean transferVolumeLock(VolumeInfo volume, Long sourceHostId, Long destHostId) {
+        StoragePoolVO pool = storagePoolDao.findById(volume.getPoolId());
+        if (pool == null) {
+            logger.error("Cannot transfer volume lock for volume {}: storage pool not found", volume.getUuid());
+            return false;
+        }
+
+        logger.info("Transferring CLVM lock for volume {} (pool: {}) from host {} to host {}", 
+                volume.getUuid(), pool.getName(), sourceHostId, destHostId);
+
+        return clvmLockManager.transferClvmVolumeLock(volume.getUuid(), volume.getId(), volume.getPath(),
+                pool, sourceHostId, destHostId);
+    }
+
+    @Override
+    public Long findVolumeLockHost(VolumeInfo volume) {
+        if (volume == null) {
+            logger.warn("Cannot find volume lock host: volume is null");
+            return null;
+        }
+
+        Long lockHostId = clvmLockManager.getClvmLockHostId(volume.getId(), volume.getUuid());
+        if (lockHostId != null) {
+            logger.debug("Found explicit lock host {} for volume {}", lockHostId, volume.getUuid());
+            return lockHostId;
+        }
+
+        Long instanceId = volume.getInstanceId();
+        if (instanceId != null) {
+            VMInstanceVO vmInstance = vmDao.findById(instanceId);
+            if (vmInstance != null && vmInstance.getHostId() != null) {
+                logger.debug("Volume {} is attached to VM {} on host {}", 
+                        volume.getUuid(), vmInstance.getUuid(), vmInstance.getHostId());
+                return vmInstance.getHostId();
+            }
+        }
+
+        StoragePoolVO pool = storagePoolDao.findById(volume.getPoolId());
+        if (pool != null && pool.getClusterId() != null) {
+            List<HostVO> hosts = _hostDao.findByClusterId(pool.getClusterId());
+            if (hosts != null && !hosts.isEmpty()) {
+                for (HostVO host : hosts) {
+                    if (host.getStatus() == com.cloud.host.Status.Up) {
+                        logger.debug("Using fallback: first UP host {} in cluster {} for volume {}", 
+                                host.getId(), pool.getClusterId(), volume.getUuid());
+                        return host.getId();
+                    }
+                }
+            }
+        }
+
+        logger.warn("Could not determine lock host for volume {}", volume.getUuid());
+        return null;
+    }
+
+    @Override
+    public VolumeInfo performLockMigration(VolumeInfo volume, Long destHostId) {
+        if (volume == null) {
+            throw new CloudRuntimeException("Cannot perform CLVM lock migration: volume is null");
+        }
+
+        String volumeUuid = volume.getUuid();
+        logger.info("Starting CLVM lock migration for volume {} (id: {}) to host {}", 
+                volumeUuid, volume.getUuid(), destHostId);
+
+        Long sourceHostId = findVolumeLockHost(volume);
+        if (sourceHostId == null) {
+            logger.warn("Could not determine source host for CLVM volume {} lock, assuming volume is not exclusively locked", 
+                    volumeUuid);
+            sourceHostId = destHostId;
+        }
+
+        if (sourceHostId.equals(destHostId)) {
+            logger.info("CLVM volume {} already has lock on destination host {}, no migration needed", 
+                    volumeUuid, destHostId);
+            return volume;
+        }
+
+        logger.info("Migrating CLVM volume {} lock from host {} to host {}", 
+                volumeUuid, sourceHostId, destHostId);
+
+        boolean success = transferVolumeLock(volume, sourceHostId, destHostId);
+        if (!success) {
+            throw new CloudRuntimeException(
+                    String.format("Failed to transfer CLVM lock for volume %s from host %s to host %s", 
+                            volumeUuid, sourceHostId, destHostId));
+        }
+
+        logger.info("Successfully migrated CLVM volume {} lock from host {} to host {}", 
+                volumeUuid, sourceHostId, destHostId);
+
+        return volFactory.getVolume(volume.getId());
+    }
+
+    @Override
+    public boolean areBothPoolsClvmType(StoragePoolType volumePoolType, StoragePoolType vmPoolType) {
+        if (volumePoolType == null || vmPoolType == null) {
+            logger.debug("Cannot check if both pools are CLVM type: one or both pool types are null");
+            return false;
+        }
+        return ClvmLockManager.isClvmPoolType(volumePoolType) &&
+               ClvmLockManager.isClvmPoolType(vmPoolType);
+    }
+
+    @Override
+    public boolean isLockTransferRequired(VolumeInfo volumeToAttach, StoragePoolType volumePoolType, StoragePoolType vmPoolType,
+                                          Long volumePoolId, Long vmPoolId, Long vmHostId) {
+        if (volumePoolType != null && !ClvmLockManager.isClvmPoolType(volumePoolType)) {
+            return false;
+        }
+
+        if (volumePoolId == null || !volumePoolId.equals(vmPoolId)) {
+            return false;
+        }
+
+        Long volumeLockHostId = findVolumeLockHost(volumeToAttach);
+
+        if (volumeLockHostId == null) {
+            VolumeVO volumeVO = _volumeDao.findById(volumeToAttach.getId());
+            if (volumeVO != null && volumeVO.getState() == Volume.State.Ready && volumeVO.getInstanceId() == null) {
+                logger.debug("CLVM volume {} is detached on same pool, lock transfer may be needed",
+                        volumeToAttach.getUuid());
+                return true;
+            }
+        }
+
+        if (volumeLockHostId != null && vmHostId != null && !volumeLockHostId.equals(vmHostId)) {
+            logger.info("CLVM lock transfer required: Volume {} lock is on host {} but VM is on host {}",
+                    volumeToAttach.getUuid(), volumeLockHostId, vmHostId);
+            return true;
+        }
+
+        return false;
+    }
+
+    @Override
+    public boolean isLightweightMigrationNeeded(StoragePoolType volumePoolType, StoragePoolType vmPoolType,
+                                                String volumePoolPath, String vmPoolPath) {
+        if (!areBothPoolsClvmType(volumePoolType, vmPoolType)) {
+            return false;
+        }
+
+        String volumeVgName = extractVgNameFromPath(volumePoolPath);
+        String vmVgName = extractVgNameFromPath(vmPoolPath);
+
+        if (volumeVgName != null && volumeVgName.equals(vmVgName)) {
+            logger.info("CLVM lightweight migration detected: Volume is in same VG ({}), only lock transfer needed (no data copy)", volumeVgName);
+            return true;
+        }
+
+        return false;
+    }
+
+    private String extractVgNameFromPath(String poolPath) {
+        if (poolPath == null) {
+            return null;
+        }
+        return poolPath.startsWith("/") ? poolPath.substring(1) : poolPath;
+    }
 }
+

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

@@ -2620,7 +2620,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
 
         if (isClvmLockTransferNeeded) {
             // CLVM lock transfer - no data copy, no pool change needed
-            newVolumeOnPrimaryStorage = executeClvmLightweightMigration(
+            newVolumeOnPrimaryStorage = executeLightweightLockMigration(
                     newVolumeOnPrimaryStorage, vm, existingVolumeOfVm,
                     "CLVM lock transfer", "same pool to different host");
         } else if (moveVolumeNeeded) {
@@ -2634,7 +2634,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
                     newVolumeOnPrimaryStorage, existingVolumeOfVm, vm);
 
             if (isClvmLightweightMigration) {
-                newVolumeOnPrimaryStorage = executeClvmLightweightMigration(
+                newVolumeOnPrimaryStorage = executeLightweightLockMigration(
                         newVolumeOnPrimaryStorage, vm, existingVolumeOfVm,
                         "CLVM lightweight migration", "different pools, same VG");
             } else {
@@ -2685,43 +2685,15 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
     }
 
     /**
-     * Checks if both storage pools are CLVM type.
+     * Checks if both storage pools are CLVM type (CLVM or CLVM_NG).
+     * Delegates to VolumeService for the actual check.
      *
      * @param volumePool Storage pool for the volume
      * @param vmPool Storage pool for the VM
-     * @return true if both pools are CLVM type
+     * @return true if both pools are CLVM type (CLVM or CLVM_NG)
      */
     private boolean areBothPoolsClvmType(StoragePoolVO volumePool, StoragePoolVO vmPool) {
-        return volumePool.getPoolType() == StoragePoolType.CLVM &&
-               vmPool.getPoolType() == StoragePoolType.CLVM;
-    }
-
-    /**
-     * Extracts the Volume Group (VG) name from a CLVM storage pool path.
-     * For CLVM, the path is typically: /vgname
-     *
-     * @param poolPath The storage pool path
-     * @return VG name, or null if path is null
-     */
-    private String extractVgNameFromPath(String poolPath) {
-        if (poolPath == null) {
-            return null;
-        }
-        return poolPath.startsWith("/") ? poolPath.substring(1) : poolPath;
-    }
-
-    /**
-     * Checks if two CLVM storage pools are in the same Volume Group.
-     *
-     * @param volumePool Storage pool for the volume
-     * @param vmPool Storage pool for the VM
-     * @return true if both pools are in the same VG
-     */
-    private boolean arePoolsInSameVolumeGroup(StoragePoolVO volumePool, StoragePoolVO vmPool) {
-        String volumeVgName = extractVgNameFromPath(volumePool.getPath());
-        String vmVgName = extractVgNameFromPath(vmPool.getPath());
-
-        return volumeVgName != null && volumeVgName.equals(vmVgName);
+        return volService.areBothPoolsClvmType(volumePool.getPoolType(), vmPool.getPoolType());
     }
 
     /**
@@ -2746,19 +2718,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
         StoragePoolVO volumePool = pools.first();
         StoragePoolVO vmPool = pools.second();
 
-        if (!areBothPoolsClvmType(volumePool, vmPool)) {
-            return false;
-        }
-
-        if (arePoolsInSameVolumeGroup(volumePool, vmPool)) {
-            String vgName = extractVgNameFromPath(volumePool.getPath());
-            logger.info("CLVM lightweight migration detected: Volume {} is in same VG ({}) as VM {} volumes, " +
-                    "only lock transfer needed (no data copy)",
-                    volumeToAttach.getUuid(), vgName, vm.getUuid());
-            return true;
-        }
-
-        return false;
+        return volService.isLightweightMigrationNeeded(volumePool.getPoolType(), vmPool.getPoolType(),
+                volumePool.getPath(), vmPool.getPath());
     }
 
     /**
@@ -2787,37 +2748,13 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
         StoragePoolVO volumePool = pools.first();
         StoragePoolVO vmPool = pools.second();
 
-        if (volumePool != null && !ClvmLockManager.isClvmPoolType(volumePool.getPoolType())) {
-            return false;
-        }
-
-        if (volumePool.getId() != vmPool.getId()) {
-            return false;
-        }
-
-        Long volumeLockHostId = findClvmVolumeLockHost(volumeToAttach);
-
         Long vmHostId = vm.getHostId();
         if (vmHostId == null) {
             vmHostId = vm.getLastHostId();
         }
 
-        if (volumeLockHostId == null) {
-            VolumeVO volumeVO = _volsDao.findById(volumeToAttach.getId());
-            if (volumeVO != null && volumeVO.getState() == Volume.State.Ready && volumeVO.getInstanceId() == null) {
-                logger.debug("CLVM volume {} is detached on same pool as VM {}, lock transfer may be needed",
-                        volumeToAttach.getUuid(), vm.getUuid());
-                return true;
-            }
-        }
-
-        if (volumeLockHostId != null && vmHostId != null && !volumeLockHostId.equals(vmHostId)) {
-            logger.info("CLVM lock transfer required: Volume {} lock is on host {} but VM {} is on host {}",
-                    volumeToAttach.getUuid(), volumeLockHostId, vm.getUuid(), vmHostId);
-            return true;
-        }
-
-        return false;
+        return volService.isLockTransferRequired(volumeToAttach, volumePool.getPoolType(), vmPool.getPoolType(),
+                volumePool.getId(), vmPool.getId(), vmHostId);
     }
 
     /**
@@ -2871,13 +2808,13 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
      * @return Updated VolumeInfo after lock migration
      * @throws CloudRuntimeException if migration fails
      */
-    private VolumeInfo executeClvmLightweightMigration(VolumeInfo volume, UserVmVO vm, VolumeVO vmExistingVolume,
+    private VolumeInfo executeLightweightLockMigration(VolumeInfo volume, UserVmVO vm, VolumeVO vmExistingVolume,
                                                         String operationType, String scenarioDescription) {
         logger.info("Performing {} for volume {} to VM {} ({})",
                 operationType, volume.getUuid(), vm.getUuid(), scenarioDescription);
 
         try {
-            return performClvmLightweightMigration(volume, vm, vmExistingVolume);
+            return performLightweightLockMigration(volume, vm, vmExistingVolume);
         } catch (Exception e) {
             logger.error("{} failed for volume {}: {}",
                     operationType, volume.getUuid(), e.getMessage(), e);
@@ -2887,9 +2824,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
 
     /**
      * Performs lightweight CLVM lock migration for volume attachment.
-     *
-     * This transfers the LVM exclusive lock from the current host to the VM's host
-     * without copying data (since CLVM volumes are on cluster-wide shared storage).
+     * Delegates to VolumeService for the actual lock migration.
      *
      * @param volume The volume to migrate locks for
      * @param vm The VM to attach the volume to
@@ -2897,13 +2832,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
      * @return Updated VolumeInfo after lock migration
      * @throws Exception if lock migration fails
      */
-    private VolumeInfo performClvmLightweightMigration(VolumeInfo volume, UserVmVO vm, VolumeVO vmExistingVolume) throws Exception {
-        String volumeUuid = volume.getUuid();
-        Long vmId = vm.getId();
-
-        logger.info("Starting CLVM lightweight lock migration for volume {} (id: {}) to VM {} (id: {})",
-                volumeUuid, volume.getId(), vm.getUuid(), vmId);
-
+    private VolumeInfo performLightweightLockMigration(VolumeInfo volume, UserVmVO vm, VolumeVO vmExistingVolume) throws CloudRuntimeException {
         Long destHostId = determineClvmLockDestinationHost(vm, vmExistingVolume);
 
         if (destHostId == null) {
@@ -2911,91 +2840,25 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
                 "Cannot determine destination host for CLVM lock migration - VM has no host and no available cluster hosts");
         }
 
-        Long sourceHostId = findClvmVolumeLockHost(volume);
-
-        if (sourceHostId == null) {
-            logger.warn("Could not determine source host for CLVM volume {} lock, " +
-                    "assuming volume is not exclusively locked", volumeUuid);
-            sourceHostId = destHostId;
+        try {
+            return volService.performLockMigration(volume, destHostId);
+        } catch (CloudRuntimeException e) {
+            logger.error("CLVM lock migration failed for volume {}: {}", volume.getUuid(), e.getMessage(), e);
+            throw e;
         }
-
-        if (sourceHostId.equals(destHostId)) {
-            logger.info("CLVM volume {} already has lock on destination host {}, no migration needed",
-                    volumeUuid, destHostId);
-            return volume;
-        }
-
-        logger.info("Migrating CLVM volume {} lock from host {} to host {}",
-                volumeUuid, sourceHostId, destHostId);
-
-        boolean success = transferClvmVolumeLock(volume, sourceHostId, destHostId);
-
-        if (!success) {
-            throw new CloudRuntimeException(
-                String.format("Failed to transfer CLVM lock for volume %s from host %s to host %s",
-                    volumeUuid, sourceHostId, destHostId));
-        }
-
-        logger.info("Successfully migrated CLVM volume {} lock from host {} to host {}",
-                volumeUuid, sourceHostId, destHostId);
-
-        return volFactory.getVolume(volume.getId());
     }
 
     /**
      * Finds which host currently has the exclusive lock on a CLVM volume.
+     * Delegates to VolumeService for the actual lookup.
      *
      * @param volume The CLVM volume
      * @return Host ID that has the exclusive lock, or null if cannot be determined
      */
-    private Long findClvmVolumeLockHost(VolumeInfo volume) {
-        Long lockHostId = clvmLockManager.getClvmLockHostId(volume.getId(), volume.getUuid());
-        if (lockHostId != null) {
-            return lockHostId;
-        }
-
-        Long instanceId = volume.getInstanceId();
-        if (instanceId != null) {
-            VMInstanceVO vmInstance = _vmInstanceDao.findById(instanceId);
-            if (vmInstance != null && vmInstance.getHostId() != null) {
-                return vmInstance.getHostId();
-            }
-        }
-
-        StoragePoolVO pool = _storagePoolDao.findById(volume.getPoolId());
-        if (pool != null && pool.getClusterId() != null) {
-            List<HostVO> hosts = _hostDao.findByClusterId(pool.getClusterId());
-            if (hosts != null && !hosts.isEmpty()) {
-                // Return first available UP host
-                for (HostVO host : hosts) {
-                    if (host.getStatus() == Status.Up) {
-                        return host.getId();
-                    }
-                }
-            }
-        }
-
-        return null;
+    private Long findVolumeLockHost(VolumeInfo volume) {
+        return volService.findVolumeLockHost(volume);
     }
 
-    /**
-     * Transfers CLVM volume exclusive lock from source host to destination host.
-     *
-     * @param volume The volume to transfer lock for
-     * @param sourceHostId Host currently holding the lock
-     * @param destHostId Host to transfer lock to
-     * @return true if successful, false otherwise
-     */
-    private boolean transferClvmVolumeLock(VolumeInfo volume, Long sourceHostId, Long destHostId) {
-        StoragePoolVO pool = _storagePoolDao.findById(volume.getPoolId());
-        if (pool == null) {
-            logger.error("Cannot find storage pool for volume {}", volume.getUuid());
-            return false;
-        }
-
-        return clvmLockManager.transferClvmVolumeLock(volume.getUuid(), volume.getId(),
-                volume.getPath(), pool, sourceHostId, destHostId);
-    }
 
     public Volume attachVolumeToVM(Long vmId, Long volumeId, Long deviceId, Boolean allowAttachForSharedFS) {
         Account caller = CallContext.current().getCallingAccount();