Allow copy of templates from secondary storages of other zone when adding a new secondary storage #12296
Conversation
…ding a new secondary storage
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12296 +/- ##
=========================================
Coverage 16.23% 16.24%
- Complexity 13371 13385 +14
=========================================
Files 5657 5657
Lines 498860 498913 +53
Branches 60543 60550 +7
=========================================
+ Hits 81003 81052 +49
- Misses 408824 408825 +1
- Partials 9033 9036 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16099 |
There was a problem hiding this comment.
Pull request overview
This PR enhances the template copying functionality when adding a new Secondary Storage server by allowing templates to be copied from Secondary Storages in other zones, in addition to the existing same-zone copying. If copying fails or is not possible, the system falls back to downloading templates from their original source URLs.
Key changes:
- Renamed configuration key from
COPY_PUBLIC_TEMPLATES_FROM_OTHER_STORAGEStoCOPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGESwith updated description - Refactored template copying logic into separate methods for same-zone and cross-zone operations
- Added comprehensive test coverage for the new cross-zone copying functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| engine/components-api/src/main/java/com/cloud/storage/StorageManager.java | Renamed and updated the configuration key to reflect the expanded functionality that now includes cross-zone template copying |
| server/src/main/java/com/cloud/storage/StorageManagerImpl.java | Updated reference to the renamed configuration key |
| engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java | Refactored tryCopyingTemplateToImageStore into separate methods for same-zone and cross-zone copy operations; added new helper methods searchAndCopyAcrossZones, searchAndCopyWithinZone, findTemplateInStores, and copyTemplateAcrossZones |
| engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java | Added test cases for cross-zone template copying scenarios including successful copy, missing destination zone, copy exception handling, and template not found cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @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); | ||
| } |
There was a problem hiding this 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.
| @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); | ||
| } |
There was a problem hiding this 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.
| return true; | ||
| } | ||
|
|
||
| logger.debug("Template [{}] not found in any image store of zone [{}]. Checking other zones", |
There was a problem hiding this comment.
The log message should end with a period for consistency with other logging statements in this class.
| logger.debug("Template [{}] not found in any image store of zone [{}]. Checking other zones", | |
| logger.debug("Template [{}] not found in any image store of zone [{}]. Checking other zones.", |
| DataCenterVO dstZone = _dcDao.findById(dstZoneId); | ||
|
|
||
| if (dstZone == null) { | ||
| logger.warn("Destination zone [{}] not found for template [{}]", |
There was a problem hiding this comment.
The log message should end with a period for consistency with other logging statements in this class.
| logger.warn("Destination zone [{}] not found for template [{}]", | |
| logger.warn("Destination zone [{}] not found for template [{}].", |
| 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 [{}]", |
There was a problem hiding this comment.
The log message should end with a period for consistency with other logging statements in this class.
| logger.error("Failed to copy template [{}] from zone [{}] to zone [{}]", | |
| logger.error("Failed to copy template [{}] from zone [{}] to zone [{}].", |
| 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; | ||
| } |
There was a problem hiding this 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.
| } | ||
|
|
||
| try { | ||
| Long userId = CallContext.current().getCallingUserId(); |
There was a problem hiding this comment.
The variable 'userId' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Long'.
| Long userId = CallContext.current().getCallingUserId(); | |
| long userId = CallContext.current().getCallingUserId(); |
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Will be a good addition. One comment around config change
Only wish there could be tighter control on the behaviour while adding the secondary storage, like passing a flag at the time of adding. But it can be covered in documentation that the operator must toggle the behaviour before adding the secondary storage.
|
|
||
| ConfigKey<Boolean> COPY_PUBLIC_TEMPLATES_FROM_OTHER_STORAGES = new ConfigKey<>(Boolean.class, "copy.public.templates.from.other.storages", | ||
| "Storage", "true", "Allow SSVMs to try copying public templates from one secondary storage to another instead of downloading them from the source.", | ||
| ConfigKey<Boolean> COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES = new ConfigKey<>(Boolean.class, "copy.templates.from.other.secondary.storages", |
There was a problem hiding this comment.
I think this will not replace the existing config but will add a new one. We may need a change in the upgrade path
3 participants
Description
When a new Secondary Storage is added, the Management Server attempts to make existing templates available on it. Templates are first copied from other Secondary Storages within the same zone (see the related PR #10363 for same zone improvements).
This PR extends the behavior to also allow templates to be copied from Secondary Storage servers in other zones. If the copying of template fails or not possible then management server falls back to the old behavior of downloading the templates from their original source URLs.
I've added a doc PR for this apache/cloudstack-documentation#611
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?