kvm: fix direct-download live storage migration

Direct-download backed volumes should not use the linked-clone live storage migration path because the backing chain is not guaranteed on the destination host. Force the KVM migration request to full clone when a migrated volume is direct-download backed, while ignoring volumes that are skipped from the migration request.

Keep direct-download volume metadata consistent across VolumeDataFactory paths and make target-connection/template-reference failures explicit. Add unit coverage for forced full-clone, skipped-volume boundaries, copied template references, ModifyTargets answers, and VolumeDataFactory propagation.

Author: kunal.behbudzade

engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java

@@ -1964,7 +1964,7 @@ protected MigrationOptions createLinkedCloneMigrationOptions(VolumeInfo srcVolum
         Storage.StoragePoolType srcPoolType = srcPool.getPoolType();
         Long srcPoolClusterId = srcPool.getClusterId();
         VMTemplateStoragePoolVO ref = templatePoolDao.findByPoolTemplate(destVolumeInfo.getPoolId(), srcVolumeInfo.getTemplateId(), null);
-        boolean updateBackingFileReference = ref == null;
+        boolean updateBackingFileReference = ref == null || !StringUtils.equals(ref.getInstallPath(), srcVolumeBackingFile);
         String backingFile = !updateBackingFileReference ? ref.getInstallPath() : srcVolumeBackingFile;
         ScopeType scopeType = srcVolumeInfo.getDataStore().getScope().getScopeType();
         return new MigrationOptions(srcPoolUuid, srcPoolType, backingFile, updateBackingFileReference, scopeType, srcPoolClusterId);
@@ -2009,6 +2009,34 @@ protected void setVolumeMigrationOptions(VolumeInfo srcVolumeInfo, VolumeInfo de
         destVolumeInfo.setMigrationOptions(migrationOptions);
     }
 
+    /**
+     * KVM/libvirt selects linked-clone or full-clone storage migration for the whole VM migration request.
+     * If any disk is backed by a direct-download template, force the request to full clone so libvirt does
+     * not use incremental shared-backing semantics for a disk whose backing chain is not guaranteed on the destination.
+     */
+    protected boolean shouldForceFullCloneMigration(Map<VolumeInfo, DataStore> volumeDataStoreMap, Host destHost) {
+        for (Map.Entry<VolumeInfo, DataStore> entry : volumeDataStoreMap.entrySet()) {
+            VolumeInfo srcVolumeInfo = entry.getKey();
+            DataStore destDataStore = entry.getValue();
+            StoragePoolVO sourceStoragePool = _storagePoolDao.findById(srcVolumeInfo.getPoolId());
+            StoragePoolVO destStoragePool = _storagePoolDao.findById(destDataStore.getId());
+
+            if (shouldSkipVolumeMigration(sourceStoragePool, destHost, destStoragePool)) {
+                continue;
+            }
+
+            if (srcVolumeInfo.isDirectDownload()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    protected boolean shouldSkipVolumeMigration(StoragePoolVO sourceStoragePool, Host destHost, StoragePoolVO destStoragePool) {
+        return (sourceStoragePool.getId() == destStoragePool.getId() && sourceStoragePool.getPoolType() == Storage.StoragePoolType.PowerFlex) ||
+                !shouldMigrateVolume(sourceStoragePool, destHost, destStoragePool);
+    }
+
     /**
      * For each disk to migrate:
      * <ul>
@@ -2036,6 +2064,10 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
             List<MigrateDiskInfo> migrateDiskInfoList = new ArrayList<MigrateDiskInfo>();
 
             Map<String, MigrateCommand.MigrateDiskInfo> migrateStorage = new HashMap<>();
+            boolean forceFullCloneMigration = shouldForceFullCloneMigration(volumeDataStoreMap, destHost);
+            if (forceFullCloneMigration) {
+                logger.info("Using full clone live storage migration for VM [{}] because one or more migrated volumes are backed by direct-download templates.", vmTO);
+            }
 
             boolean managedStorageDestination = false;
             boolean migrateNonSharedInc = false;
@@ -2047,16 +2079,12 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
                 StoragePoolVO destStoragePool = _storagePoolDao.findById(destDataStore.getId());
                 StoragePoolVO sourceStoragePool = _storagePoolDao.findById(srcVolumeInfo.getPoolId());
 
-                // do not initiate migration for the same PowerFlex/ScaleIO pool
-                if (sourceStoragePool.getId() == destStoragePool.getId() && sourceStoragePool.getPoolType() == Storage.StoragePoolType.PowerFlex) {
-                    continue;
-                }
-
-                if (!shouldMigrateVolume(sourceStoragePool, destHost, destStoragePool)) {
+                if (shouldSkipVolumeMigration(sourceStoragePool, destHost, destStoragePool)) {
                     continue;
                 }
 
-                MigrationOptions.Type migrationType = decideMigrationTypeAndCopyTemplateIfNeeded(destHost, vmInstance, srcVolumeInfo, sourceStoragePool, destStoragePool, destDataStore);
+                MigrationOptions.Type migrationType = decideMigrationTypeAndCopyTemplateIfNeeded(destHost, vmInstance, srcVolumeInfo, sourceStoragePool, destStoragePool,
+                        destDataStore, forceFullCloneMigration);
                 migrateNonSharedInc = migrateNonSharedInc || MigrationOptions.Type.LinkedClone.equals(migrationType);
 
                 VolumeVO destVolume = duplicateVolumeOnAnotherStorage(srcVolume, destStoragePool);
@@ -2068,6 +2096,7 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
                 destVolumeInfo.processEvent(Event.MigrationCopySucceeded);
                 // move the volume from Ready to Migrating
                 destVolumeInfo.processEvent(Event.MigrationRequested);
+                srcVolumeInfoToDestVolumeInfo.put(srcVolumeInfo, destVolumeInfo);
 
                 setVolumeMigrationOptions(srcVolumeInfo, destVolumeInfo, vmTO, srcHost, destStoragePool, migrationType);
 
@@ -2112,8 +2141,6 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
                 prepareDiskWithSecretConsumerDetail(vmTO, srcVolumeInfo, destVolumeInfo.getPath());
 
                 migrateStorage.put(srcVolumeInfo.getPath(), migrateDiskInfo);
-
-                srcVolumeInfoToDestVolumeInfo.put(srcVolumeInfo, destVolumeInfo);
             }
 
             PrepareForMigrationCommand pfmc = new PrepareForMigrationCommand(vmTO);
@@ -2211,7 +2238,13 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
         }
     }
 
-    private MigrationOptions.Type decideMigrationTypeAndCopyTemplateIfNeeded(Host destHost, VMInstanceVO vmInstance, VolumeInfo srcVolumeInfo, StoragePoolVO sourceStoragePool, StoragePoolVO destStoragePool, DataStore destDataStore) {
+    protected MigrationOptions.Type decideMigrationTypeAndCopyTemplateIfNeeded(Host destHost, VMInstanceVO vmInstance, VolumeInfo srcVolumeInfo, StoragePoolVO sourceStoragePool,
+            StoragePoolVO destStoragePool, DataStore destDataStore, boolean forceFullCloneMigration) {
+        if (forceFullCloneMigration) {
+            logger.debug("Skipping linked clone migration for volume [{}] because the migration request includes a direct-download backed volume.", srcVolumeInfo.getId());
+            return MigrationOptions.Type.FullClone;
+        }
+
         VMTemplateVO vmTemplate = _vmTemplateDao.findById(vmInstance.getTemplateId());
         String srcVolumeBackingFile = getVolumeBackingFile(srcVolumeInfo);
         if (StringUtils.isNotBlank(srcVolumeBackingFile) && supportStoragePoolType(destStoragePool.getPoolType(), StoragePoolType.Filesystem) &&
@@ -2449,7 +2482,11 @@ private VolumeVO duplicateVolumeOnAnotherStorage(Volume volume, StoragePoolVO st
     protected String connectHostToVolume(Host host, long storagePoolId, String iqn) {
         ModifyTargetsCommand modifyTargetsCommand = getModifyTargetsCommand(storagePoolId, iqn, true);
 
-        return sendModifyTargetsCommand(modifyTargetsCommand, host.getId()).get(0);
+        List<String> connectedPaths = sendModifyTargetsCommand(modifyTargetsCommand, host.getId());
+        if (CollectionUtils.isEmpty(connectedPaths)) {
+            throw new CloudRuntimeException(String.format("Unable to modify targets on the following host: %s because no connected path was returned for target [%s].", host.getId(), iqn));
+        }
+        return connectedPaths.get(0);
     }
 
     private void disconnectHostFromVolume(Host host, long storagePoolId, String iqn) {
@@ -2484,14 +2521,25 @@ private ModifyTargetsCommand getModifyTargetsCommand(long storagePoolId, String
     }
 
     private List<String> sendModifyTargetsCommand(ModifyTargetsCommand cmd, long hostId) {
-        ModifyTargetsAnswer modifyTargetsAnswer = (ModifyTargetsAnswer)agentManager.easySend(hostId, cmd);
+        Answer answer = agentManager.easySend(hostId, cmd);
 
-        if (modifyTargetsAnswer == null) {
+        if (answer == null) {
             throw new CloudRuntimeException("Unable to get an answer to the modify targets command");
         }
 
+        if (!(answer instanceof ModifyTargetsAnswer)) {
+            String details = StringUtils.defaultIfBlank(answer.getDetails(),
+                    String.format("Unexpected answer type returned: %s", answer.getClass().getName()));
+            throw new CloudRuntimeException(String.format("Unable to modify targets on the following host: %s due to [%s]", hostId, details));
+        }
+
+        ModifyTargetsAnswer modifyTargetsAnswer = (ModifyTargetsAnswer)answer;
+
         if (!modifyTargetsAnswer.getResult()) {
             String msg = "Unable to modify targets on the following host: " + hostId;
+            if (StringUtils.isNotBlank(modifyTargetsAnswer.getDetails())) {
+                msg = String.format("%s due to [%s]", msg, modifyTargetsAnswer.getDetails());
+            }
 
             throw new CloudRuntimeException(msg);
         }
@@ -2504,14 +2552,25 @@ private List<String> sendModifyTargetsCommand(ModifyTargetsCommand cmd, long hos
      */
     protected void updateCopiedTemplateReference(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo) {
         VMTemplateStoragePoolVO ref = templatePoolDao.findByPoolTemplate(srcVolumeInfo.getPoolId(), srcVolumeInfo.getTemplateId(), null);
-        VMTemplateStoragePoolVO newRef = new VMTemplateStoragePoolVO(destVolumeInfo.getPoolId(), ref.getTemplateId(), null);
-        newRef.setDownloadPercent(100);
-        newRef.setDownloadState(VMTemplateStorageResourceAssoc.Status.DOWNLOADED);
-        newRef.setState(ObjectInDataStoreStateMachine.State.Ready);
-        newRef.setTemplateSize(ref.getTemplateSize());
-        newRef.setLocalDownloadPath(ref.getLocalDownloadPath());
-        newRef.setInstallPath(ref.getInstallPath());
-        templatePoolDao.persist(newRef);
+        if (ref == null) {
+            throw new CloudRuntimeException(String.format("Unable to update copied template reference because source template reference was not found for pool [%s] and template [%s].",
+                    srcVolumeInfo.getPoolId(), srcVolumeInfo.getTemplateId()));
+        }
+        VMTemplateStoragePoolVO destRef = templatePoolDao.findByPoolTemplate(destVolumeInfo.getPoolId(), ref.getTemplateId(), null);
+        if (destRef == null) {
+            destRef = new VMTemplateStoragePoolVO(destVolumeInfo.getPoolId(), ref.getTemplateId(), null);
+        }
+        destRef.setDownloadPercent(100);
+        destRef.setDownloadState(VMTemplateStorageResourceAssoc.Status.DOWNLOADED);
+        destRef.setState(ObjectInDataStoreStateMachine.State.Ready);
+        destRef.setTemplateSize(ref.getTemplateSize());
+        destRef.setLocalDownloadPath(ref.getLocalDownloadPath());
+        destRef.setInstallPath(ref.getInstallPath());
+        if (destRef.getId() == 0) {
+            templatePoolDao.persist(destRef);
+        } else {
+            templatePoolDao.update(destRef.getId(), destRef);
+        }
     }
 
     /**

engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/KvmNonManagedStorageSystemDataMotionTest.java

@@ -337,6 +337,29 @@ public void migrateTemplateToTargetStorageIfNeededTestTemplateNotOnTargetHost()
         configureAndTestcopyTemplateToTargetStorageIfNeeded(null, StoragePoolType.Filesystem, 1);
     }
 
+    @Test
+    public void copyTemplateToTargetStorageIfNeededTestDirectDownloadTemplateAlreadyStaged() {
+        DataStore destDataStore = Mockito.mock(DataStore.class);
+        Host destHost = Mockito.mock(Host.class);
+        VolumeInfo srcVolumeInfo = Mockito.mock(VolumeInfo.class);
+        StoragePool srcStoragePool = Mockito.mock(StoragePool.class);
+        StoragePool destStoragePool = Mockito.mock(StoragePool.class);
+        TemplateInfo directDownloadTemplateInfo = Mockito.mock(TemplateInfo.class);
+
+        Mockito.when(destDataStore.getId()).thenReturn(11L);
+        Mockito.when(destHost.getId()).thenReturn(12L);
+        Mockito.when(srcVolumeInfo.getTemplateId()).thenReturn(13L);
+        Mockito.when(srcVolumeInfo.getVolumeType()).thenReturn(Volume.Type.ROOT);
+        Mockito.when(templateDataFactory.getReadyBypassedTemplateOnPrimaryStore(13L, 11L, 12L)).thenReturn(directDownloadTemplateInfo);
+
+        kvmNonManagedStorageDataMotionStrategy.copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, srcStoragePool, destDataStore, destStoragePool, destHost);
+
+        Mockito.verify(templateDataFactory).getReadyBypassedTemplateOnPrimaryStore(13L, 11L, 12L);
+        Mockito.verify(vmTemplatePoolDao, Mockito.never()).findByPoolTemplate(Mockito.anyLong(), Mockito.anyLong(), nullable(String.class));
+        Mockito.verify(dataStoreManagerImpl, Mockito.never()).getRandomImageStore(Mockito.anyLong());
+        Mockito.verify(kvmNonManagedStorageDataMotionStrategy, Mockito.never()).sendCopyCommand(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
+    }
+
     @Test
     public void migrateTemplateToTargetStorageIfNeededTestNonDesiredStoragePoolType() throws AgentUnavailableException, OperationTimedoutException {
         StoragePoolType[] storagePoolTypeArray = StoragePoolType.values();