Skip to content

Extend Preferred storage pool setting to Account, Domain and Cluster scope #9564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nvazquez wants to merge 6 commits into
base: main
Choose a base branch
from
+331 −30
Open

Extend Preferred storage pool setting to Account, Domain and Cluster scope #9564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
70cdd14
Preferred storage pool setting as a cluster setting
nvazquez Aug 21, 2024
6715fc6
Merge branch 'main' into 420-vm-primary-storage-allocation
nvazquez Nov 8, 2024
bb4fc89
Merge branch 'main' into 420-vm-primary-storage-allocation
nvazquez Jan 23, 2025
b3dde53
Merge branch 'main' into 420-vm-primary-storage-allocation
nvazquez Oct 27, 2026
2acce94
Refactor after rebasing
nvazquez Oct 28, 2026
45b2f17
Address review comments
nvazquez Nov 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.commons.lang3.StringUtils;

import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
Expand Down Expand Up @@ -177,7 +178,11 @@ public interface StorageManager extends StorageService {
"If set to false, the disk is created with a disk provisioning type supported by the pool. Default value is false, and this is currently supported for VMware only.",
true, ConfigKey.Scope.Zone);
ConfigKey<String> PreferredStoragePool = new ConfigKey<>(String.class, "preferred.storage.pool", "Advanced", "",

@weizhouapache weizhouapache Aug 21, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@nvazquez nvazquez Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"The UUID of preferred storage pool for allocation.", true, ConfigKey.Scope.Account, null);
"The UUID of preferred storage pool for allocation. It can be set at Account, Domain or Cluster level: " +
"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. " +
"The preferred storage pool is only used if it is suitable for the allocation, otherwise it is ignored.",
true, List.of(ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Cluster), null);

ConfigKey<Boolean> MountDisabledStoragePool = new ConfigKey<>(Boolean.class,
"mount.disabled.storage.pool",
Expand Down Expand Up @@ -447,4 +452,25 @@ void connectHostsToPool(DataStore primaryStore, List<Long> hostIds, Scope scope,
static ConfigKey<Boolean> getMountDisabledStoragePool() {
return MountDisabledStoragePool;
}

/**
* 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) {

@JoaoJandre JoaoJandre Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nvazquez nvazquez Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

if (accountId != null) {
String uuid = PreferredStoragePool.valueInScope(ConfigKey.Scope.Account, accountId, true);

@JoaoJandre JoaoJandre Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nvazquez nvazquez Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, thanks

if (StringUtils.isNotBlank(uuid)) {
return new Pair<>(ConfigKey.Scope.Account, uuid);

@Damans227 Damans227 Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@nvazquez nvazquez Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Damans227 Damans227 Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

}
}
if (clusterId != null) {
String uuid = PreferredStoragePool.valueInScope(ConfigKey.Scope.Cluster, clusterId, true);
if (StringUtils.isNotBlank(uuid)) {
return new Pair<>(ConfigKey.Scope.Cluster, uuid);
}
}
String uuid = PreferredStoragePool.value();
return StringUtils.isNotBlank(uuid) ? new Pair<>(ConfigKey.Scope.Global, uuid) : null;
}
}
28 changes: 11 additions & 17 deletions ...stration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.cloud.agent.AgentManager;
import com.cloud.deploy.DeploymentClusterPlanner;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.storage.StorageManagerImpl;
import com.cloud.storage.clvm.ClvmPoolManager;
import com.cloud.resourcelimit.ReservationHelper;
import com.cloud.storage.DiskOfferingVO;
Expand Down Expand Up @@ -396,23 +397,16 @@ private Optional<StoragePool> getMatchingStoragePool(String preferredPoolId, Lis
.findFirst();
}

private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm) {
String accountStoragePoolUuid = null;
if (vm != null) {
accountStoragePoolUuid = StorageManager.PreferredStoragePool.valueIn(vm.getAccountId());
}
Optional<StoragePool> storagePool = getMatchingStoragePool(accountStoragePoolUuid, poolList);

if (storagePool.isPresent()) {
String storagePoolToString = getReflectOnlySelectedFields(storagePool.get());
logger.debug("The storage pool [{}] was specified for this account [{}] and will be used for allocation.", storagePoolToString, vm.getAccountId());

} else {
String globalStoragePoolUuid = StorageManager.PreferredStoragePool.value();
storagePool = getMatchingStoragePool(globalStoragePoolUuid, poolList);
storagePool.ifPresent(pool -> logger.debug("The storage pool [{}] was specified in the Global Settings and will be used for allocation.",
getReflectOnlySelectedFields(pool)));
protected Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm, Long clusterId) {
Long accountId = vm != null ? vm.getAccountId() : null;
Long domainId = vm != null ? vm.getDomainId() : null;
Pair<ConfigKey.Scope, String> preferredStoragePool = StorageManagerImpl.getPreferredStoragePoolUuid(accountId, domainId, clusterId);
if (preferredStoragePool == null) {
return Optional.empty();
}
Optional<StoragePool> storagePool = getMatchingStoragePool(preferredStoragePool.second(), poolList);
storagePool.ifPresent(pool -> logger.debug("The storage pool [{}] was specified at the [{}] level (account [{}], cluster [{}]) and will be used for allocation.",
getReflectOnlySelectedFields(pool), preferredStoragePool.first(), accountId, clusterId));
return storagePool;
}

Expand All @@ -433,7 +427,7 @@ public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Lo
if (poolList != null && !poolList.isEmpty()) {
StorageUtil.traceLogStoragePools(poolList, logger, "pools to choose from: ");
// Check if the preferred storage pool can be used. If yes, use it.
Optional<StoragePool> storagePool = getPreferredStoragePool(poolList, vm);
Optional<StoragePool> storagePool = getPreferredStoragePool(poolList, vm, clusterId);
logger.trace("we have a preferred pool: {}", storagePool.isPresent());

StoragePool storage;
Expand Down
95 changes: 95 additions & 0 deletions ...tion/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.lang.reflect.Field;

Expand All @@ -36,6 +37,8 @@
import com.cloud.storage.ScopeType;
import com.cloud.storage.DataStoreRole;
import com.cloud.storage.Storage;
import com.cloud.storage.StorageManager;
import com.cloud.storage.StoragePool;
import com.cloud.storage.Volume;
import com.cloud.storage.Volume.Type;
import com.cloud.storage.VolumeVO;
Expand All @@ -62,6 +65,7 @@
import org.apache.cloudstack.framework.config.ConfigDepot;
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
import org.apache.cloudstack.secret.PassphraseVO;
import org.apache.cloudstack.secret.dao.PassphraseDao;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
Expand All @@ -79,6 +83,7 @@
import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;
import org.mockito.stubbing.Answer;
import org.springframework.test.util.ReflectionTestUtils;

import static org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService.VolumeAllocationAlgorithm;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -970,4 +975,94 @@ private Field findField(Class<?> clazz, String fieldName) {
return null;
}

private Optional<StoragePool> runGetPreferredStoragePoolTest(String accountPoolUuid, String clusterPoolUuid, String globalPoolUuid,
List<StoragePool> poolList, VirtualMachine vm, Long clusterId) {
Object previousDepot = ReflectionTestUtils.getField(ConfigKey.class, "s_depot");
ConfigDepotImpl configDepotImpl = Mockito.mock(ConfigDepotImpl.class);
String key = StorageManager.PreferredStoragePool.key();
Mockito.lenient().when(configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Account, 2L)).thenReturn(accountPoolUuid);
Mockito.lenient().when(configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Cluster, 3L)).thenReturn(clusterPoolUuid);
Mockito.lenient().when(configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Global, null)).thenReturn(globalPoolUuid);
ConfigKey.init(configDepotImpl);
try {
return volumeOrchestrator.getPreferredStoragePool(poolList, vm, clusterId);
} finally {
ConfigKey.init((ConfigDepotImpl) previousDepot);
}
}

private StoragePool mockStoragePoolWithUuid(String uuid) {
StoragePool pool = Mockito.mock(StoragePool.class);
Mockito.lenient().when(pool.getUuid()).thenReturn(uuid);
return pool;
}

private VirtualMachine mockVmWithAccount() {
VirtualMachine vm = Mockito.mock(VirtualMachine.class);
Mockito.when(vm.getAccountId()).thenReturn(2L);
return vm;
}

@Test
public void getPreferredStoragePoolAccountTakesPrecedenceOverCluster() {
StoragePool accountPool = mockStoragePoolWithUuid("account-pool");
StoragePool clusterPool = mockStoragePoolWithUuid("cluster-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest("account-pool", "cluster-pool", null,
List.of(clusterPool, accountPool), mockVmWithAccount(), 3L);

Assert.assertEquals(Optional.of(accountPool), result);
}

@Test
public void getPreferredStoragePoolUsesClusterValueWhenAccountNotSet() {
StoragePool clusterPool = mockStoragePoolWithUuid("cluster-pool");
StoragePool globalPool = mockStoragePoolWithUuid("global-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest(null, "cluster-pool", "global-pool",
List.of(globalPool, clusterPool), mockVmWithAccount(), 3L);

Assert.assertEquals(Optional.of(clusterPool), result);
}

@Test
public void getPreferredStoragePoolIgnoresClusterValueWhenNoClusterGiven() {
StoragePool clusterPool = mockStoragePoolWithUuid("cluster-pool");
StoragePool globalPool = mockStoragePoolWithUuid("global-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest(null, "cluster-pool", "global-pool",
List.of(clusterPool, globalPool), mockVmWithAccount(), null);

Assert.assertEquals(Optional.of(globalPool), result);
}

@Test
public void getPreferredStoragePoolWithoutVmUsesClusterValue() {
StoragePool clusterPool = mockStoragePoolWithUuid("cluster-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest("account-pool", "cluster-pool", null,
List.of(clusterPool), null, 3L);

Assert.assertEquals(Optional.of(clusterPool), result);
}

@Test
public void getPreferredStoragePoolNotInSuitablePools() {
StoragePool otherPool = mockStoragePoolWithUuid("other-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest(null, "cluster-pool", null,
List.of(otherPool), mockVmWithAccount(), 3L);

Assert.assertTrue(result.isEmpty());
}

@Test
public void getPreferredStoragePoolNotSet() {
StoragePool otherPool = mockStoragePoolWithUuid("other-pool");

Optional<StoragePool> result = runGetPreferredStoragePoolTest(null, null, null,
List.of(otherPool), mockVmWithAccount(), 3L);

Assert.assertTrue(result.isEmpty());
}
}
25 changes: 13 additions & 12 deletions server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.cloud.api.ApiDBUtils;
import com.cloud.gpu.dao.VgpuProfileDao;
import com.cloud.resource.ResourceState;
import com.cloud.storage.StorageManagerImpl;
import org.apache.cloudstack.affinity.AffinityGroupDomainMapVO;
import org.apache.cloudstack.affinity.AffinityGroupProcessor;
import org.apache.cloudstack.affinity.AffinityGroupService;
Expand Down Expand Up @@ -1924,7 +1925,7 @@ private boolean tryToFindPotentialPoolsToAlocateVolume(VirtualMachineProfile vmP
if (suitablePools != null && !suitablePools.isEmpty()) {
logger.debug("StoragePoolAllocator [{}] found {} suitable pools to allocate volume [{}] necessary to deploy VM [{}].",
allocator.getClass().getSimpleName(), suitablePools.size(), toBeCreated, vmProfile);
checkForPreferredStoragePool(suitablePools, vmProfile.getVirtualMachine(), suitableVolumeStoragePools, toBeCreated);
checkForPreferredStoragePool(suitablePools, vmProfile.getVirtualMachine(), plan.getClusterId(), suitableVolumeStoragePools, toBeCreated);
return true;
}
}
Expand Down Expand Up @@ -1986,10 +1987,11 @@ private boolean canReusePool(VirtualMachineProfile vmProfile, DeploymentPlan pla

private void checkForPreferredStoragePool(List<StoragePool> suitablePools,
VirtualMachine vm,
Long clusterId,
Map<Volume, List<StoragePool>> suitableVolumeStoragePools,
VolumeVO toBeCreated) {
List<StoragePool> pools = new ArrayList<>();
Optional<StoragePool> storagePool = getPreferredStoragePool(suitablePools, vm);
Optional<StoragePool> storagePool = getPreferredStoragePool(suitablePools, vm, clusterId);
storagePool.ifPresent(pools::add);

pools.addAll(suitablePools);
Expand All @@ -2005,17 +2007,16 @@ private Optional<StoragePool> getMatchingStoragePool(String preferredPoolId, Lis
.findFirst();
}

private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm) {
String accountStoragePoolUuid = StorageManager.PreferredStoragePool.valueIn(vm.getAccountId());
Optional<StoragePool> storagePool = getMatchingStoragePool(accountStoragePoolUuid, poolList);

if (storagePool.isPresent()) {
logger.debug("A storage pool is specified for this account, so we will use this storage pool for allocation: {}", storagePool.get());
} else {
String globalStoragePoolUuid = StorageManager.PreferredStoragePool.value();
storagePool = getMatchingStoragePool(globalStoragePoolUuid, poolList);
storagePool.ifPresent(pool -> logger.debug("A storage pool is specified in global setting, so we will use this storage pool for allocation: {}", pool));
protected Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm, Long clusterId) {
Long accountId = vm != null ? vm.getAccountId() : null;
Long domainId = vm != null ? vm.getDomainId() : null;
Pair<ConfigKey.Scope, String> preferredStoragePool = StorageManagerImpl.getPreferredStoragePoolUuid(accountId, domainId, clusterId);
if (preferredStoragePool == null) {
return Optional.empty();
}
Optional<StoragePool> storagePool = getMatchingStoragePool(preferredStoragePool.second(), poolList);
storagePool.ifPresent(pool -> logger.debug("A storage pool is specified at the [{}] level (account [{}], cluster [{}]), so we will use this storage pool for allocation: {}",
preferredStoragePool.first(), vm.getAccountId(), clusterId, pool));
return storagePool;
}

Expand Down
21 changes: 21 additions & 0 deletions server/src/main/java/com/cloud/storage/StorageManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,27 @@ public void setDiscoverers(List<StoragePoolDiscoverer> discoverers) {

private static final String NFS_MOUNT_OPTIONS_INCORRECT = "An incorrect mount option was specified";

/**
* Resolves the preferred storage pool UUID, checking the Account scope first, then the Domain scope,
* 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
*/
public static Pair<ConfigKey.Scope, String> getPreferredStoragePoolUuid(Long accountId, Long domainId, Long clusterId) {
Long[] resourcesIds = new Long[] {accountId, domainId, clusterId};
ConfigKey.Scope[] scopes = new ConfigKey.Scope[] {ConfigKey.Scope.Account, ConfigKey.Scope.Domain, ConfigKey.Scope.Cluster};
for (int i = 0; i < scopes.length; i++) {
if (resourcesIds[i] == null) {
continue;
}
String scopedPoolUuid = PreferredStoragePool.valueInScope(scopes[i], resourcesIds[i], true);
if (org.apache.commons.lang3.StringUtils.isNotBlank(scopedPoolUuid)) {
return new Pair<>(scopes[i], scopedPoolUuid);
}
}
String uuid = PreferredStoragePool.value();
return org.apache.commons.lang3.StringUtils.isNotBlank(uuid) ? new Pair<>(ConfigKey.Scope.Global, uuid) : null;
}

public boolean share(VMInstanceVO vm, List<VolumeVO> vols, HostVO host, boolean cancelPreviousShare) throws StorageUnavailableException {

// if pool is in maintenance and it is the ONLY pool available; reject
Expand Down
Loading
Loading