Prevent downloading templates to secondary storages that are in read-only - #13023
Prevent downloading templates to secondary storages that are in read-only#13023GaOrtiga wants to merge 7 commits into
read-only#13023Conversation
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17486 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13023 +/- ##
=============================================
- Coverage 16.26% 4.14% -12.12%
=============================================
Files 5665 405 -5260
Lines 500530 33025 -467505
Branches 60787 5898 -54889
=============================================
- Hits 81411 1370 -80041
+ Misses 410027 31479 -378548
+ Partials 9092 176 -8916
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17514 |
|
@GaOrtiga could you have a look at the test failures? |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17553 |
There was a problem hiding this comment.
Pull request overview
Fixes template download/allocation behavior so Secondary Storage marked read-only is skipped during template download and sync flows.
Changes:
- Add read-only image store checks when selecting/validating secondary storage for template creation/allocation.
- Prevent template sync/download decisions from targeting read-only image stores.
- Extend unit tests to account for the new read-only filtering behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java | Skips template allocation/download to image stores flagged as read-only. |
| server/src/test/java/com/cloud/template/HypervisorTemplateAdapterTest.java | Updates tests/mocks to support new ImageStoreDao read-only checks. |
| engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java | Skips downloads during template sync when the target image store is read-only; fixes a log message typo. |
| engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java | Adds coverage for read-only store behavior and adjusts setup for new DAO usage. |
| engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java | Avoids downloading templates to read-only image stores at the driver level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sureshanaparti thanks for the review, I applied yours and copilot's suggestions. |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17785 |
|
@GaOrtiga a failing unit test: can you have a look? |
|
@GaOrtiga , should you be re-added to the collaborators list? |
|
@DaanHoogland Yes. I have created the PR. |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18114 |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18166 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16693)
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
server/src/main/java/org/apache/cloudstack/storage/heuristics/HeuristicRuleHelper.java:277
- The exception message says the store is 'marked as read-only', but
isRemovedOrReadonly(...)also returns true when the store doesn't exist/was removed (perDataStoreManagerImpl). Update the message to reflect both cases (e.g., removed/not found or read-only) so operators get an accurate reason for the rejection.
if (dataStoreManager.isRemovedOrReadonly(dataStore)) {
throw new CloudRuntimeException(String.format("DataStore [{}] returned by heuristic rule is marked as read-only.", dataStore));
}
engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/DataStoreManagerImpl.java:44
- This logger can be
private static finalto avoid per-instance allocation and match common logging best practices in the codebase. Changing to a constant logger also discourages accidental reassignment.
protected Logger logger = LogManager.getLogger(DataStoreManagerImpl.class);
engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreManager.java:64
- Method name casing is inconsistent with common Java camelCase for 'read-only' (typically
ReadOnly). Consider renaming toisRemovedOrReadOnlyfor clarity and consistency.
boolean isRemovedOrReadonly(DataStore store);
server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java:344
- This newly added read-only filtering path in
HypervisorTemplateAdapter.isZoneAndImageStoreAvailable(...)doesn't appear to have a direct unit test assertingfalsewhen the store is read-only. Add a test that stubs the returnedImageStoreVOtoisReadonly() == trueand verifies the method returnsfalse.
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;
}
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.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
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?