kvm: refine direct-download migration checks

Avoid repeating storage pool lookups while deciding and preparing KVM live storage migration requests.

Also make the skipped-volume test assert that skipped direct-download volumes do not affect the full-clone decision.

Author: kunal.behbudzade

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

@@ -2015,11 +2015,15 @@ protected void setVolumeMigrationOptions(VolumeInfo srcVolumeInfo, VolumeInfo de
      * 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) {
+        return shouldForceFullCloneMigration(volumeDataStoreMap, destHost, new HashMap<>());
+    }
+
+    protected boolean shouldForceFullCloneMigration(Map<VolumeInfo, DataStore> volumeDataStoreMap, Host destHost, Map<Long, StoragePoolVO> storagePoolsById) {
         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());
+            StoragePoolVO sourceStoragePool = getStoragePool(storagePoolsById, srcVolumeInfo.getPoolId());
+            StoragePoolVO destStoragePool = getStoragePool(storagePoolsById, destDataStore.getId());
 
             if (shouldSkipVolumeMigration(sourceStoragePool, destHost, destStoragePool)) {
                 continue;
@@ -2032,6 +2036,17 @@ protected boolean shouldForceFullCloneMigration(Map<VolumeInfo, DataStore> volum
         return false;
     }
 
+    private StoragePoolVO getStoragePool(Map<Long, StoragePoolVO> storagePoolsById, long storagePoolId) {
+        StoragePoolVO storagePool = storagePoolsById.get(storagePoolId);
+        if (storagePool == null) {
+            storagePool = _storagePoolDao.findById(storagePoolId);
+            if (storagePool != null) {
+                storagePoolsById.put(storagePoolId, storagePool);
+            }
+        }
+        return storagePool;
+    }
+
     protected boolean shouldSkipVolumeMigration(StoragePoolVO sourceStoragePool, Host destHost, StoragePoolVO destStoragePool) {
         return (sourceStoragePool.getId() == destStoragePool.getId() && sourceStoragePool.getPoolType() == Storage.StoragePoolType.PowerFlex) ||
                 !shouldMigrateVolume(sourceStoragePool, destHost, destStoragePool);
@@ -2064,7 +2079,8 @@ 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);
+            Map<Long, StoragePoolVO> storagePoolsById = new HashMap<>();
+            boolean forceFullCloneMigration = shouldForceFullCloneMigration(volumeDataStoreMap, destHost, storagePoolsById);
             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);
             }
@@ -2076,8 +2092,8 @@ public void copyAsync(Map<VolumeInfo, DataStore> volumeDataStoreMap, VirtualMach
                 DataStore destDataStore = entry.getValue();
 
                 VolumeVO srcVolume = _volumeDao.findById(srcVolumeInfo.getId());
-                StoragePoolVO destStoragePool = _storagePoolDao.findById(destDataStore.getId());
-                StoragePoolVO sourceStoragePool = _storagePoolDao.findById(srcVolumeInfo.getPoolId());
+                StoragePoolVO destStoragePool = getStoragePool(storagePoolsById, destDataStore.getId());
+                StoragePoolVO sourceStoragePool = getStoragePool(storagePoolsById, srcVolumeInfo.getPoolId());
 
                 if (shouldSkipVolumeMigration(sourceStoragePool, destHost, destStoragePool)) {
                     continue;

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

@@ -414,16 +414,18 @@ public void shouldForceFullCloneMigrationIgnoresSkippedDirectDownloadVolumes() {
         DataStore samePowerFlexStore = Mockito.mock(DataStore.class);
         DataStore destPrimary = Mockito.mock(DataStore.class);
         Host destHost = Mockito.mock(Host.class);
-        Map<VolumeInfo, DataStore> volumeMap = new HashMap<>();
+        Map<VolumeInfo, DataStore> volumeMap = new LinkedHashMap<>();
 
         configurePoolLookup(skippedDirectDownloadVolume, samePowerFlexStore, 1L, 1L, StoragePoolType.PowerFlex, StoragePoolType.PowerFlex);
         configurePoolLookup(regularVolume, destPrimary, 3L, 4L, StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem);
+        lenient().when(skippedDirectDownloadVolume.isDirectDownload()).thenReturn(true);
         Mockito.when(regularVolume.isDirectDownload()).thenReturn(false);
 
         volumeMap.put(skippedDirectDownloadVolume, samePowerFlexStore);
         volumeMap.put(regularVolume, destPrimary);
 
         Assert.assertFalse(strategy.shouldForceFullCloneMigration(volumeMap, destHost));
+        Mockito.verify(skippedDirectDownloadVolume, Mockito.never()).isDirectDownload();
     }
 
     @Test