volume: enforce storage pool disable threshold when creating a volume on an explicit pool - #14071
Conversation
d94ed21 to
a9ef05a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It includes unrelated repository governance/workflow configuration changes that should be split out or explicitly justified to avoid surprising operational impact.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a correctness gap in the admin-only storageid volume creation path by enforcing storage pool disable-threshold / capacity checks (consistent with the normal allocation path), preventing an explicit-pool create from bypassing configured safety limits.
Changes:
- Add a
StorageManager#storagePoolHasEnoughSpace(...)check toVolumeApiServiceImpl#createVolumeOnStoragePoolbefore dispatching async volume creation. - Add unit tests covering the “disable threshold crossed” rejection and “enough space” success cases.
- Update repository/workflow governance configuration (branch protection rules + workflow activation guard).
File summaries
| File | Description |
|---|---|
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java |
Enforces storage pool capacity/disable-threshold checks when creating a volume on an explicitly specified pool. |
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java |
Adds unit tests validating rejection when thresholds are crossed and success when capacity is sufficient. |
.github/workflows/daily-issue-triage.lock.yml |
Adds a job-level repository guard to prevent activation outside apache/cloudstack. |
.asf.yaml |
Expands branch protection coverage to maintenance branches and adds required reviews/status checks. |
Review details
Suppressed comments (1)
.asf.yaml:84
- This required-status-checks block is also unrelated to the volume disable-threshold fix described in the PR. If it’s intended, it should be proposed separately (or at least called out in the PR description), since it can affect mergeability across branches.
required_status_checks:
- "Build / build"
- "pre-commit / Run pre-commit"
- "UI Build / build"
- "Simulator CI / build"
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14071 +/- ##
=========================================
Coverage 19.77% 19.78%
- Complexity 19989 19994 +5
=========================================
Files 6371 6371
Lines 575899 575909 +10
Branches 70495 70497 +2
=========================================
+ Hits 113912 113959 +47
+ Misses 449563 449515 -48
- Partials 12424 12435 +11
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:
|
… on an explicit pool createVolume with an admin-specified storageid bypassed the normal allocator path and thus skipped the pool's disable-threshold and allocated-capacity checks (checkUsagedSpace / checkPoolforSpace). createVolumeOnStoragePool now calls StorageManager.storagePoolHasEnoughSpace before creating the volume, matching the check already done for volume migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
a9ef05a to
d3dc85d
Compare
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
🟡 Changes recommended
The updated method still performs a redundant second datastore lookup after validating the first instance, which should be removed to avoid inconsistency and simplify the flow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
🟡 Changes recommended
createVolumeOnStoragePool currently contains an unclosed if block/missing exception around the new capacity check, which breaks compilation and fails to enforce the intended rejection behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
71432cf to
8cdd0f1
Compare
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ debian. SL-JID 19134 |
There was a problem hiding this comment.
🟢 Approval recommended
The change aligns the explicit-pool creation path with existing capacity/disable-threshold enforcement and includes targeted unit tests for both failure and success cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19136 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19137 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Description
createVolumeaccepts an admin-onlystorageidparameter to create the volume directly on a specified storage pool. Unlike the normal allocation path (which goes throughStoragePoolAllocator→StorageManager.storagePoolHasEnoughSpace→checkUsagedSpace), this path fetched the pool directly by ID and only validated pool status, zone match, and disk-offering compatibility — it never checked the pool's storage capacity disable threshold (storage.capacity.disablethreshold) or allocated-capacity disable threshold before creating the volume there.This means an admin passing
storageidcould push a storage pool past its configured disable threshold, defeating the safety mechanism the threshold exists for.This is not considered a security issue since the
storageidparameter is restricted toRoleType.Admin(added in 4.22.1), but it is a correctness bug.Fix
VolumeApiServiceImpl#createVolumeOnStoragePoolnow callsStorageManager#storagePoolHasEnoughSpace(the same check used bymigrateVolume) before handing the volume off toVolumeService#createVolumeAsync, and rejects the request with anInvalidParameterValueExceptionif the pool has crossed its disable threshold or doesn't have enough space.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity: Minor
Screenshots (if appropriate):
N/A
How Has This Been Tested?
Added unit tests to
VolumeApiServiceImplTest:testCreateVolumeOnStoragePool_DisableThresholdCrossed_ShouldThrow— verifies anInvalidParameterValueExceptionis thrown andcreateVolumeAsyncis never invoked when the pool has crossed its disable threshold.testCreateVolumeOnStoragePool_EnoughSpace_ShouldCreateVolume— verifies the volume is created normally when the pool has enough space.Ran the full
VolumeApiServiceImplTestsuite (161 tests, 0 failures).🤖 Generated with Claude Code