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 aa8d93d7ae7..018f5c7b61e 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 @@ -33,5 +33,5 @@ public interface StorageOrchestrationService { Future orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore); - Future orchestrateTemplateCopyAcrossZones(TemplateInfo source, DataStore sourceStore, DataStore destStore); + Future orchestrateTemplateCopyAcrossZones(TemplateInfo source, 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 d2a5dd88848..26990cecb49 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 @@ -38,6 +38,8 @@ import javax.naming.ConfigurationException; 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.VMTemplateVO; import com.cloud.storage.dao.VMTemplateDao; import com.cloud.template.TemplateManager; @@ -322,11 +324,11 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra } @Override - public Future orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore sourceStore, DataStore destStore) { + public Future orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore destStore) { Long dstZoneId = destStore.getScope().getScopeId(); DataCenterVO dstZone = dcDao.findById(dstZoneId); long userId = CallContext.current().getCallingUserId(); - return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, sourceStore, dstZone)); + return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, dstZone)); } protected Pair migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List files, MigrationPolicy migrationPolicy, int skipped) { @@ -678,17 +680,12 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra private class CrossZoneCopyTemplateTask implements Callable { private final long userId; private final TemplateInfo sourceTmpl; - private final DataStore sourceStore; private final DataCenterVO dstZone; private final String logid; - CrossZoneCopyTemplateTask(long userId, - TemplateInfo sourceTmpl, - DataStore sourceStore, - DataCenterVO dstZone) { + CrossZoneCopyTemplateTask(long userId, TemplateInfo sourceTmpl, DataCenterVO dstZone) { this.userId = userId; this.sourceTmpl = sourceTmpl; - this.sourceStore = sourceStore; this.dstZone = dstZone; this.logid = ThreadContext.get(LOGCONTEXTID); } @@ -699,16 +696,17 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra TemplateApiResult result; VMTemplateVO template = templateDao.findById(sourceTmpl.getId()); try { + DataStore sourceStore = sourceTmpl.getDataStore(); boolean success = templateManager.copy(userId, template, sourceStore, dstZone); result = new TemplateApiResult(sourceTmpl); if (!success) { result.setResult("Cross-zone template copy failed"); } - } catch (Exception e) { + } catch (StorageUnavailableException | ResourceAllocationException e) { logger.error("Exception while copying template [{}] from zone [{}] to zone [{}]", template, - sourceStore.getScope().getScopeId(), + sourceTmpl.getDataStore().getScope().getScopeId(), dstZone.getId(), e); result = new TemplateApiResult(sourceTmpl); 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 00b7bef9a5c..d32a5240e20 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 @@ -615,13 +615,11 @@ public class TemplateServiceImpl implements TemplateService { } protected boolean tryCopyingTemplateToImageStore(VMTemplateVO tmplt, DataStore destStore) { - Long destZoneId = destStore.getScope().getScopeId(); - - List storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId); - if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) { + 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); @@ -643,45 +641,54 @@ public class TemplateServiceImpl implements TemplateService { continue; } - DataStore sourceStore = findImageStorageHavingTemplate(tmplt, storesInOtherZone); - if (sourceStore == null) { - logger.debug("Template [{}] not found in any image store of zone [{}].", + 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; } - TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore); - if (sourceTmpl.getInstallPath() == null) { - logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.", - tmplt.getUniqueName(), sourceStore.getName()); - continue; - } - logger.info("Template [{}] found in zone [{}]. Initiating cross-zone copy to zone [{}].", tmplt.getUniqueName(), otherZoneId, destZoneId); - return copyTemplateAcrossZones(sourceStore, destStore, tmplt); + return copyTemplateAcrossZones(destStore, sourceTmpl); } - logger.debug("Template [{}] was not found in any zone. Cannot perform zone-to-zone copy.", - tmplt.getUniqueName()); + logger.debug("Template [{}] was not found in any zone. Cannot perform zone-to-zone copy.", tmplt.getUniqueName()); return false; } - private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore, List stores) { - for (DataStore sourceStore : stores) { + protected TemplateObject findUsableTemplate(VMTemplateVO tmplt, List imageStores) { + for (DataStore store : imageStores) { + TemplateObject tmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), store); + if (tmpl == null) { + continue; + } + + 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 storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId); + for (DataStore sourceStore : storesInSameZone) { Map existingTemplatesInSourceStore = listTemplate(sourceStore); if (existingTemplatesInSourceStore == null || !existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) { - logger.debug("Template [{}] does not exist on image store [{}]; searching another.", - tmplt.getUniqueName(), sourceStore.getName()); + logger.debug("Template [{}] does not exist on image store [{}]; searching another.", tmplt.getUniqueName(), sourceStore.getName()); continue; } TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore); if (sourceTmpl.getInstallPath() == null) { - logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.", - tmplt.getUniqueName(), sourceStore.getName()); + logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.", tmplt.getUniqueName(), sourceStore.getName()); continue; } @@ -691,36 +698,22 @@ public class TemplateServiceImpl implements TemplateService { return false; } - private DataStore findImageStorageHavingTemplate(VMTemplateVO tmplt, List stores) { - for (DataStore store : stores) { - Map templates = listTemplate(store); - if (templates != null && templates.containsKey(tmplt.getUniqueName())) { - return store; - } - } - return null; - } - - private boolean copyTemplateAcrossZones(DataStore sourceStore, - DataStore destStore, - VMTemplateVO tmplt) { + 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, tmplt.getUniqueName()); + logger.warn("Destination zone [{}] not found for template [{}].", dstZoneId, sourceTmpl.getUniqueName()); return false; } - TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore); try { - storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, sourceStore, destStore); + storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, destStore); return true; } catch (Exception e) { logger.error("Failed to copy template [{}] from zone [{}] to zone [{}].", - tmplt.getUniqueName(), - sourceStore.getScope().getScopeId(), + sourceTmpl.getUniqueName(), + sourceTmpl.getDataStore().getScope().getScopeId(), dstZoneId, e); return false; 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 ed370fde701..faf08de0061 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 @@ -213,7 +213,6 @@ public class TemplateServiceImplTest { Map templatesInOtherZone = new HashMap<>(); templatesInOtherZone.put(tmpltMock.getUniqueName(), tmpltPropMock); - Mockito.doReturn(templatesInOtherZone).when(templateService).listTemplate(otherZoneStoreMock); TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class); Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock); @@ -241,7 +240,6 @@ public class TemplateServiceImplTest { Map templates = new HashMap<>(); templates.put(tmpltMock.getUniqueName(), tmpltPropMock); - Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock); TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class); Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock); @@ -267,7 +265,6 @@ public class TemplateServiceImplTest { Map templates = new HashMap<>(); templates.put(tmpltMock.getUniqueName(), tmpltPropMock); - Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock); TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class); Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock); @@ -294,4 +291,31 @@ public class TemplateServiceImplTest { Assert.assertFalse(result); } + + @Test + public void testFindUsableTemplateReturnsTemplateWithNonNullInstallPath() { + VMTemplateVO template = Mockito.mock(VMTemplateVO.class); + Mockito.when(template.getId()).thenReturn(10L); + Mockito.when(template.getUniqueName()).thenReturn("test-template"); + + DataStore storeWithNullPath = Mockito.mock(DataStore.class); + Mockito.when(storeWithNullPath.getName()).thenReturn("store-null"); + + DataStore storeWithValidPath = Mockito.mock(DataStore.class); + TemplateObject tmplWithNullPath = Mockito.mock(TemplateObject.class); + Mockito.when(tmplWithNullPath.getInstallPath()).thenReturn(null); + + TemplateObject tmplWithValidPath = Mockito.mock(TemplateObject.class); + Mockito.when(tmplWithValidPath.getInstallPath()).thenReturn("/mnt/secondary/template.qcow2"); + + Mockito.doReturn(tmplWithNullPath).when(templateDataFactoryMock).getTemplate(10L, storeWithNullPath); + Mockito.doReturn(tmplWithValidPath).when(templateDataFactoryMock).getTemplate(10L, storeWithValidPath); + + List imageStores = List.of(storeWithNullPath, storeWithValidPath); + + TemplateObject result = templateService.findUsableTemplate(template, imageStores); + + Assert.assertNotNull(result); + Assert.assertEquals(tmplWithValidPath, result); + } } diff --git a/server/src/main/java/com/cloud/storage/ImageStoreDetailsUtil.java b/server/src/main/java/com/cloud/storage/ImageStoreDetailsUtil.java index 7e0fcc550b4..9f5aa660f4f 100755 --- a/server/src/main/java/com/cloud/storage/ImageStoreDetailsUtil.java +++ b/server/src/main/java/com/cloud/storage/ImageStoreDetailsUtil.java @@ -79,7 +79,6 @@ public class ImageStoreDetailsUtil { } public boolean isCopyTemplatesFromOtherStoragesEnabled(Long storeId, Long zoneId) { - final Map storeDetails = imageStoreDetailsDao.getDetails(storeId); final String keyWithoutDots = StorageManager.COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES.key() .replace(".", "");