winterhazel commented on code in PR #12617:
URL: https://github.com/apache/cloudstack/pull/12617#discussion_r3383645679
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1659,6 +1662,9 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume)
throws ResourceAllocationExc
if (backupSnapToSecondary) {
if (!isKvmAndFileBasedStorage) {
backupSnapshotToSecondary(payload.getAsyncBackup(),
snapshotStrategy, snapshotOnPrimary, payload.getZoneIds(),
payload.getStoragePoolIds());
+ if (!payload.getAsyncBackup() &&
(storagePool.getPoolType() == StoragePoolType.CLVM || storagePool.getPoolType()
== StoragePoolType.CLVM_NG)) {
Review Comment:
Use `ClvmPoolManager.isClvmPoolType` here
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2738,21 +2746,42 @@ private Volume orchestrateAttachVolumeToVM(Long vmId,
Long volumeId, Long device
logger.trace(String.format("is it needed to move the volume: %b?",
moveVolumeNeeded));
}
- if (moveVolumeNeeded) {
+ // Check if CLVM lock transfer is needed (even if moveVolumeNeeded is
false)
+ // This handles the case where the volume is already on the correct
storage pool
+ // but the VM is running on a different host, requiring only a lock
transfer
+ boolean isClvmLockTransferNeeded = !moveVolumeNeeded &&
+ isClvmLockTransferRequired(newVolumeOnPrimaryStorage,
existingVolumeOfVm, vm);
+
+ if (isClvmLockTransferNeeded) {
+ // CLVM lock transfer - no data copy, no pool change needed
+ newVolumeOnPrimaryStorage = executeLightweightLockMigration(
+ newVolumeOnPrimaryStorage, vm, existingVolumeOfVm,
+ "CLVM lock transfer", "same pool to different host");
+ } else if (moveVolumeNeeded) {
PrimaryDataStoreInfo primaryStore =
(PrimaryDataStoreInfo)newVolumeOnPrimaryStorage.getDataStore();
if (primaryStore.isLocal()) {
throw new CloudRuntimeException(
"Failed to attach local data volume " +
volumeToAttach.getName() + " to VM " + vm.getDisplayName() + " as migration of
local data volume is not allowed");
}
- StoragePoolVO vmRootVolumePool =
_storagePoolDao.findById(existingVolumeOfVm.getPoolId());
- try {
- HypervisorType volumeToAttachHyperType =
_volsDao.getHypervisorType(volumeToAttach.getId());
- newVolumeOnPrimaryStorage =
_volumeMgr.moveVolume(newVolumeOnPrimaryStorage,
vmRootVolumePool.getDataCenterId(), vmRootVolumePool.getPodId(),
vmRootVolumePool.getClusterId(),
- volumeToAttachHyperType);
- } catch (ConcurrentOperationException |
StorageUnavailableException e) {
- logger.debug("move volume failed", e);
- throw new CloudRuntimeException("move volume failed", e);
+ boolean isClvmLightweightMigration =
isClvmLightweightMigrationNeeded(
+ newVolumeOnPrimaryStorage, existingVolumeOfVm, vm);
+
+ if (isClvmLightweightMigration) {
+ newVolumeOnPrimaryStorage = executeLightweightLockMigration(
+ newVolumeOnPrimaryStorage, vm, existingVolumeOfVm,
+ "CLVM lightweight migration", "different pools, same
VG");
+ } else {
+ StoragePoolVO vmRootVolumePool =
_storagePoolDao.findById(existingVolumeOfVm.getPoolId());
+
+ try {
+ HypervisorType volumeToAttachHyperType =
_volsDao.getHypervisorType(volumeToAttach.getId());
+ newVolumeOnPrimaryStorage =
_volumeMgr.moveVolume(newVolumeOnPrimaryStorage,
vmRootVolumePool.getDataCenterId(), vmRootVolumePool.getPodId(),
vmRootVolumePool.getClusterId(),
+ volumeToAttachHyperType);
+ } catch (ConcurrentOperationException |
StorageUnavailableException e) {
+ logger.debug("move volume failed", e);
+ throw new CloudRuntimeException("move volume failed", e);
+ }
}
Review Comment:
This should be a straightfoward refactor. I would prefer having it on this
one already, but its up to you.
--
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]
