Copilot commented on code in PR #12296:
URL: https://github.com/apache/cloudstack/pull/12296#discussion_r2634366527
##########
engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java:
##########
@@ -183,4 +199,92 @@ public void
tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAno
Assert.assertTrue(result);
Mockito.verify(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(),
Mockito.any());
}
+
+ @Test
+ public void
tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone()
throws StorageUnavailableException, ResourceAllocationException {
+ Scope scopeMock = Mockito.mock(Scope.class);
+ Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
+ Mockito.doReturn(1L).when(scopeMock).getScopeId();
+
Mockito.doReturn(List.of(sourceStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(1L);
+
Mockito.doReturn(null).when(templateService).listTemplate(sourceStoreMock);
+ Mockito.doReturn(List.of(1L, 2L)).when(_dcDao).listAllIds();
+
+ DataStore otherZoneStoreMock = Mockito.mock(DataStore.class);
+
Mockito.doReturn(List.of(otherZoneStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(2L);
+
+ Map<String, TemplateProp> templatesInOtherZone = new HashMap<>();
+ templatesInOtherZone.put(tmpltMock.getUniqueName(), tmpltPropMock);
+
Mockito.doReturn(templatesInOtherZone).when(templateService).listTemplate(otherZoneStoreMock);
+
+ DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class);
+ Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L);
+ Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(),
Mockito.eq(tmpltMock), Mockito.eq(otherZoneStoreMock), Mockito.eq(dstZoneMock));
+
+ boolean result =
templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+ Assert.assertTrue(result);
+ }
+
+ @Test
+ public void
tryCopyingTemplateToImageStoreTestReturnsFalseWhenDestinationZoneIsMissing() {
+ Scope scopeMock = Mockito.mock(Scope.class);
+ Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
+ Mockito.doReturn(1L).when(scopeMock).getScopeId();
+ Mockito.doReturn(List.of(1L, 2L)).when(_dcDao).listAllIds();
+
Mockito.doReturn(List.of()).when(dataStoreManagerMock).getImageStoresByZoneIds(1L);
+
+ DataStore otherZoneStoreMock = Mockito.mock(DataStore.class);
+
Mockito.doReturn(List.of(otherZoneStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(2L);
+
+ Map<String, TemplateProp> templates = new HashMap<>();
+ templates.put(tmpltMock.getUniqueName(), tmpltPropMock);
+
Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock);
+ Mockito.doReturn(null).when(_dcDao).findById(1L);
+
+ boolean result =
templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+ Assert.assertFalse(result);
+ }
+
+ @Test
+ public void
tryCopyingTemplateToImageStoreTestReturnsFalseWhenCrossZoneCopyThrowsException()
throws StorageUnavailableException, ResourceAllocationException {
+ Scope scopeMock = Mockito.mock(Scope.class);
+ Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
+ Mockito.doReturn(1L).when(scopeMock).getScopeId();
+ Mockito.doReturn(List.of(1L, 2L)).when(_dcDao).listAllIds();
+
Mockito.doReturn(List.of()).when(dataStoreManagerMock).getImageStoresByZoneIds(1L);
+
+ DataStore otherZoneStoreMock = Mockito.mock(DataStore.class);
+
Mockito.doReturn(List.of(otherZoneStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(2L);
+
+ Map<String, TemplateProp> templates = new HashMap<>();
+ templates.put(tmpltMock.getUniqueName(), tmpltPropMock);
+
Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock);
+
+ DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class);
+ Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L);
+ Mockito.doThrow(new RuntimeException("copy
failed")).when(_tmpltMgr).copy(Mockito.anyLong(), Mockito.any(), Mockito.any(),
Mockito.any());
+
+ Scope sourceScopeMock = Mockito.mock(Scope.class);
+ Mockito.doReturn(sourceScopeMock).when(otherZoneStoreMock).getScope();
+ Mockito.doReturn(2L).when(sourceScopeMock).getScopeId();
+
+ boolean result =
templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+ Assert.assertFalse(result);
+ }
Review Comment:
The test needs to mock CallContext to avoid a NullPointerException. The
implementation code calls `CallContext.current().getCallingUserId()` in the
`copyTemplateAcrossZones` method (line 711 in TemplateServiceImpl.java), but
the test doesn't set up the CallContext. You should add mocking for
CallContext.current() to return a mock CallContext that provides a valid user
ID when getCallingUserId() is called.
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +616,110 @@ 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) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ 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;
+ }
+
+ List<DataStore> storesInOtherZone =
_storeMgr.getImageStoresByZoneIds(otherZoneId);
+ logger.debug("Checking zone [{}] for template [{}]...",
otherZoneId, tmplt.getUniqueName());
+
+ if (storesInOtherZone == null || storesInOtherZone.isEmpty()) {
+ logger.debug("Zone [{}] has no image stores. Skipping.",
otherZoneId);
+ continue;
+ }
+
+ DataStore sourceStore = findTemplateInStores(tmplt,
storesInOtherZone);
+ if (sourceStore == null) {
+ logger.debug("Template [{}] not found 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(sourceStore, destStore, tmplt);
+ }
+
+ 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<DataStore> stores) {
+ for (DataStore sourceStore : stores) {
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.",
+ if (existingTemplatesInSourceStore == null ||
+
!existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
+ 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("Can not copy template [{}] from image store [{}],
as it returned a null install path.", tmplt.getUniqueName(),
- sourceStore.getName());
+ logger.warn("Cannot copy template [{}] from image store [{}];
install path is null.",
+ tmplt.getUniqueName(), sourceStore.getName());
continue;
}
+
storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
return true;
}
- logger.debug("Can't copy template [{}] from another image store.",
tmplt.getUniqueName());
return false;
}
+ private DataStore findTemplateInStores(VMTemplateVO tmplt, List<DataStore>
stores) {
+ for (DataStore store : stores) {
+ Map<String, TemplateProp> templates = listTemplate(store);
+ if (templates != null &&
templates.containsKey(tmplt.getUniqueName())) {
+ return store;
+ }
+ }
+ return null;
+ }
+
+ private boolean copyTemplateAcrossZones(DataStore sourceStore,
+ DataStore destStore,
+ VMTemplateVO tmplt) {
+ Long dstZoneId = destStore.getScope().getScopeId();
+ DataCenterVO dstZone = _dcDao.findById(dstZoneId);
+
+ if (dstZone == null) {
+ logger.warn("Destination zone [{}] not found for template [{}]",
Review Comment:
The log message should end with a period for consistency with other logging
statements in this class.
```suggestion
logger.warn("Destination zone [{}] not found for template [{}].",
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +616,110 @@ 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) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ logger.debug("Template [{}] not found in any image store of zone [{}].
Checking other zones",
Review Comment:
The log message should end with a period for consistency with other logging
statements in this class.
```suggestion
logger.debug("Template [{}] not found in any image store of zone
[{}]. Checking other zones.",
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +616,110 @@ 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) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ 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;
+ }
+
+ List<DataStore> storesInOtherZone =
_storeMgr.getImageStoresByZoneIds(otherZoneId);
+ logger.debug("Checking zone [{}] for template [{}]...",
otherZoneId, tmplt.getUniqueName());
+
+ if (storesInOtherZone == null || storesInOtherZone.isEmpty()) {
+ logger.debug("Zone [{}] has no image stores. Skipping.",
otherZoneId);
+ continue;
+ }
+
+ DataStore sourceStore = findTemplateInStores(tmplt,
storesInOtherZone);
+ if (sourceStore == null) {
+ logger.debug("Template [{}] not found 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(sourceStore, destStore, tmplt);
+ }
+
+ 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<DataStore> stores) {
+ for (DataStore sourceStore : stores) {
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.",
+ if (existingTemplatesInSourceStore == null ||
+
!existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
+ 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("Can not copy template [{}] from image store [{}],
as it returned a null install path.", tmplt.getUniqueName(),
- sourceStore.getName());
+ logger.warn("Cannot copy template [{}] from image store [{}];
install path is null.",
+ tmplt.getUniqueName(), sourceStore.getName());
continue;
}
+
storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
return true;
}
- logger.debug("Can't copy template [{}] from another image store.",
tmplt.getUniqueName());
return false;
}
+ private DataStore findTemplateInStores(VMTemplateVO tmplt, List<DataStore>
stores) {
+ for (DataStore store : stores) {
+ Map<String, TemplateProp> templates = listTemplate(store);
+ if (templates != null &&
templates.containsKey(tmplt.getUniqueName())) {
+ return store;
+ }
+ }
+ return null;
+ }
+
+ private boolean copyTemplateAcrossZones(DataStore sourceStore,
+ DataStore destStore,
+ VMTemplateVO tmplt) {
+ Long dstZoneId = destStore.getScope().getScopeId();
+ DataCenterVO dstZone = _dcDao.findById(dstZoneId);
+
+ if (dstZone == null) {
+ logger.warn("Destination zone [{}] not found for template [{}]",
+ dstZoneId, tmplt.getUniqueName());
+ return false;
+ }
+
+ try {
+ Long userId = CallContext.current().getCallingUserId();
+ return _tmpltMgr.copy(userId, tmplt, sourceStore, dstZone);
+ } catch (Exception e) {
+ logger.error("Failed to copy template [{}] from zone [{}] to zone
[{}]",
Review Comment:
The log message should end with a period for consistency with other logging
statements in this class.
```suggestion
logger.error("Failed to copy template [{}] from zone [{}] to
zone [{}].",
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +616,110 @@ 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) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ 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;
+ }
+
+ List<DataStore> storesInOtherZone =
_storeMgr.getImageStoresByZoneIds(otherZoneId);
+ logger.debug("Checking zone [{}] for template [{}]...",
otherZoneId, tmplt.getUniqueName());
+
+ if (storesInOtherZone == null || storesInOtherZone.isEmpty()) {
+ logger.debug("Zone [{}] has no image stores. Skipping.",
otherZoneId);
+ continue;
+ }
+
+ DataStore sourceStore = findTemplateInStores(tmplt,
storesInOtherZone);
+ if (sourceStore == null) {
+ logger.debug("Template [{}] not found 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(sourceStore, destStore, tmplt);
+ }
+
+ 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<DataStore> stores) {
+ for (DataStore sourceStore : stores) {
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.",
+ if (existingTemplatesInSourceStore == null ||
+
!existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
+ 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("Can not copy template [{}] from image store [{}],
as it returned a null install path.", tmplt.getUniqueName(),
- sourceStore.getName());
+ logger.warn("Cannot copy template [{}] from image store [{}];
install path is null.",
+ tmplt.getUniqueName(), sourceStore.getName());
continue;
}
+
storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
return true;
}
- logger.debug("Can't copy template [{}] from another image store.",
tmplt.getUniqueName());
return false;
}
+ private DataStore findTemplateInStores(VMTemplateVO tmplt, List<DataStore>
stores) {
+ for (DataStore store : stores) {
+ Map<String, TemplateProp> templates = listTemplate(store);
+ if (templates != null &&
templates.containsKey(tmplt.getUniqueName())) {
+ return store;
+ }
+ }
+ return null;
+ }
+
+ private boolean copyTemplateAcrossZones(DataStore sourceStore,
+ DataStore destStore,
+ VMTemplateVO tmplt) {
+ Long dstZoneId = destStore.getScope().getScopeId();
+ DataCenterVO dstZone = _dcDao.findById(dstZoneId);
+
+ if (dstZone == null) {
+ logger.warn("Destination zone [{}] not found for template [{}]",
+ dstZoneId, tmplt.getUniqueName());
+ return false;
+ }
+
+ try {
+ Long userId = CallContext.current().getCallingUserId();
Review Comment:
The variable 'userId' is only assigned values of primitive type and is never
'null', but it is declared with the boxed type 'Long'.
```suggestion
long userId = CallContext.current().getCallingUserId();
```
##########
engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java:
##########
@@ -183,4 +199,92 @@ public void
tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAno
Assert.assertTrue(result);
Mockito.verify(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(),
Mockito.any());
}
+
+ @Test
+ public void
tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone()
throws StorageUnavailableException, ResourceAllocationException {
+ Scope scopeMock = Mockito.mock(Scope.class);
+ Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
+ Mockito.doReturn(1L).when(scopeMock).getScopeId();
+
Mockito.doReturn(List.of(sourceStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(1L);
+
Mockito.doReturn(null).when(templateService).listTemplate(sourceStoreMock);
+ Mockito.doReturn(List.of(1L, 2L)).when(_dcDao).listAllIds();
+
+ DataStore otherZoneStoreMock = Mockito.mock(DataStore.class);
+
Mockito.doReturn(List.of(otherZoneStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(2L);
+
+ Map<String, TemplateProp> templatesInOtherZone = new HashMap<>();
+ templatesInOtherZone.put(tmpltMock.getUniqueName(), tmpltPropMock);
+
Mockito.doReturn(templatesInOtherZone).when(templateService).listTemplate(otherZoneStoreMock);
+
+ DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class);
+ Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L);
+ Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(),
Mockito.eq(tmpltMock), Mockito.eq(otherZoneStoreMock), Mockito.eq(dstZoneMock));
+
+ boolean result =
templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
+
+ Assert.assertTrue(result);
+ }
Review Comment:
The test needs to mock CallContext to avoid a NullPointerException. The
implementation code calls `CallContext.current().getCallingUserId()` in the
`copyTemplateAcrossZones` method (line 711 in TemplateServiceImpl.java), but
the test doesn't set up the CallContext. You should add mocking for
CallContext.current() to return a mock CallContext that provides a valid user
ID when getCallingUserId() is called.
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +616,110 @@ 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) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ 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;
+ }
+
+ List<DataStore> storesInOtherZone =
_storeMgr.getImageStoresByZoneIds(otherZoneId);
+ logger.debug("Checking zone [{}] for template [{}]...",
otherZoneId, tmplt.getUniqueName());
+
+ if (storesInOtherZone == null || storesInOtherZone.isEmpty()) {
+ logger.debug("Zone [{}] has no image stores. Skipping.",
otherZoneId);
+ continue;
+ }
+
+ DataStore sourceStore = findTemplateInStores(tmplt,
storesInOtherZone);
+ if (sourceStore == null) {
+ logger.debug("Template [{}] not found 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(sourceStore, destStore, tmplt);
+ }
+
+ 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<DataStore> stores) {
+ for (DataStore sourceStore : stores) {
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.",
+ if (existingTemplatesInSourceStore == null ||
+
!existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
+ 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("Can not copy template [{}] from image store [{}],
as it returned a null install path.", tmplt.getUniqueName(),
- sourceStore.getName());
+ logger.warn("Cannot copy template [{}] from image store [{}];
install path is null.",
+ tmplt.getUniqueName(), sourceStore.getName());
continue;
}
+
storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
return true;
}
- logger.debug("Can't copy template [{}] from another image store.",
tmplt.getUniqueName());
return false;
}
+ private DataStore findTemplateInStores(VMTemplateVO tmplt, List<DataStore>
stores) {
+ for (DataStore store : stores) {
+ Map<String, TemplateProp> templates = listTemplate(store);
+ if (templates != null &&
templates.containsKey(tmplt.getUniqueName())) {
+ return store;
+ }
+ }
+ return null;
+ }
Review Comment:
The findTemplateInStores method doesn't validate the template's install path
before returning the store. In searchAndCopyWithinZone (lines 676-679), there's
a check to ensure the install path is not null before attempting the copy.
Consider adding a similar validation in findTemplateInStores or in
copyTemplateAcrossZones to avoid attempting cross-zone copies with templates
that have null install paths. This would provide consistency with the same-zone
copy logic and potentially avoid failed copy attempts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
