Copilot commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3718983760
##########
server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java:
##########
@@ -338,6 +338,11 @@ protected boolean isZoneAndImageStoreAvailable(DataStore
imageStore, Long zoneId
return false;
}
+ if (_imgStoreDao.findById(imageStore.getId()).isReadonly()) {
+ logger.info("Image store [{}] is marked as read-only. Skip
downloading template to this image store.", imageStore);
+ return false;
+ }
Review Comment:
`_imgStoreDao.findById(imageStore.getId())` can return `null`, which would
cause a NullPointerException when calling `.isReadonly()`. Consider using the
new `DataStoreManager.isRemovedOrReadonly(imageStore)` helper (which already
handles null/missing stores), or explicitly null-check the VO before
dereferencing.
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/DataStoreManagerImpl.java:
##########
@@ -199,4 +207,18 @@ public Long getStoreZoneId(long storeId, DataStoreRole
role) {
} catch (CloudRuntimeException ignored) {}
return null;
}
+
+ @Override
+ public boolean isRemovedOrReadonly(DataStore store) {
+ ImageStoreVO storeVO = imageStoreDao.findById(store.getId());
+ if (storeVO == null) {
+ logger.debug("Could not find image store with id [{}], skipping
it.", store.getId());
+ return true;
+ }
+ if (storeVO.isReadonly()) {
+ logger.debug("Image store [{}] is read-only, skipping it.",
storeVO);
+ return true;
+ }
+ return false;
+ }
Review Comment:
The method is defined on `DataStoreManager` (generic for all store
roles/types), but the implementation assumes every `DataStore` is an Image
Store and queries `ImageStoreDao` by ID. If a heuristic rule returns a
non-image store, this will likely return `null` and be treated as
removed/read-only, incorrectly rejecting valid targets. Consider either (a)
changing the API to accept a `DataStoreRole` (or inspect store type/role) and
query the correct DAO, or (b) renaming/scoping the method to image stores only
(e.g., `isImageStoreRemovedOrReadOnly`) to avoid misleading usage.
--
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]
