Copilot commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115360115
##########
engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java:
##########
@@ -104,6 +106,12 @@ public class TemplateServiceImplTest {
@Mock
DataCenterDao _dcDao;
+ @Mock
+ ImageStoreDao imageStore;
+
+ @Mock
+ ImageStoreVO imageMock;
+
Review Comment:
The mock field name `imageStore` is an ImageStoreDao, which is easy to
confuse with DataStore/ImageStore entities used throughout the test. Renaming
it to something like `imageStoreDao` would make the test intent clearer and
reduce ambiguity.
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java:
##########
@@ -175,10 +178,13 @@ public void createAsync(DataStore dataStore, DataObject
data, AsyncCompletionCal
AsyncCallbackDispatcher<BaseImageStoreDriverImpl, DownloadAnswer>
caller = AsyncCallbackDispatcher.create(this);
caller.setContext(context);
if (data.getType() == DataObjectType.TEMPLATE) {
-
caller.setCallback(caller.getTarget().createTemplateAsyncCallback(null, null));
- if (logger.isDebugEnabled()) {
- logger.debug("Downloading template to data store {}",
dataStore);
+ if (dataStoreDao.findById(dataStore.getId()).isReadonly()) {
+ logger.debug("Template [{}] will not be downloaded to image
store [{}] because this store is marked as read-only.", data.getName(),
+ dataStore.getName());
+ return;
}
Review Comment:
In the TEMPLATE branch, returning early when the image store is read-only
bypasses the async completion path: the passed callback is never completed and
the template_store_ref entry created by TemplateServiceImpl can remain stuck in
a non-terminal state. Also, ImageStoreDao.findById(...) can return null, which
would cause an NPE when calling isReadonly(). Consider handling a null VO and,
for the read-only case, completing the callback with a failed CreateCmdResult
(and/or transitioning the DataObject state to OperationFailed) instead of just
returning.
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,16 @@ public void handleSysTemplateDownload(HypervisorType
hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template,
DataStore store) {
+ if (dataStoreDao.findById(store.getId()).isReadonly()) {
+ logger.debug("Template [{}] will not be downloaded to image store
[{}] because this store is marked as read-only.", template.getUniqueName(),
+ store.getName());
+ return false;
+ }
Review Comment:
dataStoreDao.findById(store.getId()) can return null
(GenericDaoBase.findById returns null when not found). Calling isReadonly()
without a null-check can cause an NPE during template sync if the image store
DB record is missing. Consider checking for null and deciding on a safe default
(e.g., skip download) with an explanatory log message.
##########
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:
This change adds new behavior (skipping read-only image stores) but the
existing unit tests around isZoneAndImageStoreAvailable don’t include a case
that asserts a read-only store is rejected. Adding a dedicated test where the
ImageStoreVO returned by _imgStoreDao is readonly and verifying the method
returns false would prevent regressions.
##########
server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java:
##########
@@ -458,11 +463,13 @@ public void
isZoneAndImageStoreAvailableTestZoneIsDisabledShouldReturnFalse() {
@Test
public void
isZoneAndImageStoreAvailableTestImageStoreDoesNotHaveEnoughCapacityShouldReturnFalse()
{
DataStore dataStoreMock = Mockito.mock(DataStore.class);
+ ImageStoreVO ImageStoreVOMock = Mockito.mock(ImageStoreVO.class);
Long zoneId = 1L;
Set<Long> zoneSet = null;
boolean isTemplatePrivate = false;
DataCenterVO dataCenterVOMock = Mockito.mock(DataCenterVO.class);
+
Mockito.when(_imgStoreDao.findById(Mockito.anyLong())).thenReturn(ImageStoreVOMock);
Review Comment:
Variable name `ImageStoreVOMock` starts with an uppercase letter, which is
inconsistent with Java local-variable naming conventions used elsewhere in this
test file. Rename it to a lowerCamelCase name (e.g., `imageStoreVoMock`) to
improve readability and consistency.
##########
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()) {
Review Comment:
_imgStoreDao.findById(imageStore.getId()) may return null
(GenericDaoBase.findById returns null when not found). Calling isReadonly()
unconditionally here can trigger an NPE if the image store record is
missing/removed. Consider null-checking the returned ImageStoreVO and treating
a missing record as unavailable (with a log) before checking isReadonly().
--
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]
