Migrate public templates that have URLs on data migration across secondary storages

api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java

@@ -17,6 +17,7 @@
 package com.cloud.storage;
 
 import java.util.Date;
+import java.util.List;
 
 import org.apache.cloudstack.api.InternalIdentity;
 
@@ -25,6 +26,8 @@ public static enum Status {
         UNKNOWN, DOWNLOAD_ERROR, NOT_DOWNLOADED, DOWNLOAD_IN_PROGRESS, DOWNLOADED, ABANDONED, UPLOADED, NOT_UPLOADED, UPLOAD_ERROR, UPLOAD_IN_PROGRESS, CREATING, CREATED, BYPASSED
     }
 
+    List<Status> PENDING_DOWNLOAD_STATES = List.of(Status.NOT_DOWNLOADED, Status.DOWNLOAD_IN_PROGRESS);
+
     String getInstallPath();
 
     long getTemplateId();

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

@@ -208,9 +208,7 @@ protected List<DataObject> getAllReadyTemplates(DataStore srcDataStore, Map<Data
         List<TemplateInfo> files = new LinkedList<>();
         for (TemplateDataStoreVO template : templates) {
             VMTemplateVO templateVO = templateDao.findById(template.getTemplateId());
-            if (template.getState() == ObjectInDataStoreStateMachine.State.Ready && templateVO != null &&
-                    (!templateVO.isPublicTemplate() || (templateVO.isPublicTemplate() && templateVO.getUrl() == null)) &&
-                    templateVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator && templateVO.getParentTemplateId() == null) {
+            if (shouldMigrateTemplate(template, templateVO)) {
                 files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore));
             }
         }
@@ -231,6 +229,34 @@ protected List<DataObject> getAllReadyTemplates(DataStore srcDataStore, Map<Data
         return getAllReadyTemplates(srcDataStore, childTemplates, templates);
     }
 
+    /**
+     * Returns whether a template should be migrated. A template should be migrated if:
+     * <ol>
+     * <li>its state is ready, and</li>
+     * <li>its hypervisor type is not simulator, and</li>
+     * <li>it is not a child template.</li>
+     * </ol>
+     */
+    protected boolean shouldMigrateTemplate(TemplateDataStoreVO template, VMTemplateVO templateVO) {
+        if (template.getState() != State.Ready) {
+            logger.debug("Template [{}] should not be migrated as it is not ready.", template);
+            return false;
+        }
+
+        if (templateVO.getHypervisorType() == Hypervisor.HypervisorType.Simulator) {
+            logger.debug("Template [{}] should not be migrated as its hypervisor type is simulator.", template);
+            return false;
+        }
+
+        if (templateVO.getParentTemplateId() != null) {
+            logger.debug("Template [{}] should not be migrated as it has a parent template.", template);
+            return false;
+        }
+
+        logger.debug("Template [{}] should be migrated.", template);
+        return true;
+    }
+
     /** Returns parent snapshots and snapshots that do not have any children; snapshotChains comprises of the snapshot chain info
      * for each parent snapshot and the cumulative size of the chain - this is done to ensure that all the snapshots in a chain
      * are migrated to the same datastore

engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java

@@ -0,0 +1,88 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.engine.orchestration;
+
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.storage.VMTemplateVO;
+import junit.framework.TestCase;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class DataMigrationUtilityTest extends TestCase {
+
+    @Spy
+    private DataMigrationUtility dataMigrationUtility;
+
+    @Mock
+    private VMTemplateVO templateVoMock;
+
+    @Mock
+    private TemplateDataStoreVO templateDataStoreVoMock;
+
+    private void prepareForShouldMigrateTemplateTests() {
+        Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready);
+        Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM);
+        Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(null);
+    }
+
+    @Test
+    public void shouldMigrateTemplateTestReturnsFalseWhenTemplateIsNotReady() {
+        prepareForShouldMigrateTemplateTests();
+        Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Migrating);
+
+        boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void shouldMigrateTemplateTestReturnsFalseWhenHypervisorTypeIsSimulator() {
+        prepareForShouldMigrateTemplateTests();
+        Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.Simulator);
+
+        boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void shouldMigrateTemplateTestReturnsFalseWhenTemplateHasParentTemplate() {
+        prepareForShouldMigrateTemplateTests();
+        Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(1L);
+
+        boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void shouldMigrateTemplateTestReturnsTrueWhenTemplateIsReadyAndDoesNotHaveParentTemplateAndHypervisorTypeIsNotSimulator() {
+        prepareForShouldMigrateTemplateTests();
+
+        boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock);
+
+        Assert.assertTrue(result);
+    }
+}

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

@@ -24,6 +24,9 @@
 
 import javax.inject.Inject;
 
+import com.cloud.storage.VMTemplateStorageResourceAssoc;
+import com.cloud.storage.download.DownloadListener;
+import com.cloud.utils.exception.CloudRuntimeException;
 import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionService;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
@@ -118,26 +121,21 @@ public AsyncCallFuture<DataObjectResult> migrateData(DataObject srcDataObject, D
                 }
             } else if (srcDataObject instanceof TemplateInfo && templateChain != null && templateChain.containsKey(srcDataObject)) {
                 for (TemplateInfo templateInfo : templateChain.get(srcDataObject).first()) {
+                    if (templateIsOnDestination(templateInfo, destDatastore)) {
+                        res.setResult("Template already exists on destination.");
+                        res.setSuccess(true);
+                        logger.debug("Deleting template {} from source data store [{}].", srcDataObject.getTO().toString(),
+                                srcDataObject.getDataStore().getTO().toString());
+                        srcDataObject.getDataStore().delete(srcDataObject);
+                        future.complete(res);
+                        continue;
+                    }
                     destDataObject = destDatastore.create(templateInfo);
                     templateInfo.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
                     destDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
                     migrateJob(future, templateInfo, destDataObject, destDatastore);
                 }
-            }
-            else {
-                // Check if template in destination store, if yes, do not proceed
-                if (srcDataObject instanceof TemplateInfo) {
-                    logger.debug("Checking if template present at destination");
-                    TemplateDataStoreVO templateStoreVO = templateStoreDao.findByStoreTemplate(destDatastore.getId(), srcDataObject.getId());
-                    if (templateStoreVO != null) {
-                        String msg = "Template already exists in destination store";
-                        logger.debug(msg);
-                        res.setResult(msg);
-                        res.setSuccess(true);
-                        future.complete(res);
-                        return future;
-                    }
-                }
+            } else {
                 destDataObject = destDatastore.create(srcDataObject);
                 srcDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
                 destDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
@@ -160,6 +158,69 @@ public AsyncCallFuture<DataObjectResult> migrateData(DataObject srcDataObject, D
         return future;
     }
 
+    /**
+     * Returns a boolean indicating whether a template is ready on the provided data store. If the template is being downloaded,
+     * waits until the download finishes.
+     * @param srcDataObject the template.
+     * @param destDatastore the data store.
+     */
+    protected boolean templateIsOnDestination(DataObject srcDataObject, DataStore destDatastore) {
+        if (!(srcDataObject instanceof TemplateInfo)) {
+            return false;
+        }
+
+        String templateAsString = srcDataObject.getTO().toString();
+        String destDatastoreAsString = destDatastore.getTO().toString();
+        TemplateDataStoreVO templateStoreVO;
+
+        long timer = getTemplateDownloadTimeout();
+        long msToSleep = 10000L;
+        int previousDownloadPercentage = -1;
+
+        while (true) {
+            templateStoreVO = templateStoreDao.findByStoreTemplate(destDatastore.getId(), srcDataObject.getId());
+            if (templateStoreVO == null) {
+                logger.debug("{} is not present at destination [{}].", templateAsString, destDatastoreAsString);
+                return false;
+            }
+            VMTemplateStorageResourceAssoc.Status downloadState = templateStoreVO.getDownloadState();
+            if (downloadState == null || !VMTemplateStorageResourceAssoc.PENDING_DOWNLOAD_STATES.contains(downloadState)) {
+                break;
+            }
+            if (previousDownloadPercentage == templateStoreVO.getDownloadPercent()) {
+                timer -= msToSleep;
+            } else {
+                timer = getTemplateDownloadTimeout();
+            }
+            if (timer <= 0) {
+                throw new CloudRuntimeException(String.format("Timeout while waiting for %s to be downloaded to image store [%s]. " +
+                        "The download percentage has not changed for %d milliseconds.", templateAsString, destDatastoreAsString, getTemplateDownloadTimeout()));
+            }
+            waitForTemplateDownload(msToSleep, templateAsString, destDatastoreAsString);
+        }
+
+        if (templateStoreVO.getState() == ObjectInDataStoreStateMachine.State.Ready) {
+            logger.debug("{} already exists on destination [{}].", templateAsString, destDatastoreAsString);
+            return true;
+        }
+        return false;
+    }
+
+    protected long getTemplateDownloadTimeout() {
+        return DownloadListener.DOWNLOAD_TIMEOUT;
+    }
+
+    protected void waitForTemplateDownload(long msToSleep, String templateAsString, String destDatastoreAsString) {
+        logger.debug("{} is being downloaded to destination [{}]; we will verify in {} milliseconds if the download has finished.",
+                templateAsString, destDatastoreAsString, msToSleep);
+        try {
+            Thread.sleep(msToSleep);
+        } catch (InterruptedException e) {
+            logger.warn("[ignored] interrupted while waiting for template {} download to finish before trying to migrate it to data store [{}].",
+                    templateAsString, destDatastoreAsString);
+        }
+    }
+
     protected void migrateJob(AsyncCallFuture<DataObjectResult> future, DataObject srcDataObject, DataObject destDataObject, DataStore destDatastore) throws ExecutionException, InterruptedException {
         MigrateDataContext<DataObjectResult> context = new MigrateDataContext<DataObjectResult>(null, future, srcDataObject, destDataObject, destDatastore);
         AsyncCallbackDispatcher<SecondaryStorageServiceImpl, CopyCommandResult> caller = AsyncCallbackDispatcher.create(this);