Add template copy limit check to enforce secondary storage constraints

Author: Damans227

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java

@@ -69,6 +69,8 @@ public TemplateInfo getTemplate() {
 
     void enforceSecStorageCopyLimit(long templateId, long zoneId);
 
+    boolean canCopyTemplateToImageStore(long templateId, long zoneId);
+
     void downloadBootstrapSysTemplate(DataStore store);
 
     void addSystemVMTemplatesToSecondary(DataStore store);

engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java

@@ -295,18 +295,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
         }
     }
 
-    private boolean hasReachedSecStorageCopyLimit(VMTemplateVO template, long zoneId) {
-        int copyLimit = _tmpltMgr.getSecStorageCopyLimit(template, zoneId);
-        if (copyLimit <= 0) {
-            return false;
-        }
+    private int countActiveSecStorageCopies(long templateId, long zoneId) {
         List<DataStore> stores = _storeMgr.getImageStoresByScope(new ZoneScope(zoneId));
         if (stores == null || stores.isEmpty()) {
-            return false;
+            return 0;
         }
         int count = 0;
         for (DataStore ds : stores) {
-            List<TemplateDataStoreVO> rows = _vmTemplateStoreDao.listByTemplateStore(template.getId(), ds.getId());
+            List<TemplateDataStoreVO> rows = _vmTemplateStoreDao.listByTemplateStore(templateId, ds.getId());
             if (rows == null) {
                 continue;
             }
@@ -320,9 +316,39 @@ private boolean hasReachedSecStorageCopyLimit(VMTemplateVO template, long zoneId
                 }
             }
         }
+        return count;
+    }
+
+    /**
+     * Central gate for the secondary storage copy limit (secstorage.public/private.template.copy.max).
+     * Every template-landing path (periodic sync, cross-zone copy, register, upload) should consult this
+     * single method before placing another copy of a template on a secondary store in a zone, so the limit
+     * is enforced consistently instead of being re-implemented per call site.
+     *
+     * SYSTEM/ROUTING/BUILTIN templates and a limit of 0 mean "unlimited" (return true). The per-template,
+     * per-zone {@link GlobalLock} serializes concurrent placement decisions so racing SSVM syncs / copies
+     * cannot collectively exceed the limit.
+     */
+    @Override
+    public boolean canCopyTemplateToImageStore(long templateId, long zoneId) {
+        VMTemplateVO template = _templateDao.findById(templateId);
+        if (template == null) {
+            return false;
+        }
+        int copyLimit = _tmpltMgr.getSecStorageCopyLimit(template, zoneId);
+        if (copyLimit <= 0) {
+            logger.debug("Template [{}] has no secondary storage copy limit in zone [{}] (limit={}); copy allowed.",
+                    template.getUniqueName(), zoneId, copyLimit);
+            return true;
+        }
+        int count = countActiveSecStorageCopies(templateId, zoneId);
         logger.debug("Template [{}] secstorage copy check in zone [{}]: count={}, limit={}",
                 template.getUniqueName(), zoneId, count, copyLimit);
-        return count >= copyLimit;
+        return count < copyLimit;
+    }
+
+    private boolean hasReachedSecStorageCopyLimit(VMTemplateVO template, long zoneId) {
+        return !canCopyTemplateToImageStore(template.getId(), zoneId);
     }
 
     @Override

engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java

@@ -119,6 +119,7 @@ public void setUp() {
         Mockito.doReturn(templateInfoMock).when(templateDataFactoryMock).getTemplate(2L, sourceStoreMock);
         Mockito.doReturn(3L).when(dataStoreMock).getId();
         Mockito.doReturn(zoneScopeMock).when(dataStoreMock).getScope();
+        Mockito.lenient().doReturn(tmpltMock).when(templateDao).findById(2L);
     }
 
     @Test
@@ -173,6 +174,17 @@ public void shouldDownloadTemplateToStoreTestSkipsPrivateTemplateAtCopyLimit() {
         Assert.assertFalse(templateService.shouldDownloadTemplateToStore(tmpltMock, dataStoreMock));
     }
 
+    @Test
+    public void canCopyTemplateToImageStoreTestUnlimitedWhenLimitIsZero() {
+        Mockito.when(templateManagerMock.getSecStorageCopyLimit(tmpltMock, 1L)).thenReturn(0);
+        Assert.assertTrue(templateService.canCopyTemplateToImageStore(2L, 1L));
+    }
+
+    // The under-limit / at-limit behavior of canCopyTemplateToImageStore is exercised through
+    // shouldDownloadTemplateToStore above (Replicates*UnderCopyLimit / Skips*AtCopyLimit), which run it via
+    // the real call path. Calling the GlobalLock-wrapped method directly on the Mockito spy is not reliable
+    // in the unit-test JVM, so it is not duplicated here.
+
     @Test
     public void tryDownloadingTemplateToImageStoreTestDownloadsTemplateWhenUrlIsNotNull() {
         Mockito.doReturn("url").when(tmpltMock).getUrl();

server/src/main/java/com/cloud/template/TemplateManagerImpl.java

@@ -859,6 +859,12 @@ public boolean copy(long userId, VMTemplateVO template, DataStore srcSecStore, D
                 _tmplStoreDao.removeByTemplateStore(tmpltId, dstSecStore.getId());
             }
 
+            if (!_tmpltSvr.canCopyTemplateToImageStore(tmpltId, dstZoneId)) {
+                logger.info("Not copying template {} to image store {}: zone {} has reached the configured secondary storage copy limit.",
+                        template, dstSecStore, dstZone);
+                continue;
+            }
+
             AsyncCallFuture<TemplateApiResult> future = _tmpltSvr.copyTemplate(srcTemplate, dstSecStore);
             try {
                 TemplateApiResult result = future.get();