JoaoJandre commented on code in PR #12617:
URL: https://github.com/apache/cloudstack/pull/12617#discussion_r3382315110
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3150,6 +3158,11 @@ protected void migrate(final VMInstanceVO vm, final long
srcHostId, final Deploy
updateOverCommitRatioForVmProfile(profile,
dest.getHost().getClusterId());
final VirtualMachineTO to = toVmTO(profile);
+
+ if (vm.getHypervisorType() == HypervisorType.KVM &&
hasClvmVolumes(vm.getId())) {
+ executePreMigrationCommand(to, vm.getInstanceName(), srcHostId);
+ }
Review Comment:
I think this check could be inside the executePreMigrationCommand, like it
is done for the executePostMigrationCommand method.
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3320,6 +3334,29 @@ protected void migrate(final VMInstanceVO vm, final long
srcHostId, final Deploy
}
}
+ private void executePostMigrationCommand(VMInstanceVO vm, VirtualMachineTO
to, long dstHostId) {
+ if (vm.getHypervisorType() == HypervisorType.KVM &&
hasClvmVolumes(vm.getId())) {
Review Comment:
We could invert the logic and return early, reducing indentation.
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3320,6 +3334,29 @@ protected void migrate(final VMInstanceVO vm, final long
srcHostId, final Deploy
}
}
+ private void executePostMigrationCommand(VMInstanceVO vm, VirtualMachineTO
to, long dstHostId) {
+ if (vm.getHypervisorType() == HypervisorType.KVM &&
hasClvmVolumes(vm.getId())) {
+ try {
+ logger.info("Executing post-migration tasks for VM {} with
CLVM volumes on destination host {}", vm.getInstanceName(), dstHostId);
Review Comment:
Could we log the host uuid instead?
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -6441,6 +6507,32 @@ private Pair<Long, Long>
findClusterAndHostIdForVm(VirtualMachine vm) {
return findClusterAndHostIdForVm(vm, false);
}
+ private boolean hasClvmVolumes(long vmId) {
+ List<VolumeVO> volumes = _volsDao.findByInstance(vmId);
+ return volumes.stream()
+ .map(v -> _storagePoolDao.findById(v.getPoolId()))
+ .anyMatch(pool -> pool != null &&
ClvmPoolManager.isClvmPoolType(pool.getPoolType()));
+ }
+
+ private void executePreMigrationCommand(VirtualMachineTO to, String
vmInstanceName, long srcHostId) {
+ logger.info("Sending PreMigrationCommand to source host {} for VM {}
with CLVM volumes", srcHostId, vmInstanceName);
Review Comment:
same about host uuid
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -788,6 +805,121 @@ private String getVolumeIdentificationInfos(Volume
volume) {
return String.format("uuid: %s, name: %s", volume.getUuid(),
volume.getName());
}
+ /**
+ * Updates the CLVM_LOCK_HOST_ID for a migrated volume if applicable.
+ * For CLVM volumes that are attached to a VM, this updates the lock host
tracking
+ * to point to the VM's current host after volume migration.
+ *
+ * @param migratedVolume The volume that was migrated
+ * @param destPool The destination storage pool
+ * @param operationType Description of the operation (e.g., "migrated",
"live-migrated") for logging
+ */
+ private void updateClvmLockHostAfterMigration(Volume migratedVolume,
StoragePool destPool, String operationType) {
+ if (migratedVolume == null || destPool == null) {
+ return;
+ }
+
+ StoragePoolVO pool = _storagePoolDao.findById(destPool.getId());
+ if (pool == null ||
!ClvmPoolManager.isClvmPoolType(pool.getPoolType())) {
+ return;
+ }
+
+ if (migratedVolume.getInstanceId() == null) {
+ return;
+ }
+
+ VMInstanceVO vm =
vmInstanceDao.findById(migratedVolume.getInstanceId());
+ if (vm == null || vm.getHostId() == null) {
+ return;
+ }
+
+ clvmPoolManager.setClvmLockHostId(migratedVolume.getId(),
vm.getHostId());
+ logger.debug("Updated CLVM_LOCK_HOST_ID for {} volume {} to host {}
where VM {} is running",
+ operationType, migratedVolume.getUuid(), vm.getHostId(),
vm.getInstanceName());
+ }
+
+ /**
+ * Retrieves the CLVM lock host ID from any existing volume of the
specified VM.
+ * This is useful when attaching a new volume to a stopped VM - we want to
maintain
+ * consistency by using the same host that manages the VM's other CLVM
volumes.
+ *
+ * @param vmId The ID of the VM
+ * @return The host ID if found, null otherwise
+ */
+ private Long getClvmLockHostFromVmVolumes(Long vmId) {
+ if (vmId == null) {
+ return null;
+ }
+
+ List<VolumeVO> vmVolumes = _volsDao.findByInstance(vmId);
+ if (vmVolumes == null || vmVolumes.isEmpty()) {
+ return null;
+ }
+
+ for (VolumeVO volume : vmVolumes) {
+ if (volume.getPoolId() == null) {
+ continue;
+ }
+
+ StoragePoolVO pool = _storagePoolDao.findById(volume.getPoolId());
+ if (pool != null &&
ClvmPoolManager.isClvmPoolType(pool.getPoolType())) {
Review Comment:
we could invert the logic to reduce indentation.
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3320,6 +3334,29 @@ protected void migrate(final VMInstanceVO vm, final long
srcHostId, final Deploy
}
}
+ private void executePostMigrationCommand(VMInstanceVO vm, VirtualMachineTO
to, long dstHostId) {
+ if (vm.getHypervisorType() == HypervisorType.KVM &&
hasClvmVolumes(vm.getId())) {
+ try {
+ logger.info("Executing post-migration tasks for VM {} with
CLVM volumes on destination host {}", vm.getInstanceName(), dstHostId);
+ final PostMigrationCommand postMigrationCommand = new
PostMigrationCommand(to, vm.getInstanceName());
+ final Answer postMigrationAnswer = _agentMgr.send(dstHostId,
postMigrationCommand);
+
+ if (postMigrationAnswer == null ||
!postMigrationAnswer.getResult()) {
+ final String details = postMigrationAnswer != null ?
postMigrationAnswer.getDetails() : "null answer returned";
+ logger.warn("Post-migration tasks failed for VM {} on
destination host {}: {}. Migration completed but some cleanup may be needed.",
+ vm.getInstanceName(), dstHostId, details);
+ } else {
+ logger.info("Successfully completed post-migration tasks
for VM {} on destination host {}", vm.getInstanceName(), dstHostId);
+ }
+ } catch (Exception e) {
+ logger.warn("Exception during post-migration tasks for VM {}
on destination host {}: {}. Migration completed but some cleanup may be
needed.",
+ vm.getInstanceName(), dstHostId, e.getMessage(), e);
+ }
+ }
+
+ updateClvmLockHostForVmVolumes(vm.getId(), dstHostId);
Review Comment:
If the result is false, or null, we still update the lock? How do we know if
this update will be correct?
If the host does not execute the command (the command queue is full and the
command times out, or the host is restarted) we will update the lock metadata
to point to the wrong host, correct?
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -4922,6 +4980,14 @@ private void orchestrateMigrateForScale(final String
vmUuid, final long srcHostI
volumeMgr.prepareForMigration(profile, dest);
final VirtualMachineTO to = toVmTO(profile);
+
+ // Step 1: Send PreMigrationCommand to source host to convert CLVM
volumes to shared mode
+ // This must happen BEFORE PrepareForMigrationCommand on destination
to avoid lock conflicts
+ if (vm.getHypervisorType() == HypervisorType.KVM &&
hasClvmVolumes(vm.getId())) {
+ executePreMigrationCommand(to, vm.getInstanceName(), srcHostId);
Review Comment:
In this case we only execute the premigration, but not the post migration?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2132,6 +2157,23 @@ public void copyAsync(Map<VolumeInfo, DataStore>
volumeDataStoreMap, VirtualMach
throw new AgentUnavailableException("Operation timed out",
destHost.getId());
}
+ for (VolumeInfo vol : samePoolClvmVolumes) {
Review Comment:
shouldn't we check if the pool is clvm before executing this for?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2132,6 +2157,23 @@ public void copyAsync(Map<VolumeInfo, DataStore>
volumeDataStoreMap, VirtualMach
throw new AgentUnavailableException("Operation timed out",
destHost.getId());
}
+ for (VolumeInfo vol : samePoolClvmVolumes) {
+ StoragePoolVO samePoolClvmPool =
_storagePoolDao.findById(vol.getPoolId());
+ String vgName = samePoolClvmPool.getPath();
+ if (vgName.startsWith("/")) vgName = vgName.substring(1);
Review Comment:
this `if` does not follow the java coding conventions for ACS.
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java:
##########
@@ -331,6 +336,27 @@ protected Answer copyVolumeFromSnapshot(DataObject
snapObj, DataObject volObj) {
}
}
+ private void updateLockHostForVolume(EndPoint ep, DataObject volObj) {
+ if (ep != null && volObj instanceof VolumeInfo) {
+ VolumeInfo volumeInfo = (VolumeInfo) volObj;
+ StoragePool destPool = (StoragePool) volObj.getDataStore();
+ if (destPool != null &&
ClvmPoolManager.isClvmPoolType(destPool.getPoolType())) {
Review Comment:
we could invert both of these logics to reduce indentation
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2211,19 +2253,56 @@ public void copyAsync(Map<VolumeInfo, DataStore>
volumeDataStoreMap, VirtualMach
}
}
+ private void prepareDisksForMigrationForClvm(VirtualMachineTO vmTO,
Map<VolumeInfo, DataStore> volumeDataStoreMap, Host srcHost) {
+ // For CLVM/CLVM_NG source pools, convert volumes from exclusive to
shared mode
+ // on the source host BEFORE PrepareForMigrationCommand on the
destination.
+ boolean hasClvmSource = volumeDataStoreMap.keySet().stream()
+ .map(v -> _storagePoolDao.findById(v.getPoolId()))
+ .anyMatch(p -> p != null && (p.getPoolType() ==
StoragePoolType.CLVM || p.getPoolType() == StoragePoolType.CLVM_NG));
+ if (hasClvmSource && srcHost.getHypervisorType() ==
HypervisorType.KVM) {
+ logger.info("CLVM/CLVM_NG source pool detected for VM [{}],
sending PreMigrationCommand to source host [{}] to convert volumes to shared
mode.", vmTO.getName(), srcHost.getId());
+ PreMigrationCommand preMigCmd = new PreMigrationCommand(vmTO,
vmTO.getName());
+ try {
+ Answer preMigAnswer = agentManager.send(srcHost.getId(),
preMigCmd);
+ if (preMigAnswer == null || !preMigAnswer.getResult()) {
+ String details = preMigAnswer != null ?
preMigAnswer.getDetails() : "null answer returned";
+ logger.warn("PreMigrationCommand failed for CLVM/CLVM_NG
VM [{}] on source host [{}]: {}. Migration will continue but may fail if
volumes are exclusively locked.", vmTO.getName(), srcHost.getId(), details);
Review Comment:
Shouldn't we abort the migration in this case?
If we continue and it does not work, it could lead to problems in the VM, no?
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -6441,6 +6507,32 @@ private Pair<Long, Long>
findClusterAndHostIdForVm(VirtualMachine vm) {
return findClusterAndHostIdForVm(vm, false);
}
+ private boolean hasClvmVolumes(long vmId) {
+ List<VolumeVO> volumes = _volsDao.findByInstance(vmId);
+ return volumes.stream()
+ .map(v -> _storagePoolDao.findById(v.getPoolId()))
+ .anyMatch(pool -> pool != null &&
ClvmPoolManager.isClvmPoolType(pool.getPoolType()));
+ }
+
+ private void executePreMigrationCommand(VirtualMachineTO to, String
vmInstanceName, long srcHostId) {
+ logger.info("Sending PreMigrationCommand to source host {} for VM {}
with CLVM volumes", srcHostId, vmInstanceName);
+ final PreMigrationCommand preMigCmd = new PreMigrationCommand(to,
vmInstanceName);
+ Answer preMigAnswer = null;
+ try {
+ preMigAnswer = _agentMgr.send(srcHostId, preMigCmd);
+ if (preMigAnswer == null || !preMigAnswer.getResult()) {
+ final String details = preMigAnswer != null ?
preMigAnswer.getDetails() : "null answer returned";
Review Comment:
What will happen if the command was executed in the host, but the answer
could not be delivered to the Management Server? Will the next try be able to
repeat the process or will it be stuck ?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageDataMotionStrategy.java:
##########
@@ -144,12 +144,16 @@ protected boolean
isDestinationNfsPrimaryStorageClusterWide(Map<VolumeInfo, Data
}
/**
- * Configures a {@link MigrateDiskInfo} object configured for migrating a
File System volume and calls rootImageProvisioning.
+ * Configures a {@link MigrateDiskInfo} object configured for migrating a
File System volume.
*/
@Override
protected MigrateCommand.MigrateDiskInfo
configureMigrateDiskInfo(VolumeInfo srcVolumeInfo, String destPath, String
backingPath) {
- return new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(),
MigrateCommand.MigrateDiskInfo.DiskType.FILE,
MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
- MigrateCommand.MigrateDiskInfo.Source.FILE, destPath,
backingPath);
+ return new MigrateCommand.MigrateDiskInfo(srcVolumeInfo.getPath(),
+ MigrateCommand.MigrateDiskInfo.DiskType.FILE,
+ MigrateCommand.MigrateDiskInfo.DriverType.QCOW2,
+ MigrateCommand.MigrateDiskInfo.Source.FILE,
+ destPath,
+ backingPath);
Review Comment:
Is this a style change only? Is it needed?
--
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]
