Allocated volume pool allocation improvements

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

@@ -370,6 +370,24 @@ private Optional<StoragePool> getMatchingStoragePool(String preferredPoolId, Lis
     }
 
     private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm) {
+        return getPreferredStoragePool(poolList, vm, null);
+    }
+
+    private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm, Long volumePoolId) {
+        // First priority: if volume already has a pool assigned (e.g., from a failed attach attempt),
+        // prefer that pool if it's in the validated pool list
+        if (volumePoolId != null) {
+            Optional<StoragePool> volumePool = poolList.stream()
+                    .filter(pool -> pool.getId() == volumePoolId)
+                    .findFirst();
+            if (volumePool.isPresent()) {
+                logger.info("Volume's existing storage pool [{}] is available and has passed all validations. Using it for allocation.", getReflectOnlySelectedFields(volumePool.get()));
+                return volumePool;
+            } else {
+                logger.info("Volume's existing pool ID [{}] is not in the list of suitable pools. Falling back to other pool selection logic.", volumePoolId);
+            }
+        }
+        // Second priority: account-level preferred pool
         String accountStoragePoolUuid = null;
         if (vm != null) {
             accountStoragePoolUuid = StorageManager.PreferredStoragePool.valueIn(vm.getAccountId());
@@ -381,6 +399,7 @@ private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList
             logger.debug("The storage pool [{}] was specified for this account [{}] and will be used for allocation.", storagePoolToString, vm.getAccountId());
 
         } else {
+            // Third priority: global preferred pool
             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.",
@@ -393,6 +412,15 @@ private Optional<StoragePool> getPreferredStoragePool(List<StoragePool> poolList
     public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, final Set<StoragePool> avoid) {
         Long podId = retrievePod(pod, clusterId);
 
+        // If the volume already has a poolId, prefer it if available.
+        Long volumePoolId = null;
+        if (dskCh.getVolumeId() != 0) {
+            VolumeVO volume = _volsDao.findById(dskCh.getVolumeId());
+            if (volume != null) {
+                volumePoolId = volume.getPoolId();
+            }
+        }
+
         VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm);
         for (StoragePoolAllocator allocator : _storagePoolAllocators) {
 
@@ -406,7 +434,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, volumePoolId);
                 logger.trace("we have a preferred pool: {}", storagePool.isPresent());
 
                 StoragePool storage;

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

@@ -16,9 +16,13 @@
 // under the License.
 package org.apache.cloudstack.engine.orchestration;
 
+import java.lang.reflect.Method;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.Date;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.lang.reflect.Field;
 
@@ -33,6 +37,7 @@
 import com.cloud.storage.ScopeType;
 import com.cloud.storage.DataStoreRole;
 import com.cloud.storage.Storage;
+import com.cloud.storage.StoragePool;
 import com.cloud.storage.Volume;
 import com.cloud.storage.Volume.Type;
 import com.cloud.storage.VolumeVO;
@@ -640,4 +645,47 @@ public void getVolumeCheckpointPathsAndImageStoreUrlsTestReturnCheckpointIfKVMAn
         Assert.assertEquals(1, result.second().size());
     }
 
+    @SuppressWarnings("unchecked")
+    private Optional<StoragePool> invokeGetPreferredStoragePool(List<StoragePool> poolList, VirtualMachine vm, Long volumePoolId) throws Exception {
+        Method m = VolumeOrchestrator.class.getDeclaredMethod("getPreferredStoragePool", List.class, VirtualMachine.class, Long.class);
+        m.setAccessible(true);
+        return (Optional<StoragePool>) m.invoke(volumeOrchestrator, poolList, vm, volumePoolId);
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolReusesExistingVolumePool() throws Exception {
+        StoragePool other = Mockito.mock(StoragePool.class);
+        Mockito.when(other.getId()).thenReturn(3L);
+        StoragePool matching = Mockito.mock(StoragePool.class);
+        Mockito.when(matching.getId()).thenReturn(2L);
+        List<StoragePool> poolList = Arrays.asList(other, matching);
+
+        Optional<StoragePool> result = invokeGetPreferredStoragePool(poolList, null, 2L);
+
+        Assert.assertTrue(result.isPresent());
+        Assert.assertSame(matching, result.get());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolFallsBackWhenVolumePoolNotInList() throws Exception {
+        StoragePool pool = Mockito.mock(StoragePool.class);
+        Mockito.when(pool.getId()).thenReturn(3L);
+        Mockito.when(pool.getUuid()).thenReturn("uuid-3");
+        List<StoragePool> poolList = Collections.singletonList(pool);
+
+        Optional<StoragePool> result = invokeGetPreferredStoragePool(poolList, null, 99L);
+
+        Assert.assertFalse(result.isPresent());
+    }
+
+    @Test
+    public void testGetPreferredStoragePoolNoVolumePoolIdFallsBack() throws Exception {
+        StoragePool pool = Mockito.mock(StoragePool.class);
+        Mockito.when(pool.getUuid()).thenReturn("uuid-3");
+        List<StoragePool> poolList = Collections.singletonList(pool);
+
+        Optional<StoragePool> result = invokeGetPreferredStoragePool(poolList, null, null);
+
+        Assert.assertFalse(result.isPresent());
+    }
 }

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

@@ -1761,21 +1761,10 @@ protected Pair<Map<Volume, List<StoragePool>>, List<Volume>> findSuitablePoolsFo
 
         for (VolumeVO toBeCreated : volumesTobeCreated) {
             logger.debug("Checking suitable pools for volume [{}, {}] of VM [{}].", toBeCreated, toBeCreated.getVolumeType().name(), vmProfile);
-
-            if (toBeCreated.getState() == Volume.State.Allocated && toBeCreated.getPoolId() != null) {
-                toBeCreated.setPoolId(null);
-                if (!_volsDao.update(toBeCreated.getId(), toBeCreated)) {
-                    throw new CloudRuntimeException(String.format("Error updating volume [%s] to clear pool Id.", toBeCreated));
-                }
-                if (logger.isDebugEnabled()) {
-                    logger.debug("Setting pool_id to NULL for volume id={} as it is in Allocated state", toBeCreated);
-                }
-            }
-            // If the plan specifies a poolId, it means that this VM's ROOT
-            // volume is ready and the pool should be reused.
-            // In this case, also check if rest of the volumes are ready and can
-            // be reused.
-            if ((plan.getPoolId() != null || (toBeCreated.getVolumeType() == Volume.Type.DATADISK && toBeCreated.getPoolId() != null && toBeCreated.getState() == Volume.State.Ready)) &&
+            // If the plan specifies a poolId, prefer the existing pool if it is still suitable;
+            // otherwise fall through to the allocator so the VM can still start.
+            if (plan.getPoolId() != null || (toBeCreated.getPoolId() != null &&
+                    (toBeCreated.getState() == Volume.State.Ready || toBeCreated.getState() == Volume.State.Allocated)) &&
                     checkIfPoolCanBeReused(vmProfile, plan, avoid, suitableVolumeStoragePools, readyAndReusedVolumes, toBeCreated)) {
                 continue;
             }
@@ -1896,10 +1885,11 @@ private boolean canReusePool(VirtualMachineProfile vmProfile, DeploymentPlan pla
 
         if (plan.getDataCenterId() == exstPoolDcId && ((plan.getPodId() == exstPoolPodId && plan.getClusterId() == exstPoolClusterId) ||
                 (dataStore != null && dataStore.getScope() != null && dataStore.getScope().getScopeType() == ScopeType.ZONE))) {
-            logger.debug("Pool [{}] of volume [{}] used by VM [{}] fits the specified plan. No need to reallocate a pool for this volume.",
+            logger.debug("Pool [{}] of volume [{}] used by VM [{}] fits the specified plan. Planner will use existing pool for volume.",
                     pool, toBeCreated, vmProfile);
             suitablePools.add(pool);
             suitableVolumeStoragePools.put(toBeCreated, suitablePools);
+            // Allocated/Creating volumes still need to be created, so they aren't ready to use.
             if (!(toBeCreated.getState() == Volume.State.Allocated || toBeCreated.getState() == Volume.State.Creating)) {
                 readyAndReusedVolumes.add(toBeCreated);
             }

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -3873,6 +3873,11 @@ public boolean storagePoolCompatibleWithVolumePool(StoragePool pool, Volume volu
                 logger.debug(String.format("Pool [%s] with type [%s] does not match volume [%s] pool type [%s].", pool, pool.getPoolType(), volume, volumePool.getPoolType()));
                 return false;
             }
+        } else if (volume.getState() == Volume.State.Allocated) {
+            // For volumes in Allocated state that have a poolId (e.g., from a failed attach/create attempt),
+            // allow pool allocation. The volume hasn't been physically created yet, so it can be allocated to any compatible pool.
+            // This enables retry scenarios where encryption or other operations failed during the first attach attempt.
+            return true;
         } else {
             logger.debug(String.format("Cannot check compatibility of pool [%s] because volume [%s] is not in [%s] state.", pool, volume, Volume.State.Ready));
             return false;

server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java

@@ -136,9 +136,10 @@
 import java.util.Set;
 
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
 @RunWith(SpringJUnit4ClassRunner.class)
@@ -811,15 +812,49 @@ public void findSuitablePoolsForVolumesTest() throws Exception {
         Mockito.when(volDao.findUsableVolumesForInstance(1L)).thenReturn(Arrays.asList(vol1));
         Mockito.when(volDao.findByInstanceAndType(1L, Volume.Type.ROOT)).thenReturn(Arrays.asList(vol1));
         Mockito.when(_dataStoreManager.getPrimaryDataStore(vol1.getPoolId())).thenReturn((DataStore) primaryDataStore);
+        Mockito.when(primaryDataStore.isInMaintenance()).thenReturn(true);
         Mockito.when(avoids.shouldAvoid(storagePool)).thenReturn(Boolean.FALSE);
 
         Mockito.doReturn(Arrays.asList(storagePool)).when(allocator).allocateToPool(diskProfile, vmProfile, plan,
                 avoids, 10);
-        Mockito.when(volDao.update(vol1.getId(), vol1)).thenReturn(true);
         _dpm.findSuitablePoolsForVolumes(vmProfile, plan, avoids, 10);
-        verify(vol1, times(1)).setPoolId(null);
-        assertTrue(vol1.getPoolId() == null);
+        // Allocated volume with an existing poolId should NOT have its poolId cleared;
+        // it goes through the pool suitability check and falls back to the allocator if the pool is unsuitable.
+        verify(vol1, never()).setPoolId(null);
+        assertNotNull(vol1.getPoolId());
+    }
+
+    @Test
+    public void testFindSuitablePoolsReusesPoolForAllocatedRootVolume() {
+        VolumeVO vol = Mockito.spy(new VolumeVO("root", dataCenterId, podId, 1L, 1L, instanceId, "folder", "path",
+                Storage.ProvisioningType.THIN, (long) 10 << 30, Volume.Type.ROOT));
+        Mockito.when(vol.getId()).thenReturn(1L);
+        vol.setState(Volume.State.Allocated);
+        vol.setPoolId(1L);
+
+        PrimaryDataStore pool = Mockito.mock(PrimaryDataStore.class);
+        Mockito.when(pool.getId()).thenReturn(1L);
+        Mockito.when(pool.isInMaintenance()).thenReturn(false);
+        Mockito.when(pool.getDataCenterId()).thenReturn(dataCenterId);
+        Mockito.when(pool.getPodId()).thenReturn(podId);
+        Mockito.when(pool.getClusterId()).thenReturn(clusterId);
+
+        // plan has no poolId, so reuse must come from the volume's own poolId (the broadened, non-DATADISK-only path).
+        DataCenterDeployment plan = new DataCenterDeployment(dataCenterId, podId, clusterId, null, null, null);
 
+        Mockito.when(vmProfile.getId()).thenReturn(1L);
+        Mockito.when(volDao.findUsableVolumesForInstance(1L)).thenReturn(Arrays.asList(vol));
+        Mockito.when(volDao.findByInstanceAndType(1L, Volume.Type.ROOT)).thenReturn(Arrays.asList(vol));
+        Mockito.when(_dataStoreManager.getPrimaryDataStore(vol.getPoolId())).thenReturn((DataStore) pool);
+        Mockito.when(avoids.shouldAvoid(pool)).thenReturn(Boolean.FALSE);
+
+        Pair<Map<Volume, List<StoragePool>>, List<Volume>> result = _dpm.findSuitablePoolsForVolumes(vmProfile, plan, avoids, 10);
+
+        assertTrue(result.first().containsKey(vol));
+        assertTrue(result.first().get(vol).contains(pool));
+        // Allocated volumes still need to be created, so they are not treated as ready-and-reused.
+        assertFalse(result.second().contains(vol));
+        verify(vol, never()).setPoolId(null);
     }
 
     // This is so ugly but everything is so intertwined...

server/src/test/java/com/cloud/storage/StorageManagerImplTest.java

@@ -262,7 +262,7 @@ public void storagePoolCompatibleWithVolumePoolTestVolumeWithPoolIdInAllocatedSt
         PrimaryDataStoreDao storagePoolDao = Mockito.mock(PrimaryDataStoreDao.class);
         storageManagerImpl._storagePoolDao = storagePoolDao;
         Mockito.doReturn(storagePool).when(storagePoolDao).findById(volume.getPoolId());
-        Assert.assertFalse(storageManagerImpl.storagePoolCompatibleWithVolumePool(storagePool, volume));
+        Assert.assertTrue(storageManagerImpl.storagePoolCompatibleWithVolumePool(storagePool, volume));
 
     }