Extend Preferred storage pool setting to cluster and account scope

engine/components-api/src/main/java/com/cloud/storage/StorageManager.java

@@ -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;
@@ -177,7 +178,10 @@ 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", "",
-            "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 or Cluster level: " +
+                    "the Account 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.Cluster), null);
 
     ConfigKey<Boolean> MountDisabledStoragePool = new ConfigKey<>(Boolean.class,
             "mount.disabled.storage.pool",
@@ -447,4 +451,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) {
+        if (accountId != null) {
+            String uuid = PreferredStoragePool.valueInScope(ConfigKey.Scope.Account, accountId, true);
+            if (StringUtils.isNotBlank(uuid)) {
+                return new Pair<>(ConfigKey.Scope.Account, uuid);
+            }
+        }
+        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;
+    }
 }

engine/components-api/src/test/java/com/cloud/storage/StorageManagerTest.java

@@ -0,0 +1,117 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.storage;
+
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import com.cloud.utils.Pair;
+
+public class StorageManagerTest {
+
+    private static final long ACCOUNT_ID = 2L;
+    private static final long CLUSTER_ID = 3L;
+    private static final String ACCOUNT_POOL_UUID = "account-pool-uuid";
+    private static final String CLUSTER_POOL_UUID = "cluster-pool-uuid";
+    private static final String GLOBAL_POOL_UUID = "global-pool-uuid";
+
+    private ConfigDepotImpl configDepot;
+    private Object previousDepot;
+
+    @Before
+    public void setUp() {
+        previousDepot = ReflectionTestUtils.getField(ConfigKey.class, "s_depot");
+        configDepot = Mockito.mock(ConfigDepotImpl.class);
+        ConfigKey.init(configDepot);
+    }
+
+    @After
+    public void tearDown() {
+        ConfigKey.init((ConfigDepotImpl) previousDepot);
+    }
+
+    private void mockPreferredStoragePool(ConfigKey.Scope scope, Long id, String uuid) {
+        Mockito.when(configDepot.getConfigStringValue(StorageManager.PreferredStoragePool.key(), scope, id)).thenReturn(uuid);
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidNotSetAtAnyLevel() {
+        Assert.assertNull(StorageManager.getPreferredStoragePoolUuid(ACCOUNT_ID, CLUSTER_ID));
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidSetAtAccountLevel() {
+        mockPreferredStoragePool(ConfigKey.Scope.Account, ACCOUNT_ID, ACCOUNT_POOL_UUID);
+        mockPreferredStoragePool(ConfigKey.Scope.Cluster, CLUSTER_ID, CLUSTER_POOL_UUID);
+        mockPreferredStoragePool(ConfigKey.Scope.Global, null, GLOBAL_POOL_UUID);
+
+        Pair<ConfigKey.Scope, String> result = StorageManager.getPreferredStoragePoolUuid(ACCOUNT_ID, CLUSTER_ID);
+
+        Assert.assertEquals(ConfigKey.Scope.Account, result.first());
+        Assert.assertEquals(ACCOUNT_POOL_UUID, result.second());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidSetAtClusterLevel() {
+        mockPreferredStoragePool(ConfigKey.Scope.Cluster, CLUSTER_ID, CLUSTER_POOL_UUID);
+        mockPreferredStoragePool(ConfigKey.Scope.Global, null, GLOBAL_POOL_UUID);
+
+        Pair<ConfigKey.Scope, String> result = StorageManager.getPreferredStoragePoolUuid(ACCOUNT_ID, CLUSTER_ID);
+
+        Assert.assertEquals(ConfigKey.Scope.Cluster, result.first());
+        Assert.assertEquals(CLUSTER_POOL_UUID, result.second());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidEmptyAccountValueFallsBackToCluster() {
+        mockPreferredStoragePool(ConfigKey.Scope.Account, ACCOUNT_ID, "");
+        mockPreferredStoragePool(ConfigKey.Scope.Cluster, CLUSTER_ID, CLUSTER_POOL_UUID);
+
+        Pair<ConfigKey.Scope, String> result = StorageManager.getPreferredStoragePoolUuid(ACCOUNT_ID, CLUSTER_ID);
+
+        Assert.assertEquals(ConfigKey.Scope.Cluster, result.first());
+        Assert.assertEquals(CLUSTER_POOL_UUID, result.second());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidSetAtGlobalLevel() {
+        mockPreferredStoragePool(ConfigKey.Scope.Global, null, GLOBAL_POOL_UUID);
+
+        Pair<ConfigKey.Scope, String> result = StorageManager.getPreferredStoragePoolUuid(ACCOUNT_ID, CLUSTER_ID);
+
+        Assert.assertEquals(ConfigKey.Scope.Global, result.first());
+        Assert.assertEquals(GLOBAL_POOL_UUID, result.second());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolUuidNullIdsUseGlobalLevel() {
+        mockPreferredStoragePool(ConfigKey.Scope.Global, null, GLOBAL_POOL_UUID);
+
+        Pair<ConfigKey.Scope, String> result = StorageManager.getPreferredStoragePoolUuid(null, null);
+
+        Assert.assertEquals(ConfigKey.Scope.Global, result.first());
+        Assert.assertEquals(GLOBAL_POOL_UUID, result.second());
+        Mockito.verify(configDepot, Mockito.never()).getConfigStringValue(Mockito.anyString(), Mockito.eq(ConfigKey.Scope.Account), Mockito.any());
+        Mockito.verify(configDepot, Mockito.never()).getConfigStringValue(Mockito.anyString(), Mockito.eq(ConfigKey.Scope.Cluster), Mockito.any());
+    }
+}

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java

@@ -396,23 +396,15 @@ 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;
+        Pair<ConfigKey.Scope, String> preferredStoragePool = StorageManager.getPreferredStoragePoolUuid(accountId, 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;
     }
 
@@ -433,7 +425,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;

engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java

@@ -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;
 
@@ -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;
@@ -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;
@@ -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;
@@ -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());
+    }
 }

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

@@ -1924,7 +1924,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;
             }
         }
@@ -1986,10 +1986,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);
@@ -2005,17 +2006,14 @@ 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) {
+        Pair<ConfigKey.Scope, String> preferredStoragePool = StorageManager.getPreferredStoragePoolUuid(vm.getAccountId(), 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;
     }