Migrate public templates that have URLs on data migration across secondary storages (#10364)

Co-authored-by: Fabricio Duarte <fabricio.duarte@scclouds.com.br>

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 interface VMTemplateStorageResourceAssoc extends InternalIdentity {
         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 @@ public class DataMigrationUtility {
         List<TemplateInfo> files = new LinkedList<>();
         for (TemplateDataStoreVO template : templates) {
             VMTemplateVO templateVO = templateDao.findById(template.getTemplateId());
-            if (template.getState() == ObjectInDataStoreStateMachine.State.Ready && templateVO != null &&
+            if (shouldMigrateTemplate(template, templateVO)) {
-                    (!templateVO.isPublicTemplate() || (templateVO.isPublicTemplate() && templateVO.getUrl() == null)) &&
-                    templateVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator && templateVO.getParentTemplateId() == null) {
                 files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore));
             }
         }
@@ -231,6 +229,34 @@ public class DataMigrationUtility {
         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 java.util.concurrent.ExecutionException;
 
 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 class SecondaryStorageServiceImpl implements SecondaryStorageService {
                 }
             } 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 {
-            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;
-                    }
-                }
                 destDataObject = destDatastore.create(srcDataObject);
                 srcDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
                 destDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested);
@@ -160,6 +158,69 @@ public class SecondaryStorageServiceImpl implements SecondaryStorageService {
         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);

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

@@ -0,0 +1,138 @@
+// 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.storage.image;
+
+import com.cloud.agent.api.to.DataStoreTO;
+import com.cloud.storage.VMTemplateStorageResourceAssoc;
+import com.cloud.utils.exception.CloudRuntimeException;
+import junit.framework.TestCase;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
+import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
+import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
+import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
+import org.apache.cloudstack.storage.to.TemplateObjectTO;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SecondaryStorageServiceImplTest extends TestCase {
+
+    @Spy
+    @InjectMocks
+    private SecondaryStorageServiceImpl secondaryStorageService;
+
+    @Mock
+    TemplateDataStoreDao templateDataStoreDaoMock;
+
+    @Mock
+    TemplateDataStoreVO templateDataStoreVoMock;
+
+    @Mock
+    TemplateInfo templateInfoMock;
+
+    @Mock
+    TemplateObjectTO templateObjectToMock;
+
+    @Mock
+    DataStore dataStoreMock;
+
+    @Mock
+    DataStoreTO dataStoreToMock;
+
+    private void prepareForTemplateIsOnDestinationTests() {
+        long dataStoreId = 1;
+        long templateId = 2;
+
+        Mockito.when(dataStoreMock.getId()).thenReturn(dataStoreId);
+        Mockito.when(dataStoreMock.getTO()).thenReturn(dataStoreToMock);
+        Mockito.when(templateInfoMock.getId()).thenReturn(templateId);
+        Mockito.when(templateInfoMock.getTO()).thenReturn(templateObjectToMock);
+        Mockito.doReturn(templateDataStoreVoMock).when(templateDataStoreDaoMock).findByStoreTemplate(dataStoreId, templateId);
+        Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready);
+    }
+
+    @Test
+    public void templateIsOnDestinationTestReturnsFalseWhenTemplateStoreRefDoesNotExist() {
+        prepareForTemplateIsOnDestinationTests();
+        Mockito.doReturn(null).when(templateDataStoreDaoMock).findByStoreTemplate(Mockito.anyLong(), Mockito.anyLong());
+
+        boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void templateIsOnDestinationTestReturnsTrueWhenTemplateIsReady() {
+        prepareForTemplateIsOnDestinationTests();
+
+        boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+
+        Assert.assertTrue(result);
+    }
+
+    @Test
+    public void templateIsOnDestinationTestReturnsFalseWhenTemplateIsNotReady() {
+        prepareForTemplateIsOnDestinationTests();
+        Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Creating);
+
+        boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void templateIsOnDestinationTestReturnsTrueIfTemplateIsDownloadedSuccessfully() {
+        prepareForTemplateIsOnDestinationTests();
+        Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS);
+        Mockito.doAnswer(I -> Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOADED)).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString());
+
+        boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+
+        Assert.assertTrue(result);
+    }
+
+    @Test
+    public void templateIsOnDestinationTestReturnsFalseIfTemplateIsNotDownloadedSuccessfully() {
+        prepareForTemplateIsOnDestinationTests();
+        Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS);
+        Mockito.doAnswer(I -> {
+            Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_ERROR);
+            Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Failed);
+            return "mocked download fail";
+        }).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString());
+
+        boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+
+        Assert.assertFalse(result);
+    }
+
+    @Test(expected = CloudRuntimeException.class)
+    public void templateIsOnDestinationTestThrowsExceptionIfDownloadTimesOut() {
+        prepareForTemplateIsOnDestinationTests();
+        Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS);
+        Mockito.doReturn(0L).when(secondaryStorageService).getTemplateDownloadTimeout();
+
+        secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock);
+    }
+}

server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java

@@ -151,11 +151,6 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation<Templa
         activeTmpltSearch.and("store_id", activeTmpltSearch.entity().getDataStoreId(), SearchCriteria.Op.EQ);
         activeTmpltSearch.and("type", activeTmpltSearch.entity().getTemplateType(), SearchCriteria.Op.EQ);
         activeTmpltSearch.and("templateState", activeTmpltSearch.entity().getTemplateState(), SearchCriteria.Op.EQ);
-        activeTmpltSearch.and().op("public", activeTmpltSearch.entity().isPublicTemplate(), SearchCriteria.Op.EQ);
-        activeTmpltSearch.or().op("publicNoUrl", activeTmpltSearch.entity().isPublicTemplate(), SearchCriteria.Op.EQ);
-        activeTmpltSearch.and("url", activeTmpltSearch.entity().getUrl(), SearchCriteria.Op.NULL);
-        activeTmpltSearch.cp();
-        activeTmpltSearch.cp();
         activeTmpltSearch.done();
 
         publicTmpltSearch = createSearchBuilder();
@@ -687,8 +682,6 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation<Templa
         sc.setParameters("store_id", storeId);
         sc.setParameters("type", TemplateType.USER);
         sc.setParameters("templateState", VirtualMachineTemplate.State.Active);
-        sc.setParameters("public", Boolean.FALSE);
-        sc.setParameters("publicNoUrl",Boolean.TRUE);
         return searchIncludingRemoved(sc, null, null, false);
     }
 

server/src/main/java/com/cloud/storage/download/DownloadActiveState.java

@@ -76,7 +76,7 @@ public abstract class DownloadActiveState extends DownloadState {
             getDownloadListener().log("handleTimeout, updateMs=" + updateMs + ", curr state= " + getName(), Level.TRACE);
         }
         String newState = getName();
-        if (updateMs > 5 * DownloadListener.STATUS_POLL_INTERVAL) {
+        if (updateMs > DownloadListener.DOWNLOAD_TIMEOUT) {
             newState = Status.DOWNLOAD_ERROR.toString();
             getDownloadListener().log("timeout: transitioning to download error state, currstate=" + getName(), Level.DEBUG);
         } else if (updateMs > 3 * DownloadListener.STATUS_POLL_INTERVAL) {

server/src/main/java/com/cloud/storage/download/DownloadListener.java

@@ -100,6 +100,7 @@ public class DownloadListener implements Listener {
     protected Logger logger = LogManager.getLogger(getClass());
     public static final int SMALL_DELAY = 100;
     public static final long STATUS_POLL_INTERVAL = 10000L;
+    public static final long DOWNLOAD_TIMEOUT = 5 * STATUS_POLL_INTERVAL;
 
     public static final String DOWNLOADED = Status.DOWNLOADED.toString();
     public static final String NOT_DOWNLOADED = Status.NOT_DOWNLOADED.toString();