From 76d6d8302880a0a4898c812197c244acbd3a06c5 Mon Sep 17 00:00:00 2001 From: anishadas Date: Wed, 30 Sep 2026 18:53:23 +0530 Subject: [PATCH] Allocated volume pool allocation improvements. - Allow recreate volume for Allocated volumess with pool id - Change pool allocation logic to prioritise exising pool - Fix VM deployment logic for allocated volume reuse --- .../orchestration/VolumeOrchestrator.java | 30 +++++++++++- .../orchestration/VolumeOrchestratorTest.java | 48 +++++++++++++++++++ .../deploy/DeploymentPlanningManagerImpl.java | 22 +++------ .../com/cloud/storage/StorageManagerImpl.java | 5 ++ .../DeploymentPlanningManagerImplTest.java | 43 +++++++++++++++-- .../cloud/storage/StorageManagerImplTest.java | 2 +- 6 files changed, 128 insertions(+), 22 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index da827b17ff34..b9bf88d85a40 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -370,6 +370,24 @@ private Optional getMatchingStoragePool(String preferredPoolId, Lis } private Optional getPreferredStoragePool(List poolList, VirtualMachine vm) { + return getPreferredStoragePool(poolList, vm, null); + } + + private Optional getPreferredStoragePool(List 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 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 getPreferredStoragePool(List 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 getPreferredStoragePool(List poolList public StoragePool findStoragePool(DiskProfile dskCh, DataCenter dc, Pod pod, Long clusterId, Long hostId, VirtualMachine vm, final Set 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 = getPreferredStoragePool(poolList, vm); + Optional storagePool = getPreferredStoragePool(poolList, vm, volumePoolId); logger.trace("we have a preferred pool: {}", storagePool.isPresent()); StoragePool storage; diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java index 259dfaa6b1fa..74ef11d7f332 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java +++ b/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 invokeGetPreferredStoragePool(List poolList, VirtualMachine vm, Long volumePoolId) throws Exception { + Method m = VolumeOrchestrator.class.getDeclaredMethod("getPreferredStoragePool", List.class, VirtualMachine.class, Long.class); + m.setAccessible(true); + return (Optional) 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 poolList = Arrays.asList(other, matching); + + Optional 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 poolList = Collections.singletonList(pool); + + Optional 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 poolList = Collections.singletonList(pool); + + Optional result = invokeGetPreferredStoragePool(poolList, null, null); + + Assert.assertFalse(result.isPresent()); + } } diff --git a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java index f163a3d52a5a..e6e24697bbdf 100644 --- a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java +++ b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java @@ -1761,21 +1761,10 @@ protected Pair>, List> 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); } diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 0b2aca08e084..c7e33d023785 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/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; diff --git a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java b/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java index ace043bcc526..dbdaabce99c7 100644 --- a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java +++ b/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>, List> 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... diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 8f88800d549f..30d99dbe3769 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/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)); }