From 55e211c3c5c8b341f001cbd29e9a6699dea499ce Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 9 Jan 2026 13:22:59 +0530 Subject: [PATCH] Combine template copy and download into a single asynchronous operation --- .../service/StorageOrchestrationService.java | 4 +- .../api/storage/TemplateService.java | 4 +- .../orchestration/StorageOrchestrator.java | 44 +++++--------- .../storage/image/TemplateServiceImpl.java | 57 ++++++++++++++++--- .../image/TemplateServiceImplTest.java | 47 +++++++++------ .../cloud/template/TemplateManagerImpl.java | 14 ++++- 6 files changed, 108 insertions(+), 62 deletions(-) diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java index 018f5c7b61e..140478dea73 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java @@ -31,7 +31,5 @@ public interface StorageOrchestrationService { MigrationResponse migrateResources(Long srcImgStoreId, Long destImgStoreId, List templateIdList, List snapshotIdList); - Future orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore); - - Future orchestrateTemplateCopyAcrossZones(TemplateInfo source, DataStore destStore); + Future orchestrateTemplateCopyFromSecondaryStores(long templateId, DataStore destStore); } diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java index a8861d5acc6..269eb4f1c21 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java @@ -80,4 +80,6 @@ public interface TemplateService { List getTemplateDatadisksOnImageStore(TemplateInfo templateInfo, String configurationId); AsyncCallFuture copyTemplateToImageStore(DataObject source, DataStore destStore); -} + + void handleTemplateCopyFromSecondaryStores(long templateId, DataStore destStore); + } diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java index 26990cecb49..49ded1ba136 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java @@ -44,7 +44,6 @@ import com.cloud.storage.VMTemplateVO; import com.cloud.storage.dao.VMTemplateDao; import com.cloud.template.TemplateManager; import org.apache.cloudstack.api.response.MigrationResponse; -import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService; import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; @@ -53,6 +52,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageServic 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; @@ -116,6 +116,8 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra @Inject VMTemplateDao templateDao; @Inject + TemplateDataFactory templateDataFactory; + @Inject DataCenterDao dcDao; @@ -324,11 +326,9 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra } @Override - public Future orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore destStore) { + public Future orchestrateTemplateCopyFromSecondaryStores(long srcTemplateId, DataStore destStore) { Long dstZoneId = destStore.getScope().getScopeId(); - DataCenterVO dstZone = dcDao.findById(dstZoneId); - long userId = CallContext.current().getCallingUserId(); - return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, dstZone)); + return submit(dstZoneId, new CopyTemplateFromSecondaryStorageTask(srcTemplateId, destStore)); } protected Pair migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List files, MigrationPolicy migrationPolicy, int skipped) { @@ -677,16 +677,14 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra } } - private class CrossZoneCopyTemplateTask implements Callable { - private final long userId; - private final TemplateInfo sourceTmpl; - private final DataCenterVO dstZone; + private class CopyTemplateFromSecondaryStorageTask implements Callable { + private final long srcTemplateId; + private final DataStore destStore; private final String logid; - CrossZoneCopyTemplateTask(long userId, TemplateInfo sourceTmpl, DataCenterVO dstZone) { - this.userId = userId; - this.sourceTmpl = sourceTmpl; - this.dstZone = dstZone; + CopyTemplateFromSecondaryStorageTask(long srcTemplateId, DataStore destStore) { + this.srcTemplateId = srcTemplateId; + this.destStore = destStore; this.logid = ThreadContext.get(LOGCONTEXTID); } @@ -694,25 +692,13 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra public TemplateApiResult call() { ThreadContext.put(LOGCONTEXTID, logid); TemplateApiResult result; - VMTemplateVO template = templateDao.findById(sourceTmpl.getId()); + long destZoneId = destStore.getScope().getScopeId(); + TemplateInfo sourceTmpl = templateDataFactory.getTemplate(srcTemplateId, DataStoreRole.Image); try { - DataStore sourceStore = sourceTmpl.getDataStore(); - boolean success = templateManager.copy(userId, template, sourceStore, dstZone); - + templateService.handleTemplateCopyFromSecondaryStores(srcTemplateId, destStore); 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 { - tryCleaningUpExecutor(dstZone.getId()); + tryCleaningUpExecutor(destZoneId); ThreadContext.clearAll(); } diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index c6a04f73981..ce31e853d78 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -31,6 +31,8 @@ import java.util.concurrent.ExecutionException; 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; @@ -70,6 +72,7 @@ import org.apache.cloudstack.storage.to.TemplateObjectTO; 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; @@ -548,10 +551,7 @@ public class TemplateServiceImpl implements TemplateService { } if (availHypers.contains(tmplt.getHypervisorType())) { - boolean copied = imageStoreDetailsUtil.isCopyTemplatesFromOtherStoragesEnabled(storeId, 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()); } @@ -598,6 +598,16 @@ public class TemplateServiceImpl implements TemplateService { } + @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(), @@ -686,8 +696,17 @@ public class TemplateServiceImpl implements TemplateService { return false; } - storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore); - return true; + TemplateApiResult result; + AsyncCallFuture 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) { @@ -699,9 +718,29 @@ public class TemplateServiceImpl implements TemplateService { return false; } + TemplateApiResult result; try { - storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, destStore); - return true; + 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(), @@ -710,6 +749,8 @@ public class TemplateServiceImpl implements TemplateService { e); return false; } + + return result.isSuccess(); } @Override diff --git a/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java index 726c354b270..fc2a0e925cf 100644 --- a/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java +++ b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java @@ -20,13 +20,18 @@ package org.apache.cloudstack.storage.image; import com.cloud.dc.DataCenterVO; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.StorageUnavailableException; +import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.template.TemplateProp; import com.cloud.template.TemplateManager; +import com.cloud.user.Account; +import com.cloud.user.User; +import org.apache.cloudstack.context.CallContext; 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; @@ -48,6 +53,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.mockito.Mockito.mock; + @RunWith(MockitoJUnitRunner.class) public class TemplateServiceImplTest { @@ -88,6 +95,9 @@ public class TemplateServiceImplTest { @Mock DataCenterDao _dcDao; + @Mock + VMTemplateDao templateDao; + @Mock TemplateManager _tmpltMgr; @@ -103,7 +113,6 @@ public class TemplateServiceImplTest { 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); } @@ -168,7 +177,7 @@ public class TemplateServiceImplTest { boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock); Assert.assertFalse(result); - Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any()); + Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyFromSecondaryStores(Mockito.anyLong(), Mockito.any()); } @Test @@ -184,22 +193,11 @@ public class TemplateServiceImplTest { boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock); Assert.assertFalse(result); - Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any()); + Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyFromSecondaryStores(Mockito.anyLong(), 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()); - } - - @Test - public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone() { + public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone() throws StorageUnavailableException, ResourceAllocationException { Scope scopeMock = Mockito.mock(Scope.class); Mockito.doReturn(scopeMock).when(destStoreMock).getScope(); Mockito.doReturn(1L).when(scopeMock).getScopeId(); @@ -222,6 +220,7 @@ public class TemplateServiceImplTest { DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class); Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L); + Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any()); boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock); @@ -256,7 +255,7 @@ public class TemplateServiceImplTest { } @Test - public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIsScheduled() { + public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIsScheduled() throws StorageUnavailableException, ResourceAllocationException { Scope scopeMock = Mockito.mock(Scope.class); Mockito.doReturn(scopeMock).when(destStoreMock).getScope(); Mockito.doReturn(1L).when(scopeMock).getScopeId(); @@ -270,15 +269,27 @@ public class TemplateServiceImplTest { Map templates = new HashMap<>(); templates.put("unique-name", tmpltPropMock); - Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock); TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class); Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock); Mockito.doReturn("/mnt/secondary/template.qcow2").when(sourceTmplMock).getInstallPath(); + Mockito.doReturn(100L).when(sourceTmplMock).getId(); + + DataStore sourceStoreMock = Mockito.mock(DataStore.class); + Scope sourceScopeMock = Mockito.mock(Scope.class); + Mockito.doReturn(sourceStoreMock).when(sourceTmplMock).getDataStore(); DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class); Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L); + VMTemplateVO templateVoMock = Mockito.mock(VMTemplateVO.class); + Mockito.doReturn(templateVoMock).when(templateDao).findById(100L); + + Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any()); + + Account account = mock(Account.class); + User user = mock(User.class); + CallContext callContext = mock(CallContext.class); boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock); diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index ba8e5714180..3e5e0e65edb 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -838,6 +838,9 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, // Copy will just find one eligible image store for the destination zone // and copy template there, not propagate to all image stores // for that zone + + boolean copied = false; + for (DataStore dstSecStore : dstSecStores) { TemplateDataStoreVO dstTmpltStore = _tmplStoreDao.findByStoreTemplate(dstSecStore.getId(), tmpltId); if (dstTmpltStore != null && dstTmpltStore.getDownloadState() == Status.DOWNLOADED) { @@ -852,9 +855,12 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, TemplateApiResult result = future.get(); if (result.isFailed()) { logger.debug("Copy Template failed for image store {}: {}", dstSecStore, result.getResult()); + _tmplStoreDao.removeByTemplateStore(tmpltId, dstSecStore.getId()); continue; // try next image store } + copied = true; + _tmpltDao.addTemplateToZone(template, dstZoneId); if (account.getId() != Account.ACCOUNT_ID_SYSTEM) { @@ -882,12 +888,14 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, } } } + + return true; + } catch (Exception ex) { - logger.debug("Failed to copy Template to image store:{} ,will try next one", dstSecStore); + logger.debug("Failed to copy Template to image store:{} ,will try next one", dstSecStore, ex); } } - return true; - + return copied; } @Override