diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index b8912526fdf2..1dba33f93085 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -143,6 +143,8 @@ public interface TemplateManager { TemplateType validateTemplateType(BaseCmd cmd, boolean isAdmin, boolean isCrossZones); + DataStore verifyHeuristicRulesForZone(VMTemplateVO template, Long zoneId); + List getTemplateDisksOnImageStore(VirtualMachineTemplate template, DataStoreRole role, String configurationId); static Boolean getValidateUrlIsResolvableBeforeRegisteringTemplateValue() { 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 38e0d0d081cb..0ff1db0cd3a5 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 @@ -288,21 +288,41 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) { } } - protected boolean isSkipTemplateStoreDownload(VMTemplateVO template, Long zoneId) { - if (template.isPublicTemplate()) { + protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) { + Long zoneId = store.getScope().getScopeId(); + DataStore directedStore = _tmpltMgr.verifyHeuristicRulesForZone(template, zoneId); + if (directedStore != null && store.getId() != directedStore.getId()) { + logger.info("Template [{}] will not be download to image store [{}], as a heuristic rule is directing it to another store.", + template.getUniqueName(), store.getName()); return false; } + + if (template.isPublicTemplate()) { + logger.debug("Download of template [{}] to image store [{}] cannot be skipped, as it is public.", template.getUniqueName(), + store.getName()); + return true; + } + if (template.isFeatured()) { - return false; + logger.debug("Download of template [{}] to image store [{}] cannot be skipped, as it is featured.", template.getUniqueName(), + store.getName()); + return true; } + if (TemplateType.SYSTEM.equals(template.getTemplateType())) { - return false; + logger.debug("Download of template [{}] to image store [{}] cannot be skipped, as it is a system VM template.", + template.getUniqueName(),store.getName()); + return true; } + if (zoneId != null && _vmTemplateStoreDao.findByTemplateZone(template.getId(), zoneId, DataStoreRole.Image) == null) { - logger.debug("Template {} is not present on any image store for the zone ID: {}, its download cannot be skipped", template, zoneId); - return false; + logger.debug("Download of template [{}] to image store [{}] cannot be skipped, as it is not present on any image store of zone [{}].", + template.getUniqueName(), store.getName(), zoneId); + return true; } - return true; + + logger.info("Skipping download of template [{}] to image store [{}].", template.getUniqueName(), store.getName()); + return false; } @Override @@ -534,8 +554,7 @@ public void handleTemplateSync(DataStore store) { continue; } // if this is private template, skip sync to a new image store - if (isSkipTemplateStoreDownload(tmplt, zoneId)) { - logger.info("Skip sync downloading private template {} to a new image store", tmplt); + if (!shouldDownloadTemplateToStore(tmplt, store)) { continue; } 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 bc6f37b201a6..8de8c16d5398 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 @@ -18,9 +18,13 @@ */ package org.apache.cloudstack.storage.image; +import com.cloud.template.TemplateManager; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InjectMocks; @@ -43,42 +47,59 @@ public class TemplateServiceImplTest { @Mock TemplateDataStoreDao templateDataStoreDao; + @Mock + DataStore dataStoreMock; + + @Mock + TemplateManager templateManagerMock; + + @Mock + VMTemplateVO templateVoMock; + + @Mock + ZoneScope zoneScopeMock; + + @Before + public void setUp() { + Mockito.doReturn(3L).when(dataStoreMock).getId(); + Mockito.doReturn(4L).when(templateVoMock).getId(); + Mockito.doReturn(zoneScopeMock).when(dataStoreMock).getScope(); + } + + @Test + public void shouldDownloadTemplateToStoreTestSkipsTemplateDirectedToAnotherStorage() { + DataStore destinedStore = Mockito.mock(DataStore.class); + Mockito.doReturn(dataStoreMock.getId() + 1L).when(destinedStore).getId(); + Mockito.when(templateManagerMock.verifyHeuristicRulesForZone(templateVoMock, zoneScopeMock.getScopeId())).thenReturn(destinedStore); + Assert.assertFalse(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); + } + @Test - public void testIsSkipTemplateStoreDownloadPublicTemplate() { - VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class); - Mockito.when(templateVO.isPublicTemplate()).thenReturn(true); - Assert.assertFalse(templateService.isSkipTemplateStoreDownload(templateVO, 1L)); + public void shouldDownloadTemplateToStoreTestDownloadsPublicTemplate() { + Mockito.when(templateVoMock.isPublicTemplate()).thenReturn(true); + Assert.assertTrue(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); } @Test - public void testIsSkipTemplateStoreDownloadFeaturedTemplate() { - VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class); - Mockito.when(templateVO.isFeatured()).thenReturn(true); - Assert.assertFalse(templateService.isSkipTemplateStoreDownload(templateVO, 1L)); + public void shouldDownloadTemplateToStoreTestDownloadsFeaturedTemplate() { + Mockito.when(templateVoMock.isFeatured()).thenReturn(true); + Assert.assertTrue(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); } @Test - public void testIsSkipTemplateStoreDownloadSystemTemplate() { - VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class); - Mockito.when(templateVO.getTemplateType()).thenReturn(Storage.TemplateType.SYSTEM); - Assert.assertFalse(templateService.isSkipTemplateStoreDownload(templateVO, 1L)); + public void shouldDownloadTemplateToStoreTestDownloadsSystemTemplate() { + Mockito.when(templateVoMock.getTemplateType()).thenReturn(Storage.TemplateType.SYSTEM); + Assert.assertTrue(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); } @Test - public void testIsSkipTemplateStoreDownloadPrivateNoRefTemplate() { - VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class); - long id = 1L; - Mockito.when(templateVO.getId()).thenReturn(id); - Mockito.when(templateDataStoreDao.findByTemplateZone(id, id, DataStoreRole.Image)).thenReturn(null); - Assert.assertFalse(templateService.isSkipTemplateStoreDownload(templateVO, id)); + public void shouldDownloadTemplateToStoreTestDownloadsPrivateNoRefTemplate() { + Assert.assertTrue(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); } @Test - public void testIsSkipTemplateStoreDownloadPrivateExistingTemplate() { - VMTemplateVO templateVO = Mockito.mock(VMTemplateVO.class); - long id = 1L; - Mockito.when(templateVO.getId()).thenReturn(id); - Mockito.when(templateDataStoreDao.findByTemplateZone(id, id, DataStoreRole.Image)).thenReturn(Mockito.mock(TemplateDataStoreVO.class)); - Assert.assertTrue(templateService.isSkipTemplateStoreDownload(templateVO, id)); + public void shouldDownloadTemplateToStoreTestSkipsPrivateExistingTemplate() { + Mockito.when(templateDataStoreDao.findByTemplateZone(templateVoMock.getId(), zoneScopeMock.getScopeId(), DataStoreRole.Image)).thenReturn(Mockito.mock(TemplateDataStoreVO.class)); + Assert.assertFalse(templateService.shouldDownloadTemplateToStore(templateVoMock, dataStoreMock)); } } diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java index f7eb654141d4..011fb4419f67 100644 --- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java @@ -61,7 +61,6 @@ import org.apache.cloudstack.framework.async.AsyncRpcContext; import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.PublishScope; -import org.apache.cloudstack.secstorage.heuristics.HeuristicType; import org.apache.cloudstack.storage.command.TemplateOrVolumePostUploadCommand; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; @@ -308,7 +307,7 @@ protected void createTemplateWithinZones(TemplateProfile profile, VMTemplateVO t for (long zoneId : zonesIds) { - DataStore imageStore = verifyHeuristicRulesForZone(template, zoneId); + DataStore imageStore = templateMgr.verifyHeuristicRulesForZone(template, zoneId); if (imageStore == null) { List imageStores = getImageStoresThrowsExceptionIfNotFound(zoneId, profile); @@ -327,16 +326,6 @@ protected List getImageStoresThrowsExceptionIfNotFound(long zoneId, T return imageStores; } - protected DataStore verifyHeuristicRulesForZone(VMTemplateVO template, Long zoneId) { - HeuristicType heuristicType; - if (ImageFormat.ISO.equals(template.getFormat())) { - heuristicType = HeuristicType.ISO; - } else { - heuristicType = HeuristicType.TEMPLATE; - } - return heuristicRuleHelper.getImageStoreIfThereIsHeuristicRule(zoneId, heuristicType, template); - } - protected void standardImageStoreAllocation(List imageStores, VMTemplateVO template) { Set zoneSet = new HashSet(); Collections.shuffle(imageStores); @@ -432,7 +421,7 @@ public List doInTransaction(TransactionStatus } Long zoneId = zoneIdList.get(0); - DataStore imageStore = verifyHeuristicRulesForZone(template, zoneId); + DataStore imageStore = templateMgr.verifyHeuristicRulesForZone(template, zoneId); List payloads = new LinkedList<>(); if (imageStore == null) { diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 9f23bdef142d..a023aa4fa0f1 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -2327,6 +2327,17 @@ public TemplateType validateTemplateType(BaseCmd cmd, boolean isAdmin, boolean i return templateType; } + @Override + public DataStore verifyHeuristicRulesForZone(VMTemplateVO template, Long zoneId) { + HeuristicType heuristicType; + if (ImageFormat.ISO.equals(template.getFormat())) { + heuristicType = HeuristicType.ISO; + } else { + heuristicType = HeuristicType.TEMPLATE; + } + return heuristicRuleHelper.getImageStoreIfThereIsHeuristicRule(zoneId, heuristicType, template); + } + void validateDetails(VMTemplateVO template, Map details) { if (MapUtils.isEmpty(details)) { return; diff --git a/server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java b/server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java index 2a6d7af434a3..e2a97be469ff 100644 --- a/server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java +++ b/server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java @@ -49,7 +49,6 @@ import org.apache.cloudstack.framework.events.Event; import org.apache.cloudstack.framework.events.EventDistributor; import org.apache.cloudstack.framework.messagebus.MessageBus; -import org.apache.cloudstack.secstorage.heuristics.HeuristicType; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; import org.apache.cloudstack.storage.heuristics.HeuristicRuleHelper; @@ -339,7 +338,7 @@ public void createTemplateWithinZonesTestZoneIdsNotNullShouldNotCallListAllZones Mockito.when(templateProfileMock.getZoneIdList()).thenReturn(zoneIds); Mockito.doReturn(null).when(_adapter).getImageStoresThrowsExceptionIfNotFound(Mockito.any(Long.class), Mockito.any(TemplateProfile.class)); - Mockito.doReturn(null).when(_adapter).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); + Mockito.doReturn(null).when(_templateMgr).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); Mockito.doNothing().when(_adapter).standardImageStoreAllocation(Mockito.isNull(), Mockito.any(VMTemplateVO.class)); _adapter.createTemplateWithinZones(templateProfileMock, vmTemplateVOMock); @@ -355,7 +354,7 @@ public void createTemplateWithinZonesTestZoneDoesNotHaveActiveHeuristicRulesShou Mockito.when(templateProfileMock.getZoneIdList()).thenReturn(zoneIds); Mockito.doReturn(null).when(_adapter).getImageStoresThrowsExceptionIfNotFound(Mockito.any(Long.class), Mockito.any(TemplateProfile.class)); - Mockito.doReturn(null).when(_adapter).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); + Mockito.doReturn(null).when(_templateMgr).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); Mockito.doNothing().when(_adapter).standardImageStoreAllocation(Mockito.isNull(), Mockito.any(VMTemplateVO.class)); _adapter.createTemplateWithinZones(templateProfileMock, vmTemplateVOMock); @@ -371,7 +370,7 @@ public void createTemplateWithinZonesTestZoneWithHeuristicRuleShouldCallValidate List zoneIds = List.of(1L); Mockito.when(templateProfileMock.getZoneIdList()).thenReturn(zoneIds); - Mockito.doReturn(dataStoreMock).when(_adapter).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); + Mockito.doReturn(dataStoreMock).when(_templateMgr).verifyHeuristicRulesForZone(Mockito.any(VMTemplateVO.class), Mockito.anyLong()); Mockito.doNothing().when(_adapter).validateSecondaryStorageAndCreateTemplate(Mockito.any(List.class), Mockito.any(VMTemplateVO.class), Mockito.isNull()); _adapter.createTemplateWithinZones(templateProfileMock, vmTemplateVOMock); @@ -409,26 +408,6 @@ public void getImageStoresThrowsExceptionIfNotFoundTestNonEmptyImageStoreShouldN _adapter.getImageStoresThrowsExceptionIfNotFound(zoneId, templateProfileMock); } - @Test - public void verifyHeuristicRulesForZoneTestTemplateIsISOFormatShouldCheckForISOHeuristicType() { - VMTemplateVO vmTemplateVOMock = Mockito.mock(VMTemplateVO.class); - - Mockito.when(vmTemplateVOMock.getFormat()).thenReturn(ImageFormat.ISO); - _adapter.verifyHeuristicRulesForZone(vmTemplateVOMock, 1L); - - Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.ISO, vmTemplateVOMock); - } - - @Test - public void verifyHeuristicRulesForZoneTestTemplateNotISOFormatShouldCheckForTemplateHeuristicType() { - VMTemplateVO vmTemplateVOMock = Mockito.mock(VMTemplateVO.class); - - Mockito.when(vmTemplateVOMock.getFormat()).thenReturn(ImageFormat.QCOW2); - _adapter.verifyHeuristicRulesForZone(vmTemplateVOMock, 1L); - - Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.TEMPLATE, vmTemplateVOMock); - } - @Test public void isZoneAndImageStoreAvailableTestZoneIdIsNullShouldReturnFalse() { DataStore dataStoreMock = Mockito.mock(DataStore.class); diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 98b1c05dba83..00cce4953cd6 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -750,6 +750,26 @@ public void testDeleteTemplateWithTemplateType() { Assert.assertNull(type); } + @Test + public void verifyHeuristicRulesForZoneTestTemplateIsISOFormatShouldCheckForISOHeuristicType() { + VMTemplateVO vmTemplateVOMock = Mockito.mock(VMTemplateVO.class); + + Mockito.when(vmTemplateVOMock.getFormat()).thenReturn(Storage.ImageFormat.ISO); + templateManager.verifyHeuristicRulesForZone(vmTemplateVOMock, 1L); + + Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.ISO, vmTemplateVOMock); + } + + @Test + public void verifyHeuristicRulesForZoneTestTemplateNotISOFormatShouldCheckForTemplateHeuristicType() { + VMTemplateVO vmTemplateVOMock = Mockito.mock(VMTemplateVO.class); + + Mockito.when(vmTemplateVOMock.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); + templateManager.verifyHeuristicRulesForZone(vmTemplateVOMock, 1L); + + Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.TEMPLATE, vmTemplateVOMock); + } + @Configuration @ComponentScan(basePackageClasses = {TemplateManagerImpl.class}, includeFilters = {@ComponentScan.Filter(value = TestConfiguration.Library.class, type = FilterType.CUSTOM)},