Allow copy of templates from secondary storages of other zone when adding a new secondary storage (#12296)

* Allow copy of templates from secondary storages of other zone when adding a new secondary storage

* Add API param and UI changes on add secondary storage page

* Make copy template across zones non blocking

* Code fixes

* unused imports

* Add copy template flag in zone wizard and remove NFS checks

* Fix UI

* Label fixes

* code optimizations

* code refactoring

* missing changes

* Combine template copy and download into a single asynchronous operation

* unused import and fixed conflicts

* unused code

* update config message

* Fix configuration setting value on add secondary storage page

* Removed unused code

* Update unit tests

Author: harikrishna-patnala

api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java

@@ -29,6 +29,11 @@
 import com.cloud.exception.DiscoveryException;
 import com.cloud.storage.ImageStore;
 import com.cloud.user.Account;
+import org.apache.commons.collections.MapUtils;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
 
 @APICommand(name = "addSecondaryStorage", description = "Adds secondary storage.", responseObject = ImageStoreResponse.class,
         requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
@@ -44,6 +49,9 @@ public class AddSecondaryStorageCmd extends BaseCmd {
     @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "The Zone ID for the secondary storage")
     protected Long zoneId;
 
+    @Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, description = "Details in key/value pairs using format details[i].keyname=keyvalue. Example: details[0].copytemplatesfromothersecondarystorages=true")
+    protected Map details;
+
     /////////////////////////////////////////////////////
     /////////////////// Accessors ///////////////////////
     /////////////////////////////////////////////////////
@@ -56,6 +64,20 @@ public Long getZoneId() {
         return zoneId;
     }
 
+    public Map<String, String> getDetails() {
+        Map<String, String> detailsMap = new HashMap<>();
+        if (MapUtils.isNotEmpty(details)) {
+            Collection<?> props = details.values();
+            for (Object prop : props) {
+                HashMap<String, String> detail = (HashMap<String, String>) prop;
+                for (Map.Entry<String, String> entry: detail.entrySet()) {
+                    detailsMap.put(entry.getKey(),entry.getValue());
+                }
+            }
+        }
+        return detailsMap;
+    }
+
     /////////////////////////////////////////////////////
     /////////////// API Implementation///////////////////
     /////////////////////////////////////////////////////
@@ -68,7 +90,7 @@ public long getEntityOwnerId() {
     @Override
     public void execute(){
         try{
-            ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), null);
+            ImageStore result = _storageService.discoverImageStore(null, getUrl(), "NFS", getZoneId(), getDetails());
             ImageStoreResponse storeResponse = null;
             if (result != null ) {
                     storeResponse = _responseGenerator.createImageStoreResponse(result);

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

@@ -22,7 +22,6 @@
 
 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;
 
@@ -31,5 +30,5 @@ public interface StorageOrchestrationService {
 
     MigrationResponse migrateResources(Long srcImgStoreId, Long destImgStoreId, List<Long> templateIdList, List<Long> snapshotIdList);
 
-    Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore);
+    Future<TemplateApiResult> orchestrateTemplateCopyFromSecondaryStores(long templateId, DataStore destStore);
 }

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

@@ -80,4 +80,6 @@ public TemplateInfo getTemplate() {
     List<DatadiskTO> getTemplateDatadisksOnImageStore(TemplateInfo templateInfo, String configurationId);
 
     AsyncCallFuture<TemplateApiResult> copyTemplateToImageStore(DataObject source, DataStore destStore);
-}
+
+    void handleTemplateCopyFromSecondaryStores(long templateId, DataStore destStore);
+ }

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

@@ -220,8 +220,9 @@ 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.",
+    ConfigKey<Boolean> COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES = new ConfigKey<>(Boolean.class, "copy.templates.from.other.secondary.storages",
+            "Storage", "true", "When enabled, this feature allows templates to be copied from existing Secondary Storage servers (within the same zone or across zones) " +
+            "while adding a new Secondary Storage. If the copy operation fails, the system falls back to downloading the template from the source URL.",
             true, ConfigKey.Scope.Zone, null);
 
     /**

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

@@ -36,6 +36,9 @@
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
+import com.cloud.dc.dao.DataCenterDao;
+import com.cloud.storage.dao.VMTemplateDao;
+import com.cloud.template.TemplateManager;
 import org.apache.cloudstack.api.response.MigrationResponse;
 import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
@@ -45,6 +48,7 @@
 import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
 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.TemplateDataFactory;
 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;
@@ -103,6 +107,15 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
     VolumeDataStoreDao volumeDataStoreDao;
     @Inject
     DataMigrationUtility migrationHelper;
+    @Inject
+    TemplateManager templateManager;
+    @Inject
+    VMTemplateDao templateDao;
+    @Inject
+    TemplateDataFactory templateDataFactory;
+    @Inject
+    DataCenterDao dcDao;
+
 
     ConfigKey<Double> ImageStoreImbalanceThreshold = new ConfigKey<>("Advanced", Double.class,
             "image.store.imbalance.threshold",
@@ -304,8 +317,9 @@ public MigrationResponse migrateResources(Long srcImgStoreId, Long destImgStoreI
     }
 
     @Override
-    public Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore) {
-        return submit(destStore.getScope().getScopeId(), new CopyTemplateTask(source, destStore));
+    public Future<TemplateApiResult> orchestrateTemplateCopyFromSecondaryStores(long srcTemplateId, DataStore destStore) {
+        Long dstZoneId = destStore.getScope().getScopeId();
+        return submit(dstZoneId, new CopyTemplateFromSecondaryStorageTask(srcTemplateId, destStore));
     }
 
     protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy, int skipped) {
@@ -624,13 +638,13 @@ public DataObjectResult call() {
         }
     }
 
-    private class CopyTemplateTask implements Callable<TemplateApiResult> {
-        private TemplateInfo sourceTmpl;
-        private DataStore destStore;
-        private String logid;
+    private class CopyTemplateFromSecondaryStorageTask implements Callable<TemplateApiResult> {
+        private final long srcTemplateId;
+        private final DataStore destStore;
+        private final String logid;
 
-        public CopyTemplateTask(TemplateInfo sourceTmpl, DataStore destStore) {
-            this.sourceTmpl = sourceTmpl;
+        CopyTemplateFromSecondaryStorageTask(long srcTemplateId, DataStore destStore) {
+            this.srcTemplateId = srcTemplateId;
             this.destStore = destStore;
             this.logid = ThreadContext.get(LOGCONTEXTID);
         }
@@ -639,17 +653,16 @@ public CopyTemplateTask(TemplateInfo sourceTmpl, DataStore destStore) {
         public TemplateApiResult call() {
             ThreadContext.put(LOGCONTEXTID, logid);
             TemplateApiResult result;
-            AsyncCallFuture<TemplateApiResult> future = templateService.copyTemplateToImageStore(sourceTmpl, destStore);
+            long destZoneId = destStore.getScope().getScopeId();
+            TemplateInfo sourceTmpl = templateDataFactory.getTemplate(srcTemplateId, DataStoreRole.Image);
             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());
+                templateService.handleTemplateCopyFromSecondaryStores(srcTemplateId, destStore);
                 result = new TemplateApiResult(sourceTmpl);
-                result.setResult(e.getMessage());
+            } finally {
+                tryCleaningUpExecutor(destZoneId);
+                ThreadContext.clearAll();
             }
-            tryCleaningUpExecutor(destStore.getScope().getScopeId());
-            ThreadContext.clearAll();
+
             return result;
         }
     }

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

@@ -31,6 +31,8 @@
 
 import javax.inject.Inject;
 
+import com.cloud.exception.StorageUnavailableException;
+import org.apache.cloudstack.context.CallContext;
 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;
@@ -67,9 +69,11 @@
 import org.apache.cloudstack.storage.image.datastore.ImageStoreEntity;
 import org.apache.cloudstack.storage.image.store.TemplateObject;
 import org.apache.cloudstack.storage.to.TemplateObjectTO;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.ThreadContext;
 import org.springframework.stereotype.Component;
 
 import com.cloud.agent.api.Answer;
@@ -567,10 +571,7 @@ public void handleTemplateSync(DataStore store) {
                             }
 
                             if (availHypers.contains(tmplt.getHypervisorType())) {
-                                boolean copied = isCopyFromOtherStoragesEnabled(zoneId) && tryCopyingTemplateToImageStore(tmplt, store);
-                                if (!copied) {
-                                    tryDownloadingTemplateToImageStore(tmplt, store);
-                                }
+                                storageOrchestrator.orchestrateTemplateCopyFromSecondaryStores(tmplt.getId(), store);
                             } else {
                                 logger.info("Skip downloading template {} since current data center does not have hypervisor {}", tmplt, tmplt.getHypervisorType());
                             }
@@ -617,6 +618,16 @@ public void handleTemplateSync(DataStore store) {
 
     }
 
+    @Override
+    public void handleTemplateCopyFromSecondaryStores(long templateId, DataStore destStore) {
+        VMTemplateVO template = _templateDao.findById(templateId);
+        long zoneId = destStore.getScope().getScopeId();
+        boolean copied = imageStoreDetailsUtil.isCopyTemplatesFromOtherStoragesEnabled(destStore.getId(), zoneId) && tryCopyingTemplateToImageStore(template, destStore);
+        if (!copied) {
+            tryDownloadingTemplateToImageStore(template, destStore);
+        }
+    }
+
     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(),
@@ -634,28 +645,134 @@ protected void tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore
     }
 
     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());
+        if (searchAndCopyWithinZone(tmplt, destStore)) {
+            return true;
+        }
+
+        Long destZoneId = destStore.getScope().getScopeId();
+        logger.debug("Template [{}] not found in any image store of zone [{}]. Checking other zones.",
+                tmplt.getUniqueName(), destZoneId);
+
+        return searchAndCopyAcrossZones(tmplt, destStore, destZoneId);
+    }
+
+    private boolean searchAndCopyAcrossZones(VMTemplateVO tmplt, DataStore destStore, Long destZoneId) {
+        List<Long> allZoneIds = _dcDao.listAllIds();
+        for (Long otherZoneId : allZoneIds) {
+            if (otherZoneId.equals(destZoneId)) {
                 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());
+
+            List<DataStore> storesInOtherZone = _storeMgr.getImageStoresByZoneIds(otherZoneId);
+            logger.debug("Checking zone [{}] for template [{}]...", otherZoneId, tmplt.getUniqueName());
+
+            if (CollectionUtils.isEmpty(storesInOtherZone)) {
+                logger.debug("Zone [{}] has no image stores. Skipping.", otherZoneId);
                 continue;
             }
-            storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
-            return true;
+
+            TemplateObject sourceTmpl = findUsableTemplate(tmplt, storesInOtherZone);
+            if (sourceTmpl == null) {
+                logger.debug("Template [{}] not found with a valid install path in any image store of zone [{}].",
+                        tmplt.getUniqueName(), otherZoneId);
+                continue;
+            }
+
+            logger.info("Template [{}] found in zone [{}]. Initiating cross-zone copy to zone [{}].",
+                    tmplt.getUniqueName(), otherZoneId, destZoneId);
+
+            return copyTemplateAcrossZones(destStore, sourceTmpl);
         }
-        logger.debug("Can't copy template [{}] from another image store.", tmplt.getUniqueName());
+
+        logger.debug("Template [{}] was not found in any zone. Cannot perform zone-to-zone copy.", tmplt.getUniqueName());
         return false;
     }
 
+    protected TemplateObject findUsableTemplate(VMTemplateVO tmplt, List<DataStore> imageStores) {
+        for (DataStore store : imageStores) {
+
+            Map<String, TemplateProp> templates = listTemplate(store);
+            if (templates == null || !templates.containsKey(tmplt.getUniqueName())) {
+                continue;
+            }
+
+            TemplateObject tmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), store);
+            if (tmpl.getInstallPath() == null) {
+                logger.debug("Template [{}] found in image store [{}] but install path is null. Skipping.",
+                        tmplt.getUniqueName(), store.getName());
+                continue;
+            }
+            return tmpl;
+        }
+        return null;
+    }
+
+    private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore) {
+        Long destZoneId = destStore.getScope().getScopeId();
+        List<DataStore> storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId);
+
+        TemplateObject sourceTmpl = findUsableTemplate(tmplt, storesInSameZone);
+        if (sourceTmpl == null) {
+            return false;
+        }
+
+        TemplateApiResult result;
+        AsyncCallFuture<TemplateApiResult> future = 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());
+        }
+        return result.isSuccess();
+    }
+
+    private boolean copyTemplateAcrossZones(DataStore destStore, TemplateObject sourceTmpl) {
+        Long dstZoneId = destStore.getScope().getScopeId();
+        DataCenterVO dstZone = _dcDao.findById(dstZoneId);
+
+        if (dstZone == null) {
+            logger.warn("Destination zone [{}] not found for template [{}].", dstZoneId, sourceTmpl.getUniqueName());
+            return false;
+        }
+
+        TemplateApiResult result;
+        try {
+            VMTemplateVO template = _templateDao.findById(sourceTmpl.getId());
+            try {
+                DataStore sourceStore = sourceTmpl.getDataStore();
+                long userId = CallContext.current().getCallingUserId();
+                boolean success = _tmpltMgr.copy(userId, template, sourceStore, dstZone);
+
+                result = new TemplateApiResult(sourceTmpl);
+                if (!success) {
+                    result.setResult("Cross-zone template copy failed");
+                }
+            } catch (StorageUnavailableException | ResourceAllocationException e) {
+                logger.error("Exception while copying template [{}] from zone [{}] to zone [{}]",
+                        template,
+                        sourceTmpl.getDataStore().getScope().getScopeId(),
+                        dstZone.getId(),
+                        e);
+                result = new TemplateApiResult(sourceTmpl);
+                result.setResult(e.getMessage());
+            } finally {
+                ThreadContext.clearAll();
+            }
+        } catch (Exception e) {
+            logger.error("Failed to copy template [{}] from zone [{}] to zone [{}].",
+                    sourceTmpl.getUniqueName(),
+                    sourceTmpl.getDataStore().getScope().getScopeId(),
+                    dstZoneId,
+                    e);
+            return false;
+        }
+
+        return result.isSuccess();
+    }
+
     @Override
     public AsyncCallFuture<TemplateApiResult> copyTemplateToImageStore(DataObject source, DataStore destStore) {
         TemplateObject sourceTmpl = (TemplateObject) source;
@@ -699,10 +816,6 @@ protected Void copyTemplateToImageStoreCallback(AsyncCallbackDispatcher<Template
         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());