From 832b924ec96d44a8fb2ae39562bf47f27c7943f4 Mon Sep 17 00:00:00 2001 From: Annie Li Date: Mon, 6 Nov 2023 13:29:11 -0800 Subject: [PATCH 1/2] clear pool id if volume allocation fails and leave volume state as Allocated with a pool id assigned --- .../deploy/DeploymentPlanningManagerImpl.java | 10 ++ .../DeploymentPlanningManagerImplTest.java | 115 +++++++++++++++++- .../cloud/storage/StorageManagerImplTest.java | 30 +++++ 3 files changed, 149 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java index cf4e7fdb2ccb..0ef462bb96c3 100644 --- a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java +++ b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java @@ -1651,6 +1651,16 @@ protected Pair>, List> findSuitablePoolsFo for (VolumeVO toBeCreated : volumesTobeCreated) { s_logger.debug("Checking suitable pools for volume (Id, Type): (" + toBeCreated.getId() + "," + toBeCreated.getVolumeType().name() + ")"); + 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.getId())); + } + if (s_logger.isDebugEnabled()) { + String msg = String.format("Setting pool_id to NULL for volume id=%s as it is in Allocated state", toBeCreated.getId()); + s_logger.debug(msg); + } + } // 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 diff --git a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java b/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java index d79010ee5c06..459bb7b382a9 100644 --- a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java +++ b/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java @@ -19,6 +19,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import java.io.IOException; import java.util.ArrayList; @@ -33,6 +35,7 @@ import com.cloud.dc.ClusterDetailsVO; import com.cloud.dc.DataCenter; +import com.cloud.dc.HostPodVO; import com.cloud.gpu.GPU; import com.cloud.host.Host; import com.cloud.host.HostVO; @@ -40,19 +43,26 @@ import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.Storage; import com.cloud.storage.StoragePool; +import com.cloud.storage.StoragePoolStatus; import com.cloud.storage.VMTemplateVO; import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VMTemplateDao; +import com.cloud.template.VirtualMachineTemplate; +import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.dao.AccountDao; import com.cloud.utils.Pair; +import com.cloud.vm.DiskProfile; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.Type; import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.VirtualMachineProfileImpl; import org.apache.cloudstack.affinity.dao.AffinityGroupDomainMapDao; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.commons.collections.CollectionUtils; import org.junit.Assert; @@ -67,6 +77,7 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.Spy; +import org.powermock.api.mockito.PowerMockito; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.ComponentScan; import org.springframework.context.annotation.ComponentScan.Filter; @@ -197,6 +208,18 @@ public class DeploymentPlanningManagerImplTest { @Mock ConfigurationDao configDao; + @Mock + AccountManager _accountMgr; + + @Inject + DiskOfferingDao _diskOfferingDao; + + @Mock + DataStoreManager _dataStoreManager; + + @Inject + HostPodDao _podDao; + private static final long dataCenterId = 1L; private static final long instanceId = 123L; private static final long hostId = 0L; @@ -242,6 +265,8 @@ public void testSetUp() { List planners = new ArrayList(); planners.add(_planner); _dpm.setPlanners(planners); + StoragePoolAllocator allocator = Mockito.mock(StoragePoolAllocator.class); + _dpm.setStoragePoolAllocators(Arrays.asList(allocator)); Mockito.when(host.getId()).thenReturn(hostId); Mockito.doNothing().when(_dpm).avoidDisabledResources(vmProfile, dc, avoids); @@ -388,12 +413,12 @@ private void prepareAndVerifyAvoidDisabledResourcesTest(int timesRouter, int tim _dpm.avoidDisabledResources(vmProfile, dc, avoids); - Mockito.verify(_dpm, Mockito.times(timesRouter)).isRouterDeployableInDisabledResources(); - Mockito.verify(_dpm, Mockito.times(timesAdminVm)).isAdminVmDeployableInDisabledResources(); - Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledDataCenters(dc, avoids); - Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledPods(dc, avoids); - Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledClusters(dc, avoids); - Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledHosts(dc, avoids); + verify(_dpm, times(timesRouter)).isRouterDeployableInDisabledResources(); + verify(_dpm, times(timesAdminVm)).isAdminVmDeployableInDisabledResources(); + verify(_dpm, times(timesDisabledResource)).avoidDisabledDataCenters(dc, avoids); + verify(_dpm, times(timesDisabledResource)).avoidDisabledPods(dc, avoids); + verify(_dpm, times(timesDisabledResource)).avoidDisabledClusters(dc, avoids); + verify(_dpm, times(timesDisabledResource)).avoidDisabledHosts(dc, avoids); Mockito.reset(_dpm); } @@ -702,6 +727,84 @@ public void failEncRootPlannerHostSupportingEncryptionTest() { } } + @Test + public void findSuitablePoolsForVolumesTest() throws Exception { + Long diskOfferingId = 1L; + HostVO host = Mockito.spy(new HostVO("host")); + Map hostDetails = new HashMap<>() { + { + put(Host.HOST_VOLUME_ENCRYPTION, "true"); + } + }; + host.setDetails(hostDetails); + Mockito.when(host.getStatus()).thenReturn(Status.Up); + + VolumeVO vol1 = Mockito.spy(new VolumeVO("vol1", dataCenterId, podId, 1L, 1L, instanceId, "folder", "path", + Storage.ProvisioningType.THIN, (long) 10 << 30, Volume.Type.ROOT)); + Mockito.when(vol1.getId()).thenReturn(1L); + vol1.setState(Volume.State.Allocated); + vol1.setPassphraseId(1L); + vol1.setPoolId(1L); + vol1.setDiskOfferingId(diskOfferingId); + + StoragePoolVO storagePool = new StoragePoolVO(); + storagePool.setStatus(StoragePoolStatus.Maintenance); + storagePool.setId(vol1.getPoolId()); + storagePool.setDataCenterId(dataCenterId); + storagePool.setPodId(podId); + storagePool.setClusterId(clusterId); + + DiskProfile diskProfile = Mockito.mock(DiskProfile.class); + + StoragePoolAllocator allocator = Mockito.mock(StoragePoolAllocator.class); + + DataCenterDeployment plan = new DataCenterDeployment(dataCenterId, podId, clusterId, null, null, null); + + Account account = Mockito.mock(Account.class); + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(vmProfile.getOwner()).thenReturn(account); + Mockito.when(_accountMgr.isRootAdmin(account.getId())).thenReturn(Boolean.FALSE); + + Mockito.when(_dcDao.findById(dataCenterId)).thenReturn(dc); + Mockito.when(dc.getAllocationState()).thenReturn(AllocationState.Enabled); + + HostPodVO podVo = Mockito.mock(HostPodVO.class); + Mockito.when(podVo.getAllocationState()).thenReturn(AllocationState.Enabled); + Mockito.doReturn(podVo).when(_podDao).findById(podId); + + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getAllocationState()).thenReturn(AllocationState.Enabled); + Mockito.when(_clusterDao.findById(clusterId)).thenReturn(cluster); + + DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); + + Mockito.when(_diskOfferingDao.findById(vol1.getDiskOfferingId())).thenReturn(diskOffering); + VirtualMachineTemplate vmt = Mockito.mock(VirtualMachineTemplate.class); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(vmProfile.getServiceOffering()).thenReturn(serviceOffering); + + PrimaryDataStore primaryDataStore = Mockito.mock(PrimaryDataStore.class); + + Mockito.when(vmt.getFormat()).thenReturn(Storage.ImageFormat.ISO); + Mockito.when(vmProfile.getTemplate()).thenReturn(vmt); + + Mockito.when(vmProfile.getId()).thenReturn(1L); + Mockito.when(vmProfile.getType()).thenReturn(VirtualMachine.Type.User); + 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(avoids.shouldAvoid(storagePool)).thenReturn(Boolean.FALSE); + PowerMockito.whenNew(DiskProfile.class).withAnyArguments().thenReturn(diskProfile); + + 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); + + } // This is so ugly but everything is so intertwined... private DeploymentClusterPlanner setupMocksForPlanDeploymentHostTests(HostVO host, VolumeVO vol1) { long diskOfferingId = 345L; diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index e7004ba7c5d3..20e1be95e52a 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -19,6 +19,8 @@ import java.util.ArrayList; import java.util.List; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -128,4 +130,32 @@ public void testIsVolumeSuspectedDestroyDuplicateTrue() { Assert.assertTrue(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume)); } + @Test + public void storagePoolCompatibleWithVolumePoolTestVolumeWithPoolIdInAllocatedState() { + StoragePoolVO storagePool = new StoragePoolVO(); + storagePool.setPoolType(Storage.StoragePoolType.PowerFlex); + storagePool.setId(1L); + VolumeVO volume = new VolumeVO(); + volume.setState(Volume.State.Allocated); + volume.setPoolId(1L); + PrimaryDataStoreDao storagePoolDao = Mockito.mock(PrimaryDataStoreDao.class); + storageManagerImpl._storagePoolDao = storagePoolDao; + Mockito.doReturn(storagePool).when(storagePoolDao).findById(volume.getPoolId()); + Assert.assertFalse(storageManagerImpl.storagePoolCompatibleWithVolumePool(storagePool, volume)); + + } + + @Test + public void storagePoolCompatibleWithVolumePoolTestVolumeWithoutPoolIdInAllocatedState() { + StoragePoolVO storagePool = new StoragePoolVO(); + storagePool.setPoolType(Storage.StoragePoolType.PowerFlex); + storagePool.setId(1L); + VolumeVO volume = new VolumeVO(); + volume.setState(Volume.State.Allocated); + PrimaryDataStoreDao storagePoolDao = Mockito.mock(PrimaryDataStoreDao.class); + storageManagerImpl._storagePoolDao = storagePoolDao; + Assert.assertTrue(storageManagerImpl.storagePoolCompatibleWithVolumePool(storagePool, volume)); + + } + } From a8a930fe3303892770c768d0867cfc11ce6db49e Mon Sep 17 00:00:00 2001 From: Annie Li Date: Wed, 8 Nov 2023 10:36:08 -0800 Subject: [PATCH 2/2] clear_pool_id_if_volume_allocation_fails --- .../deploy/DeploymentPlanningManagerImplTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java b/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java index 459bb7b382a9..ea9e2bb20908 100644 --- a/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java +++ b/server/src/test/java/com/cloud/deploy/DeploymentPlanningManagerImplTest.java @@ -413,12 +413,12 @@ private void prepareAndVerifyAvoidDisabledResourcesTest(int timesRouter, int tim _dpm.avoidDisabledResources(vmProfile, dc, avoids); - verify(_dpm, times(timesRouter)).isRouterDeployableInDisabledResources(); - verify(_dpm, times(timesAdminVm)).isAdminVmDeployableInDisabledResources(); - verify(_dpm, times(timesDisabledResource)).avoidDisabledDataCenters(dc, avoids); - verify(_dpm, times(timesDisabledResource)).avoidDisabledPods(dc, avoids); - verify(_dpm, times(timesDisabledResource)).avoidDisabledClusters(dc, avoids); - verify(_dpm, times(timesDisabledResource)).avoidDisabledHosts(dc, avoids); + Mockito.verify(_dpm, Mockito.times(timesRouter)).isRouterDeployableInDisabledResources(); + Mockito.verify(_dpm, Mockito.times(timesAdminVm)).isAdminVmDeployableInDisabledResources(); + Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledDataCenters(dc, avoids); + Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledPods(dc, avoids); + Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledClusters(dc, avoids); + Mockito.verify(_dpm, Mockito.times(timesDisabledResource)).avoidDisabledHosts(dc, avoids); Mockito.reset(_dpm); }