Copilot commented on code in PR #12877:
URL: https://github.com/apache/cloudstack/pull/12877#discussion_r2989421717
##########
server/src/main/java/com/cloud/template/TemplateAdapterBase.java:
##########
@@ -212,12 +204,15 @@ protected boolean isZoneAndImageStoreAvailable(DataStore
imageStore, Long zoneId
* {@link TemplateProfile#getZoneIdList()}.
*/
protected void postUploadAllocation(List<DataStore> imageStores,
VMTemplateVO template, List<TemplateOrVolumePostUploadCommand> payloads) {
Review Comment:
This method JavaDoc no longer matches the behavior: allocation is now
governed by replica-limit config keys (public/private can both be limited), not
strictly by template visibility (private=random vs public=all). Please update
the comment to reflect the new replica-limit semantics (and that the limits are
zone-scoped).
##########
engine/components-api/src/main/java/com/cloud/template/TemplateManager.java:
##########
@@ -64,6 +66,18 @@ public interface TemplateManager {
true,
ConfigKey.Scope.Global);
+ ConfigKey<Integer> PublicTemplateSecStorageCopy = new
ConfigKey<Integer>("Advanced", Integer.class,
+ PublicTemplateSecStorageCopyCK, "0",
+ "Maximum number of secondary storage pools to which a public
template is copied. " +
+ "0 means copy to all secondary storage pools (default behavior).",
+ true, ConfigKey.Scope.Zone);
+
+ ConfigKey<Integer> PrivateTemplateSecStorageCopy = new
ConfigKey<Integer>("Advanced", Integer.class,
+ PrivateTemplateSecStorageCopyCK, "1",
+ "Maximum number of secondary storage pools to which a private
template is copied. " +
+ "Default is 1 to preserve existing behavior.",
+ true, ConfigKey.Scope.Zone);
Review Comment:
The config key descriptions don’t mention that the limits are evaluated
per-zone (and the keys themselves are zone-scoped). Consider clarifying the
wording (e.g., “per zone”) so operators understand how replica limits are
applied across multiple zones/image stores.
##########
server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java:
##########
@@ -308,16 +311,18 @@ protected List<DataStore>
getImageStoresThrowsExceptionIfNotFound(long zoneId, T
}
protected void standardImageStoreAllocation(List<DataStore> imageStores,
VMTemplateVO template) {
- Set<Long> zoneSet = new HashSet<Long>();
+ int replicaLimit = isPrivateTemplate(template)
+ ? TemplateManager.PrivateTemplateSecStorageCopy.value()
+ : TemplateManager.PublicTemplateSecStorageCopy.value();
Collections.shuffle(imageStores);
- validateSecondaryStorageAndCreateTemplate(imageStores, template,
zoneSet);
+ validateSecondaryStorageAndCreateTemplate(imageStores, template, new
HashMap<>(), replicaLimit);
Review Comment:
`standardImageStoreAllocation` derives `replicaLimit` using
`ConfigKey#value()`, which only reads the global value; these keys are
`Scope.Zone`, so per-zone settings won’t take effect. Since `imageStores` is
zone-specific (from `getImageStoresByZoneIds(zoneId)`), derive the zone ID from
the list (e.g., first store) and use `valueIn(zoneId)` (or compute the limit
per-store inside the loop).
##########
server/src/main/java/com/cloud/template/TemplateAdapterBase.java:
##########
@@ -212,12 +204,15 @@ protected boolean isZoneAndImageStoreAvailable(DataStore
imageStore, Long zoneId
* {@link TemplateProfile#getZoneIdList()}.
*/
protected void postUploadAllocation(List<DataStore> imageStores,
VMTemplateVO template, List<TemplateOrVolumePostUploadCommand> payloads) {
- Set<Long> zoneSet = new HashSet<>();
+ int replicaLimit = isPrivateTemplate(template)
+ ? TemplateManager.PrivateTemplateSecStorageCopy.value()
+ : TemplateManager.PublicTemplateSecStorageCopy.value();
+ Map<Long, Integer> zoneCopyCount = new HashMap<>();
Review Comment:
`PublicTemplateSecStorageCopy`/`PrivateTemplateSecStorageCopy` are declared
with `ConfigKey.Scope.Zone`, but here `value()` is used, which always reads the
*global* value (see `ConfigKey#value()`), ignoring per-zone configuration.
Compute the limit per image store/zone using `valueIn(zoneId)` (or
`valueInScope(Scope.Zone, zoneId)`), and avoid reading it once before iterating
image stores across multiple zones.
##########
server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java:
##########
@@ -294,7 +294,10 @@ protected void createTemplateWithinZones(TemplateProfile
profile, VMTemplateVO t
List<DataStore> imageStores =
getImageStoresThrowsExceptionIfNotFound(zoneId, profile);
standardImageStoreAllocation(imageStores, template);
} else {
- validateSecondaryStorageAndCreateTemplate(List.of(imageStore),
template, null);
+ int replicaLimit = isPrivateTemplate(template)
+ ? TemplateManager.PrivateTemplateSecStorageCopy.value()
+ : TemplateManager.PublicTemplateSecStorageCopy.value();
Review Comment:
`PublicTemplateSecStorageCopy`/`PrivateTemplateSecStorageCopy` are
`Scope.Zone` config keys, but `value()` always reads the global value, so
zone-level overrides will never apply. In this loop the zone ID is already
available (`zoneId`), so use `valueIn(zoneId)` when deriving `replicaLimit`.
```suggestion
?
TemplateManager.PrivateTemplateSecStorageCopy.valueIn(zoneId)
:
TemplateManager.PublicTemplateSecStorageCopy.valueIn(zoneId);
```
--
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]
