winterhazel commented on code in PR #12296:
URL: https://github.com/apache/cloudstack/pull/12296#discussion_r2651290650
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/host/AddSecondaryStorageCmd.java:
##########
@@ -44,6 +49,9 @@ public class AddSecondaryStorageCmd extends BaseCmd {
@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")
Review Comment:
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
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java:
##########
@@ -653,4 +674,51 @@ public TemplateApiResult call() {
return result;
}
}
+
+ private class CrossZoneCopyTemplateTask implements
Callable<TemplateApiResult> {
+ private final long userId;
+ private final TemplateInfo sourceTmpl;
+ private final DataStore sourceStore;
+ private final DataCenterVO dstZone;
+ private final String logid;
+
+ CrossZoneCopyTemplateTask(long userId,
+ TemplateInfo sourceTmpl,
+ DataStore sourceStore,
+ DataCenterVO dstZone) {
+ this.userId = userId;
+ this.sourceTmpl = sourceTmpl;
+ this.sourceStore = sourceStore;
+ this.dstZone = dstZone;
+ this.logid = ThreadContext.get(LOGCONTEXTID);
+ }
+
+ @Override
+ public TemplateApiResult call() {
+ ThreadContext.put(LOGCONTEXTID, logid);
+ TemplateApiResult result;
+ VMTemplateVO template = templateDao.findById(sourceTmpl.getId());
+ try {
+ boolean success = templateManager.copy(userId, template,
sourceStore, dstZone);
+
+ result = new TemplateApiResult(sourceTmpl);
+ if (!success) {
+ result.setResult("Cross-zone template copy failed");
+ }
+ } catch (Exception e) {
+ logger.error("Exception while copying template [{}] from zone
[{}] to zone [{}]",
+ template,
+ sourceStore.getScope().getScopeId(),
+ dstZone.getId(),
+ e);
+ result = new TemplateApiResult(sourceTmpl);
+ result.setResult(e.getMessage());
+ } finally {
+ tryCleaningUpExecutor(dstZone.getId());
+ ThreadContext.clearAll();
+ }
Review Comment:
I would place these 2 lines outside the finally so that there's less idented
code
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java:
##########
@@ -308,6 +321,14 @@ public Future<TemplateApiResult>
orchestrateTemplateCopyToImageStore(TemplateInf
return submit(destStore.getScope().getScopeId(), new
CopyTemplateTask(source, destStore));
}
+ @Override
+ public Future<TemplateApiResult>
orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore
sourceStore, DataStore destStore) {
Review Comment:
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()`
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +615,118 @@ protected void
tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore
}
protected boolean tryCopyingTemplateToImageStore(VMTemplateVO tmplt,
DataStore destStore) {
- Long zoneId = destStore.getScope().getScopeId();
- List<DataStore> storesInZone =
_storeMgr.getImageStoresByZoneIds(zoneId);
- for (DataStore sourceStore : storesInZone) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
+ return true;
+ }
+
+ logger.debug("Template [{}] not found in any image store of zone [{}].
Checking other zones.",
+ tmplt.getUniqueName(), destZoneId);
+
+ return searchAndCopyAcrossZones(tmplt, destStore, destZoneId);
+ }
+
+ private boolean searchAndCopyAcrossZones(VMTemplateVO tmplt, DataStore
destStore, Long destZoneId) {
+ List<Long> allZoneIds = _dcDao.listAllIds();
+ for (Long otherZoneId : allZoneIds) {
+ if (otherZoneId.equals(destZoneId)) {
+ continue;
+ }
+
+ List<DataStore> storesInOtherZone =
_storeMgr.getImageStoresByZoneIds(otherZoneId);
+ logger.debug("Checking zone [{}] for template [{}]...",
otherZoneId, tmplt.getUniqueName());
+
+ if (storesInOtherZone == null || storesInOtherZone.isEmpty()) {
+ logger.debug("Zone [{}] has no image stores. Skipping.",
otherZoneId);
+ continue;
+ }
+
+ 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;
+ }
Review Comment:
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`.
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -615,28 +615,118 @@ protected void
tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore
}
protected boolean tryCopyingTemplateToImageStore(VMTemplateVO tmplt,
DataStore destStore) {
- Long zoneId = destStore.getScope().getScopeId();
- List<DataStore> storesInZone =
_storeMgr.getImageStoresByZoneIds(zoneId);
- for (DataStore sourceStore : storesInZone) {
+ Long destZoneId = destStore.getScope().getScopeId();
+
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
Review Comment:
Move this to inside `searchAndCopyWithinZone`?
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java:
##########
@@ -653,4 +674,51 @@ public TemplateApiResult call() {
return result;
}
}
+
+ private class CrossZoneCopyTemplateTask implements
Callable<TemplateApiResult> {
+ private final long userId;
+ private final TemplateInfo sourceTmpl;
+ private final DataStore sourceStore;
+ private final DataCenterVO dstZone;
+ private final String logid;
+
+ CrossZoneCopyTemplateTask(long userId,
+ TemplateInfo sourceTmpl,
+ DataStore sourceStore,
+ DataCenterVO dstZone) {
+ this.userId = userId;
+ this.sourceTmpl = sourceTmpl;
+ this.sourceStore = sourceStore;
+ this.dstZone = dstZone;
+ this.logid = ThreadContext.get(LOGCONTEXTID);
+ }
+
+ @Override
+ public TemplateApiResult call() {
+ ThreadContext.put(LOGCONTEXTID, logid);
+ TemplateApiResult result;
+ VMTemplateVO template = templateDao.findById(sourceTmpl.getId());
+ try {
+ boolean success = templateManager.copy(userId, template,
sourceStore, dstZone);
+
+ result = new TemplateApiResult(sourceTmpl);
+ if (!success) {
+ result.setResult("Cross-zone template copy failed");
+ }
+ } catch (Exception e) {
Review Comment:
Catch only the relevant exceptions (`StorageUnavailableException` and
`ResourceAllocationException`) instead?
--
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]
