winterhazel commented on code in PR #12296:
URL: https://github.com/apache/cloudstack/pull/12296#discussion_r2653574957
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -643,45 +641,54 @@ private boolean searchAndCopyAcrossZones(VMTemplateVO
tmplt, DataStore destStore
continue;
}
- DataStore sourceStore = findImageStorageHavingTemplate(tmplt,
storesInOtherZone);
- if (sourceStore == null) {
- logger.debug("Template [{}] not found in any image store of
zone [{}].",
+ TemplateObject sourceTmpl = findUsableTemplate(tmplt,
storesInOtherZone);
+ if (sourceTmpl == null) {
+ logger.debug("Template [{}] not found with a valid install
path 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;
- }
-
logger.info("Template [{}] found in zone [{}]. Initiating
cross-zone copy to zone [{}].",
tmplt.getUniqueName(), otherZoneId, destZoneId);
- return copyTemplateAcrossZones(sourceStore, destStore, tmplt);
+ return copyTemplateAcrossZones(destStore, sourceTmpl);
}
- logger.debug("Template [{}] was not found in any zone. Cannot perform
zone-to-zone copy.",
- tmplt.getUniqueName());
+ logger.debug("Template [{}] was not found in any zone. Cannot perform
zone-to-zone copy.", tmplt.getUniqueName());
return false;
}
- private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore
destStore, List<DataStore> stores) {
- for (DataStore sourceStore : stores) {
+ protected TemplateObject findUsableTemplate(VMTemplateVO tmplt,
List<DataStore> imageStores) {
+ for (DataStore store : imageStores) {
+ TemplateObject tmpl = (TemplateObject)
_templateFactory.getTemplate(tmplt.getId(), store);
+ if (tmpl == null) {
+ continue;
+ }
+
+ if (tmpl.getInstallPath() == null) {
+ logger.debug("Template [{}] found in image store [{}] but
install path is null. Skipping.",
+ tmplt.getUniqueName(), store.getName());
+ continue;
+ }
+ return tmpl;
Review Comment:
It would be nice to keep the check with `listTemplate` before returning the
template to ensure that the template does in fact exist in the storage (and
that its store reference entry is not inconsistent)
```
Map<String, TemplateProp> templates = listTemplate(store);
if (templates == null || !templates.containsKey(tmplt.getUniqueName())) {
continue;
}
```
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -643,45 +641,54 @@ private boolean searchAndCopyAcrossZones(VMTemplateVO
tmplt, DataStore destStore
continue;
}
- DataStore sourceStore = findImageStorageHavingTemplate(tmplt,
storesInOtherZone);
- if (sourceStore == null) {
- logger.debug("Template [{}] not found in any image store of
zone [{}].",
+ TemplateObject sourceTmpl = findUsableTemplate(tmplt,
storesInOtherZone);
+ if (sourceTmpl == null) {
+ logger.debug("Template [{}] not found with a valid install
path 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;
- }
-
logger.info("Template [{}] found in zone [{}]. Initiating
cross-zone copy to zone [{}].",
tmplt.getUniqueName(), otherZoneId, destZoneId);
- return copyTemplateAcrossZones(sourceStore, destStore, tmplt);
+ return copyTemplateAcrossZones(destStore, sourceTmpl);
}
- logger.debug("Template [{}] was not found in any zone. Cannot perform
zone-to-zone copy.",
- tmplt.getUniqueName());
+ logger.debug("Template [{}] was not found in any zone. Cannot perform
zone-to-zone copy.", tmplt.getUniqueName());
return false;
}
- private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore
destStore, List<DataStore> stores) {
- for (DataStore sourceStore : stores) {
+ protected TemplateObject findUsableTemplate(VMTemplateVO tmplt,
List<DataStore> imageStores) {
+ for (DataStore store : imageStores) {
+ TemplateObject tmpl = (TemplateObject)
_templateFactory.getTemplate(tmplt.getId(), store);
+ if (tmpl == null) {
+ continue;
+ }
+
+ if (tmpl.getInstallPath() == null) {
+ logger.debug("Template [{}] found in image store [{}] but
install path is null. Skipping.",
+ tmplt.getUniqueName(), store.getName());
+ continue;
+ }
+ return tmpl;
+ }
+ return null;
+ }
+
+ private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore
destStore) {
+ Long destZoneId = destStore.getScope().getScopeId();
+ List<DataStore> storesInSameZone =
_storeMgr.getImageStoresByZoneIds(destZoneId);
+ for (DataStore sourceStore : storesInSameZone) {
Review Comment:
This for loop can be replaced with a call to `findUsableTemplate`. If it
returns null, then return false. If it returns a template, then call
`storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl,
destStore);` and return true
--
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]
