Allow copy of templates from secondary storages of other zone when adding a new secondary storage #12296

harikrishna-patnala · 2025-12-18T12:42:53Z

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

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

…ding a new secondary storage

codecov · 2025-12-18T12:53:04Z

Codecov Report

❌ Patch coverage is 44.71545% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.24%. Comparing base (e8200a0) to head (fbbbbd0).
⚠️ Report is 6 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...tack/engine/orchestration/StorageOrchestrator.java	0.00%	32 Missing ⚠️
...api/command/admin/host/AddSecondaryStorageCmd.java	0.00%	14 Missing ⚠️
.../cloudstack/storage/image/TemplateServiceImpl.java	79.41%	11 Missing and 3 partials ⚠️
.../java/com/cloud/storage/ImageStoreDetailsUtil.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               4.20   #12296    +/-   ##
==========================================
  Coverage     16.23%   16.24%            
- Complexity    13371    13390    +19     
==========================================
  Files          5657     5657            
  Lines        498860   499008   +148     
  Branches      60543    60567    +24     
==========================================
+ Hits          81003    81057    +54     
- Misses       408824   408914    +90     
- Partials       9033     9037     +4

Flag	Coverage Δ
uitests	`4.00% <ø> (-0.01%)`	⬇️
unittests	`17.10% <44.71%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

harikrishna-patnala · 2025-12-19T07:28:39Z

@blueorangutan package

blueorangutan · 2025-12-19T07:30:03Z

@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.

blueorangutan · 2025-12-19T08:51:30Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16099

Copilot

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_STORAGES to COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES with 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.

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

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

shwstppr

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.

engine/components-api/src/main/java/com/cloud/storage/StorageManager.java

harikrishna-patnala · 2025-12-22T05:39:24Z

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.

@DaanHoogland also suggested similar change in the doc PR apache/cloudstack-documentation#611 (comment). I also agree to have better control while adding the secondary storage. This will need a parameter in the addSecondaryStorage API which should override the global setting value. We can also keep it in the UI with default value as the value in the global/zone setting.

If that makes sense, I'll work on the related changes.

abh1sar

Please have a look at copilot comments as well

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

abh1sar · 2025-12-23T02:34:42Z

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

+        for (DataStore store : stores) {
+            Map<String, TemplateProp> templates = listTemplate(store);
+            if (templates != null && templates.containsKey(tmplt.getUniqueName())) {
+                return store;


We can add the sourceTmpl.getInstallPath() != null check here as well

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

abh1sar · 2025-12-27T04:41:43Z

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.

@DaanHoogland also suggested similar change in the doc PR apache/cloudstack-documentation#611 (comment). I also agree to have better control while adding the secondary storage. This will need a parameter in the addSecondaryStorage API which should override the global setting value. We can also keep it in the UI with default value as the value in the global/zone setting.

If that makes sense, I'll work on the related changes.

I actually don't get why we need control over this while adding the secondary storage. Currently the templates are downloaded via url, and after this PR, they will be downloaded via the other secondary storages. I am not able to understand how a user would be affected and why they would need to know about the internal implementation while adding the secondary storage.
But It's good to have the config in case something doesn't work while complying the templates from other secondary storage to have a fallback.

DaanHoogland · 2025-12-28T10:39:52Z

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.

@DaanHoogland also suggested similar change in the doc PR apache/cloudstack-documentation#611 (comment). I also agree to have better control while adding the secondary storage. This will need a parameter in the addSecondaryStorage API which should override the global setting value. We can also keep it in the UI with default value as the value in the global/zone setting.
If that makes sense, I'll work on the related changes.

I actually don't get why we need control over this while adding the secondary storage. Currently the templates are downloaded via url, and after this PR, they will be downloaded via the other secondary storages. I am not able to understand how a user would be affected and why they would need to know about the internal implementation while adding the secondary storage. But It's good to have the config in case something doesn't work while complying the templates from other secondary storage to have a fallback.

@abh1sar , I think it it is rather a good question, asking for a justification of such a config. I think however I can formulate one or two.
Most obvious is that zones have a local site containing prefab templates that differ in local or according to local laws.
Consequently (in the future/some cases) one might want to have control over from which zone to copy templates.
Another reason may be that traffic from one zone to another may be slower than from the original sites (not likely but also far from improbable).
That all said, I am fine with making the behaviour default once proven to be adequate. Right now it gives the illusion of controllability, which is not genuine.

harikrishna-patnala · 2025-12-29T04:52:45Z

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.

@DaanHoogland also suggested similar change in the doc PR apache/cloudstack-documentation#611 (comment). I also agree to have better control while adding the secondary storage. This will need a parameter in the addSecondaryStorage API which should override the global setting value. We can also keep it in the UI with default value as the value in the global/zone setting.
If that makes sense, I'll work on the related changes.

I actually don't get why we need control over this while adding the secondary storage. Currently the templates are downloaded via url, and after this PR, they will be downloaded via the other secondary storages. I am not able to understand how a user would be affected and why they would need to know about the internal implementation while adding the secondary storage. But It's good to have the config in case something doesn't work while complying the templates from other secondary storage to have a fallback.

@abh1sar , I think it it is rather a good question, asking for a justification of such a config. I think however I can formulate one or two. Most obvious is that zones have a local site containing prefab templates that differ in local or according to local laws. Consequently (in the future/some cases) one might want to have control over from which zone to copy templates. Another reason may be that traffic from one zone to another may be slower than from the original sites (not likely but also far from improbable). That all said, I am fine with making the behaviour default once proven to be adequate. Right now it gives the illusion of controllability, which is not genuine.

Yes @DaanHoogland and also those URLs may not be available all the time as well. @abh1sar to answer your question, these config and options in the API are available to the operators only and not the users. Also the optional flag in the UI is set based on the value in the global setting. If it is set to false, then in the UI for the API parameter we set to false by default.

harikrishna-patnala · 2025-12-29T04:53:05Z

@blueorangutan package

blueorangutan · 2025-12-29T04:54:04Z

@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.

abh1sar · 2025-12-29T05:45:58Z

thanks @DaanHoogland and @harikrishna-patnala

blueorangutan · 2025-12-29T06:02:58Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16181

ui/src/views/infra/AddSecondaryStorage.vue

abh1sar

CLGTM

harikrishna-patnala · 2025-12-29T09:33:21Z

@blueorangutan package

blueorangutan · 2025-12-29T09:34:04Z

@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.

ui/public/locales/en.json

abh1sar · 2025-12-29T09:36:51Z

ui/public/locales/en.json

 "label.copy.consoleurl": "Copy console URL to clipboard",
 "label.copyid": "Copy ID",
 "label.copy.password": "Copy password",
 "label.copy.templates.from.other.secondary.storages": "Copy templates from other storages instead of fetching from URLs",


nit: Copy Templates from other storages instead of fetching from URLs

also, can't we combine this and the next one?

there are alignment issues in the zone wizard with the long text, so I've used shorter one there and added explanation in the header.

harikrishna-patnala · 2025-12-29T10:11:03Z

@blueorangutan package

blueorangutan · 2025-12-29T10:12:05Z

@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.

blueorangutan · 2025-12-29T10:47:36Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16189

blueorangutan · 2025-12-29T11:23:56Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16190

winterhazel

@harikrishna-patnala nice improvement. I've left some minor remarks.

winterhazel · 2025-12-29T16:09:00Z

api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java

    @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "the Zone ID for the secondary storage")
    protected Long zoneId;

+    @Parameter(name = ApiConstants.DETAILS, type = CommandType.MAP, description = "Details in key/value pairs using format details[i].keyname=keyvalue. Example: details[0].copytemplatesfromothersecondarystorages=true")


I usually prefer having parameters instead of passing things as a detail, so that it is automatically added to the API's documentation, but I'm ok if you keep it like this seeing that there's already documentation and a UI change for it

winterhazel · 2025-12-29T16:16:25Z

...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

+            } finally {
+                tryCleaningUpExecutor(dstZone.getId());
+                ThreadContext.clearAll();
+            }


I would place these 2 lines outside the finally so that there's less idented code

winterhazel · 2025-12-29T16:17:14Z

...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

+                if (!success) {
+                    result.setResult("Cross-zone template copy failed");
+                }
+            } catch (Exception e) {


Catch only the relevant exceptions (StorageUnavailableException and ResourceAllocationException) instead?

winterhazel · 2025-12-29T16:19:03Z

...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

    }

+    @Override
+    public Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore sourceStore, DataStore destStore) {


You can reduce the number of parameters in this method. There's no need to pass the sourceStore, you can get it via templateInfo.getDataStore()

winterhazel · 2025-12-29T16:26:30Z

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

-        for (DataStore sourceStore : storesInZone) {
+        Long destZoneId = destStore.getScope().getScopeId();
+
+        List<DataStore> storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId);


Move this to inside searchAndCopyWithinZone?

winterhazel · 2025-12-29T16:40:31Z

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

+            DataStore sourceStore = findImageStorageHavingTemplate(tmplt, storesInOtherZone);
+            if (sourceStore == null) {
+                logger.debug("Template [{}] not found 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;
+            }


In the following situation:

2 Zones: A, B

ACS is attempting to copy from A to B

A has 2 image stores: A1, A2

A1 has an inconsistent entry for the template with a null install path, A2 has a consistent entry

If findImageStorageHavingTemplate returns A1, ACS will not copy the template from image store A2 to zone B, even though it is possible.

It is better to loop through the secondary storages of zone A, search for a template entry with a install path that is not null, and return it. This way, you can also deduplicate the code in searchAndCopyWithinZone.

Allow copy of templates from secondary storages of other zone when ad…

1c448d3

…ding a new secondary storage

boring-cyborg bot added the component:storage label Dec 18, 2025

harikrishna-patnala added the status:needs-testing label Dec 18, 2025

harikrishna-patnala mentioned this pull request Dec 18, 2025

Allow copy of templates from secondary storages of same or other zone when adding a new secondary storage apache/cloudstack-documentation#611

Open

DaanHoogland requested review from Copilot, shwstppr and winterhazel December 19, 2025 09:18

Copilot started reviewing on behalf of DaanHoogland December 19, 2025 09:19 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

shwstppr reviewed Dec 19, 2025

View reviewed changes

engine/components-api/src/main/java/com/cloud/storage/StorageManager.java Show resolved Hide resolved

Add API param and UI changes on add secondary storage page

a6d8bf6

abh1sar reviewed Dec 23, 2025

View reviewed changes

harikrishna-patnala added 2 commits December 26, 2025 14:25

Make copy template across zones non blocking

b3232a4

Code fixes

7a2a9d5

boring-cyborg bot added component:api component:orchestration component:UI labels Dec 26, 2025

unused imports

1797f46

abh1sar reviewed Dec 29, 2025

View reviewed changes

ui/src/views/infra/AddSecondaryStorage.vue Outdated Show resolved Hide resolved

ui/src/views/infra/AddSecondaryStorage.vue Show resolved Hide resolved

Add copy template flag in zone wizard and remove NFS checks

3556d74

abh1sar approved these changes Dec 29, 2025

View reviewed changes

Fix UI

b3916fa

abh1sar approved these changes Dec 29, 2025

View reviewed changes

Label fixes

fbbbbd0

winterhazel reviewed Dec 29, 2025

View reviewed changes

Allow copy of templates from secondary storages of other zone when adding a new secondary storage #12296

Are you sure you want to change the base?

Allow copy of templates from secondary storages of other zone when adding a new secondary storage #12296

Conversation

harikrishna-patnala commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

codecov bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

harikrishna-patnala commented Dec 19, 2025

Uh oh!

blueorangutan commented Dec 19, 2025

Uh oh!

blueorangutan commented Dec 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harikrishna-patnala commented Dec 22, 2025

Uh oh!

abh1sar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abh1sar commented Dec 27, 2025

Uh oh!

DaanHoogland commented Dec 28, 2025

Uh oh!

harikrishna-patnala commented Dec 29, 2025

Uh oh!

harikrishna-patnala commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

abh1sar commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

Uh oh!

Uh oh!

abh1sar left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala commented Dec 29, 2025

Uh oh!

blueorangutan commented Dec 29, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala commented Dec 29, 2025

Uh oh!

harikrishna-patnala commented Dec 18, 2025 •

edited

Loading

codecov bot commented Dec 18, 2025 •

edited

Loading