Extend Preferred storage pool setting to Account, Domain and Cluster scope by nvazquez · Pull Request #9564 · apache/cloudstack

nvazquez · 2024-08-21T13:39:12Z

Description

This PR changes the preferred.storage.pool setting scope from Account to Account, Domain, and Cluster scope.

The Account value takes precedence over the Domain value, the Domain value takes precedence over the Cluster value, which takes precedence over the Global value.

Fixes: #8959

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

nvazquez · 2024-08-21T13:39:58Z

@blueorangutan package

blueorangutan · 2024-08-21T13:42:04Z

@nvazquez 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.

codecov · 2024-08-21T13:53:06Z

Codecov Report

❌ Patch coverage is 58.69565% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.77%. Comparing base (b535ae8) to head (45b2f17).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/java/com/cloud/storage/StorageManager.java	7.69%	12 Missing ⚠️
...om/cloud/deploy/DeploymentPlanningManagerImpl.java	45.45%	3 Missing and 3 partials ⚠️
...stack/engine/orchestration/VolumeOrchestrator.java	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9564      +/-   ##
============================================
+ Coverage     20.66%   20.77%   +0.10%     
- Complexity    21720    21915     +195     
============================================
  Files          6492     6496       +4     
  Lines        590728   591634     +906     
  Branches      72448    72594     +146     
============================================
+ Hits         122101   122886     +785     
- Misses       455355   455408      +53     
- Partials      13272    13340      +68

Flag	Coverage Δ
uitests	`3.88% <ø> (ø)`
unittests	`22.07% <58.69%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2024-08-21T14:37:15Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10733

weizhouapache · 2024-08-21T14:43:16Z

            true, ConfigKey.Scope.Zone);
-    ConfigKey<String> PreferredStoragePool = new ConfigKey<String>(String.class, "preferred.storage.pool", "Advanced", "",
-            "The UUID of preferred storage pool for allocation.", true, ConfigKey.Scope.Account, null);
+    ConfigKey<String> PreferredStoragePool = new ConfigKey<>(String.class, "preferred.storage.pool", "Advanced", "",


I wonder if we can have two configurations with same key

1 for global/zone/cluster

2 for global/domain/account

changing from Account to Cluster may be disliked by some users (maybe)

Thanks @weizhouapache - there is now an option to define multiple scopes per setting: https://docs.cloudstack.apache.org/en/4.22.0.0/installguide/configuration.html#granular-global-configuration-parameters. I will explore this

nvazquez · 2024-08-21T15:27:19Z

@blueorangutan test

blueorangutan · 2024-08-21T15:28:03Z

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2024-08-22T05:35:58Z

[SF] Trillian test result (tid-11126)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 48418 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9564-t11126-kvm-ol8.zip
Smoke tests completed. 139 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

github-actions · 2025-01-08T13:23:24Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2025-01-23T12:16:51Z

@nvazquez should this target 4.19?

nvazquez · 2025-01-23T13:11:42Z

@DaanHoogland the issue this PR fixes was on the 4.19.2 milestone but I think it shouldn't be there as 4.20.0 is already out and changing the scope of the setting would be conflicting, perhaps it should be targeted to 4.20 instead of main?

DaanHoogland · 2025-01-23T13:56:59Z

@nvazquez we can change to v21 as well as it is an enhancement...?

nvazquez · 2025-01-23T14:09:24Z

Sure @DaanHoogland fixed

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

nvazquez · 2026-10-28T02:17:11Z

@blueorangutan package

blueorangutan · 2026-10-28T02:18:05Z

@nvazquez 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.

blueorangutan · 2026-10-28T03:48:31Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19783

nvazquez · 2026-10-28T10:21:41Z

@blueorangutan test

blueorangutan · 2026-10-28T10:24:04Z

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

Damans227 · 2026-10-28T13:15:48Z

+        if (accountId != null) {
+            String uuid = PreferredStoragePool.valueInScope(ConfigKey.Scope.Account, accountId, true);
+            if (StringUtils.isNotBlank(uuid)) {
+                return new Pair<>(ConfigKey.Scope.Account, uuid);


@nvazquez if the account's pool isnt usable for this vm, should we still try the cluster and then the global pool? right now it stops at the account value, so the cluster setting is ignored

I think that may add some complexity to the logic, but can be a good enhancement. Would like to address it in a separate follow up PR, leaving this PR to the extension of the setting only

@nvazquez fine to move the cluster and domain fallback to a follow up, those are new. can we keep the global fallback here though? main still tries the global pool when the account pool isnt usable, so this would break setups that rely on it today

blueorangutan · 2026-10-29T03:39:01Z

[SF] Trillian test result (tid-17299)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58381 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9564-t17299-kvm-ol8.zip
Smoke tests completed. 157 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

JoaoJandre · 2026-10-29T12:57:33Z

+     */
+    static Pair<ConfigKey.Scope, String> getPreferredStoragePoolUuid(Long accountId, Long clusterId) {
+        if (accountId != null) {
+            String uuid = PreferredStoragePool.valueInScope(ConfigKey.Scope.Account, accountId, true);


Since we are already doing account scope, should we also add a domain scope?

Added, thanks

JoaoJandre · 2026-10-29T13:04:27Z

+     * Resolves the preferred storage pool UUID, checking the Account scope first, then the Cluster scope and finally the Global value.
+     * @return a pair of the scope where the value was found and the storage pool UUID, or null if it is not set at any level
+     */
+    static Pair<ConfigKey.Scope, String> getPreferredStoragePoolUuid(Long accountId, Long clusterId) {


I don't understand why implement this in the interface. We should move the implementation to StorageManagerImpl

Thanks, fixed

nvazquez · 2026-11-01T22:49:37Z

@blueorangutan package

blueorangutan · 2026-11-01T22:50:05Z

@nvazquez 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.

Copilot

🟡 Changes recommended

Pool resolution stops at an unsuitable account-level preference instead of falling back to a suitable global pool, leaving allocation to allocator ordering in both deployment planning and volume orchestration.

1 open issue

Skip unsuitable account pool and fall back to global preference · New

_{Review effort: Balanced}

+        if (preferredStoragePool == null) {
+            return Optional.empty();
        }
+        Optional<StoragePool> storagePool = getMatchingStoragePool(preferredStoragePool.second(), poolList);


blueorangutan · 2026-11-02T00:33:52Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19871

nvazquez · 2026-11-02T00:41:31Z

@blueorangutan test

blueorangutan · 2026-11-02T00:42:04Z

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-11-02T18:20:29Z

[SF] Trillian test result (tid-17439)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58829 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9564-t17439-kvm-ol8.zip
Smoke tests completed. 157 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

JoaoJandre

CLGTM. Did not test it

Preferred storage pool setting as a cluster setting

70cdd14

boring-cyborg Bot added the component:orchestration label Aug 21, 2024

weizhouapache reviewed Aug 21, 2024

View reviewed changes

Merge branch 'main' into 420-vm-primary-storage-allocation

6715fc6

github-actions Bot added the status:has-conflicts label Jan 8, 2025

Merge branch 'main' into 420-vm-primary-storage-allocation

bb4fc89

github-actions Bot removed the status:has-conflicts label Jan 23, 2025

DaanHoogland changed the base branch from main to 4.19 January 23, 2025 12:16

boring-cyborg Bot added component:agent component:build component:integration labels Jan 23, 2025

DaanHoogland changed the base branch from 4.19 to main January 23, 2025 12:16

nvazquez added this to the 4.21.0 milestone Jan 23, 2025

sureshanaparti added this to Apache CloudStack 4.21.0 Jun 5, 2025

sureshanaparti moved this to In Progress in Apache CloudStack 4.21.0 Jun 5, 2025

nvazquez modified the milestones: 4.21.0, 4.22.0 Jul 25, 2025

nvazquez removed this from Apache CloudStack 4.21.0 Jul 25, 2025

nvazquez marked this pull request as ready for review October 28, 2026 02:15

Copilot AI balanced review requested due to automatic review settings October 28, 2026 02:15

Copilot AI reviewed Oct 28, 2026

nvazquez requested review from Damans227, Pearl1594, sureshanaparti and weizhouapache October 28, 2026 02:16

github-actions Bot removed the status:has-conflicts label Oct 28, 2026

Damans227 reviewed Oct 28, 2026

View reviewed changes

JoaoJandre reviewed Oct 29, 2026

View reviewed changes

nvazquez added the type:enhancement label Nov 1, 2026

Address review comments

45b2f17

Copilot AI balanced review requested due to automatic review settings November 1, 2026 22:47

boring-cyborg Bot added the component:storage label Nov 1, 2026

Copilot started reviewing on behalf of nvazquez November 1, 2026 22:47

View session

nvazquez changed the title ~~Extend Preferred storage pool setting to cluster and account scope~~ Extend Preferred storage pool setting to Account, Domain and Cluster scope Nov 1, 2026

Copilot AI reviewed Nov 1, 2026

View reviewed changes

Comment thread server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java

if (preferredStoragePool == null) {

return Optional.empty();

}

Optional<StoragePool> storagePool = getMatchingStoragePool(preferredStoragePool.second(), poolList);

JoaoJandre approved these changes Nov 4, 2026

View reviewed changes

Conversation

nvazquez commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

nvazquez commented Aug 21, 2024

Uh oh!

blueorangutan commented Aug 21, 2024

Uh oh!

codecov Bot commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Aug 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Aug 21, 2024

Uh oh!

blueorangutan commented Aug 21, 2024

Uh oh!

blueorangutan commented Aug 22, 2024

Uh oh!

github-actions Bot commented Jan 8, 2025

Uh oh!

DaanHoogland commented Jan 23, 2025

Uh oh!

nvazquez commented Jan 23, 2025

Uh oh!

DaanHoogland commented Jan 23, 2025

Uh oh!

nvazquez commented Jan 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

nvazquez commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Nov 1, 2026

Uh oh!

blueorangutan commented Nov 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

blueorangutan commented Nov 2, 2026

nvazquez commented Aug 21, 2024 •

edited

Loading

codecov Bot commented Aug 21, 2024 •

edited

Loading