You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "GaOrtiga (via GitHub)" <gi...@apache.org> on 2026/04/14 13:36:17 UTC
[PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
GaOrtiga opened a new pull request, #13023:
URL: https://github.com/apache/cloudstack/pull/13023
### Description
When a Secondary Storage is marked as read-only, download of new templates is still performed in it. This behavior has been fixed, making it so that read-only storages are ignored during the template download process.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
I put a secondary in the zone in the read-only state and verified that it no longer would be selected for the download.
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4543626991
@DaanHoogland Yes. I have created the [PR](https://github.com/apache/cloudstack/pull/13246).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4633446580
@GaOrtiga a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4593410776
@GaOrtiga a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115324998
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java:
##########
@@ -126,6 +127,8 @@ public abstract class BaseImageStoreDriverImpl implements ImageStoreDriver {
AgentManager agentMgr;
@Inject
DataStoreManager dataStoreManager;
+ @Inject
+ ImageStoreDao dataStoreDao;
Review Comment:
```suggestion
ImageStoreDao imageStoreDao;
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5185313304
<b>[SF] Trillian test result (tid-16693)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 53064 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13023-t16693-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5175528890
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4280686220
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17553
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4268887502
@GaOrtiga could you have a look at the test failures?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4244546017
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3341428394
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
Review Comment:
the `isRemovedOrReadonly` already sends logs
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
Review Comment:
the `isRemovedOrReadonly` method already sends logs
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4257624842
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17514
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3396414933
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java:
##########
@@ -178,13 +175,10 @@ public void createAsync(DataStore dataStore, DataObject data, AsyncCompletionCal
AsyncCallbackDispatcher<BaseImageStoreDriverImpl, DownloadAnswer> caller = AsyncCallbackDispatcher.create(this);
caller.setContext(context);
if (data.getType() == DataObjectType.TEMPLATE) {
- if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
- DownloadAnswer ans = new DownloadAnswer("Data store is removed or in read-only mode", VMTemplateStorageResourceAssoc.Status.UNKNOWN);
- caller.complete(ans);
- return;
- }
caller.setCallback(caller.getTarget().createTemplateAsyncCallback(null, null));
- logger.debug("Downloading template [{}] to data store [{}].", data.getName(), dataStore.getName());
+ if (logger.isDebugEnabled()) {
Review Comment:
@winterhazel I was only undoing the changes I had made on this file, as they are no longer necessary. So I'm no longer touching that part of the code
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4407025505
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17785
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4401834938
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
+ }
+
Long zoneId = store.getScope().getScopeId();
DataStore directedStore = _tmpltMgr.verifyHeuristicRulesForZone(template, zoneId);
Review Comment:
Got it. Sure.
That check also has other exceptions thrown, meaning this will fix an already existing issue, which is a plus.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3721948587
##########
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 one makes sense, implementing
##########
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:
this one doesnt
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3336395919
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
Review Comment:
We could have a log here
##########
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 (dataStoreManager.isRemovedOrReadonly(dataStore)) {
Review Comment:
Doesn't it make more sense to filter out the removed/read-only secondary storages in the methods responsible for choosing a secondary storage for migration/template creation from volume instead of here in `createAsync`?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115313321
##########
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()) {
Review Comment:
```suggestion
ImageStoreVO storeVO = imageStoreDao.findById(store.getId());
if (storeVO == null) {
// add log
return false;
}
if (storeVO.isReadonly()) {
```
npe for removed image store, can check store exists before _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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115307351
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -163,6 +164,8 @@ public class TemplateServiceImpl implements TemplateService {
TemplateDataFactory imageFactory;
@Inject
StorageOrchestrationService storageOrchestrator;
+ @Inject
+ ImageStoreDao dataStoreDao;
Review Comment:
```suggestion
ImageStoreDao imageStoreDao;
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5175519261
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5581949268
@GaOrtiga :
```
10:09:16 [ERROR] Errors:
10:09:16 [ERROR] HypervisorTemplateAdapterTest.unnecessary Mockito stubbings ? UnnecessaryStubbing
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5759463066
@DaanHoogland thanks for the heads up
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4280274985
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4406887344
@GaOrtiga a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4485577753
@GaOrtiga a failing unit test:
```
16:01:25 [ERROR] Failures:
16:01:25 [ERROR] TemplateServiceImplTest.shouldDownloadTemplateToStoreTestSkipsWhenStorageIsReadOnly:179
```
can you have a look?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5581408580
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5812985057
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4244558234
@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4400892384
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
+ }
+
Long zoneId = store.getScope().getScopeId();
DataStore directedStore = _tmpltMgr.verifyHeuristicRulesForZone(template, zoneId);
Review Comment:
the throw is in the rule check this calls, not here. if the rule points at a read-only store the rule check now throws, so syncing any other store in that zone stops at the first template and skips the rest. should we catch it here and return false?
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -163,6 +164,8 @@ public class TemplateServiceImpl implements TemplateService {
TemplateDataFactory imageFactory;
@Inject
StorageOrchestrationService storageOrchestrator;
+ @Inject
+ ImageStoreDao imageStoreDao;
Review Comment:
@GaOrtiga is this used anywhere? looks like it can go
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3336437202
##########
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 (dataStoreManager.isRemovedOrReadonly(dataStore)) {
Review Comment:
Doesn't it make more sense to filter out the removed/read-only secondary storages in the methods responsible for choosing a secondary storage for migration/template creation from volume/template copy instead of here in `createAsync`?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4633839479
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18166
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115325756
##########
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:
same here
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4245644756
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13023?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.15%. Comparing base ([`8eb162c`](https://app.codecov.io/gh/apache/cloudstack/commit/8eb162cb996d2020447c1cdce69a43e04c0a5e69?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0d2f3d6`](https://app.codecov.io/gh/apache/cloudstack/commit/0d2f3d6864a892fea63cd5b374ddab91399677e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (8eb162c) and HEAD (0d2f3d6). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (8eb162c) | HEAD (0d2f3d6) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.20 #13023 +/- ##
=============================================
- Coverage 16.26% 4.15% -12.11%
=============================================
Files 5665 404 -5261
Lines 500530 32966 -467564
Branches 60787 5893 -54894
=============================================
- Hits 81411 1370 -80041
+ Misses 410027 31420 -378607
+ Partials 9092 176 -8916
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13023/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.15% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/13023?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3396376655
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java:
##########
@@ -178,13 +175,10 @@ public void createAsync(DataStore dataStore, DataObject data, AsyncCompletionCal
AsyncCallbackDispatcher<BaseImageStoreDriverImpl, DownloadAnswer> caller = AsyncCallbackDispatcher.create(this);
caller.setContext(context);
if (data.getType() == DataObjectType.TEMPLATE) {
- if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
- DownloadAnswer ans = new DownloadAnswer("Data store is removed or in read-only mode", VMTemplateStorageResourceAssoc.Status.UNKNOWN);
- caller.complete(ans);
- return;
- }
caller.setCallback(caller.getTarget().createTemplateAsyncCallback(null, null));
- logger.debug("Downloading template [{}] to data store [{}].", data.getName(), dataStore.getName());
+ if (logger.isDebugEnabled()) {
Review Comment:
Please remove this `if`
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5581582538
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19146
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5759478431
@GaOrtiga a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115325756
##########
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:
same here
can have a common method to check image store exists or not and then _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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r3115332312
##########
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:
use common method if possible
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4593395185
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4244665961
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17486
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4633433095
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5581436482
@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-5760143513
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19297
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4400390296
##########
server/src/main/java/org/apache/cloudstack/storage/heuristics/HeuristicRuleHelper.java:
##########
@@ -272,6 +272,10 @@ public DataStore interpretHeuristicRule(String rule, HeuristicType heuristicType
"returning a valid UUID.", scriptReturn, rule));
}
+ if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
+ throw new CloudRuntimeException(String.format("DataStore [{}] returned by heuristic rule is marked as read-only.", dataStore));
Review Comment:
fixed it, thanks
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4402965217
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -163,6 +164,8 @@ public class TemplateServiceImpl implements TemplateService {
TemplateDataFactory imageFactory;
@Inject
StorageOrchestrationService storageOrchestrator;
+ @Inject
+ ImageStoreDao imageStoreDao;
Review Comment:
removed it
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4594100787
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18114
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4257451611
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4257444998
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4280280301
@GaOrtiga a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4541839116
@GaOrtiga , should you be re-added to the collaborators list?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4406871914
@sureshanaparti thanks for the review, I applied yours and copilot's suggestions.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-4406872931
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4284708474
##########
server/src/main/java/org/apache/cloudstack/storage/heuristics/HeuristicRuleHelper.java:
##########
@@ -272,6 +272,10 @@ public DataStore interpretHeuristicRule(String rule, HeuristicType heuristicType
"returning a valid UUID.", scriptReturn, rule));
}
+ if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
+ throw new CloudRuntimeException(String.format("DataStore [{}] returned by heuristic rule is marked as read-only.", dataStore));
Review Comment:
should this be %s instead of {}? String.format leaves the {} as is so the store never shows up in the error
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
+ }
+
Long zoneId = store.getScope().getScopeId();
DataStore directedStore = _tmpltMgr.verifyHeuristicRulesForZone(template, zoneId);
Review Comment:
what happens to the template sync when the rule points at a read-only store? looks like this now throws mid loop and the rest of the templates for that store get skipped
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on code in PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#discussion_r4400418974
##########
engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:
##########
@@ -295,10 +298,14 @@ public void handleSysTemplateDownload(HypervisorType hostHyper, Long dcId) {
}
protected boolean shouldDownloadTemplateToStore(VMTemplateVO template, DataStore store) {
+ if (_storeMgr.isRemovedOrReadonly(store)) {
+ return false;
+ }
+
Long zoneId = store.getScope().getScopeId();
DataStore directedStore = _tmpltMgr.verifyHeuristicRulesForZone(template, zoneId);
Review Comment:
Im not sure I understand the question. There is no exception being thrown, it just returns `false` (same behavior that happens in other situations of the same method).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prevent downloading templates to secondary storages that are in `read-only` [cloudstack]
Posted by "sudo87 (via GitHub)" <gi...@apache.org>.
sudo87 commented on PR #13023:
URL: https://github.com/apache/cloudstack/pull/13023#issuecomment-6487575105
@GaOrtiga looks like few more unused imports:
```
[ERROR] /home/runner/work/cloudstack/cloudstack/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java:67:8: Unused import - org.apache.cloudstack.storage.datastore.db.ImageStoreDao. [UnusedImports]
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org