Prioritize copying templates from other secondary storages instead of downloading them (#10363)

* Prioritize copying templates from other secondary storages instead of downloading them

* Treat some corner cases

engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java

@@ -18,12 +18,18 @@
 package org.apache.cloudstack.engine.orchestration.service;
 
 import java.util.List;
+import java.util.concurrent.Future;
 
 import org.apache.cloudstack.api.response.MigrationResponse;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService.TemplateApiResult;
 import org.apache.cloudstack.storage.ImageStoreService.MigrationPolicy;
 
 public interface StorageOrchestrationService {
     MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy);
 
     MigrationResponse migrateResources(Long srcImgStoreId, Long destImgStoreId, List<Long> templateIdList, List<Long> snapshotIdList);
+
+    Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore);
 }

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

@@ -78,4 +78,6 @@ public interface TemplateService {
     AsyncCallFuture<TemplateApiResult> createDatadiskTemplateAsync(TemplateInfo parentTemplate, TemplateInfo dataDiskTemplate, String path, String diskId, long fileSize, boolean bootable);
 
     List<DatadiskTO> getTemplateDatadisksOnImageStore(TemplateInfo templateInfo, String configurationId);
+
+    AsyncCallFuture<TemplateApiResult> copyTemplateToImageStore(DataObject source, DataStore destStore);
 }

engine/components-api/src/main/java/com/cloud/storage/StorageManager.java

@@ -220,6 +220,10 @@ public interface StorageManager extends StorageService {
             "storage.pool.host.connect.workers", "1",
             "Number of worker threads to be used to connect hosts to a primary storage", true);
 
+    ConfigKey<Boolean> COPY_PUBLIC_TEMPLATES_FROM_OTHER_STORAGES = new ConfigKey<>(Boolean.class, "copy.public.templates.from.other.storages",
+            "Storage", "true", "Allow SSVMs to try copying public templates from one secondary storage to another instead of downloading them from the source.",
+            true, ConfigKey.Scope.Zone, null);
+
     /**
      * should we execute in sequence not involving any storages?
      * @return tru if commands should execute in sequence

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java

@@ -22,10 +22,12 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import javax.inject.Inject;
 
@@ -206,12 +208,22 @@ public class DataMigrationUtility {
 
     protected List<DataObject> getAllReadyTemplates(DataStore srcDataStore, Map<DataObject, Pair<List<TemplateInfo>, Long>> childTemplates, List<TemplateDataStoreVO> templates) {
         List<TemplateInfo> files = new LinkedList<>();
+        Set<Long> idsForMigration = new HashSet<>();
+
         for (TemplateDataStoreVO template : templates) {
-            VMTemplateVO templateVO = templateDao.findById(template.getTemplateId());
+            long templateId = template.getTemplateId();
-            if (shouldMigrateTemplate(template, templateVO)) {
+            if (idsForMigration.contains(templateId)) {
-                files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore));
+                logger.warn("Template store reference [{}] is duplicated; not considering it for migration.", template);
+                continue;
             }
+            VMTemplateVO templateVO = templateDao.findById(templateId);
+            if (!shouldMigrateTemplate(template, templateVO)) {
+                continue;
+            }
+            files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore));
+            idsForMigration.add(templateId);
         }
+
         for (TemplateInfo template: files) {
             List<VMTemplateVO> children = templateDao.listByParentTemplatetId(template.getId());
             List<TemplateInfo> temps = new ArrayList<>();
@@ -221,6 +233,7 @@ public class DataMigrationUtility {
             }
             childTemplates.put(template, new Pair<>(temps, getTotalChainSize(temps)));
         }
+
         return (List<DataObject>) (List<?>) files;
     }
 
@@ -263,16 +276,37 @@ public class DataMigrationUtility {
      */
     protected List<DataObject> getAllReadySnapshotsAndChains(DataStore srcDataStore, Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChains, List<SnapshotDataStoreVO> snapshots) {
         List<SnapshotInfo> files = new LinkedList<>();
+        Set<Long> idsForMigration = new HashSet<>();
+
         for (SnapshotDataStoreVO snapshot : snapshots) {
-            SnapshotVO snapshotVO = snapshotDao.findById(snapshot.getSnapshotId());
+            long snapshotId = snapshot.getSnapshotId();
-            if (snapshot.getState() == ObjectInDataStoreStateMachine.State.Ready &&
+            if (idsForMigration.contains(snapshotId)) {
-                    snapshotVO != null && snapshotVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator
+                logger.warn("Snapshot store reference [{}] is duplicated; not considering it for migration.", snapshot);
-                    && snapshot.getParentSnapshotId() == 0 ) {
+                continue;
-                SnapshotInfo snap = snapshotFactory.getSnapshot(snapshotVO.getSnapshotId(), snapshot.getDataStoreId(), snapshot.getRole());
-                if (snap != null) {
-                    files.add(snap);
-                }
             }
+            if (snapshot.getState() != ObjectInDataStoreStateMachine.State.Ready) {
+                logger.warn("Not migrating snapshot [{}] because its state is not ready.", snapshot);
+                continue;
+            }
+            SnapshotVO snapshotVO = snapshotDao.findById(snapshotId);
+            if (snapshotVO == null) {
+                logger.debug("Not migrating snapshot [{}] because we could not find its database entry.", snapshot);
+                continue;
+            }
+            if (snapshotVO.getHypervisorType() == Hypervisor.HypervisorType.Simulator) {
+                logger.debug("Not migrating snapshot [{}] because its hypervisor type is simulator.", snapshot);
+                continue;
+            }
+            if (snapshot.getParentSnapshotId() != 0) {
+                continue; // The child snapshot will be migrated in the for loop below.
+            }
+            SnapshotInfo snap = snapshotFactory.getSnapshot(snapshotVO.getSnapshotId(), snapshot.getDataStoreId(), snapshot.getRole());
+            if (snap == null) {
+                logger.debug("Not migrating snapshot [{}] because we could not get its information.", snapshot);
+                continue;
+            }
+            files.add(snap);
+            idsForMigration.add(snapshotId);
         }
 
         for (SnapshotInfo parent : files) {
@@ -285,7 +319,7 @@ public class DataMigrationUtility {
                     chain.addAll(children);
                 }
             }
-            snapshotChains.put(parent, new Pair<List<SnapshotInfo>, Long>(chain, getTotalChainSize(chain)));
+            snapshotChains.put(parent, new Pair<>(chain, getTotalChainSize(chain)));
         }
 
         return (List<DataObject>) (List<?>) files;
@@ -306,14 +340,31 @@ public class DataMigrationUtility {
 
     protected List<DataObject> getAllReadyVolumes(DataStore srcDataStore, List<VolumeDataStoreVO> volumes) {
         List<DataObject> files = new LinkedList<>();
+        Set<Long> idsForMigration = new HashSet<>();
+
         for (VolumeDataStoreVO volume : volumes) {
-            if (volume.getState() == ObjectInDataStoreStateMachine.State.Ready) {
+            long volumeId = volume.getVolumeId();
-                VolumeInfo volumeInfo = volumeFactory.getVolume(volume.getVolumeId(), srcDataStore);
+            if (idsForMigration.contains(volumeId)) {
-                if (volumeInfo != null && volumeInfo.getHypervisorType() != Hypervisor.HypervisorType.Simulator) {
+                logger.warn("Volume store reference [{}] is duplicated; not considering it for migration.", volume);
-                    files.add(volumeInfo);
+                continue;
-                }
             }
+            if (volume.getState() != ObjectInDataStoreStateMachine.State.Ready) {
+                logger.debug("Not migrating volume [{}] because its state is not ready.", volume);
+                continue;
+            }
+            VolumeInfo volumeInfo = volumeFactory.getVolume(volume.getVolumeId(), srcDataStore);
+            if (volumeInfo == null) {
+                logger.debug("Not migrating volume [{}] because we could not get its information.", volume);
+                continue;
+            }
+            if (volumeInfo.getHypervisorType() == Hypervisor.HypervisorType.Simulator) {
+                logger.debug("Not migrating volume [{}] because its hypervisor type is simulator.", volume);
+                continue;
+            }
+            files.add(volumeInfo);
+            idsForMigration.add(volumeId);
         }
+
         return files;
     }
 
@@ -325,10 +376,9 @@ public class DataMigrationUtility {
     /** Returns the count of active SSVMs - SSVM with agents in connected state, so as to dynamically increase the thread pool
      * size when SSVMs scale
      */
-    protected int activeSSVMCount(DataStore dataStore) {
+    protected int activeSSVMCount(Long zoneId) {
-        long datacenterId = dataStore.getScope().getScopeId();
         List<SecondaryStorageVmVO> ssvms =
-                secStorageVmDao.getSecStorageVmListInStates(null, datacenterId, VirtualMachine.State.Running, VirtualMachine.State.Migrating);
+                secStorageVmDao.getSecStorageVmListInStates(null, zoneId, VirtualMachine.State.Running, VirtualMachine.State.Migrating);
         int activeSSVMs = 0;
         for (SecondaryStorageVmVO vm : ssvms) {
             String name = "s-"+vm.getId()+"-VM";

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

@@ -46,6 +46,8 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageServic
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService.TemplateApiResult;
 import org.apache.cloudstack.framework.async.AsyncCallFuture;
 import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.framework.config.Configurable;
@@ -71,9 +73,12 @@ import com.cloud.storage.dao.SnapshotDao;
 import com.cloud.utils.Pair;
 import com.cloud.utils.component.ManagerBase;
 import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.logging.log4j.ThreadContext;
 
 public class StorageOrchestrator extends ManagerBase implements StorageOrchestrationService, Configurable {
 
+    private static final String LOGCONTEXTID = "logcontextid";
+
     @Inject
     SnapshotDataStoreDao snapshotDataStoreDao;
     @Inject
@@ -91,6 +96,8 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
     @Inject
     private SecondaryStorageService secStgSrv;
     @Inject
+    TemplateService templateService;
+    @Inject
     TemplateDataStoreDao templateDataStoreDao;
     @Inject
     VolumeDataStoreDao volumeDataStoreDao;
@@ -106,6 +113,9 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
 
     Integer numConcurrentCopyTasksPerSSVM = 2;
 
+    private final Map<Long, ThreadPoolExecutor> zoneExecutorMap = new HashMap<>();
+    private final Map<Long, Integer> zonePendingWorkCountMap = new HashMap<>();
+
     @Override
     public String getConfigComponentName() {
         return StorageOrchestrationService.class.getName();
@@ -167,8 +177,6 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         double meanstddev = getStandardDeviation(storageCapacities);
         double threshold = ImageStoreImbalanceThreshold.value();
         MigrationResponse response = null;
-        ThreadPoolExecutor executor = new ThreadPoolExecutor(numConcurrentCopyTasksPerSSVM , numConcurrentCopyTasksPerSSVM, 30,
-                TimeUnit.MINUTES, new MigrateBlockingQueue<>(numConcurrentCopyTasksPerSSVM));
         Date start = new Date();
         if (meanstddev < threshold && migrationPolicy == MigrationPolicy.BALANCE) {
             logger.debug("mean std deviation of the image stores is below threshold, no migration required");
@@ -177,7 +185,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         }
 
         int skipped = 0;
-        List<Future<AsyncCallFuture<DataObjectResult>>> futures = new ArrayList<>();
+        List<Future<DataObjectResult>> futures = new ArrayList<>();
         while (true) {
             DataObject chosenFileForMigration = null;
             if (files.size() > 0) {
@@ -206,7 +214,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
             }
 
             if (shouldMigrate(chosenFileForMigration, srcDatastore.getId(), destDatastoreId, storageCapacities, snapshotChains, childTemplates, migrationPolicy)) {
-                storageCapacities = migrateAway(chosenFileForMigration, storageCapacities, snapshotChains, childTemplates, srcDatastore, destDatastoreId, executor, futures);
+                storageCapacities = migrateAway(chosenFileForMigration, storageCapacities, snapshotChains, childTemplates, srcDatastore, destDatastoreId, futures);
             } else {
                 if (migrationPolicy == MigrationPolicy.BALANCE) {
                     continue;
@@ -217,7 +225,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
             }
         }
         Date end = new Date();
-        handleSnapshotMigration(srcDataStoreId, start, end, migrationPolicy, futures, storageCapacities, executor);
+        handleSnapshotMigration(srcDataStoreId, start, end, migrationPolicy, futures, storageCapacities);
         return handleResponse(futures, migrationPolicy, message, success);
     }
 
@@ -250,9 +258,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         storageCapacities = getStorageCapacities(storageCapacities, srcImgStoreId);
         storageCapacities = getStorageCapacities(storageCapacities, destImgStoreId);
 
-        ThreadPoolExecutor executor = new ThreadPoolExecutor(numConcurrentCopyTasksPerSSVM, numConcurrentCopyTasksPerSSVM, 30,
+        List<Future<DataObjectResult>> futures = new ArrayList<>();
-                TimeUnit.MINUTES, new MigrateBlockingQueue<>(numConcurrentCopyTasksPerSSVM));
-        List<Future<AsyncCallFuture<DataObjectResult>>> futures = new ArrayList<>();
         Date start = new Date();
 
         while (true) {
@@ -272,7 +278,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
             }
 
             if (storageCapacityBelowThreshold(storageCapacities, destImgStoreId)) {
-                storageCapacities = migrateAway(chosenFileForMigration, storageCapacities, snapshotChains, childTemplates, srcDatastore, destImgStoreId, executor, futures);
+                storageCapacities = migrateAway(chosenFileForMigration, storageCapacities, snapshotChains, childTemplates, srcDatastore, destImgStoreId, futures);
             } else {
                 message = "Migration failed. Destination store doesn't have enough capacity for migration";
                 success = false;
@@ -289,7 +295,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
                 SnapshotInfo snapshotInfo = snapshotFactory.getSnapshot(snap.getSnapshotId(), snap.getDataStoreId(), DataStoreRole.Image);
                 SnapshotInfo parentSnapshot = snapshotInfo.getParent();
                 if (snapshotInfo.getDataStore().getId() == srcImgStoreId && parentSnapshot != null && migratedSnapshotIdList.contains(parentSnapshot.getSnapshotId())) {
-                    futures.add(executor.submit(new MigrateDataTask(snapshotInfo, srcDatastore, dataStoreManager.getDataStore(destImgStoreId, DataStoreRole.Image))));
+                    futures.add(submit(srcDatastore.getScope().getScopeId(), new MigrateDataTask(snapshotInfo, srcDatastore, dataStoreManager.getDataStore(destImgStoreId, DataStoreRole.Image))));
                 }
             });
         }
@@ -297,6 +303,11 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         return handleResponse(futures, null, message, success);
     }
 
+    @Override
+    public Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore) {
+        return submit(destStore.getScope().getScopeId(), new CopyTemplateTask(source, destStore));
+    }
+
     protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy, int skipped) {
         String message = "";
         boolean success = true;
@@ -332,19 +343,10 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
             Map<DataObject, Pair<List<TemplateInfo>, Long>> templateChains,
             DataStore srcDatastore,
             Long destDatastoreId,
-            ThreadPoolExecutor executor,
+            List<Future<DataObjectResult>> futures) {
-            List<Future<AsyncCallFuture<DataObjectResult>>> futures) {
 
         Long fileSize = migrationHelper.getFileSize(chosenFileForMigration, snapshotChains, templateChains);
-
         storageCapacities = assumeMigrate(storageCapacities, srcDatastore.getId(), destDatastoreId, fileSize);
-        long activeSsvms = migrationHelper.activeSSVMCount(srcDatastore);
-        long totalJobs = activeSsvms * numConcurrentCopyTasksPerSSVM;
-        // Increase thread pool size with increase in number of SSVMs
-        if ( totalJobs > executor.getCorePoolSize()) {
-            executor.setMaximumPoolSize((int) (totalJobs));
-            executor.setCorePoolSize((int) (totalJobs));
-        }
 
         MigrateDataTask task = new MigrateDataTask(chosenFileForMigration, srcDatastore, dataStoreManager.getDataStore(destDatastoreId, DataStoreRole.Image));
         if (chosenFileForMigration instanceof SnapshotInfo ) {
@@ -353,19 +355,64 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         if (chosenFileForMigration instanceof TemplateInfo) {
             task.setTemplateChain(templateChains);
         }
-        futures.add((executor.submit(task)));
+        futures.add(submit(srcDatastore.getScope().getScopeId(), task));
         logger.debug("Migration of {}: {} is initiated.", chosenFileForMigration.getType().name(), chosenFileForMigration.getUuid());
         return storageCapacities;
     }
 
+    protected <T> Future<T> submit(Long zoneId, Callable<T> task) {
+        ThreadPoolExecutor executor;
+        synchronized (this) {
+            if (!zoneExecutorMap.containsKey(zoneId)) {
+                zoneExecutorMap.put(zoneId, new ThreadPoolExecutor(numConcurrentCopyTasksPerSSVM, numConcurrentCopyTasksPerSSVM,
+                        30, TimeUnit.MINUTES, new MigrateBlockingQueue<>(numConcurrentCopyTasksPerSSVM)));
+                zonePendingWorkCountMap.put(zoneId, 0);
+            }
+            zonePendingWorkCountMap.merge(zoneId, 1, Integer::sum);
+            scaleExecutorIfNecessary(zoneId);
+            executor = zoneExecutorMap.get(zoneId);
+        }
+        return executor.submit(task);
 
+    }
 
-    private MigrationResponse handleResponse(List<Future<AsyncCallFuture<DataObjectResult>>> futures, MigrationPolicy migrationPolicy, String message, boolean success) {
+    protected void scaleExecutorIfNecessary(Long zoneId) {
+        long activeSsvms = migrationHelper.activeSSVMCount(zoneId);
+        long totalJobs = activeSsvms * numConcurrentCopyTasksPerSSVM;
+        ThreadPoolExecutor executor = zoneExecutorMap.get(zoneId);
+        if (totalJobs > executor.getCorePoolSize()) {
+            logger.debug("Scaling up executor of zone [{}] from [{}] to [{}] threads.", zoneId, executor.getCorePoolSize(),
+                    totalJobs);
+            executor.setMaximumPoolSize((int) (totalJobs));
+            executor.setCorePoolSize((int) (totalJobs));
+        }
+    }
+
+    protected synchronized void tryCleaningUpExecutor(Long zoneId) {
+        if (!zoneExecutorMap.containsKey(zoneId)) {
+            logger.debug("No executor exists for zone [{}].", zoneId);
+            return;
+        }
+
+        zonePendingWorkCountMap.merge(zoneId, -1, Integer::sum);
+        Integer pendingWorkCount = zonePendingWorkCountMap.get(zoneId);
+        if (pendingWorkCount > 0) {
+            logger.debug("Not cleaning executor of zone [{}] yet, as there is [{}] pending work.", zoneId, pendingWorkCount);
+            return;
+        }
+
+        logger.debug("Cleaning executor of zone [{}].", zoneId);
+        ThreadPoolExecutor executor = zoneExecutorMap.get(zoneId);
+        zoneExecutorMap.remove(zoneId);
+        executor.shutdown();
+    }
+
+    private MigrationResponse handleResponse(List<Future<DataObjectResult>> futures, MigrationPolicy migrationPolicy, String message, boolean success) {
         int successCount = 0;
-        for (Future<AsyncCallFuture<DataObjectResult>> future : futures) {
+        for (Future<DataObjectResult> future : futures) {
             try {
-                AsyncCallFuture<DataObjectResult> res = future.get();
+                DataObjectResult res = future.get();
-                if (res.get().isSuccess()) {
+                if (res.isSuccess()) {
                     successCount++;
                 }
             } catch ( InterruptedException | ExecutionException e) {
@@ -379,7 +426,7 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
     }
 
     private void handleSnapshotMigration(Long srcDataStoreId, Date start, Date end, MigrationPolicy policy,
-                                          List<Future<AsyncCallFuture<DataObjectResult>>> futures, Map<Long, Pair<Long, Long>> storageCapacities, ThreadPoolExecutor executor) {
+                                          List<Future<DataObjectResult>> futures, Map<Long, Pair<Long, Long>> storageCapacities) {
         DataStore srcDatastore = dataStoreManager.getDataStore(srcDataStoreId, DataStoreRole.Image);
         List<SnapshotDataStoreVO> snaps = snapshotDataStoreDao.findSnapshots(srcDataStoreId, start, end);
         if (!snaps.isEmpty()) {
@@ -395,12 +442,12 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
                         storeId = dstores.get(1);
                     }
                     DataStore datastore =  dataStoreManager.getDataStore(storeId, DataStoreRole.Image);
-                    futures.add(executor.submit(new MigrateDataTask(snapshotInfo, srcDatastore, datastore)));
+                    futures.add(submit(srcDatastore.getScope().getScopeId(), new MigrateDataTask(snapshotInfo, srcDatastore, datastore)));
                 }
                 if (parentSnapshot != null) {
                     DataStore parentDS = dataStoreManager.getDataStore(parentSnapshot.getDataStore().getId(), DataStoreRole.Image);
                     if (parentDS.getId() != snapshotInfo.getDataStore().getId()) {
-                        futures.add(executor.submit(new MigrateDataTask(snapshotInfo, srcDatastore, parentDS)));
+                        futures.add(submit(srcDatastore.getScope().getScopeId(), new MigrateDataTask(snapshotInfo, srcDatastore, parentDS)));
                     }
                 }
             }
@@ -527,16 +574,19 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         return standardDeviation.evaluate(metricValues, mean);
     }
 
-    private class MigrateDataTask implements Callable<AsyncCallFuture<DataObjectResult>> {
+    private class MigrateDataTask implements Callable<DataObjectResult> {
         private DataObject file;
         private DataStore srcDataStore;
         private DataStore destDataStore;
         private Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChain;
         private Map<DataObject, Pair<List<TemplateInfo>, Long>> templateChain;
+        private String logid;
+
         public MigrateDataTask(DataObject file, DataStore srcDataStore, DataStore destDataStore) {
             this.file = file;
             this.srcDataStore = srcDataStore;
             this.destDataStore = destDataStore;
+            this.logid = ThreadContext.get(LOGCONTEXTID);
         }
 
         public void setSnapshotChains(Map<DataObject, Pair<List<SnapshotInfo>, Long>> snapshotChain) {
@@ -557,8 +607,50 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
         }
 
         @Override
-        public AsyncCallFuture<DataObjectResult> call() throws Exception {
+        public DataObjectResult call() {
-            return secStgSrv.migrateData(file, srcDataStore, destDataStore, snapshotChain, templateChain);
+            ThreadContext.put(LOGCONTEXTID, logid);
+            DataObjectResult result;
+            AsyncCallFuture<DataObjectResult> future = secStgSrv.migrateData(file, srcDataStore, destDataStore, snapshotChain, templateChain);
+            try {
+                result = future.get();
+            } catch (ExecutionException | InterruptedException e) {
+                logger.warn("Exception while migrating data to another secondary storage: {}", e.toString());
+                result = new DataObjectResult(file);
+                result.setResult(e.toString());
+            }
+            tryCleaningUpExecutor(srcDataStore.getScope().getScopeId());
+            ThreadContext.clearAll();
+            return result;
+        }
+    }
+
+    private class CopyTemplateTask implements Callable<TemplateApiResult> {
+        private TemplateInfo sourceTmpl;
+        private DataStore destStore;
+        private String logid;
+
+        public CopyTemplateTask(TemplateInfo sourceTmpl, DataStore destStore) {
+            this.sourceTmpl = sourceTmpl;
+            this.destStore = destStore;
+            this.logid = ThreadContext.get(LOGCONTEXTID);
+        }
+
+        @Override
+        public TemplateApiResult call() {
+            ThreadContext.put(LOGCONTEXTID, logid);
+            TemplateApiResult result;
+            AsyncCallFuture<TemplateApiResult> future = templateService.copyTemplateToImageStore(sourceTmpl, destStore);
+            try {
+                result = future.get();
+            } catch (ExecutionException | InterruptedException e) {
+                logger.warn("Exception while copying template [{}] from image store [{}] to image store [{}]: {}",
+                        sourceTmpl.getUniqueName(), sourceTmpl.getDataStore().getName(), destStore.getName(), e.toString());
+                result = new TemplateApiResult(sourceTmpl);
+                result.setResult(e.getMessage());
+            }
+            tryCleaningUpExecutor(destStore.getScope().getScopeId());
+            ThreadContext.clearAll();
+            return result;
         }
     }
 }

engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java

@@ -31,6 +31,7 @@ import java.util.concurrent.ExecutionException;
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
 import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionService;
@@ -42,7 +43,6 @@ import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector;
 import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
 import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event;
 import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.State;
-import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
 import org.apache.cloudstack.engine.subsystem.api.storage.StorageCacheManager;
 import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
@@ -58,6 +58,7 @@ import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.framework.messagebus.MessageBus;
 import org.apache.cloudstack.framework.messagebus.PublishScope;
 import org.apache.cloudstack.storage.command.CommandResult;
+import org.apache.cloudstack.storage.command.CopyCmdAnswer;
 import org.apache.cloudstack.storage.command.DeleteCommand;
 import org.apache.cloudstack.storage.datastore.DataObjectManager;
 import org.apache.cloudstack.storage.datastore.ObjectInDataStoreManager;
@@ -77,7 +78,6 @@ import com.cloud.agent.api.storage.ListTemplateCommand;
 import com.cloud.agent.api.to.DatadiskTO;
 import com.cloud.alert.AlertManager;
 import com.cloud.configuration.Config;
-import com.cloud.configuration.Resource;
 import com.cloud.configuration.Resource.ResourceType;
 import com.cloud.dc.DataCenterVO;
 import com.cloud.dc.dao.ClusterDao;
@@ -157,6 +157,8 @@ public class TemplateServiceImpl implements TemplateService {
     ImageStoreDetailsUtil imageStoreDetailsUtil;
     @Inject
     TemplateDataFactory imageFactory;
+    @Inject
+    StorageOrchestrationService storageOrchestrator;
 
     class TemplateOpContext<T> extends AsyncRpcContext<T> {
         final TemplateObject template;
@@ -320,7 +322,6 @@ public class TemplateServiceImpl implements TemplateService {
             if (syncLock.lock(3)) {
                 try {
                     Long zoneId = store.getScope().getScopeId();
-
                     Map<String, TemplateProp> templateInfos = listTemplate(store);
                     if (templateInfos == null) {
                         return;
@@ -529,10 +530,6 @@ public class TemplateServiceImpl implements TemplateService {
                         availHypers.add(HypervisorType.None); // bug 9809: resume ISO
                         // download.
                         for (VMTemplateVO tmplt : toBeDownloaded) {
-                            if (tmplt.getUrl() == null) { // If url is null, skip downloading
-                                logger.info("Skip downloading template {} since no url is specified.", tmplt);
-                                continue;
-                            }
                             // if this is private template, skip sync to a new image store
                             if (isSkipTemplateStoreDownload(tmplt, zoneId)) {
                                 logger.info("Skip sync downloading private template {} to a new image store", tmplt);
@@ -551,14 +548,10 @@ public class TemplateServiceImpl implements TemplateService {
                             }
 
                             if (availHypers.contains(tmplt.getHypervisorType())) {
-                                logger.info("Downloading template {} to image store {}", tmplt, store);
+                                boolean copied = isCopyFromOtherStoragesEnabled(zoneId) && tryCopyingTemplateToImageStore(tmplt, store);
-                                associateTemplateToZone(tmplt.getId(), zoneId);
+                                if (!copied) {
-                                TemplateInfo tmpl = _templateFactory.getTemplate(tmplt.getId(), store);
+                                    tryDownloadingTemplateToImageStore(tmplt, store);
-                                TemplateOpContext<TemplateApiResult> context = new TemplateOpContext<>(null,(TemplateObject)tmpl, null);
+                                }
-                                AsyncCallbackDispatcher<TemplateServiceImpl, TemplateApiResult> caller = AsyncCallbackDispatcher.create(this);
-                                caller.setCallback(caller.getTarget().createTemplateAsyncCallBack(null, null));
-                                caller.setContext(context);
-                                createTemplateAsync(tmpl, store, caller);
                             } else {
                                 logger.info("Skip downloading template {} since current data center does not have hypervisor {}", tmplt, tmplt.getHypervisorType());
                             }
@@ -605,6 +598,127 @@ public class TemplateServiceImpl implements TemplateService {
 
     }
 
+    protected void tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore destStore) {
+        if (tmplt.getUrl() == null) {
+            logger.info("Not downloading template [{}] to image store [{}], as it has no URL.", tmplt.getUniqueName(),
+                    destStore.getName());
+            return;
+        }
+        logger.info("Downloading template [{}] to image store [{}].", tmplt.getUniqueName(), destStore.getName());
+        associateTemplateToZone(tmplt.getId(), destStore.getScope().getScopeId());
+        TemplateInfo tmpl = _templateFactory.getTemplate(tmplt.getId(), destStore);
+        TemplateOpContext<TemplateApiResult> context = new TemplateOpContext<>(null,(TemplateObject)tmpl, null);
+        AsyncCallbackDispatcher<TemplateServiceImpl, TemplateApiResult> caller = AsyncCallbackDispatcher.create(this);
+        caller.setCallback(caller.getTarget().createTemplateAsyncCallBack(null, null));
+        caller.setContext(context);
+        createTemplateAsync(tmpl, destStore, caller);
+    }
+
+    protected boolean tryCopyingTemplateToImageStore(VMTemplateVO tmplt, DataStore destStore) {
+        Long zoneId = destStore.getScope().getScopeId();
+        List<DataStore> storesInZone = _storeMgr.getImageStoresByZoneIds(zoneId);
+        for (DataStore sourceStore : storesInZone) {
+            Map<String, TemplateProp> existingTemplatesInSourceStore = listTemplate(sourceStore);
+            if (existingTemplatesInSourceStore == null || !existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
+                logger.debug("Template [{}] does not exist on image store [{}]; searching on another one.",
+                        tmplt.getUniqueName(), sourceStore.getName());
+                continue;
+            }
+            TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore);
+            if (sourceTmpl.getInstallPath() == null) {
+                logger.warn("Can not copy template [{}] from image store [{}], as it returned a null install path.", tmplt.getUniqueName(),
+                        sourceStore.getName());
+                continue;
+            }
+            storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
+            return true;
+        }
+        logger.debug("Can't copy template [{}] from another image store.", tmplt.getUniqueName());
+        return false;
+    }
+
+    @Override
+    public AsyncCallFuture<TemplateApiResult> copyTemplateToImageStore(DataObject source, DataStore destStore) {
+        TemplateObject sourceTmpl = (TemplateObject) source;
+        logger.debug("Copying template [{}] from image store [{}] to [{}].", sourceTmpl.getUniqueName(), sourceTmpl.getDataStore().getName(),
+                destStore.getName());
+        TemplateObject destTmpl = (TemplateObject) destStore.create(sourceTmpl);
+        destTmpl.processEvent(Event.CreateOnlyRequested);
+
+        AsyncCallFuture<TemplateApiResult> future = new AsyncCallFuture<>();
+        TemplateOpContext<TemplateApiResult> context = new TemplateOpContext<>(null, destTmpl, future);
+        AsyncCallbackDispatcher<TemplateServiceImpl, CopyCommandResult> caller = AsyncCallbackDispatcher.create(this);
+        caller.setCallback(caller.getTarget().copyTemplateToImageStoreCallback(null, null)).setContext(context);
+
+        if (source.getDataStore().getId() == destStore.getId()) {
+            logger.debug("Destination image store [{}] is the same as the origin; returning success to normalize the metadata.");
+            CopyCmdAnswer answer = new CopyCmdAnswer(source.getTO());
+            CopyCommandResult result = new CopyCommandResult("", answer);
+            caller.complete(result);
+            return future;
+        }
+
+        _motionSrv.copyAsync(sourceTmpl, destTmpl, caller);
+        return future;
+    }
+
+    protected Void copyTemplateToImageStoreCallback(AsyncCallbackDispatcher<TemplateServiceImpl, CopyCommandResult> callback, TemplateOpContext<TemplateApiResult> context) {
+        TemplateInfo tmplt = context.getTemplate();
+        CopyCommandResult result = callback.getResult();
+        AsyncCallFuture<TemplateApiResult> future = context.getFuture();
+        TemplateApiResult res = new TemplateApiResult(tmplt);
+        if (result.isSuccess()) {
+            logger.info("Copied template [{}] to image store [{}].", tmplt.getUniqueName(), tmplt.getDataStore().getName());
+            tmplt.processEvent(Event.OperationSuccessed, result.getAnswer());
+            publishTemplateCreation(tmplt);
+        } else {
+            logger.warn("Failed to copy template [{}] to image store [{}].", tmplt.getUniqueName(), tmplt.getDataStore().getName());
+            res.setResult(result.getResult());
+            tmplt.processEvent(Event.OperationFailed);
+        }
+        future.complete(res);
+        return null;
+    }
+
+    protected boolean isCopyFromOtherStoragesEnabled(Long zoneId) {
+        return StorageManager.COPY_PUBLIC_TEMPLATES_FROM_OTHER_STORAGES.valueIn(zoneId);
+    }
+
+    protected void publishTemplateCreation(TemplateInfo tmplt) {
+        VMTemplateVO tmpltVo = _templateDao.findById(tmplt.getId());
+
+        if (tmpltVo.isPublicTemplate()) {
+            _messageBus.publish(null, TemplateManager.MESSAGE_REGISTER_PUBLIC_TEMPLATE_EVENT, PublishScope.LOCAL, tmpltVo.getId());
+        }
+
+        Long size = tmplt.getSize();
+        if (size == null) {
+            return;
+        }
+
+        DataStore store = tmplt.getDataStore();
+        TemplateDataStoreVO tmpltStore = _vmTemplateStoreDao.findByStoreTemplate(store.getId(), tmpltVo.getId());
+
+        long physicalSize = 0;
+        if (tmpltStore != null) {
+            physicalSize = tmpltStore.getPhysicalSize();
+        } else {
+            logger.warn("No entry found in template_store_ref for template [{}] and image store [{}] at the end of registering template!",
+                    tmpltVo.getUniqueName(), store.getName());
+        }
+
+        Long zoneId = store.getScope().getScopeId();
+        if (zoneId != null) {
+            String usageEvent = tmplt.getFormat() == ImageFormat.ISO ? EventTypes.EVENT_ISO_CREATE : EventTypes.EVENT_TEMPLATE_CREATE;
+            UsageEventUtils.publishUsageEvent(usageEvent, tmpltVo.getAccountId(), zoneId, tmpltVo.getId(), tmpltVo.getName(),
+                    null, null, physicalSize, size, VirtualMachineTemplate.class.getName(), tmpltVo.getUuid());
+        } else {
+            logger.warn("Zone-wide image store [{}] has a null scope ID.", store);
+        }
+
+        _resourceLimitMgr.incrementResourceCount(tmpltVo.getAccountId(), ResourceType.secondary_storage, size);
+    }
+
     // persist entry in template_zone_ref table. zoneId can be empty for
     // region-wide image store, in that case,
     // we will associate the template to all the zones.
@@ -650,45 +764,14 @@ public class TemplateServiceImpl implements TemplateService {
 
     protected Void createTemplateAsyncCallBack(AsyncCallbackDispatcher<TemplateServiceImpl, TemplateApiResult> callback,
                                                TemplateOpContext<TemplateApiResult> context) {
-        TemplateInfo template = context.template;
         TemplateApiResult result = callback.getResult();
         if (result.isSuccess()) {
-            VMTemplateVO tmplt = _templateDao.findById(template.getId());
+            publishTemplateCreation(context.template);
-            // need to grant permission for public templates
-            if (tmplt.isPublicTemplate()) {
-                _messageBus.publish(null, TemplateManager.MESSAGE_REGISTER_PUBLIC_TEMPLATE_EVENT, PublishScope.LOCAL, tmplt.getId());
-            }
-            long accountId = tmplt.getAccountId();
-            if (template.getSize() != null) {
-                // publish usage event
-                String etype = EventTypes.EVENT_TEMPLATE_CREATE;
-                if (tmplt.getFormat() == ImageFormat.ISO) {
-                    etype = EventTypes.EVENT_ISO_CREATE;
-                }
-                // get physical size from template_store_ref table
-                long physicalSize = 0;
-                DataStore ds = template.getDataStore();
-                TemplateDataStoreVO tmpltStore = _vmTemplateStoreDao.findByStoreTemplate(ds.getId(), template.getId());
-                if (tmpltStore != null) {
-                    physicalSize = tmpltStore.getPhysicalSize();
-                } else {
-                    logger.warn("No entry found in template_store_ref for template: {} and image store: {} at the end of registering template!", template, ds);
-                }
-                Scope dsScope = ds.getScope();
-                if (dsScope.getScopeId() != null) {
-                    UsageEventUtils.publishUsageEvent(etype, template.getAccountId(), dsScope.getScopeId(), template.getId(), template.getName(), null, null,
-                            physicalSize, template.getSize(), VirtualMachineTemplate.class.getName(), template.getUuid());
-                } else {
-                    logger.warn("Zone scope image store {} has a null scope id", ds);
-                }
-                _resourceLimitMgr.incrementResourceCount(accountId, Resource.ResourceType.secondary_storage, template.getSize());
-            }
         }
-
         return null;
     }
 
-    private Map<String, TemplateProp> listTemplate(DataStore ssStore) {
+    protected Map<String, TemplateProp> listTemplate(DataStore ssStore) {
         String nfsVersion = imageStoreDetailsUtil.getNfsVersion(ssStore.getId());
         ListTemplateCommand cmd = new ListTemplateCommand(ssStore.getTO(), nfsVersion);
         EndPoint ep = _epSelector.select(ssStore);

engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java

@@ -18,9 +18,17 @@
  */
 package org.apache.cloudstack.storage.image;
 
+import com.cloud.storage.template.TemplateProp;
+import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
 import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
+import org.apache.cloudstack.storage.image.store.TemplateObject;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
@@ -33,6 +41,10 @@ import com.cloud.storage.DataStoreRole;
 import com.cloud.storage.Storage;
 import com.cloud.storage.VMTemplateVO;
 
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
 @RunWith(MockitoJUnitRunner.class)
 public class TemplateServiceImplTest {
 
@@ -43,6 +55,49 @@ public class TemplateServiceImplTest {
     @Mock
     TemplateDataStoreDao templateDataStoreDao;
 
+    @Mock
+    TemplateDataFactoryImpl templateDataFactoryMock;
+
+    @Mock
+    DataStoreManager dataStoreManagerMock;
+
+    @Mock
+    VMTemplateVO tmpltMock;
+
+    @Mock
+    TemplateProp tmpltPropMock;
+
+    @Mock
+    TemplateObject templateInfoMock;
+
+    @Mock
+    DataStore sourceStoreMock;
+
+    @Mock
+    DataStore destStoreMock;
+
+    @Mock
+    Scope zoneScopeMock;
+
+    @Mock
+    StorageOrchestrationService storageOrchestrator;
+
+    Map<String, TemplateProp> templatesInSourceStore = new HashMap<>();
+
+    @Before
+    public void setUp() {
+        Long zoneId = 1L;
+        Mockito.doReturn(2L).when(tmpltMock).getId();
+        Mockito.doReturn("unique-name").when(tmpltMock).getUniqueName();
+        Mockito.doReturn(zoneId).when(zoneScopeMock).getScopeId();
+        Mockito.doReturn(zoneScopeMock).when(destStoreMock).getScope();
+        Mockito.doReturn(List.of(sourceStoreMock, destStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(zoneId);
+        Mockito.doReturn(templatesInSourceStore).when(templateService).listTemplate(sourceStoreMock);
+        Mockito.doReturn(null).when(templateService).listTemplate(destStoreMock);
+        Mockito.doReturn("install-path").when(templateInfoMock).getInstallPath();
+        Mockito.doReturn(templateInfoMock).when(templateDataFactoryMock).getTemplate(2L, sourceStoreMock);
+    }
+
     @Test
     public void testIsSkipTemplateStoreDownloadPublicTemplate() {
         VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class);
@@ -81,4 +136,51 @@ public class TemplateServiceImplTest {
         Mockito.when(templateDataStoreDao.findByTemplateZone(id, id, DataStoreRole.Image)).thenReturn(Mockito.mock(TemplateDataStoreVO.class));
         Assert.assertTrue(templateService.isSkipTemplateStoreDownload(templateVO, id));
     }
+
+    @Test
+    public void tryDownloadingTemplateToImageStoreTestDownloadsTemplateWhenUrlIsNotNull() {
+        Mockito.doReturn("url").when(tmpltMock).getUrl();
+        Mockito.doNothing().when(templateService).associateTemplateToZone(Mockito.anyLong(), Mockito.any(Long.class));
+
+        templateService.tryDownloadingTemplateToImageStore(tmpltMock, destStoreMock);
+
+        Mockito.verify(templateService).createTemplateAsync(Mockito.any(), Mockito.any(), Mockito.any());
+    }
+
+    @Test
+    public void tryDownloadingTemplateToImageStoreTestDoesNothingWhenUrlIsNull() {
+        templateService.tryDownloadingTemplateToImageStore(tmpltMock, destStoreMock);
+
+        Mockito.verify(templateService, Mockito.never()).createTemplateAsync(Mockito.any(), Mockito.any(), Mockito.any());
+    }
+
+    @Test
+    public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenTemplateDoesNotExistOnAnotherImageStore() {
+        boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+        Assert.assertFalse(result);
+        Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
+    }
+
+    @Test
+    public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenInstallPathIsNull() {
+        templatesInSourceStore.put(tmpltMock.getUniqueName(), tmpltPropMock);
+        Mockito.doReturn(null).when(templateInfoMock).getInstallPath();
+
+        boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+        Assert.assertFalse(result);
+        Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
+    }
+
+    @Test
+    public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherStorageAndTaskWasScheduled() {
+        templatesInSourceStore.put(tmpltMock.getUniqueName(), tmpltPropMock);
+        Mockito.doReturn(new AsyncCallFuture<>()).when(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
+
+        boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+        Assert.assertTrue(result);
+        Mockito.verify(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
+    }
 }

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

@@ -4194,7 +4194,8 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
                 VmwareAllowParallelExecution,
                 DataStoreDownloadFollowRedirects,
                 AllowVolumeReSizeBeyondAllocation,
-                StoragePoolHostConnectWorkers
+                StoragePoolHostConnectWorkers,
+                COPY_PUBLIC_TEMPLATES_FROM_OTHER_STORAGES
         };
     }