From f4b0e1d80a05555d7bc4f729b625d0da42390cd1 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Thu, 6 Nov 2025 17:36:58 +0530 Subject: [PATCH 1/6] Add unit tests for listHostsForMigration --- .../cloud/server/ManagementServerImpl.java | 2 +- .../server/ManagementServerImplTest.java | 1562 ++++++++++++++++- 2 files changed, 1560 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 58ac6891e5f5..179d691e101a 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1923,7 +1923,7 @@ private List findAllSuitableStoragePoolsForDetachedVolume(Volume vo return suitablePools; } - private Pair, Integer> searchForServers(final Long startIndex, final Long pageSize, final Object name, final Object type, + Pair, Integer> searchForServers(final Long startIndex, final Long pageSize, final Object name, final Object type, final Object state, final Object zone, final Object pod, final Object cluster, final Object id, final Object keyword, final Object resourceState, final Object haHosts, final Object hypervisorType, final Object hypervisorVersion, final Object... excludes) { final Filter searchFilter = new Filter(HostVO.class, "id", Boolean.TRUE, startIndex, pageSize); diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index b26cd455cfbb..4d5c5af2be12 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -16,17 +16,34 @@ // under the License. package com.cloud.server; +import com.cloud.api.ApiDBUtils; +import com.cloud.dc.DataCenterVO; import com.cloud.dc.Vlan.VlanType; +import com.cloud.dc.dao.DataCenterDao; +import com.cloud.deploy.DeploymentPlanningManager; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; +import com.cloud.gpu.GPU; import com.cloud.host.DetailVO; import com.cloud.host.Host; +import com.cloud.host.Host.Type; import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.host.dao.HostDetailsDao; +import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.network.IpAddress; import com.cloud.network.IpAddressManagerImpl; import com.cloud.network.dao.IPAddressVO; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.service.dao.ServiceOfferingDao; +import com.cloud.service.dao.ServiceOfferingDetailsDao; +import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.VMTemplateVO; +import com.cloud.storage.Volume; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.VMTemplateDao; +import com.cloud.storage.dao.VolumeDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.SSHKeyPair; @@ -37,14 +54,25 @@ import com.cloud.user.dao.SSHKeyPairDao; import com.cloud.user.dao.UserDataDao; import com.cloud.utils.Pair; +import com.cloud.utils.Ternary; import com.cloud.utils.db.Filter; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.UserVmDetailVO; import com.cloud.vm.UserVmVO; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.UserVmDetailsDao; +import com.cloud.vm.dao.VMInstanceDao; +import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; +import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; +import com.cloud.agent.manager.allocator.HostAllocator; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd; @@ -63,6 +91,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; @@ -74,6 +103,7 @@ import java.lang.reflect.Field; import java.util.ArrayList; import java.util.List; +import java.util.Map; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.nullable; @@ -123,15 +153,60 @@ public class ManagementServerImplTest { @Mock UserDataManager userDataManager; - @Spy - ManagementServerImpl spy = new ManagementServerImpl(); - @Mock UserVmDetailsDao userVmDetailsDao; @Mock HostDetailsDao hostDetailsDao; + + @Mock + VMInstanceDao vmInstanceDao; + + @Mock + HostDao hostDao; + + @Mock + ServiceOfferingDetailsDao serviceOfferingDetailsDao; + + @Mock + VolumeDao volumeDao; + + @Mock + ServiceOfferingDao offeringDao; + + @Mock + DiskOfferingDao diskOfferingDao; + + @Mock + HypervisorCapabilitiesDao hypervisorCapabilitiesDao; + + @Mock + DataStoreManager dataStoreManager; + + @Mock + PrimaryDataStoreDao primaryDataStoreDao; + + @Mock + DpdkHelper dpdkHelper; + + @Mock + AffinityGroupVMMapDao affinityGroupVMMapDao; + + @Mock + DeploymentPlanningManager dpMgr; + + @Mock + DataCenterDao dcDao; + + @Mock + HostAllocator hostAllocator; + + @InjectMocks + @Spy + ManagementServerImpl spy; + private AutoCloseable closeable; + private MockedStatic apiDBUtilsMock; @Before public void setup() throws IllegalAccessException, NoSuchFieldException { @@ -145,10 +220,21 @@ public void setup() throws IllegalAccessException, NoSuchFieldException { spy._UserVmDetailsDao = userVmDetailsDao; spy._detailsDao = hostDetailsDao; spy.userDataManager = userDataManager; + + spy.setHostAllocators(List.of(hostAllocator)); + + // Mock ApiDBUtils static method + apiDBUtilsMock = Mockito.mockStatic(ApiDBUtils.class); + // Return empty list to avoid architecture filtering in most tests + apiDBUtilsMock.when(() -> ApiDBUtils.listZoneClustersArchs(Mockito.anyLong())) + .thenReturn(new ArrayList<>()); } @After public void tearDown() throws Exception { + if (apiDBUtilsMock != null) { + apiDBUtilsMock.close(); + } CallContext.unregister(); closeable.close(); } @@ -684,4 +770,1474 @@ public void testZoneWideVolumeRequiresStorageMotionDriverDependent() { Mockito.when(driver.zoneWideVolumesAvailableWithoutClusterMotion()).thenReturn(false); Assert.assertTrue(spy.zoneWideVolumeRequiresStorageMotion(dataStore, host1, host2)); } + + // ============= Tests for listHostsForMigrationOfVM ============= + + @Test(expected = PermissionDeniedException.class) + public void testListHostsForMigrationOfVMNonRootAdmin() { + mockRunningVM(1L, HypervisorType.KVM); + Account caller = Mockito.mock(Account.class); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(_accountMgr.isRootAdmin(caller.getId())).thenReturn(false); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMNullVM() { + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(null); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMNotRunning() { + VMInstanceVO vm = mockVM(1L, HypervisorType.KVM, State.Stopped); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMUnsupportedHypervisor() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.BareMetal); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMLxcUserVM() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.LXC); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test + public void testListHostsForMigrationOfVMGpuEnabled() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + + // Mock GPU detail + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(Mockito.mock(com.cloud.service.ServiceOfferingDetailsVO.class)); + + Ternary, Integer>, List, java.util.Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + Assert.assertNotNull(result); + Assert.assertEquals(0, result.first().first().size()); + Assert.assertEquals(Integer.valueOf(0), result.first().second()); + Assert.assertEquals(0, result.second().size()); + } + + @Test + public void testListHostsForMigrationOfVMWithSystemVM() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.VMware); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.ConsoleProxy); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // System VMs can use storage motion + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null)) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.VMware); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.VMware); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + // Verify this doesn't throw exception - system VMs should be migrateable + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify storage motion capability was checked + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + + // Verify result structure and data + Assert.assertNotNull(result); + Assert.assertNotNull("All hosts list should not be null", result.first()); + Assert.assertNotNull("Suitable hosts list should not be null", result.second()); + Assert.assertNotNull("Storage motion map should not be null", result.third()); + + // Verify all hosts returned (from searchForServers) + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("All hosts list should contain 2 hosts", 2, result.first().first().size()); + + // Verify suitable hosts (from host allocator) + Assert.assertEquals("Should return 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Suitable hosts should contain host1", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Suitable hosts should contain host2", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithDomainRouter() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.DomainRouter); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + // Verify domain router can be migrated + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify hypervisor capabilities were checked + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + + // Verify result contains expected hosts + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should return 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Result should contain host 101", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Result should contain host 102", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithMultipleVolumes() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + // Multiple volumes - root and data disk + VolumeVO rootVolume = mockVolume(1L, 1L); + VolumeVO dataVolume = mockVolume(2L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(rootVolume, dataVolume)); + + DiskOfferingVO sharedOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(1L)).thenReturn(sharedOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, rootVolume); + + // Verify multiple volumes are handled + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify all volumes were checked + Mockito.verify(volumeDao).findCreatedByInstance(vm.getId()); + Mockito.verify(diskOfferingDao, Mockito.times(2)).findById(1L); + + // Verify result + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should return 2 suitable hosts for migration", 2, result.second().size()); + Assert.assertTrue("Suitable hosts should include host 101", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Suitable hosts should include host 102", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMWithMixedStorage() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.XenServer); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // No storage motion support + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.XenServer, null)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + // Mixed storage - one shared, one local + VolumeVO sharedVolume = mockVolume(1L, 1L); + VolumeVO localVolume = mockVolume(2L, 2L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(sharedVolume, localVolume)); + + DiskOfferingVO sharedOffering = mockSharedDiskOffering(1L); + DiskOfferingVO localOffering = mockLocalDiskOffering(2L); + Mockito.when(diskOfferingDao.findById(sharedVolume.getDiskOfferingId())).thenReturn(sharedOffering); + Mockito.when(diskOfferingDao.findById(localVolume.getDiskOfferingId())).thenReturn(localOffering); + + // Should throw exception because we have local storage without storage motion support + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test + public void testListHostsForMigrationOfVMKVMWithNullHypervisorVersion() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + // KVM host with null hypervisor version + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // KVM null version should be treated as empty string + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify KVM null version was converted to empty string + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + + // Verify result data + Assert.assertNotNull(result); + Assert.assertEquals("Total hosts should be 2", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Suitable hosts should be 2", 2, result.second().size()); + Assert.assertTrue("Should contain host 101", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Should contain host 102", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMNonUefiVm() { + // Test VM migration for non-UEFI VM (regular VM migration flow) + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // No UEFI detail - VM is NOT UEFI-enabled + Mockito.when(userVmDetailsDao.findDetail(vm.getId(), ApiConstants.BootType.UEFI.toString())) + .thenReturn(null); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify hosts are returned for migration (non-UEFI VMs don't need UEFI-enabled hosts) + Assert.assertNotNull(result); + Assert.assertEquals("Should have 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Should contain host 101", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Should contain host 102", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithUefiVmClusterScope() { + // Test UEFI VM migration with cluster-scoped search (no storage motion) + // This exercises the code path where filteredHosts is NULL and allocateTo without list is called + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // No storage motion support - cluster-scoped search + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + // Mock filterUefiHostsForMigration to return success with filtered hosts (only host1 is UEFI-compatible) + List uefiCompatibleHosts = List.of(host1); + Pair> uefiFilterResult = new Pair<>(true, uefiCompatibleHosts); + Mockito.doReturn(uefiFilterResult).when(spy).filterUefiHostsForMigration( + Mockito.anyList(), Mockito.isNull(), Mockito.any()); + + // Setup other mocks + Mockito.when(dpdkHelper.isVMDpdkEnabled(vm.getId())).thenReturn(false); + Mockito.when(affinityGroupVMMapDao.countAffinityGroupsForVm(vm.getId())).thenReturn(0L); + DataCenterVO dc = Mockito.mock(DataCenterVO.class); + Mockito.when(dcDao.findById(srcHost.getDataCenterId())).thenReturn(dc); + Mockito.doNothing().when(dpMgr).checkForNonDedicatedResources(Mockito.any(), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(dpMgr).reorderHostsByPriority(Mockito.any(), Mockito.anyList()); + + // After UEFI filtering, filteredHosts is set to uefiCompatibleHosts (line 1582) + // So allocateTo WITH list parameter is called (line 1608) even in cluster scope + Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList(), Mockito.anyInt(), Mockito.anyBoolean())) + .thenReturn(new ArrayList<>(uefiCompatibleHosts)); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify result structure + Assert.assertNotNull("Result should not be null", result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("All hosts list should contain 2 hosts", 2, result.first().first().size()); + + // Verify only UEFI-compatible hosts are in suitable list + Assert.assertEquals("Should have 1 UEFI-compatible suitable host", 1, result.second().size()); + Assert.assertTrue("Host 101 should be the only UEFI-compatible host", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertFalse("Host 102 should not be in suitable hosts (not UEFI-compatible)", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithUefiVmZoneWideScope() { + // Test UEFI VM migration with zone-wide search (storage motion enabled) + // This exercises the code path where filteredHosts IS populated and allocateTo WITH list is called + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.VMware); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // Storage motion supported - zone-wide search with filteredHosts + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null)) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for zone-wide search (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.VMware); + HostVO host2 = mockHost(102L, 2L, 1L, 1L, HypervisorType.VMware); // Different cluster + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + // Mock filterUefiHostsForMigration to return success with filtered hosts (only host1 is UEFI-compatible) + List uefiCompatibleHosts = List.of(host1); + Pair> uefiFilterResult = new Pair<>(true, uefiCompatibleHosts); + Mockito.doReturn(uefiFilterResult).when(spy).filterUefiHostsForMigration( + Mockito.anyList(), Mockito.anyList(), Mockito.any()); + + // Override hostAllocator to return only UEFI-compatible hosts + // Uses allocateTo WITH list parameter (filteredHosts is populated in zone-wide search) + Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList(), Mockito.anyInt(), Mockito.anyBoolean())) + .thenReturn(new ArrayList<>(uefiCompatibleHosts)); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify result structure + Assert.assertNotNull("Result should not be null", result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("All hosts list should contain 2 hosts", 2, result.first().first().size()); + + // Verify only UEFI-compatible hosts are in suitable list + Assert.assertEquals("Should have 1 UEFI-compatible suitable host", 1, result.second().size()); + Assert.assertTrue("Host 101 should be the only UEFI-compatible host", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertFalse("Host 102 should not be in suitable hosts (not UEFI-compatible)", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + + // Verify storage motion map is populated + Assert.assertNotNull("Storage motion map should not be null", result.third()); + } + + @Test + public void testListHostsForMigrationOfVMUefiFilteringReturnsEmpty() { + // Test case where UEFI filtering results in no suitable hosts + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + // Mock filterUefiHostsForMigration FIRST to return false (no UEFI-enabled hosts found) + // This simulates the scenario where UEFI VM has no compatible hosts + Pair> uefiFilterResult = new Pair<>(false, null); + Mockito.doReturn(uefiFilterResult).when(spy).filterUefiHostsForMigration( + Mockito.anyList(), Mockito.isNull(), Mockito.any()); + + // Note: No other mocks needed because when filterUefiHostsForMigration returns false, + // the method returns early and doesn't proceed to host allocation or other processing + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify result structure + Assert.assertNotNull("Result should not be null", result); + Assert.assertNotNull("All hosts list should not be null", result.first()); + Assert.assertNotNull("Suitable hosts list should not be null", result.second()); + Assert.assertNotNull("Storage motion map should not be null", result.third()); + + // Verify all hosts are still returned (from searchForServers) + Assert.assertEquals("Should still return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("All hosts list should contain 2 hosts", 2, result.first().first().size()); + + // Verify suitable hosts list is empty due to UEFI filtering + Assert.assertEquals("Should have 0 suitable hosts after UEFI filtering", 0, result.second().size()); + + // Verify storage motion map is empty + Assert.assertTrue("Storage motion map should be empty when no suitable hosts", result.third().isEmpty()); + } + + @Test + public void testListHostsForMigrationOfVMStorageMotionCapabilityCheck() { + // Test User VM with VMware - should check storage motion for User VMs + VMInstanceVO userVm = mockRunningVM(1L, HypervisorType.VMware); + Mockito.when(userVm.getType()).thenReturn(VirtualMachine.Type.User); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(userVm); + Mockito.when(serviceOfferingDetailsDao.findDetail(userVm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware); + Mockito.when(hostDao.findById(userVm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null)) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(userVm.getId(), userVm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(userVm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.VMware); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.VMware); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(userVm, srcHost, hosts, volume, true); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify storage motion capability was checked for User VM + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + + // Verify response data + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Host 101 should be in suitable list", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be in suitable list", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithAllSupportedHypervisors() { + // Test each supported hypervisor type + HypervisorType[] supportedTypes = { + HypervisorType.XenServer, + HypervisorType.VMware, + HypervisorType.KVM, + HypervisorType.Ovm, + HypervisorType.Hyperv, + HypervisorType.Ovm3 + }; + + for (HypervisorType hypervisorType : supportedTypes) { + VMInstanceVO vm = mockRunningVM(1L, hypervisorType); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, hypervisorType); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + String version = hypervisorType == HypervisorType.KVM ? "" : null; + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(hypervisorType, version)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, hypervisorType); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, hypervisorType); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify validation passed for this hypervisor + Assert.assertNotNull("Result should not be null for " + hypervisorType, result); + Assert.assertEquals("Should have 2 total hosts for " + hypervisorType, + Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts for " + hypervisorType, + 2, result.second().size()); + + // Reset mocks for next iteration + Mockito.reset(vmInstanceDao, hostDao, serviceOfferingDetailsDao, volumeDao, + diskOfferingDao, hypervisorCapabilitiesDao, offeringDao, dpdkHelper, + affinityGroupVMMapDao, dpMgr, dcDao, hostAllocator, dataStoreManager); + Mockito.reset(spy); + } + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMSourceHostNotFound() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(null); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test(expected = InvalidParameterValueException.class) + public void testListHostsForMigrationOfVMLocalStorageNoStorageMotion() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer); + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.XenServer); + Account caller = mockRootAdminAccount(); + + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // Mock storage motion not supported + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.XenServer, null)) + .thenReturn(false); + + // Mock local storage usage + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockLocalDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + } + + @Test + public void testListHostsForMigrationOfVMStorageMotionCheckForSystemVM() { + // Test that storage motion capability is checked for System VMs + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.VMware); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.ConsoleProxy); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // Storage motion supported for VMware + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null)) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.VMware); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.VMware); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify that storage motion capability was checked for system VM (VMware is in hypervisorTypes list) + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + + // Verify response structure + Assert.assertNotNull(result); + Assert.assertEquals("Should have 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertTrue("Should have suitable hosts", result.second().size() > 0); + } + + @Test + public void testListHostsForMigrationOfVMStorageMotionCheckForUserVM() { + // Test that storage motion capability is checked for User VMs + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // Storage motion supported for User VM with KVM + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify User VM can migrate with storage (User VM type always checks) + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + + // Verify response data + Assert.assertNotNull(result); + Assert.assertEquals("Should have 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertTrue("Should have suitable hosts", result.second().size() > 0); + } + + @Test + public void testListHostsForMigrationOfVMWithoutStorageMotionClusterScope() { + // When storage motion not supported, should search only in same cluster + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.XenServer); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // No storage motion support + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.XenServer, null)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers - verify cluster scope is used + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.XenServer); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.XenServer); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.eq(0L), Mockito.eq(20L), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.eq(1L), // cluster=1L + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.eq(100L)); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify XenServer without storage motion was checked + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.XenServer, null); + // Verify cluster-scoped search was used (not zone-wide) + Mockito.verify(spy).searchForServers( + Mockito.eq(0L), Mockito.eq(20L), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.eq(1L), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.eq(100L)); + + // Verify response data + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Should contain host 101", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Should contain host 102", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithNoVolumes() { + // Edge case: VM with no volumes + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + // No volumes + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(new ArrayList<>()); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + // Set up mocks without volume since there are no volumes + Pair> uefiResult = new Pair<>(true, hosts); + Mockito.doReturn(uefiResult).when(spy).filterUefiHostsForMigration( + Mockito.anyList(), Mockito.isNull(), Mockito.any()); + Mockito.when(dpdkHelper.isVMDpdkEnabled(vm.getId())).thenReturn(false); + Mockito.when(affinityGroupVMMapDao.countAffinityGroupsForVm(vm.getId())).thenReturn(0L); + DataCenterVO dc = Mockito.mock(DataCenterVO.class); + Mockito.when(dcDao.findById(1L)).thenReturn(dc); + Mockito.doNothing().when(dpMgr).checkForNonDedicatedResources(Mockito.any(), Mockito.any(), Mockito.any()); + Mockito.doNothing().when(dpMgr).reorderHostsByPriority(Mockito.any(), Mockito.anyList()); + + Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList(), Mockito.anyInt(), Mockito.anyBoolean())) + .thenReturn(new ArrayList<>(hosts)); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Should still process without throwing exception for usesLocal check + Mockito.verify(volumeDao).findCreatedByInstance(vm.getId()); + + // Verify response + Assert.assertNotNull(result); + Assert.assertEquals("Should have 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts even with no volumes", 2, result.second().size()); + Assert.assertTrue("Storage motion map should be empty for VM with no volumes", + result.third().isEmpty()); + } + + @Test + public void testListHostsForMigrationOfVMOverloadedMethod() { + // Test the overloaded method that takes vmId instead of vm object + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search with keyword + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.eq("keyword-test"), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + // Call overloaded method with vmId + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, "keyword-test"); + + // Verify VM was fetched by ID + Mockito.verify(vmInstanceDao).findById(1L); + + // Verify keyword was passed to searchForServers + Mockito.verify(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.eq("keyword-test"), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + // Verify response data + Assert.assertNotNull(result); + Assert.assertEquals("Should have 2 hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + } + + @Test + public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() { + // VMware should check storage motion even for non-User VMs + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.VMware); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.DomainRouter); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + // VMware with DomainRouter should still check storage motion + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null)) + .thenReturn(true); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers with zone-wide scope (storage motion enabled) + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.VMware); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.VMware); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.any(HypervisorType.class), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume, true); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify VMware always checks storage motion (hypervisorTypes list includes VMware) + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + + // Verify response + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Host 101 should be in the list", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + } + + @Test + public void testListHostsForMigrationOfVMOvmHypervisor() { + // Test OVM hypervisor support + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Ovm); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Ovm); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Ovm, null)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Ovm); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Ovm); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify OVM is in supported hypervisors list + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Ovm, null); + + // Verify response + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts for OVM", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts for OVM", 2, result.second().size()); + Assert.assertTrue("Host 101 should be available", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be available", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMHypervHypervisor() { + // Test Hyperv hypervisor support + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Hyperv); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Hyperv); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Hyperv, null)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Hyperv); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Hyperv); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify Hyperv is in supported hypervisors list + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Hyperv, null); + + // Verify response + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts for Hyperv", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts for Hyperv", 2, result.second().size()); + Assert.assertTrue("Host 101 should be available", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be available", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMOvm3Hypervisor() { + // Test Ovm3 hypervisor support + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Ovm3); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Ovm3); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Ovm3, null)) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Ovm3); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Ovm3); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify Ovm3 is in supported hypervisors list + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Ovm3, null); + + // Verify response + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts for Ovm3", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts for Ovm3", 2, result.second().size()); + Assert.assertTrue("Host 101 should be available", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be available", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + @Test + public void testListHostsForMigrationOfVMWithNullKeyword() { + // Test with null keyword parameter + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + + Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); + Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) + .thenReturn(null); + + HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.KVM); + Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); + + Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.KVM, "")) + .thenReturn(false); + + ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); + + VolumeVO volume = mockVolume(1L, 1L); + Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); + + DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); + Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); + + // Mock searchForServers for cluster-scoped search + HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); + HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); + List hosts = List.of(host1, host2); + Pair, Integer> hostsPair = new Pair<>(hosts, 2); + Mockito.doReturn(hostsPair).when(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + setupMigrationMocks(vm, srcHost, hosts, volume); + + Ternary, Integer>, List, Map> result = + spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + + // Verify null keyword is handled + Mockito.verify(vmInstanceDao).findById(1L); + + // Verify searchForServers was called with null keyword + Mockito.verify(spy).searchForServers( + Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), + Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), + Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); + + // Verify response data + Assert.assertNotNull(result); + Assert.assertEquals("Should return 2 total hosts", Integer.valueOf(2), result.first().second()); + Assert.assertEquals("Should have 2 suitable hosts", 2, result.second().size()); + Assert.assertTrue("Host 101 should be available", + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be available", + result.second().stream().anyMatch(h -> h.getId() == 102L)); + } + + // Note: Tests for success scenarios with complex flows (managed storage, zone-wide volumes, + // DPDK exclusion, affinity groups, architecture filtering) require full setup with mocking + // private methods like hasSuitablePoolsForVolume(), excludeNonDPDKEnabledHosts(), and + // filterUefiHostsForMigration() which are better suited for integration tests. + + // ============= Helper methods for tests ============= + + /** + * Sets up common mocks for successful migration tests + * For storage motion tests, set forceStorageMotion=true to configure volume in same cluster + * (which avoids complex filtering logic for cross-cluster storage motion) + */ + private void setupMigrationMocks(VMInstanceVO vm, HostVO srcHost, + List targetHosts, VolumeVO volume) { + setupMigrationMocks(vm, srcHost, targetHosts, volume, false); + } + + private void setupMigrationMocks(VMInstanceVO vm, HostVO srcHost, + List targetHosts, VolumeVO volume, + boolean forceStorageMotion) { + // Mock dataStoreManager for volume pool lookup (lenient as not used in all paths) + // For storage motion tests, put volume in same cluster to avoid complex filtering + PrimaryDataStore primaryDataStore = Mockito.mock(PrimaryDataStore.class); + Mockito.when(dataStoreManager.getPrimaryDataStore(volume.getPoolId())).thenReturn(primaryDataStore); + // If not forceStorageMotion, volume is in same cluster (no storage motion needed) + // If forceStorageMotion, set volClusterId to null (zone-wide storage) + Mockito.when(primaryDataStore.getClusterId()).thenReturn(forceStorageMotion ? null : srcHost.getClusterId()); + + // Mock zoneWideVolumeRequiresStorageMotion for zone-wide volumes + if (forceStorageMotion) { + Mockito.doReturn(false).when(spy).zoneWideVolumeRequiresStorageMotion( + Mockito.any(), Mockito.any(), Mockito.any()); + } + + // Mock filterUefiHostsForMigration - must return hosts properly + Pair> uefiResult = new Pair<>(true, new ArrayList<>(targetHosts)); + Mockito.doReturn(uefiResult).when(spy).filterUefiHostsForMigration( + Mockito.anyList(), Mockito.anyList(), Mockito.any()); + + // Mock DPDK check + Mockito.when(dpdkHelper.isVMDpdkEnabled(vm.getId())).thenReturn(false); + + // Mock affinity group count + Mockito.when(affinityGroupVMMapDao.countAffinityGroupsForVm(vm.getId())).thenReturn(0L); + + // Mock datacenter + DataCenterVO dc = Mockito.mock(DataCenterVO.class); + Mockito.when(dcDao.findById(srcHost.getDataCenterId())).thenReturn(dc); + + // Mock dedicated resources check + Mockito.doNothing().when(dpMgr).checkForNonDedicatedResources( + Mockito.any(), Mockito.any(), Mockito.any()); + + // Mock priority reordering + Mockito.doNothing().when(dpMgr).reorderHostsByPriority(Mockito.any(), Mockito.anyList()); + + // Mock host allocators - both signatures + // 1. Version with filteredHosts list (used when canMigrateWithStorage = true) + Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList(), Mockito.anyInt(), Mockito.anyBoolean())) + .thenReturn(new ArrayList<>(targetHosts)); + // 2. Version without filteredHosts list (used when canMigrateWithStorage = false) + Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyInt(), Mockito.anyBoolean())) + .thenReturn(new ArrayList<>(targetHosts)); + } + + private VMInstanceVO mockRunningVM(Long id, HypervisorType hypervisorType) { + return mockVM(id, hypervisorType, State.Running); + } + + private VMInstanceVO mockVM(Long id, HypervisorType hypervisorType, State state) { + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + when(vm.getId()).thenReturn(id); + when(vm.getInstanceName()).thenReturn("VM-" + id); + when(vm.getState()).thenReturn(state); + when(vm.getHypervisorType()).thenReturn(hypervisorType); + when(vm.getHostId()).thenReturn(100L); + when(vm.getServiceOfferingId()).thenReturn(1L); + when(vm.getType()).thenReturn(VirtualMachine.Type.User); + when(vm.getUuid()).thenReturn("uuid-" + id); + when(vm.getDataCenterId()).thenReturn(1L); + return vm; + } + + private Account mockRootAdminAccount() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(_accountMgr.isRootAdmin(1L)).thenReturn(true); + return account; + } + + private HostVO mockHost(Long id, Long clusterId, Long podId, Long dcId, HypervisorType hypervisorType) { + HostVO host = new HostVO("guid-" + id); + ReflectionTestUtils.setField(host, "id", id); + ReflectionTestUtils.setField(host, "clusterId", clusterId); + ReflectionTestUtils.setField(host, "podId", podId); + ReflectionTestUtils.setField(host, "dataCenterId", dcId); + ReflectionTestUtils.setField(host, "hypervisorType", hypervisorType); + ReflectionTestUtils.setField(host, "type", Host.Type.Routing); + ReflectionTestUtils.setField(host, "hypervisorVersion", null); + return host; + } + + private VolumeVO mockVolume(Long id, Long poolId) { + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getId()).thenReturn(id); + when(volume.getPoolId()).thenReturn(poolId); + when(volume.getDiskOfferingId()).thenReturn(1L); + when(volume.getVolumeType()).thenReturn(Volume.Type.ROOT); + return volume; + } + + private DiskOfferingVO mockLocalDiskOffering(Long id) { + DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); + Mockito.when(diskOffering.getId()).thenReturn(id); + Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(true); + return diskOffering; + } + + private DiskOfferingVO mockSharedDiskOffering(Long id) { + DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); + Mockito.when(diskOffering.getId()).thenReturn(id); + Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false); + return diskOffering; + } } From 1f28df8a2d1186df38f326ad9df079689dfd6626 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Thu, 6 Nov 2025 17:35:46 +0530 Subject: [PATCH 2/6] Refactor listHostsforMigrationofVM method --- .../cloud/server/ManagementServerImpl.java | 125 +++++++++++++++--- .../server/ManagementServerImplTest.java | 14 +- 2 files changed, 110 insertions(+), 29 deletions(-) diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 179d691e101a..ca9b2bf2dadc 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1454,17 +1454,27 @@ protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDat return false; } - @Override - public Ternary, Integer>, List, Map> listHostsForMigrationOfVM(final VirtualMachine vm, final Long startIndex, final Long pageSize, - final String keyword, List vmList) { - - validateVmForHostMigration(vm); + /** + * Get technically compatible hosts for VM migration (storage, hypervisor, UEFI filtering). + * This determines which hosts are technically capable of hosting the VM based on + * storage requirements, hypervisor capabilities, and UEFI requirements. + * + * @param vm The virtual machine to migrate + * @param startIndex Starting index for pagination + * @param pageSize Page size for pagination + * @param keyword Keyword filter for host search + * @return Ternary containing: (all hosts with count, filtered compatible hosts, storage motion requirements map) + */ + protected Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( + final VirtualMachine vm, + final Long startIndex, + final Long pageSize, + final String keyword) { + // GPU check if (_serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()) != null) { logger.info(" Live Migration of GPU enabled VM : " + vm.getInstanceName() + " is not supported"); - // Return empty list. - return new Ternary<>(new Pair<>(new ArrayList<>(), 0), - new ArrayList<>(), new HashMap<>()); + return new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); } final long srcHostId = vm.getHostId(); @@ -1478,6 +1488,7 @@ public Ternary, Integer>, List, Map, Integer>, List, Map allHosts = null; List filteredHosts = null; final Map requiresStorageMotion = new HashMap<>(); - DataCenterDeployment plan = null; + if (canMigrateWithStorage) { Long podId = !VirtualMachine.Type.User.equals(vm.getType()) ? srcHost.getPodId() : null; allHostsPair = searchForServers(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), podId, null, null, keyword, @@ -1562,7 +1573,6 @@ public Ternary, Integer>, List, Map(new Pair<>(allHosts, allHostsPair.second()), new ArrayList<>(), new HashMap<>()); } - plan = new DataCenterDeployment(srcHost.getDataCenterId(), podId, null, null, null, null); } else { final Long cluster = srcHost.getClusterId(); if (logger.isDebugEnabled()) { @@ -1571,19 +1581,37 @@ public Ternary, Integer>, List, Map, Integer> otherHosts = new Pair<>(allHosts, allHostsPair.second()); + final Pair, Integer> allHostsPairResult = new Pair<>(allHosts, allHostsPair.second()); Pair> uefiFilteredResult = filterUefiHostsForMigration(allHosts, filteredHosts, vm); if (!uefiFilteredResult.first()) { - return new Ternary<>(otherHosts, new ArrayList<>(), new HashMap<>()); + return new Ternary<>(allHostsPairResult, new ArrayList<>(), new HashMap<>()); } filteredHosts = uefiFilteredResult.second(); - List suitableHosts = new ArrayList<>(); + return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion); + } + + /** + * Apply affinity group constraints and other exclusion rules for VM migration. + * This builds an ExcludeList based on affinity groups, DPDK requirements, and dedicated resources. + * + * @param vm The virtual machine to migrate + * @param vmProfile The VM profile + * @param plan The deployment plan + * @param vmList List of VMs with current/simulated placements for affinity processing + * @return ExcludeList containing hosts to avoid + */ + protected ExcludeList applyAffinityConstraints( + final VirtualMachine vm, + final VirtualMachineProfile vmProfile, + final DataCenterDeployment plan, + final List vmList) { + final ExcludeList excludes = new ExcludeList(); - excludes.addHost(srcHostId); + excludes.addHost(vm.getHostId()); if (dpdkHelper.isVMDpdkEnabled(vm.getId())) { excludeNonDPDKEnabledHosts(plan, excludes); @@ -1599,13 +1627,37 @@ public Ternary, Integer>, List, Map getCapableSuitableHosts( + final VirtualMachine vm, + final VirtualMachineProfile vmProfile, + final DataCenterDeployment plan, + final List compatibleHosts, + final ExcludeList excludes, + final Host srcHost) { + + List suitableHosts = new ArrayList<>(); + for (final HostAllocator allocator : hostAllocators) { - if (CollectionUtils.isNotEmpty(filteredHosts)) { - suitableHosts = allocator.allocateTo(vmProfile, plan, Host.Type.Routing, excludes, filteredHosts, HostAllocator.RETURN_UPTO_ALL, false); + if (CollectionUtils.isNotEmpty(compatibleHosts)) { + suitableHosts = allocator.allocateTo(vmProfile, plan, Host.Type.Routing, excludes, compatibleHosts, HostAllocator.RETURN_UPTO_ALL, false); } else { suitableHosts = allocator.allocateTo(vmProfile, plan, Host.Type.Routing, excludes, HostAllocator.RETURN_UPTO_ALL, false); } @@ -1631,6 +1683,43 @@ public Ternary, Integer>, List, Map, Integer>, List, Map> listHostsForMigrationOfVM(final VirtualMachine vm, final Long startIndex, final Long pageSize, + final String keyword, List vmList) { + + validateVmForHostMigration(vm); + + // Get technically compatible hosts (storage, hypervisor, UEFI) + Ternary, Integer>, List, Map> compatibilityResult = + getTechnicallyCompatibleHosts(vm, startIndex, pageSize, keyword); + + Pair, Integer> allHostsPair = compatibilityResult.first(); + List filteredHosts = compatibilityResult.second(); + Map requiresStorageMotion = compatibilityResult.third(); + + // If no compatible hosts, return early + if (CollectionUtils.isEmpty(filteredHosts)) { + final Pair, Integer> otherHosts = new Pair<>(allHostsPair.first(), allHostsPair.second()); + return new Ternary<>(otherHosts, new ArrayList<>(), requiresStorageMotion); + } + + // Create deployment plan and VM profile + final Host srcHost = _hostDao.findById(vm.getHostId()); + final DataCenterDeployment plan = new DataCenterDeployment( + srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null); + final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl( + vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null); + + // Apply affinity constraints + final ExcludeList excludes = applyAffinityConstraints(vm, vmProfile, plan, vmList); + + // Get hosts with capacity + List suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes, srcHost); + + final Pair, Integer> otherHosts = new Pair<>(allHostsPair.first(), allHostsPair.second()); return new Ternary<>(otherHosts, suitableHosts, requiresStorageMotion); } diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index 4d5c5af2be12..cc43f710290d 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -1134,10 +1134,6 @@ public void testListHostsForMigrationOfVMNonUefiVm() { DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); - // No UEFI detail - VM is NOT UEFI-enabled - Mockito.when(userVmDetailsDao.findDetail(vm.getId(), ApiConstants.BootType.UEFI.toString())) - .thenReturn(null); - // Mock searchForServers for cluster-scoped search HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.KVM); HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.KVM); @@ -1206,7 +1202,7 @@ public void testListHostsForMigrationOfVMWithUefiVmClusterScope() { List uefiCompatibleHosts = List.of(host1); Pair> uefiFilterResult = new Pair<>(true, uefiCompatibleHosts); Mockito.doReturn(uefiFilterResult).when(spy).filterUefiHostsForMigration( - Mockito.anyList(), Mockito.isNull(), Mockito.any()); + Mockito.anyList(), Mockito.anyList(), Mockito.any()); // Setup other mocks Mockito.when(dpdkHelper.isVMDpdkEnabled(vm.getId())).thenReturn(false); @@ -1351,7 +1347,7 @@ public void testListHostsForMigrationOfVMUefiFilteringReturnsEmpty() { // This simulates the scenario where UEFI VM has no compatible hosts Pair> uefiFilterResult = new Pair<>(false, null); Mockito.doReturn(uefiFilterResult).when(spy).filterUefiHostsForMigration( - Mockito.anyList(), Mockito.isNull(), Mockito.any()); + Mockito.anyList(), Mockito.anyList(), Mockito.any()); // Note: No other mocks needed because when filterUefiHostsForMigration returns false, // the method returns early and doesn't proceed to host allocation or other processing @@ -1747,7 +1743,7 @@ public void testListHostsForMigrationOfVMWithNoVolumes() { // Set up mocks without volume since there are no volumes Pair> uefiResult = new Pair<>(true, hosts); Mockito.doReturn(uefiResult).when(spy).filterUefiHostsForMigration( - Mockito.anyList(), Mockito.isNull(), Mockito.any()); + Mockito.anyList(), Mockito.anyList(), Mockito.any()); Mockito.when(dpdkHelper.isVMDpdkEnabled(vm.getId())).thenReturn(false); Mockito.when(affinityGroupVMMapDao.countAffinityGroupsForVm(vm.getId())).thenReturn(0L); DataCenterVO dc = Mockito.mock(DataCenterVO.class); @@ -2175,10 +2171,6 @@ private void setupMigrationMocks(VMInstanceVO vm, HostVO srcHost, Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyList(), Mockito.anyInt(), Mockito.anyBoolean())) .thenReturn(new ArrayList<>(targetHosts)); - // 2. Version without filteredHosts list (used when canMigrateWithStorage = false) - Mockito.when(hostAllocator.allocateTo(Mockito.any(), Mockito.any(), Mockito.any(), - Mockito.any(), Mockito.anyInt(), Mockito.anyBoolean())) - .thenReturn(new ArrayList<>(targetHosts)); } private VMInstanceVO mockRunningVM(Long id, HypervisorType hypervisorType) { From c6c5689d58a14a6d0e86743d1bca67d52fde76dc Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Thu, 6 Nov 2025 22:11:06 +0530 Subject: [PATCH 3/6] Optimise DRS plan generation --- .../com/cloud/server/ManagementService.java | 29 + .../cluster/ClusterDrsAlgorithm.java | 32 + .../apache/cloudstack/cluster/Balanced.java | 14 +- .../apache/cloudstack/cluster/Condensed.java | 15 +- .../cloud/server/ManagementServerImpl.java | 38 +- .../cluster/ClusterDrsServiceImpl.java | 164 +++++- .../cluster/ClusterDrsServiceImplTest.java | 555 +++++++++++++++++- 7 files changed, 795 insertions(+), 52 deletions(-) diff --git a/api/src/main/java/com/cloud/server/ManagementService.java b/api/src/main/java/com/cloud/server/ManagementService.java index d6d7f7a59c68..21d6c35dab4b 100644 --- a/api/src/main/java/com/cloud/server/ManagementService.java +++ b/api/src/main/java/com/cloud/server/ManagementService.java @@ -71,6 +71,8 @@ import com.cloud.capacity.Capacity; import com.cloud.dc.Pod; import com.cloud.dc.Vlan; +import com.cloud.deploy.DeploymentPlan; +import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.ManagementServerException; import com.cloud.exception.ResourceUnavailableException; @@ -91,6 +93,7 @@ import com.cloud.vm.InstanceGroup; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.Type; +import com.cloud.vm.VirtualMachineProfile; /** * Hopefully this is temporary. @@ -452,6 +455,32 @@ public interface ManagementService { Ternary, Integer>, List, Map> listHostsForMigrationOfVM(VirtualMachine vm, Long startIndex, Long pageSize, String keyword, List vmList); + /** + * Get technically compatible hosts for VM migration (storage, hypervisor, UEFI filtering). + * This is a helper method that can be used independently for caching in DRS planning. + * + * @param vm The virtual machine to migrate + * @param startIndex Starting index for pagination + * @param pageSize Page size for pagination + * @param keyword Keyword filter for host search + * @return Ternary containing: (all hosts with count, filtered compatible hosts, storage motion requirements map) + */ + Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( + VirtualMachine vm, Long startIndex, Long pageSize, String keyword); + + /** + * Apply affinity group constraints and other exclusion rules for VM migration. + * This is a helper method that can be used independently for per-iteration affinity checks in DRS. + * + * @param vm The virtual machine to migrate + * @param vmProfile The VM profile + * @param plan The deployment plan + * @param vmList List of VMs with current/simulated placements for affinity processing + * @return ExcludeList containing hosts to avoid + */ + ExcludeList applyAffinityConstraints(VirtualMachine vm, VirtualMachineProfile vmProfile, + DeploymentPlan plan, List vmList); + /** * List storage pools for live migrating of a volume. The API returns list of all pools in the cluster to which the * volume can be migrated. Current pool is not included in the list. In case of vSphere datastore cluster storage pools, diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java index 665f95842b0d..beaf1df4d4ab 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java @@ -85,6 +85,38 @@ Ternary getMetrics(Cluster cluster, VirtualMachine vm, S Map> hostMemoryMap, Boolean requiresStorageMotion) throws ConfigurationException; + /** + * Determines the metrics for a given virtual machine and destination host in a DRS cluster. + * This overloaded version accepts a pre-calculated pre-imbalance value to avoid recalculating + * it for every VM-host combination within the same iteration. + * + * @param cluster + * the cluster to check + * @param vm + * the virtual machine to check + * @param serviceOffering + * the service offering for the virtual machine + * @param destHost + * the destination host for the virtual machine + * @param hostCpuMap + * a map of host IDs to the Ternary of used, reserved and total CPU on each host + * @param hostMemoryMap + * a map of host IDs to the Ternary of used, reserved and total memory on each host + * @param requiresStorageMotion + * whether storage motion is required for the virtual machine + * @param preImbalance + * the pre-calculated cluster imbalance before migration (null to calculate it) + * + * @return a ternary containing improvement, cost, benefit + */ + default Ternary getMetrics(Cluster cluster, VirtualMachine vm, ServiceOffering serviceOffering, + Host destHost, Map> hostCpuMap, + Map> hostMemoryMap, + Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { + // Default implementation delegates to the original method for backward compatibility + return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion); + } + /** * Calculates the imbalance of the cluster after a virtual machine migration. * diff --git a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java index b6a5ed1aac1a..90743f3e893f 100644 --- a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java +++ b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java @@ -74,9 +74,21 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin Map> hostCpuMap, Map> hostMemoryMap, Boolean requiresStorageMotion) throws ConfigurationException { Double preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); + return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion, preImbalance); + } + + @Override + public Ternary getMetrics(Cluster cluster, VirtualMachine vm, + ServiceOffering serviceOffering, Host destHost, + Map> hostCpuMap, Map> hostMemoryMap, + Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { + // Use provided pre-imbalance if available, otherwise calculate it + if (preImbalance == null) { + preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); + } Double postImbalance = getImbalancePostMigration(serviceOffering, vm, destHost, hostCpuMap, hostMemoryMap); - logger.debug("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); // This needs more research to determine the cost and benefit of a migration diff --git a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java index 70c5acd951fe..4fa80198b1a2 100644 --- a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java +++ b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java @@ -78,9 +78,22 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin Boolean requiresStorageMotion) throws ConfigurationException { Double preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); + return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion, preImbalance); + } + + @Override + public Ternary getMetrics(Cluster cluster, VirtualMachine vm, + ServiceOffering serviceOffering, Host destHost, + Map> hostCpuMap, Map> hostMemoryMap, + Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { + // Use provided pre-imbalance if available, otherwise calculate it + if (preImbalance == null) { + preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), + new ArrayList<>(hostMemoryMap.values()), null); + } Double postImbalance = getImbalancePostMigration(serviceOffering, vm, destHost, hostCpuMap, hostMemoryMap); - logger.debug("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); // This needs more research to determine the cost and benefit of a migration diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index ca9b2bf2dadc..3031ec2157c6 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -692,6 +692,7 @@ import com.cloud.dc.dao.PodVlanMapDao; import com.cloud.dc.dao.VlanDao; import com.cloud.deploy.DataCenterDeployment; +import com.cloud.deploy.DeploymentPlan; import com.cloud.deploy.DeploymentPlanner; import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.deploy.DeploymentPlanningManager; @@ -1465,7 +1466,8 @@ protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDat * @param keyword Keyword filter for host search * @return Ternary containing: (all hosts with count, filtered compatible hosts, storage motion requirements map) */ - protected Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( + @Override + public Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( final VirtualMachine vm, final Long startIndex, final Long pageSize, @@ -1474,7 +1476,8 @@ protected Ternary, Integer>, List, Map> // GPU check if (_serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()) != null) { logger.info(" Live Migration of GPU enabled VM : " + vm.getInstanceName() + " is not supported"); - return new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); + return new Ternary, Integer>, List, Map>( + new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); } final long srcHostId = vm.getHostId(); @@ -1571,7 +1574,8 @@ protected Ternary, Integer>, List, Map> } if (CollectionUtils.isEmpty(filteredHosts)) { - return new Ternary<>(new Pair<>(allHosts, allHostsPair.second()), new ArrayList<>(), new HashMap<>()); + return new Ternary, Integer>, List, Map>( + new Pair<>(allHosts, allHostsPair.second()), new ArrayList<>(), new HashMap<>()); } } else { final Long cluster = srcHost.getClusterId(); @@ -1584,14 +1588,16 @@ protected Ternary, Integer>, List, Map> filteredHosts = allHosts; } - final Pair, Integer> allHostsPairResult = new Pair<>(allHosts, allHostsPair.second()); + final Pair, Integer> allHostsPairResult = new Pair<>(allHosts, allHostsPair.second()); Pair> uefiFilteredResult = filterUefiHostsForMigration(allHosts, filteredHosts, vm); if (!uefiFilteredResult.first()) { - return new Ternary<>(allHostsPairResult, new ArrayList<>(), new HashMap<>()); + return new Ternary, Integer>, List, Map>( + allHostsPairResult, new ArrayList<>(), new HashMap<>()); } filteredHosts = uefiFilteredResult.second(); - return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion); + return new Ternary, Integer>, List, Map>( + allHostsPairResult, filteredHosts, requiresStorageMotion); } /** @@ -1604,17 +1610,15 @@ protected Ternary, Integer>, List, Map> * @param vmList List of VMs with current/simulated placements for affinity processing * @return ExcludeList containing hosts to avoid */ - protected ExcludeList applyAffinityConstraints( - final VirtualMachine vm, - final VirtualMachineProfile vmProfile, - final DataCenterDeployment plan, - final List vmList) { - + @Override + public ExcludeList applyAffinityConstraints(VirtualMachine vm, VirtualMachineProfile vmProfile, DeploymentPlan plan, List vmList) { final ExcludeList excludes = new ExcludeList(); excludes.addHost(vm.getHostId()); if (dpdkHelper.isVMDpdkEnabled(vm.getId())) { - excludeNonDPDKEnabledHosts(plan, excludes); + if (plan instanceof DataCenterDeployment) { + excludeNonDPDKEnabledHosts((DataCenterDeployment) plan, excludes); + } } // call affinitygroup chain @@ -1649,7 +1653,7 @@ protected List getCapableSuitableHosts( final VirtualMachine vm, final VirtualMachineProfile vmProfile, final DataCenterDeployment plan, - final List compatibleHosts, + final List compatibleHosts, final ExcludeList excludes, final Host srcHost) { @@ -1693,11 +1697,11 @@ public Ternary, Integer>, List, Map, Integer>, List, Map> compatibilityResult = + Ternary, Integer>, List, Map> compatibilityResult = getTechnicallyCompatibleHosts(vm, startIndex, pageSize, keyword); - Pair, Integer> allHostsPair = compatibilityResult.first(); - List filteredHosts = compatibilityResult.second(); + Pair, Integer> allHostsPair = compatibilityResult.first(); + List filteredHosts = compatibilityResult.second(); Map requiresStorageMotion = compatibilityResult.third(); // If no compatible hosts, return early diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index a662d47d4541..84edfb44d009 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -24,6 +24,8 @@ import com.cloud.api.query.vo.HostJoinVO; import com.cloud.dc.ClusterVO; import com.cloud.dc.dao.ClusterDao; +import com.cloud.deploy.DataCenterDeployment; +import com.cloud.deploy.DeploymentPlanner.ExcludeList; import com.cloud.domain.Domain; import com.cloud.event.ActionEventUtils; import com.cloud.event.EventTypes; @@ -51,6 +53,8 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachineProfile; +import com.cloud.vm.VirtualMachineProfileImpl; import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.api.ApiCommandResourceType; @@ -62,6 +66,8 @@ import org.apache.cloudstack.api.response.ClusterDrsPlanMigrationResponse; import org.apache.cloudstack.api.response.ClusterDrsPlanResponse; import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.affinity.AffinityGroupVMMapVO; +import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; import org.apache.cloudstack.cluster.dao.ClusterDrsPlanDao; import org.apache.cloudstack.cluster.dao.ClusterDrsPlanMigrationDao; import org.apache.cloudstack.context.CallContext; @@ -81,13 +87,16 @@ import java.util.Collections; import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.Timer; import java.util.TimerTask; import java.util.stream.Collectors; import static com.cloud.org.Grouping.AllocationState.Disabled; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterImbalance; public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsService, PluggableService { @@ -125,6 +134,9 @@ public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsServ @Inject ManagementServer managementServer; + @Inject + AffinityGroupVMMapDao affinityGroupVMMapDao; + List drsAlgorithms = new ArrayList<>(); Map drsAlgorithmMap = new HashMap<>(); @@ -357,10 +369,86 @@ List> getDrsPlan(Cluster cluster, serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId())); } + // PHASE 1: Pre-compute technical compatibility (once per eligible VM - never changes) + Map> vmToCompatibleHostsCache = new HashMap<>(); + Map> vmToStorageMotionCache = new HashMap<>(); + + for (VirtualMachine vm : vmList) { + // Skip ineligible VMs + if (vm.getType().isUsedBySystem() || + vm.getState() != VirtualMachine.State.Running || + (MapUtils.isNotEmpty(vm.getDetails()) && + "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) { + continue; + } + + try { + Ternary, Integer>, List, Map> compatibility = + managementServer.getTechnicallyCompatibleHosts(vm, 0L, 500L, null); + + List compatibleHosts = compatibility.second(); // Get filtered hosts + Map requiresStorageMotion = compatibility.third(); + + if (compatibleHosts != null && !compatibleHosts.isEmpty()) { + vmToCompatibleHostsCache.put(vm.getId(), compatibleHosts); + vmToStorageMotionCache.put(vm.getId(), requiresStorageMotion); + } + } catch (Exception e) { + logger.debug("Could not get compatible hosts for VM {}: {}", vm, e.getMessage()); + } + } + + // Pre-fetch affinity group mappings for all eligible VMs (once, before iterations) + // This allows us to skip expensive affinity processing for VMs without affinity groups + Set vmsWithAffinityGroups = new HashSet<>(); + for (VirtualMachine vm : vmList) { + if (vmToCompatibleHostsCache.containsKey(vm.getId())) { + // Check if VM has any affinity groups - if list is empty, VM has no affinity groups + List affinityGroupMappings = affinityGroupVMMapDao.listByInstanceId(vm.getId()); + if (affinityGroupMappings != null && !affinityGroupMappings.isEmpty()) { + vmsWithAffinityGroups.add(vm.getId()); + } + } + } + + // PHASE 2: Iteration loop with affinity check per iteration while (iteration < maxIterations && algorithm.needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()))) { + + logger.debug("Starting DRS iteration {} for cluster {}", iteration + 1, cluster); + // Re-evaluate affinity constraints with current (simulated) VM placements + Map vmToExcludesMap = new HashMap<>(); + for (VirtualMachine vm : vmList) { + if (vmToCompatibleHostsCache.containsKey(vm.getId())) { + Host srcHost = hostMap.get(vm.getHostId()); + if (srcHost != null) { + // Only call expensive applyAffinityConstraints for VMs with affinity groups + // For VMs without affinity groups, create minimal ExcludeList (just source host) + ExcludeList excludes; + if (vmsWithAffinityGroups.contains(vm.getId())) { + DataCenterDeployment plan = new DataCenterDeployment( + srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), + null, null, null); + VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, + vmIdServiceOfferingMap.get(vm.getId()), null, null); + + excludes = managementServer.applyAffinityConstraints( + vm, vmProfile, plan, vmList); + } else { + // VM has no affinity groups - create minimal ExcludeList (just source host) + excludes = new ExcludeList(); + excludes.addHost(vm.getHostId()); + } + vmToExcludesMap.put(vm.getId(), excludes); + } + } + } + + logger.debug("Completed affinity evaluation for DRS iteration {} for cluster {}", iteration + 1, cluster); + Pair bestMigration = getBestMigration(cluster, algorithm, vmList, - vmIdServiceOfferingMap, hostCpuMap, hostMemoryMap); + vmIdServiceOfferingMap, hostCpuMap, hostMemoryMap, + vmToCompatibleHostsCache, vmToStorageMotionCache, vmToExcludesMap); VirtualMachine vm = bestMigration.first(); Host destHost = bestMigration.second(); if (destHost == null || vm == null || originalHostIdVmIdMap.get(destHost.getId()).contains(vm.getId())) { @@ -372,8 +460,6 @@ List> getDrsPlan(Cluster cluster, ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId()); migrationPlan.add(new Ternary<>(vm, hostMap.get(vm.getHostId()), hostMap.get(destHost.getId()))); - hostVmMap.get(vm.getHostId()).remove(vm); - hostVmMap.get(destHost.getId()).add(vm); hostVmMap.get(vm.getHostId()).remove(vm); hostVmMap.get(destHost.getId()).add(vm); @@ -429,6 +515,12 @@ Map> getHostVmMap(List hostList, List getBestMigration(Cluster cluster, ClusterDrsAlgorithm List vmList, Map vmIdServiceOfferingMap, Map> hostCpuCapacityMap, - Map> hostMemoryCapacityMap) throws ConfigurationException { + Map> hostMemoryCapacityMap, + Map> vmToCompatibleHostsCache, + Map> vmToStorageMotionCache, + Map vmToExcludesMap) throws ConfigurationException { + // Pre-calculate cluster imbalance once per iteration (same for all VM-host combinations) + Double preImbalance = getClusterImbalance(cluster.getId(), + new ArrayList<>(hostCpuCapacityMap.values()), + new ArrayList<>(hostMemoryCapacityMap.values()), + null); + double improvement = 0; Pair bestMigration = new Pair<>(null, null); for (VirtualMachine vm : vmList) { if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running || (MapUtils.isNotEmpty(vm.getDetails()) && - vm.getDetails().get(VmDetailConstants.SKIP_DRS).equalsIgnoreCase("true")) + "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) ) { continue; } - Ternary, Integer>, List, Map> hostsForMigrationOfVM = managementServer - .listHostsForMigrationOfVM( - vm, 0L, 500L, null, vmList); - List compatibleDestinationHosts = hostsForMigrationOfVM.first().first(); - List suitableDestinationHosts = hostsForMigrationOfVM.second(); - Map requiresStorageMotion = hostsForMigrationOfVM.third(); + // Use cached compatible hosts + List compatibleHosts = vmToCompatibleHostsCache.get(vm.getId()); + Map requiresStorageMotion = vmToStorageMotionCache.get(vm.getId()); + ExcludeList excludes = vmToExcludesMap.get(vm.getId()); - for (Host destHost : compatibleDestinationHosts) { - if (!suitableDestinationHosts.contains(destHost) || cluster.getId() != destHost.getClusterId()) { + if (compatibleHosts == null || compatibleHosts.isEmpty()) { + continue; + } + + ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId()); + if (serviceOffering == null) { + continue; + } + + // Pre-calculate VM resource requirements for quick capacity filtering + long vmCpu = (long) serviceOffering.getCpu() * serviceOffering.getSpeed(); + long vmMemory = serviceOffering.getRamSize() * 1024L * 1024L; + + for (Host destHost : compatibleHosts) { + if (cluster.getId() != destHost.getClusterId()) { + continue; + } + + // Check affinity constraints + if (excludes != null && excludes.shouldAvoid(destHost)) { + continue; + } + + // Quick capacity pre-filter: skip hosts that don't have enough capacity + Ternary destHostCpu = hostCpuCapacityMap.get(destHost.getId()); + Ternary destHostMemory = hostMemoryCapacityMap.get(destHost.getId()); + if (destHostCpu == null || destHostMemory == null) { continue; } + + long destHostAvailableCpu = (destHostCpu.third() - destHostCpu.second()) - destHostCpu.first(); + long destHostAvailableMemory = (destHostMemory.third() - destHostMemory.second()) - destHostMemory.first(); + + if (destHostAvailableCpu < vmCpu || destHostAvailableMemory < vmMemory) { + continue; // Skip hosts without sufficient capacity + } + + // getMetrics uses updated capacity maps - this is the real capacity check + // Pass pre-calculated pre-imbalance to avoid recalculating it for every VM-host combination Ternary metrics = algorithm.getMetrics(cluster, vm, - vmIdServiceOfferingMap.get(vm.getId()), destHost, hostCpuCapacityMap, hostMemoryCapacityMap, - requiresStorageMotion.get(destHost)); + serviceOffering, destHost, hostCpuCapacityMap, hostMemoryCapacityMap, + requiresStorageMotion.getOrDefault(destHost, false), preImbalance); Double currentImprovement = metrics.first(); Double cost = metrics.second(); diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java index 81aac9e4b54d..a8b40bddd18b 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -42,6 +42,7 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.api.command.admin.cluster.GenerateClusterDrsPlanCmd; import org.apache.cloudstack.api.response.ClusterDrsPlanMigrationResponse; @@ -168,9 +169,14 @@ public void testGetDrsPlan() throws ConfigurationException { VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm1.getId()).thenReturn(1L); Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm2.getHostId()).thenReturn(2L); + Mockito.when(vm2.getId()).thenReturn(2L); + Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running); List hostList = new ArrayList<>(); hostList.add(host1); @@ -201,10 +207,11 @@ public void testGetDrsPlan() throws ConfigurationException { Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn( true, false); - Mockito.when( - clusterDrsService.getBestMigration(Mockito.any(Cluster.class), Mockito.any(ClusterDrsAlgorithm.class), - Mockito.anyList(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap())).thenReturn( - new Pair<>(vm1, host2)); + + Mockito.doReturn(new Pair<>(vm1, host2)).when(clusterDrsService).getBestMigration( + Mockito.any(Cluster.class), Mockito.any(ClusterDrsAlgorithm.class), + Mockito.anyList(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap(), + Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap()); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn( serviceOffering); Mockito.when(hostJoinDao.searchByIds(host1.getId(), host2.getId())).thenReturn(List.of(hostJoin1, hostJoin2)); @@ -219,6 +226,484 @@ public void testGetDrsPlan() throws ConfigurationException { assertEquals(1, iterations.size()); } + @Test + public void testGetDrsPlanWithDisabledCluster() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Disabled); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + } + + @Test + public void testGetDrsPlanWithZeroMaxIterations() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + List> result = clusterDrsService.getDrsPlan(cluster, 0); + assertEquals(0, result.size()); + } + + @Test + public void testGetDrsPlanWithNegativeMaxIterations() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + List> result = clusterDrsService.getDrsPlan(cluster, -1); + assertEquals(0, result.size()); + } + + @Test + public void testGetDrsPlanWithSystemVMs() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + VMInstanceVO systemVm = Mockito.mock(VMInstanceVO.class); + Mockito.when(systemVm.getId()).thenReturn(1L); + Mockito.when(systemVm.getHostId()).thenReturn(1L); + Mockito.when(systemVm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(systemVm); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + // System VM should not be processed for compatibility check + Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(systemVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + VMInstanceVO stoppedVm = Mockito.mock(VMInstanceVO.class); + Mockito.when(stoppedVm.getId()).thenReturn(1L); + Mockito.when(stoppedVm.getHostId()).thenReturn(1L); + Mockito.when(stoppedVm.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(stoppedVm.getState()).thenReturn(VirtualMachine.State.Stopped); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(stoppedVm); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + // Stopped VM should not be processed for compatibility check + Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(stoppedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + VMInstanceVO skippedVm = Mockito.mock(VMInstanceVO.class); + Mockito.when(skippedVm.getId()).thenReturn(1L); + Mockito.when(skippedVm.getHostId()).thenReturn(1L); + Mockito.when(skippedVm.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(skippedVm.getState()).thenReturn(VirtualMachine.State.Running); + Map details = new HashMap<>(); + details.put(VmDetailConstants.SKIP_DRS, "true"); + Mockito.when(skippedVm.getDetails()).thenReturn(details); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(skippedVm); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + // VM with SKIP_DRS flag should not be processed + Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(skippedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm1.getId()).thenReturn(1L); + Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(vm1); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + // Return empty compatible hosts list + Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any())).thenReturn( + new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>())); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + Mockito.verify(managementServer, Mockito.times(1)).getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + + VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm1.getId()).thenReturn(1L); + Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(vm1); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + // Throw exception during compatibility check + Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any())).thenThrow(new RuntimeException("Test exception")); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + // Exception should be caught and logged, not propagated + Mockito.verify(managementServer, Mockito.times(1)).getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + Mockito.when(host1.getDataCenterId()).thenReturn(1L); + Mockito.when(host1.getPodId()).thenReturn(1L); + Mockito.when(host1.getClusterId()).thenReturn(1L); + + VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm1.getId()).thenReturn(1L); + Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); + + List hostList = new ArrayList<>(); + hostList.add(host1); + + List vmList = new ArrayList<>(); + vmList.add(vm1); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); + + HostVO compatibleHost = Mockito.mock(HostVO.class); + Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any())).thenReturn( + new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + + // Return null migration (no best migration found) + Mockito.doReturn(new Pair<>(null, null)).when(clusterDrsService).getBestMigration( + Mockito.any(Cluster.class), Mockito.any(ClusterDrsAlgorithm.class), + Mockito.anyList(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap(), + Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap()); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + } + + @Test + public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + Mockito.when(host1.getDataCenterId()).thenReturn(1L); + Mockito.when(host1.getPodId()).thenReturn(1L); + Mockito.when(host1.getClusterId()).thenReturn(1L); + + HostVO host2 = Mockito.mock(HostVO.class); + Mockito.when(host2.getId()).thenReturn(2L); + + VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm1.getId()).thenReturn(1L); + Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); + + VMInstanceVO vm2 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm2.getId()).thenReturn(2L); + Mockito.when(vm2.getHostId()).thenReturn(1L); + Mockito.when(vm2.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm2.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm2.getDetails()).thenReturn(Collections.emptyMap()); + + List hostList = new ArrayList<>(); + hostList.add(host1); + hostList.add(host2); + + List vmList = new ArrayList<>(); + vmList.add(vm1); + vmList.add(vm2); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + HostJoinVO hostJoin2 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin2.getId()).thenReturn(2L); + Mockito.when(hostJoin2.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin2.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin2.getCpus()).thenReturn(4); + Mockito.when(hostJoin2.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin2.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin2.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin2.getTotalMemory()).thenReturn(8192L); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + Mockito.when(serviceOffering.getCpu()).thenReturn(1); + Mockito.when(serviceOffering.getRamSize()).thenReturn(1024); + Mockito.when(serviceOffering.getSpeed()).thenReturn(1000); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn( + true, true, false); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1, hostJoin2)); + + HostVO compatibleHost = Mockito.mock(HostVO.class); + Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.any(), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any())).thenReturn( + new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + + // Return migrations for first two iterations, then null + Mockito.doReturn(new Pair<>(vm1, host2), new Pair<>(vm2, host2), new Pair<>(null, null)) + .when(clusterDrsService).getBestMigration( + Mockito.any(Cluster.class), Mockito.any(ClusterDrsAlgorithm.class), + Mockito.anyList(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap(), + Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap()); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(2, result.size()); + Mockito.verify(balancedAlgorithm, Mockito.times(3)).needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList()); + } + + @Test + public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationException { + ClusterVO cluster = Mockito.mock(ClusterVO.class); + Mockito.when(cluster.getId()).thenReturn(1L); + Mockito.when(cluster.getAllocationState()).thenReturn(Grouping.AllocationState.Enabled); + + HostVO host1 = Mockito.mock(HostVO.class); + Mockito.when(host1.getId()).thenReturn(1L); + Mockito.when(host1.getDataCenterId()).thenReturn(1L); + Mockito.when(host1.getPodId()).thenReturn(1L); + Mockito.when(host1.getClusterId()).thenReturn(1L); + + HostVO host2 = Mockito.mock(HostVO.class); + Mockito.when(host2.getId()).thenReturn(2L); + + VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); + Mockito.when(vm1.getId()).thenReturn(1L); + Mockito.when(vm1.getHostId()).thenReturn(1L); + Mockito.when(vm1.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm1.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(vm1.getDetails()).thenReturn(Collections.emptyMap()); + + List hostList = new ArrayList<>(); + hostList.add(host1); + hostList.add(host2); + + List vmList = new ArrayList<>(); + vmList.add(vm1); + + HostJoinVO hostJoin1 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin1.getId()).thenReturn(1L); + Mockito.when(hostJoin1.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin1.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getCpus()).thenReturn(4); + Mockito.when(hostJoin1.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin1.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin1.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin1.getTotalMemory()).thenReturn(8192L); + + HostJoinVO hostJoin2 = Mockito.mock(HostJoinVO.class); + Mockito.when(hostJoin2.getId()).thenReturn(2L); + Mockito.when(hostJoin2.getCpuUsedCapacity()).thenReturn(1000L); + Mockito.when(hostJoin2.getCpuReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin2.getCpus()).thenReturn(4); + Mockito.when(hostJoin2.getSpeed()).thenReturn(1000L); + Mockito.when(hostJoin2.getMemUsedCapacity()).thenReturn(1024L); + Mockito.when(hostJoin2.getMemReservedCapacity()).thenReturn(0L); + Mockito.when(hostJoin2.getTotalMemory()).thenReturn(8192L); + + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + + Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); + Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); + Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); + Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1, hostJoin2)); + + HostVO compatibleHost = Mockito.mock(HostVO.class); + Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), + Mockito.anyLong(), Mockito.any())).thenReturn( + new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); + Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + + // Return migration to original host (host1) - should break the loop + Mockito.doReturn(new Pair<>(vm1, host1)).when(clusterDrsService).getBestMigration( + Mockito.any(Cluster.class), Mockito.any(ClusterDrsAlgorithm.class), + Mockito.anyList(), Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap(), + Mockito.anyMap(), Mockito.anyMap(), Mockito.anyMap()); + + List> result = clusterDrsService.getDrsPlan(cluster, 5); + assertEquals(0, result.size()); + // Should break early when VM would migrate to original host + } + @Test(expected = InvalidParameterValueException.class) public void testGenerateDrsPlanClusterNotFound() { Mockito.when(clusterDao.findById(1L)).thenReturn(null); @@ -387,18 +872,36 @@ public void testGetBestMigration() throws ConfigurationException { vmIdServiceOfferingMap.put(vm.getId(), serviceOffering); } - Mockito.when(managementServer.listHostsForMigrationOfVM(vm1, 0L, 500L, null, vmList)).thenReturn( - new Ternary<>(new Pair<>(List.of(destHost), 1), List.of(destHost), Map.of(destHost, false))); - Mockito.when(managementServer.listHostsForMigrationOfVM(vm2, 0L, 500L, null, vmList)).thenReturn( - new Ternary<>(new Pair<>(List.of(destHost), 1), List.of(destHost), Map.of(destHost, false))); - Mockito.when(balancedAlgorithm.getMetrics(cluster, vm1, serviceOffering, destHost, new HashMap<>(), - new HashMap<>(), false)).thenReturn(new Ternary<>(1.0, 0.5, 1.5)); + // Create caches for the new method signature + Map> vmToCompatibleHostsCache = new HashMap<>(); + vmToCompatibleHostsCache.put(vm1.getId(), List.of(destHost)); + vmToCompatibleHostsCache.put(vm2.getId(), List.of(destHost)); + + Map> vmToStorageMotionCache = new HashMap<>(); + vmToStorageMotionCache.put(vm1.getId(), Map.of(destHost, false)); + vmToStorageMotionCache.put(vm2.getId(), Map.of(destHost, false)); + + Map vmToExcludesMap = new HashMap<>(); + vmToExcludesMap.put(vm1.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + vmToExcludesMap.put(vm2.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + + // Create capacity maps with dummy data for getClusterImbalance + Map> hostCpuCapacityMap = new HashMap<>(); + hostCpuCapacityMap.put(destHost.getId(), new Ternary<>(1000L, 0L, 2000L)); + Map> hostMemoryCapacityMap = new HashMap<>(); + hostMemoryCapacityMap.put(destHost.getId(), new Ternary<>(1024L * 1024L * 1024L, 0L, 2L * 1024L * 1024L * 1024L)); + + // Mock getMetrics for vm1 (better improvement - should be selected) + Mockito.when(balancedAlgorithm.getMetrics(cluster, vm1, serviceOffering, destHost, hostCpuCapacityMap, + hostMemoryCapacityMap, false, 0.0)).thenReturn(new Ternary<>(1.0, 0.5, 1.5)); - Mockito.when(balancedAlgorithm.getMetrics(cluster, vm2, serviceOffering, destHost, new HashMap<>(), - new HashMap<>(), false)).thenReturn(new Ternary<>(1.0, 2.5, 1.5)); + // Mock getMetrics for vm2 (worse improvement) + Mockito.when(balancedAlgorithm.getMetrics(cluster, vm2, serviceOffering, destHost, hostCpuCapacityMap, + hostMemoryCapacityMap, false, 0.0)).thenReturn(new Ternary<>(1.0, 2.5, 1.5)); Pair bestMigration = clusterDrsService.getBestMigration(cluster, balancedAlgorithm, - vmList, vmIdServiceOfferingMap, new HashMap<>(), new HashMap<>()); + vmList, vmIdServiceOfferingMap, hostCpuCapacityMap, hostMemoryCapacityMap, + vmToCompatibleHostsCache, vmToStorageMotionCache, vmToExcludesMap); assertEquals(destHost, bestMigration.second()); assertEquals(vm1, bestMigration.first()); @@ -443,12 +946,28 @@ public void testGetBestMigrationDifferentCluster() throws ConfigurationException vmIdServiceOfferingMap.put(vm.getId(), serviceOffering); } - Mockito.when(managementServer.listHostsForMigrationOfVM(vm1, 0L, 500L, null, vmList)).thenReturn( - new Ternary<>(new Pair<>(List.of(destHost), 1), List.of(destHost), Map.of(destHost, false))); - Mockito.when(managementServer.listHostsForMigrationOfVM(vm2, 0L, 500L, null, vmList)).thenReturn( - new Ternary<>(new Pair<>(List.of(destHost), 1), List.of(destHost), Map.of(destHost, false))); + // Create caches for the new method signature + Map> vmToCompatibleHostsCache = new HashMap<>(); + vmToCompatibleHostsCache.put(vm1.getId(), List.of(destHost)); + vmToCompatibleHostsCache.put(vm2.getId(), List.of(destHost)); + + Map> vmToStorageMotionCache = new HashMap<>(); + vmToStorageMotionCache.put(vm1.getId(), Map.of(destHost, false)); + vmToStorageMotionCache.put(vm2.getId(), Map.of(destHost, false)); + + Map vmToExcludesMap = new HashMap<>(); + vmToExcludesMap.put(vm1.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + vmToExcludesMap.put(vm2.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); + + // Create capacity maps with dummy data for getClusterImbalance + Map> hostCpuCapacityMap = new HashMap<>(); + hostCpuCapacityMap.put(destHost.getId(), new Ternary<>(1000L, 0L, 2000L)); + Map> hostMemoryCapacityMap = new HashMap<>(); + hostMemoryCapacityMap.put(destHost.getId(), new Ternary<>(1024L * 1024L * 1024L, 0L, 2L * 1024L * 1024L * 1024L)); + Pair bestMigration = clusterDrsService.getBestMigration(cluster, balancedAlgorithm, - vmList, vmIdServiceOfferingMap, new HashMap<>(), new HashMap<>()); + vmList, vmIdServiceOfferingMap, hostCpuCapacityMap, hostMemoryCapacityMap, + vmToCompatibleHostsCache, vmToStorageMotionCache, vmToExcludesMap); assertNull(bestMigration.second()); assertNull(bestMigration.first()); From 182a02dee34c898004f733b8a1c4fd2d26d64523 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Fri, 7 Nov 2025 12:35:28 +0530 Subject: [PATCH 4/6] Refactor ClusterDRSAlgortihm to improve performance --- .../cluster/ClusterDrsAlgorithm.java | 221 +++++++++--------- .../apache/cloudstack/cluster/Balanced.java | 27 +-- .../cloudstack/cluster/BalancedTest.java | 61 ++++- .../apache/cloudstack/cluster/Condensed.java | 29 +-- .../cloudstack/cluster/CondensedTest.java | 61 ++++- .../cluster/ClusterDrsServiceImpl.java | 56 ++++- .../server/ManagementServerImplTest.java | 2 +- .../cluster/ClusterDrsServiceImplTest.java | 73 ++---- 8 files changed, 317 insertions(+), 213 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java index beaf1df4d4ab..03022682ed86 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java @@ -22,7 +22,6 @@ import com.cloud.host.Host; import com.cloud.offering.ServiceOffering; import com.cloud.org.Cluster; -import com.cloud.utils.Pair; import com.cloud.utils.Ternary; import com.cloud.utils.component.Adapter; import com.cloud.vm.VirtualMachine; @@ -39,6 +38,10 @@ import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsMetricUseRatio; public interface ClusterDrsAlgorithm extends Adapter { + // Reusable stateless calculator objects (thread-safe) - created once, reused for all calculations + // Apache Commons Math Mean and StandardDeviation are stateless and thread-safe + Mean MEAN_CALCULATOR = new Mean(); + StandardDeviation STDDEV_CALCULATOR = new StandardDeviation(false); /** * Determines whether a DRS operation is needed for a given cluster and host-VM @@ -59,111 +62,118 @@ public interface ClusterDrsAlgorithm extends Adapter { boolean needsDrs(Cluster cluster, List> cpuList, List> memoryList) throws ConfigurationException; - /** - * Determines the metrics for a given virtual machine and destination host in a DRS cluster. - * - * @param clusterId - * the ID of the cluster to check - * @param vm - * the virtual machine to check - * @param serviceOffering - * the service offering for the virtual machine - * @param destHost - * the destination host for the virtual machine - * @param hostCpuMap - * a map of host IDs to the Ternary of used, reserved and total CPU on each host - * @param hostMemoryMap - * a map of host IDs to the Ternary of used, reserved and total memory on each host - * @param requiresStorageMotion - * whether storage motion is required for the virtual machine + * Calculates the metrics (improvement, cost, benefit) for migrating a VM to a destination host. Improvement is + * calculated based on the change in cluster imbalance before and after the migration. * + * @param cluster the cluster to check + * @param vm the virtual machine to check + * @param serviceOffering the service offering for the virtual machine + * @param destHost the destination host for the virtual machine + * @param hostCpuMap a map of host IDs to the Ternary of used, reserved and total CPU on each host + * @param hostMemoryMap a map of host IDs to the Ternary of used, reserved and total memory on each host + * @param requiresStorageMotion whether storage motion is required for the virtual machine + * @param preImbalance the pre-calculated cluster imbalance before migration (null to calculate it) + * @param baseMetricsArray pre-calculated array of all host metrics before migration + * @param hostIdToIndexMap mapping from host ID to index in the metrics array * @return a ternary containing improvement, cost, benefit */ Ternary getMetrics(Cluster cluster, VirtualMachine vm, ServiceOffering serviceOffering, Host destHost, Map> hostCpuMap, Map> hostMemoryMap, - Boolean requiresStorageMotion) throws ConfigurationException; + Boolean requiresStorageMotion, Double preImbalance, + double[] baseMetricsArray, Map hostIdToIndexMap) throws ConfigurationException; /** - * Determines the metrics for a given virtual machine and destination host in a DRS cluster. - * This overloaded version accepts a pre-calculated pre-imbalance value to avoid recalculating - * it for every VM-host combination within the same iteration. - * - * @param cluster - * the cluster to check - * @param vm - * the virtual machine to check - * @param serviceOffering - * the service offering for the virtual machine - * @param destHost - * the destination host for the virtual machine - * @param hostCpuMap - * a map of host IDs to the Ternary of used, reserved and total CPU on each host - * @param hostMemoryMap - * a map of host IDs to the Ternary of used, reserved and total memory on each host - * @param requiresStorageMotion - * whether storage motion is required for the virtual machine - * @param preImbalance - * the pre-calculated cluster imbalance before migration (null to calculate it) + * Calculates the cluster imbalance after migrating a VM to a destination host. * - * @return a ternary containing improvement, cost, benefit + * @param vm the virtual machine being migrated + * @param destHost the destination host for the virtual machine + * @param clusterId the cluster ID + * @param vmMetric the VM's resource consumption metric + * @param baseMetricsArray pre-calculated array of all host metrics before migration + * @param hostIdToIndexMap mapping from host ID to index in the metrics array + * @return the cluster imbalance after migration */ - default Ternary getMetrics(Cluster cluster, VirtualMachine vm, ServiceOffering serviceOffering, - Host destHost, Map> hostCpuMap, - Map> hostMemoryMap, - Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { - // Default implementation delegates to the original method for backward compatibility - return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion); + default Double getImbalancePostMigration(VirtualMachine vm, + Host destHost, Long clusterId, long vmMetric, double[] baseMetricsArray, + Map hostIdToIndexMap, Map> hostCpuMap, + Map> hostMemoryMap) { + // Create a copy of the base array and adjust only the two affected hosts + double[] adjustedMetrics = new double[baseMetricsArray.length]; + System.arraycopy(baseMetricsArray, 0, adjustedMetrics, 0, baseMetricsArray.length); + + long destHostId = destHost.getId(); + long vmHostId = vm.getHostId(); + + // Adjust source host (remove VM resources) + Integer sourceIndex = hostIdToIndexMap.get(vmHostId); + if (sourceIndex != null && sourceIndex < adjustedMetrics.length) { + Map> sourceMetricsMap = getClusterDrsMetric(clusterId).equals("cpu") ? hostCpuMap : hostMemoryMap; + Ternary sourceMetrics = sourceMetricsMap.get(vmHostId); + if (sourceMetrics != null) { + adjustedMetrics[sourceIndex] = getMetricValuePostMigration(clusterId, sourceMetrics, vmMetric, vmHostId, destHostId, vmHostId); + } + } + + // Adjust destination host (add VM resources) + Integer destIndex = hostIdToIndexMap.get(destHostId); + if (destIndex != null && destIndex < adjustedMetrics.length) { + Map> destMetricsMap = getClusterDrsMetric(clusterId).equals("cpu") ? hostCpuMap : hostMemoryMap; + Ternary destMetrics = destMetricsMap.get(destHostId); + if (destMetrics != null) { + adjustedMetrics[destIndex] = getMetricValuePostMigration(clusterId, destMetrics, vmMetric, destHostId, destHostId, vmHostId); + } + } + + return calculateImbalance(adjustedMetrics); } /** - * Calculates the imbalance of the cluster after a virtual machine migration. - * - * @param serviceOffering - * the service offering for the virtual machine - * @param vm - * the virtual machine being migrated - * @param destHost - * the destination host for the virtual machine - * @param hostCpuMap - * a map of host IDs to the Ternary of used, reserved and total CPU on each host - * @param hostMemoryMap - * a map of host IDs to the Ternary of used, reserved and total memory on each host + * Calculate imbalance from a double array. Imbalance is defined as standard deviation divided by mean. * - * @return a pair containing the CPU and memory imbalance of the cluster after the migration + * Uses reusable stateless calculator objects to avoid object creation overhead. + * @param values array of metric values + * @return calculated imbalance */ - default Double getImbalancePostMigration(ServiceOffering serviceOffering, VirtualMachine vm, - Host destHost, Map> hostCpuMap, - Map> hostMemoryMap) throws ConfigurationException { - Pair>> pair = getHostMetricsMapAndType(destHost.getClusterId(), serviceOffering, hostCpuMap, hostMemoryMap); - long vmMetric = pair.first(); - Map> hostMetricsMap = pair.second(); - - List list = new ArrayList<>(); - for (Long hostId : hostMetricsMap.keySet()) { - list.add(getMetricValuePostMigration(destHost.getClusterId(), hostMetricsMap.get(hostId), vmMetric, hostId, destHost.getId(), vm.getHostId())); + private static double calculateImbalance(double[] values) { + if (values == null || values.length == 0) { + return 0.0; } - return getImbalance(list); + // Reuse static final calculator objects (thread-safe, stateless) + double mean = MEAN_CALCULATOR.evaluate(values); + if (mean == 0.0) { + return 0.0; // Avoid division by zero + } + double stdDev = STDDEV_CALCULATOR.evaluate(values, mean); + return stdDev / mean; } - private Pair>> getHostMetricsMapAndType(Long clusterId, - ServiceOffering serviceOffering, Map> hostCpuMap, - Map> hostMemoryMap) throws ConfigurationException { + /** + * Helper method to get VM metric based on cluster configuration. + */ + static long getVmMetric(ServiceOffering serviceOffering, Long clusterId) throws ConfigurationException { String metric = getClusterDrsMetric(clusterId); - Pair>> pair; switch (metric) { case "cpu": - pair = new Pair<>((long) serviceOffering.getCpu() * serviceOffering.getSpeed(), hostCpuMap); - break; + return (long) serviceOffering.getCpu() * serviceOffering.getSpeed(); case "memory": - pair = new Pair<>(serviceOffering.getRamSize() * 1024L * 1024L, hostMemoryMap); - break; + return serviceOffering.getRamSize() * 1024L * 1024L; default: throw new ConfigurationException( String.format("Invalid metric: %s for cluster: %d", metric, clusterId)); } - return pair; + } + + /** + * Helper method to calculate metrics from pre and post imbalance values. + * Subclasses can override this to implement different improvement calculations. + */ + default Ternary calculateMetricsFromImbalances(Double preImbalance, Double postImbalance) { + final double improvement = preImbalance - postImbalance; + final double cost = 0.0; + final double benefit = 1.0; + return new Ternary<>(improvement, cost, benefit); } private Double getMetricValuePostMigration(Long clusterId, Ternary metrics, long vmMetric, @@ -183,9 +193,26 @@ private Double getMetricValuePostMigration(Long clusterId, Ternary metricList) { - Double clusterMeanMetric = getClusterMeanMetric(metricList); - Double clusterStandardDeviation = getClusterStandardDeviation(metricList, clusterMeanMetric); - return clusterStandardDeviation / clusterMeanMetric; + if (metricList == null || metricList.isEmpty()) { + return 0.0; + } + // Convert List to double[] once, avoiding repeated conversions + double[] values = new double[metricList.size()]; + int index = 0; + for (Double value : metricList) { + if (value != null) { + values[index++] = value; + } + } + + // Trim array if some values were null + if (index < values.length) { + double[] trimmed = new double[index]; + System.arraycopy(values, 0, trimmed, 0, index); + values = trimmed; + } + + return calculateImbalance(values); } static String getClusterDrsMetric(long clusterId) { @@ -213,36 +240,6 @@ static Double getMetricValue(long clusterId, long used, long free, long total, F return null; } - /** - * Mean is the average of a collection or set of metrics. In context of a DRS - * cluster, the cluster metrics defined as the average metrics value for some - * metric (such as CPU, memory etc.) for every resource such as host. - * Cluster Mean Metric, mavg = (∑mi) / N, where mi is a measurable metric for a - * resource ‘i’ in a cluster with total N number of resources. - */ - static Double getClusterMeanMetric(List metricList) { - return new Mean().evaluate(metricList.stream().mapToDouble(i -> i).toArray()); - } - - /** - * Standard deviation is defined as the square root of the absolute squared sum - * of difference of a metric from its mean for every resource divided by the - * total number of resources. In context of the DRS, the cluster standard - * deviation is the standard deviation based on a metric of resources in a - * cluster such as for the allocation or utilisation CPU/memory metric of hosts - * in a cluster. - * Cluster Standard Deviation, σc = sqrt((∑∣mi−mavg∣^2) / N), where mavg is the - * mean metric value and mi is a measurable metric for some resource ‘i’ in the - * cluster with total N number of resources. - */ - static Double getClusterStandardDeviation(List metricList, Double mean) { - if (mean != null) { - return new StandardDeviation(false).evaluate(metricList.stream().mapToDouble(i -> i).toArray(), mean); - } else { - return new StandardDeviation(false).evaluate(metricList.stream().mapToDouble(i -> i).toArray()); - } - } - static boolean getDrsMetricUseRatio(long clusterId) { return ClusterDrsMetricUseRatio.valueIn(clusterId); } diff --git a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java index 90743f3e893f..099a686bec7e 100644 --- a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java +++ b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java @@ -68,35 +68,26 @@ public String getName() { return "balanced"; } - @Override - public Ternary getMetrics(Cluster cluster, VirtualMachine vm, - ServiceOffering serviceOffering, Host destHost, - Map> hostCpuMap, Map> hostMemoryMap, - Boolean requiresStorageMotion) throws ConfigurationException { - Double preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); - return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion, preImbalance); - } @Override public Ternary getMetrics(Cluster cluster, VirtualMachine vm, ServiceOffering serviceOffering, Host destHost, Map> hostCpuMap, Map> hostMemoryMap, - Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { + Boolean requiresStorageMotion, Double preImbalance, + double[] baseMetricsArray, Map hostIdToIndexMap) throws ConfigurationException { // Use provided pre-imbalance if available, otherwise calculate it if (preImbalance == null) { preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); } - Double postImbalance = getImbalancePostMigration(serviceOffering, vm, destHost, hostCpuMap, hostMemoryMap); - logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", + // Use optimized post-imbalance calculation that adjusts only affected hosts + Double postImbalance = getImbalancePostMigration(vm, destHost, + cluster.getId(), ClusterDrsAlgorithm.getVmMetric(serviceOffering, cluster.getId()), + baseMetricsArray, hostIdToIndexMap, hostCpuMap, hostMemoryMap); + + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {} (optimized)", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); - // This needs more research to determine the cost and benefit of a migration - // TODO: Cost should be a factor of the VM size and the host capacity - // TODO: Benefit should be a factor of the VM size and the host capacity and the number of VMs on the host - final double improvement = preImbalance - postImbalance; - final double cost = 0.0; - final double benefit = 1.0; - return new Ternary<>(improvement, cost, benefit); + return calculateMetricsFromImbalances(preImbalance, postImbalance); } } diff --git a/plugins/drs/cluster/balanced/src/test/java/org/apache/cloudstack/cluster/BalancedTest.java b/plugins/drs/cluster/balanced/src/test/java/org/apache/cloudstack/cluster/BalancedTest.java index d51606719584..e955a10f2a56 100644 --- a/plugins/drs/cluster/balanced/src/test/java/org/apache/cloudstack/cluster/BalancedTest.java +++ b/plugins/drs/cluster/balanced/src/test/java/org/apache/cloudstack/cluster/BalancedTest.java @@ -43,6 +43,9 @@ import java.util.List; import java.util.Map; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterDrsMetric; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterImbalance; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getMetricValue; import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsImbalanceThreshold; import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsMetric; import static org.junit.Assert.assertEquals; @@ -119,6 +122,48 @@ public void tearDown() throws Exception { closeable.close(); } + /** + * Helper method to prepare metrics data for getMetrics calls with optimized signature. + * Calculates pre-imbalance and builds baseMetricsArray and hostIdToIndexMap. + * + * @return a Ternary containing preImbalance, baseMetricsArray, and hostIdToIndexMap + */ + private Ternary> prepareMetricsData() throws ConfigurationException { + // Calculate pre-imbalance + Double preImbalance = getClusterImbalance(clusterId, new ArrayList<>(hostCpuFreeMap.values()), + new ArrayList<>(hostMemoryFreeMap.values()), null); + + // Build baseMetricsArray and hostIdToIndexMap + String metricType = getClusterDrsMetric(clusterId); + Map> baseMetricsMap = "cpu".equals(metricType) ? hostCpuFreeMap : hostMemoryFreeMap; + double[] baseMetricsArray = new double[baseMetricsMap.size()]; + Map hostIdToIndexMap = new HashMap<>(); + + int index = 0; + for (Map.Entry> entry : baseMetricsMap.entrySet()) { + Long hostId = entry.getKey(); + Ternary metrics = entry.getValue(); + long used = metrics.first(); + long actualTotal = metrics.third() - metrics.second(); + long free = actualTotal - metrics.first(); + Double metricValue = getMetricValue(clusterId, used, free, actualTotal, null); + if (metricValue != null) { + baseMetricsArray[index] = metricValue; + hostIdToIndexMap.put(hostId, index); + index++; + } + } + + // Trim array if some values were null + if (index < baseMetricsArray.length) { + double[] trimmed = new double[index]; + System.arraycopy(baseMetricsArray, 0, trimmed, 0, index); + baseMetricsArray = trimmed; + } + + return new Ternary<>(preImbalance, baseMetricsArray, hostIdToIndexMap); + } + /** * needsDrs tests *

Scenarios to test for needsDrs @@ -183,8 +228,14 @@ public void needsDrsWithUnknown() throws NoSuchFieldException, IllegalAccessExce @Test public void getMetricsWithCpu() throws NoSuchFieldException, IllegalAccessException, ConfigurationException { overrideDefaultConfigValue(ClusterDrsMetric, "_defaultValue", "cpu"); + + Ternary> metricsData = prepareMetricsData(); + Double preImbalance = metricsData.first(); + double[] baseMetricsArray = metricsData.second(); + Map hostIdToIndexMap = metricsData.third(); + Ternary result = balanced.getMetrics(cluster, vm3, serviceOffering, destHost, - hostCpuFreeMap, hostMemoryFreeMap, false); + hostCpuFreeMap, hostMemoryFreeMap, false, preImbalance, baseMetricsArray, hostIdToIndexMap); assertEquals(0.0, result.first(), 0.01); assertEquals(0.0, result.second(), 0.0); assertEquals(1.0, result.third(), 0.0); @@ -197,8 +248,14 @@ public void getMetricsWithCpu() throws NoSuchFieldException, IllegalAccessExcept @Test public void getMetricsWithMemory() throws NoSuchFieldException, IllegalAccessException, ConfigurationException { overrideDefaultConfigValue(ClusterDrsMetric, "_defaultValue", "memory"); + + Ternary> metricsData = prepareMetricsData(); + Double preImbalance = metricsData.first(); + double[] baseMetricsArray = metricsData.second(); + Map hostIdToIndexMap = metricsData.third(); + Ternary result = balanced.getMetrics(cluster, vm3, serviceOffering, destHost, - hostCpuFreeMap, hostMemoryFreeMap, false); + hostCpuFreeMap, hostMemoryFreeMap, false, preImbalance, baseMetricsArray, hostIdToIndexMap); assertEquals(0.4, result.first(), 0.01); assertEquals(0, result.second(), 0.0); assertEquals(1, result.third(), 0.0); diff --git a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java index 4fa80198b1a2..befef69f2c9a 100644 --- a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java +++ b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java @@ -75,33 +75,22 @@ public String getName() { public Ternary getMetrics(Cluster cluster, VirtualMachine vm, ServiceOffering serviceOffering, Host destHost, Map> hostCpuMap, Map> hostMemoryMap, - Boolean requiresStorageMotion) throws ConfigurationException { - Double preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), - new ArrayList<>(hostMemoryMap.values()), null); - return getMetrics(cluster, vm, serviceOffering, destHost, hostCpuMap, hostMemoryMap, requiresStorageMotion, preImbalance); - } - - @Override - public Ternary getMetrics(Cluster cluster, VirtualMachine vm, - ServiceOffering serviceOffering, Host destHost, - Map> hostCpuMap, Map> hostMemoryMap, - Boolean requiresStorageMotion, Double preImbalance) throws ConfigurationException { + Boolean requiresStorageMotion, Double preImbalance, + double[] baseMetricsArray, Map hostIdToIndexMap) throws ConfigurationException { // Use provided pre-imbalance if available, otherwise calculate it if (preImbalance == null) { preImbalance = ClusterDrsAlgorithm.getClusterImbalance(cluster.getId(), new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()), null); } - Double postImbalance = getImbalancePostMigration(serviceOffering, vm, destHost, hostCpuMap, hostMemoryMap); - logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost: {} destHost: {}", + // Use optimized post-imbalance calculation that adjusts only affected hosts + Double postImbalance = getImbalancePostMigration(vm, destHost, + cluster.getId(), ClusterDrsAlgorithm.getVmMetric(serviceOffering, cluster.getId()), + baseMetricsArray, hostIdToIndexMap, hostCpuMap, hostMemoryMap); + + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {} (optimized)", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); - // This needs more research to determine the cost and benefit of a migration - // TODO: Cost should be a factor of the VM size and the host capacity - // TODO: Benefit should be a factor of the VM size and the host capacity and the number of VMs on the host - final double improvement = postImbalance - preImbalance; - final double cost = 0; - final double benefit = 1; - return new Ternary<>(improvement, cost, benefit); + return calculateMetricsFromImbalances(postImbalance, preImbalance); } } diff --git a/plugins/drs/cluster/condensed/src/test/java/org/apache/cloudstack/cluster/CondensedTest.java b/plugins/drs/cluster/condensed/src/test/java/org/apache/cloudstack/cluster/CondensedTest.java index 3d3896704dab..b2a5e6bf84f1 100644 --- a/plugins/drs/cluster/condensed/src/test/java/org/apache/cloudstack/cluster/CondensedTest.java +++ b/plugins/drs/cluster/condensed/src/test/java/org/apache/cloudstack/cluster/CondensedTest.java @@ -43,6 +43,9 @@ import java.util.List; import java.util.Map; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterDrsMetric; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterImbalance; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getMetricValue; import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsImbalanceThreshold; import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsMetric; import static org.junit.Assert.assertEquals; @@ -121,6 +124,48 @@ public void tearDown() throws Exception { closeable.close(); } + /** + * Helper method to prepare metrics data for getMetrics calls with optimized signature. + * Calculates pre-imbalance and builds baseMetricsArray and hostIdToIndexMap. + * + * @return a Ternary containing preImbalance, baseMetricsArray, and hostIdToIndexMap + */ + private Ternary> prepareMetricsData() throws ConfigurationException { + // Calculate pre-imbalance + Double preImbalance = getClusterImbalance(clusterId, new ArrayList<>(hostCpuFreeMap.values()), + new ArrayList<>(hostMemoryFreeMap.values()), null); + + // Build baseMetricsArray and hostIdToIndexMap + String metricType = getClusterDrsMetric(clusterId); + Map> baseMetricsMap = "cpu".equals(metricType) ? hostCpuFreeMap : hostMemoryFreeMap; + double[] baseMetricsArray = new double[baseMetricsMap.size()]; + Map hostIdToIndexMap = new HashMap<>(); + + int index = 0; + for (Map.Entry> entry : baseMetricsMap.entrySet()) { + Long hostId = entry.getKey(); + Ternary metrics = entry.getValue(); + long used = metrics.first(); + long actualTotal = metrics.third() - metrics.second(); + long free = actualTotal - metrics.first(); + Double metricValue = getMetricValue(clusterId, used, free, actualTotal, null); + if (metricValue != null) { + baseMetricsArray[index] = metricValue; + hostIdToIndexMap.put(hostId, index); + index++; + } + } + + // Trim array if some values were null + if (index < baseMetricsArray.length) { + double[] trimmed = new double[index]; + System.arraycopy(baseMetricsArray, 0, trimmed, 0, index); + baseMetricsArray = trimmed; + } + + return new Ternary<>(preImbalance, baseMetricsArray, hostIdToIndexMap); + } + /** *

needsDrs tests *

Scenarios to test for needsDrs @@ -185,8 +230,14 @@ public void needsDrsWithUnknown() throws NoSuchFieldException, IllegalAccessExce @Test public void getMetricsWithCpu() throws NoSuchFieldException, IllegalAccessException, ConfigurationException { overrideDefaultConfigValue(ClusterDrsMetric, "_defaultValue", "cpu"); + + Ternary> metricsData = prepareMetricsData(); + Double preImbalance = metricsData.first(); + double[] baseMetricsArray = metricsData.second(); + Map hostIdToIndexMap = metricsData.third(); + Ternary result = condensed.getMetrics(cluster, vm3, serviceOffering, destHost, - hostCpuFreeMap, hostMemoryFreeMap, false); + hostCpuFreeMap, hostMemoryFreeMap, false, preImbalance, baseMetricsArray, hostIdToIndexMap); assertEquals(0.0, result.first(), 0.0); assertEquals(0, result.second(), 0.0); assertEquals(1, result.third(), 0.0); @@ -199,8 +250,14 @@ public void getMetricsWithCpu() throws NoSuchFieldException, IllegalAccessExcept @Test public void getMetricsWithMemory() throws NoSuchFieldException, IllegalAccessException, ConfigurationException { overrideDefaultConfigValue(ClusterDrsMetric, "_defaultValue", "memory"); + + Ternary> metricsData = prepareMetricsData(); + Double preImbalance = metricsData.first(); + double[] baseMetricsArray = metricsData.second(); + Map hostIdToIndexMap = metricsData.third(); + Ternary result = condensed.getMetrics(cluster, vm3, serviceOffering, destHost, - hostCpuFreeMap, hostMemoryFreeMap, false); + hostCpuFreeMap, hostMemoryFreeMap, false, preImbalance, baseMetricsArray, hostIdToIndexMap); assertEquals(-0.4, result.first(), 0.01); assertEquals(0, result.second(), 0.0); assertEquals(1, result.third(), 0.0); diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index 84edfb44d009..393faf98a224 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -97,6 +97,8 @@ import static com.cloud.org.Grouping.AllocationState.Disabled; import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterImbalance; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getClusterDrsMetric; +import static org.apache.cloudstack.cluster.ClusterDrsAlgorithm.getMetricValue; public class ClusterDrsServiceImpl extends ManagerBase implements ClusterDrsService, PluggableService { @@ -369,7 +371,9 @@ List> getDrsPlan(Cluster cluster, serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId())); } - // PHASE 1: Pre-compute technical compatibility (once per eligible VM - never changes) + // PHASE 1: Pre-compute suitable hosts (once per eligible VM - never changes) + // Use listHostsForMigrationOfVM to get hosts validated by getCapableSuitableHosts + // This ensures DRS uses the same validation as "find host for migration" command Map> vmToCompatibleHostsCache = new HashMap<>(); Map> vmToStorageMotionCache = new HashMap<>(); @@ -383,18 +387,20 @@ List> getDrsPlan(Cluster cluster, } try { - Ternary, Integer>, List, Map> compatibility = - managementServer.getTechnicallyCompatibleHosts(vm, 0L, 500L, null); + // Use listHostsForMigrationOfVM to get suitable hosts (validated by getCapableSuitableHosts) + // This ensures the same validation as the "find host for migration" command + Ternary, Integer>, List, Map> hostsForMigration = + managementServer.listHostsForMigrationOfVM(vm, 0L, 500L, null, vmList); - List compatibleHosts = compatibility.second(); // Get filtered hosts - Map requiresStorageMotion = compatibility.third(); + List suitableHosts = hostsForMigration.second(); // Get suitable hosts (validated by HostAllocator) + Map requiresStorageMotion = hostsForMigration.third(); - if (compatibleHosts != null && !compatibleHosts.isEmpty()) { - vmToCompatibleHostsCache.put(vm.getId(), compatibleHosts); + if (suitableHosts != null && !suitableHosts.isEmpty()) { + vmToCompatibleHostsCache.put(vm.getId(), suitableHosts); vmToStorageMotionCache.put(vm.getId(), requiresStorageMotion); } } catch (Exception e) { - logger.debug("Could not get compatible hosts for VM {}: {}", vm, e.getMessage()); + logger.debug("Could not get suitable hosts for VM {}: {}", vm, e.getMessage()); } } @@ -540,6 +546,35 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm new ArrayList<>(hostMemoryCapacityMap.values()), null); + // Pre-calculate base metrics array once per iteration for optimized imbalance calculation + // This reduces complexity from O(V × H × H) to O(V × H) per iteration + String metricType = getClusterDrsMetric(cluster.getId()); + Map> baseMetricsMap = "cpu".equals(metricType) ? hostCpuCapacityMap : hostMemoryCapacityMap; + double[] baseMetricsArray = new double[baseMetricsMap.size()]; + Map hostIdToIndexMap = new HashMap<>(); + + int index = 0; + for (Map.Entry> entry : baseMetricsMap.entrySet()) { + Long hostId = entry.getKey(); + Ternary metrics = entry.getValue(); + long used = metrics.first(); + long actualTotal = metrics.third() - metrics.second(); + long free = actualTotal - metrics.first(); + Double metricValue = getMetricValue(cluster.getId(), used, free, actualTotal, null); + if (metricValue != null) { + baseMetricsArray[index] = metricValue; + hostIdToIndexMap.put(hostId, index); + index++; + } + } + + // Trim array if some values were null + if (index < baseMetricsArray.length) { + double[] trimmed = new double[index]; + System.arraycopy(baseMetricsArray, 0, trimmed, 0, index); + baseMetricsArray = trimmed; + } + double improvement = 0; Pair bestMigration = new Pair<>(null, null); @@ -594,10 +629,11 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm } // getMetrics uses updated capacity maps - this is the real capacity check - // Pass pre-calculated pre-imbalance to avoid recalculating it for every VM-host combination + // Pass pre-calculated pre-imbalance and base metrics array for optimized calculation Ternary metrics = algorithm.getMetrics(cluster, vm, serviceOffering, destHost, hostCpuCapacityMap, hostMemoryCapacityMap, - requiresStorageMotion.getOrDefault(destHost, false), preImbalance); + requiresStorageMotion.getOrDefault(destHost, false), preImbalance, + baseMetricsArray, hostIdToIndexMap); Double currentImprovement = metrics.first(); Double cost = metrics.second(); diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index cc43f710290d..af746e02ccc1 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -883,7 +883,7 @@ public void testListHostsForMigrationOfVMWithSystemVM() { setupMigrationMocks(vm, srcHost, hosts, volume, true); - // Verify this doesn't throw exception - system VMs should be migrateable + // Verify this doesn't throw exception - system VMs should be migratable Ternary, Integer>, List, Map> result = spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java index a8b40bddd18b..01a02dd136c3 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -44,6 +44,7 @@ import com.cloud.vm.VirtualMachine; import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.VMInstanceDao; +import org.apache.cloudstack.affinity.dao.AffinityGroupVMMapDao; import org.apache.cloudstack.api.command.admin.cluster.GenerateClusterDrsPlanCmd; import org.apache.cloudstack.api.response.ClusterDrsPlanMigrationResponse; import org.apache.cloudstack.api.response.ClusterDrsPlanResponse; @@ -117,6 +118,9 @@ public class ClusterDrsServiceImplTest { @Mock private VMInstanceDao vmInstanceDao; + @Mock + private AffinityGroupVMMapDao affinityGroupVMMapDao; + @Spy @InjectMocks private ClusterDrsServiceImpl clusterDrsService = new ClusterDrsServiceImpl(); @@ -421,14 +425,9 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); - // Return empty compatible hosts list - Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), - Mockito.anyLong(), Mockito.any())).thenReturn( - new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>())); - List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); - Mockito.verify(managementServer, Mockito.times(1)).getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + Mockito.verify(managementServer, Mockito.times(1)).listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); } @Test @@ -471,14 +470,10 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); - // Throw exception during compatibility check - Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), - Mockito.anyLong(), Mockito.any())).thenThrow(new RuntimeException("Test exception")); - List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); // Exception should be caught and logged, not propagated - Mockito.verify(managementServer, Mockito.times(1)).getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); + Mockito.verify(managementServer, Mockito.times(1)).listHostsForMigrationOfVM(Mockito.eq(vm1), Mockito.anyLong(), Mockito.anyLong(), Mockito.any(), Mockito.anyList()); } @Test @@ -489,9 +484,6 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { HostVO host1 = Mockito.mock(HostVO.class); Mockito.when(host1.getId()).thenReturn(1L); - Mockito.when(host1.getDataCenterId()).thenReturn(1L); - Mockito.when(host1.getPodId()).thenReturn(1L); - Mockito.when(host1.getClusterId()).thenReturn(1L); VMInstanceVO vm1 = Mockito.mock(VMInstanceVO.class); Mockito.when(vm1.getId()).thenReturn(1L); @@ -525,11 +517,6 @@ public void testGetDrsPlanWithNoBestMigration() throws ConfigurationException { Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); HostVO compatibleHost = Mockito.mock(HostVO.class); - Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), - Mockito.anyLong(), Mockito.any())).thenReturn( - new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); - Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), - Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); // Return null migration (no best migration found) Mockito.doReturn(new Pair<>(null, null)).when(clusterDrsService).getBestMigration( @@ -549,9 +536,6 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException HostVO host1 = Mockito.mock(HostVO.class); Mockito.when(host1.getId()).thenReturn(1L); - Mockito.when(host1.getDataCenterId()).thenReturn(1L); - Mockito.when(host1.getPodId()).thenReturn(1L); - Mockito.when(host1.getClusterId()).thenReturn(1L); HostVO host2 = Mockito.mock(HostVO.class); Mockito.when(host2.getId()).thenReturn(2L); @@ -611,11 +595,6 @@ public void testGetDrsPlanWithMultipleIterations() throws ConfigurationException Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1, hostJoin2)); HostVO compatibleHost = Mockito.mock(HostVO.class); - Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.any(), Mockito.anyLong(), - Mockito.anyLong(), Mockito.any())).thenReturn( - new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); - Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), - Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); // Return migrations for first two iterations, then null Mockito.doReturn(new Pair<>(vm1, host2), new Pair<>(vm2, host2), new Pair<>(null, null)) @@ -637,9 +616,6 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce HostVO host1 = Mockito.mock(HostVO.class); Mockito.when(host1.getId()).thenReturn(1L); - Mockito.when(host1.getDataCenterId()).thenReturn(1L); - Mockito.when(host1.getPodId()).thenReturn(1L); - Mockito.when(host1.getClusterId()).thenReturn(1L); HostVO host2 = Mockito.mock(HostVO.class); Mockito.when(host2.getId()).thenReturn(2L); @@ -687,11 +663,6 @@ public void testGetDrsPlanWithMigrationToOriginalHost() throws ConfigurationExce Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1, hostJoin2)); HostVO compatibleHost = Mockito.mock(HostVO.class); - Mockito.when(managementServer.getTechnicallyCompatibleHosts(Mockito.eq(vm1), Mockito.anyLong(), - Mockito.anyLong(), Mockito.any())).thenReturn( - new Ternary<>(new Pair<>(List.of(compatibleHost), 1), List.of(compatibleHost), Map.of(compatibleHost, false))); - Mockito.when(managementServer.applyAffinityConstraints(Mockito.any(), Mockito.any(), - Mockito.any(), Mockito.anyList())).thenReturn(Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); // Return migration to original host (host1) - should break the loop Mockito.doReturn(new Pair<>(vm1, host1)).when(clusterDrsService).getBestMigration( @@ -885,21 +856,27 @@ public void testGetBestMigration() throws ConfigurationException { vmToExcludesMap.put(vm1.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); vmToExcludesMap.put(vm2.getId(), Mockito.mock(com.cloud.deploy.DeploymentPlanner.ExcludeList.class)); - // Create capacity maps with dummy data for getClusterImbalance + // Create capacity maps with dummy data for getClusterImbalance (include both source and dest hosts) Map> hostCpuCapacityMap = new HashMap<>(); - hostCpuCapacityMap.put(destHost.getId(), new Ternary<>(1000L, 0L, 2000L)); + hostCpuCapacityMap.put(host.getId(), new Ternary<>(2000L, 0L, 3000L)); // Source host + hostCpuCapacityMap.put(destHost.getId(), new Ternary<>(1000L, 0L, 2000L)); // Dest host Map> hostMemoryCapacityMap = new HashMap<>(); - hostMemoryCapacityMap.put(destHost.getId(), new Ternary<>(1024L * 1024L * 1024L, 0L, 2L * 1024L * 1024L * 1024L)); - - // Mock getMetrics for vm1 (better improvement - should be selected) - Mockito.when(balancedAlgorithm.getMetrics(cluster, vm1, serviceOffering, destHost, hostCpuCapacityMap, - hostMemoryCapacityMap, false, 0.0)).thenReturn(new Ternary<>(1.0, 0.5, 1.5)); - - // Mock getMetrics for vm2 (worse improvement) - Mockito.when(balancedAlgorithm.getMetrics(cluster, vm2, serviceOffering, destHost, hostCpuCapacityMap, - hostMemoryCapacityMap, false, 0.0)).thenReturn(new Ternary<>(1.0, 2.5, 1.5)); - - Pair bestMigration = clusterDrsService.getBestMigration(cluster, balancedAlgorithm, + hostMemoryCapacityMap.put(host.getId(), new Ternary<>(2L * 1024L * 1024L * 1024L, 0L, 3L * 1024L * 1024L * 1024L)); // Source host + hostMemoryCapacityMap.put(destHost.getId(), new Ternary<>(1024L * 1024L * 1024L, 0L, 2L * 1024L * 1024L * 1024L)); // Dest host + + // Mock getMetrics for the optimized 10-parameter version used by getBestMigration + // Return better improvement for vm1, worse for vm2 + Mockito.doReturn(new Ternary<>(1.0, 0.5, 1.5)).when(balancedAlgorithm).getMetrics( + Mockito.eq(cluster), Mockito.eq(vm1), Mockito.any(ServiceOffering.class), + Mockito.eq(destHost), Mockito.eq(hostCpuCapacityMap), Mockito.eq(hostMemoryCapacityMap), Mockito.any(Boolean.class), + Mockito.any(Double.class), Mockito.any(double[].class), Mockito.any(Map.class)); + Mockito.doReturn(new Ternary<>(0.5, 2.5, 1.5)).when(balancedAlgorithm).getMetrics( + Mockito.eq(cluster), Mockito.eq(vm2), Mockito.any(ServiceOffering.class), + Mockito.eq(destHost), Mockito.eq(hostCpuCapacityMap), Mockito.eq(hostMemoryCapacityMap), Mockito.any(Boolean.class), + Mockito.any(Double.class), Mockito.any(double[].class), Mockito.any(Map.class)); + + + Pair bestMigration = clusterDrsService.getBestMigration(cluster, balancedAlgorithm, vmList, vmIdServiceOfferingMap, hostCpuCapacityMap, hostMemoryCapacityMap, vmToCompatibleHostsCache, vmToStorageMotionCache, vmToExcludesMap); From 10445f35a77f1e9f8c583dbed4882fd4ff367106 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Sat, 8 Nov 2025 03:28:04 +0530 Subject: [PATCH 5/6] minor cleanup --- .../com/cloud/server/ManagementService.java | 13 -- .../cluster/ClusterDrsAlgorithm.java | 15 +- .../apache/cloudstack/cluster/Balanced.java | 2 +- .../apache/cloudstack/cluster/Condensed.java | 2 +- .../cloud/server/ManagementServerImpl.java | 17 +- .../cluster/ClusterDrsServiceImpl.java | 5 +- .../server/ManagementServerImplTest.java | 174 +----------------- .../cluster/ClusterDrsServiceImplTest.java | 17 +- 8 files changed, 33 insertions(+), 212 deletions(-) diff --git a/api/src/main/java/com/cloud/server/ManagementService.java b/api/src/main/java/com/cloud/server/ManagementService.java index 21d6c35dab4b..2670b657df13 100644 --- a/api/src/main/java/com/cloud/server/ManagementService.java +++ b/api/src/main/java/com/cloud/server/ManagementService.java @@ -455,19 +455,6 @@ public interface ManagementService { Ternary, Integer>, List, Map> listHostsForMigrationOfVM(VirtualMachine vm, Long startIndex, Long pageSize, String keyword, List vmList); - /** - * Get technically compatible hosts for VM migration (storage, hypervisor, UEFI filtering). - * This is a helper method that can be used independently for caching in DRS planning. - * - * @param vm The virtual machine to migrate - * @param startIndex Starting index for pagination - * @param pageSize Page size for pagination - * @param keyword Keyword filter for host search - * @return Ternary containing: (all hosts with count, filtered compatible hosts, storage motion requirements map) - */ - Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( - VirtualMachine vm, Long startIndex, Long pageSize, String keyword); - /** * Apply affinity group constraints and other exclusion rules for VM migration. * This is a helper method that can be used independently for per-iteration affinity checks in DRS. diff --git a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java index 03022682ed86..368487c2b9b0 100644 --- a/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java +++ b/api/src/main/java/org/apache/cloudstack/cluster/ClusterDrsAlgorithm.java @@ -25,6 +25,7 @@ import com.cloud.utils.Ternary; import com.cloud.utils.component.Adapter; import com.cloud.vm.VirtualMachine; +import org.apache.commons.collections.CollectionUtils; import org.apache.commons.math3.stat.descriptive.moment.Mean; import org.apache.commons.math3.stat.descriptive.moment.StandardDeviation; @@ -38,8 +39,7 @@ import static org.apache.cloudstack.cluster.ClusterDrsService.ClusterDrsMetricUseRatio; public interface ClusterDrsAlgorithm extends Adapter { - // Reusable stateless calculator objects (thread-safe) - created once, reused for all calculations - // Apache Commons Math Mean and StandardDeviation are stateless and thread-safe + Mean MEAN_CALCULATOR = new Mean(); StandardDeviation STDDEV_CALCULATOR = new StandardDeviation(false); @@ -130,7 +130,8 @@ default Double getImbalancePostMigration(VirtualMachine vm, } /** - * Calculate imbalance from a double array. Imbalance is defined as standard deviation divided by mean. + * Calculate imbalance from an array of metric values. + * Imbalance is defined as standard deviation divided by mean. * * Uses reusable stateless calculator objects to avoid object creation overhead. * @param values array of metric values @@ -140,7 +141,7 @@ private static double calculateImbalance(double[] values) { if (values == null || values.length == 0) { return 0.0; } - // Reuse static final calculator objects (thread-safe, stateless) + double mean = MEAN_CALCULATOR.evaluate(values); if (mean == 0.0) { return 0.0; // Avoid division by zero @@ -167,9 +168,11 @@ static long getVmMetric(ServiceOffering serviceOffering, Long clusterId) throws /** * Helper method to calculate metrics from pre and post imbalance values. - * Subclasses can override this to implement different improvement calculations. */ default Ternary calculateMetricsFromImbalances(Double preImbalance, Double postImbalance) { + // This needs more research to determine the cost and benefit of a migration + // TODO: Cost should be a factor of the VM size and the host capacity + // TODO: Benefit should be a factor of the VM size and the host capacity and the number of VMs on the host final double improvement = preImbalance - postImbalance; final double cost = 0.0; final double benefit = 1.0; @@ -193,7 +196,7 @@ private Double getMetricValuePostMigration(Long clusterId, Ternary metricList) { - if (metricList == null || metricList.isEmpty()) { + if (CollectionUtils.isEmpty(metricList)) { return 0.0; } // Convert List to double[] once, avoiding repeated conversions diff --git a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java index 099a686bec7e..902ab0900bd7 100644 --- a/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java +++ b/plugins/drs/cluster/balanced/src/main/java/org/apache/cloudstack/cluster/Balanced.java @@ -85,7 +85,7 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin cluster.getId(), ClusterDrsAlgorithm.getVmMetric(serviceOffering, cluster.getId()), baseMetricsArray, hostIdToIndexMap, hostCpuMap, hostMemoryMap); - logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {} (optimized)", + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {}", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); return calculateMetricsFromImbalances(preImbalance, postImbalance); diff --git a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java index befef69f2c9a..d672ddfda615 100644 --- a/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java +++ b/plugins/drs/cluster/condensed/src/main/java/org/apache/cloudstack/cluster/Condensed.java @@ -88,7 +88,7 @@ public Ternary getMetrics(Cluster cluster, VirtualMachin cluster.getId(), ClusterDrsAlgorithm.getVmMetric(serviceOffering, cluster.getId()), baseMetricsArray, hostIdToIndexMap, hostCpuMap, hostMemoryMap); - logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {} (optimized)", + logger.trace("Cluster {} pre-imbalance: {} post-imbalance: {} Algorithm: {} VM: {} srcHost ID: {} destHost: {}", cluster, preImbalance, postImbalance, getName(), vm, vm.getHostId(), destHost); return calculateMetricsFromImbalances(postImbalance, preImbalance); diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 3031ec2157c6..1e744aa62906 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1466,8 +1466,7 @@ protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDat * @param keyword Keyword filter for host search * @return Ternary containing: (all hosts with count, filtered compatible hosts, storage motion requirements map) */ - @Override - public Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( + Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( final VirtualMachine vm, final Long startIndex, final Long pageSize, @@ -1475,9 +1474,8 @@ public Ternary, Integer>, List, Map, Integer>, List, Map>( - new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); + logger.info("Live Migration of GPU enabled VM : {} is not supported", vm); + return new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); } final long srcHostId = vm.getHostId(); @@ -1574,8 +1572,7 @@ public Ternary, Integer>, List, Map, Integer>, List, Map>( - new Pair<>(allHosts, allHostsPair.second()), new ArrayList<>(), new HashMap<>()); + return new Ternary<>(new Pair<>(allHosts, allHostsPair.second()), new ArrayList<>(), new HashMap<>()); } } else { final Long cluster = srcHost.getClusterId(); @@ -1591,13 +1588,11 @@ public Ternary, Integer>, List, Map, Integer> allHostsPairResult = new Pair<>(allHosts, allHostsPair.second()); Pair> uefiFilteredResult = filterUefiHostsForMigration(allHosts, filteredHosts, vm); if (!uefiFilteredResult.first()) { - return new Ternary, Integer>, List, Map>( - allHostsPairResult, new ArrayList<>(), new HashMap<>()); + return new Ternary<>(allHostsPairResult, new ArrayList<>(), new HashMap<>()); } filteredHosts = uefiFilteredResult.second(); - return new Ternary, Integer>, List, Map>( - allHostsPairResult, filteredHosts, requiresStorageMotion); + return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion); } /** diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index 393faf98a224..3d08e9740286 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -77,6 +77,7 @@ import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO; import org.apache.cloudstack.jobs.JobInfo; import org.apache.cloudstack.managed.context.ManagedContextTimerTask; +import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; import org.apache.commons.lang3.time.DateUtils; @@ -411,7 +412,7 @@ List> getDrsPlan(Cluster cluster, if (vmToCompatibleHostsCache.containsKey(vm.getId())) { // Check if VM has any affinity groups - if list is empty, VM has no affinity groups List affinityGroupMappings = affinityGroupVMMapDao.listByInstanceId(vm.getId()); - if (affinityGroupMappings != null && !affinityGroupMappings.isEmpty()) { + if (CollectionUtils.isNotEmpty(affinityGroupMappings)) { vmsWithAffinityGroups.add(vm.getId()); } } @@ -591,7 +592,7 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm Map requiresStorageMotion = vmToStorageMotionCache.get(vm.getId()); ExcludeList excludes = vmToExcludesMap.get(vm.getId()); - if (compatibleHosts == null || compatibleHosts.isEmpty()) { + if (CollectionUtils.isEmpty(compatibleHosts)) { continue; } diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index af746e02ccc1..da2005f61368 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -1480,12 +1480,19 @@ public void testListHostsForMigrationOfVMWithAllSupportedHypervisors() { Ternary, Integer>, List, Map> result = spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); + // Verify hypervisor is in supported hypervisors list + Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(hypervisorType, version); + // Verify validation passed for this hypervisor Assert.assertNotNull("Result should not be null for " + hypervisorType, result); - Assert.assertEquals("Should have 2 total hosts for " + hypervisorType, + Assert.assertEquals("Should return 2 total hosts for " + hypervisorType, Integer.valueOf(2), result.first().second()); Assert.assertEquals("Should have 2 suitable hosts for " + hypervisorType, 2, result.second().size()); + Assert.assertTrue("Host 101 should be available for " + hypervisorType, + result.second().stream().anyMatch(h -> h.getId() == 101L)); + Assert.assertTrue("Host 102 should be available for " + hypervisorType, + result.second().stream().anyMatch(h -> h.getId() == 102L)); // Reset mocks for next iteration Mockito.reset(vmInstanceDao, hostDao, serviceOfferingDetailsDao, volumeDao, @@ -1883,170 +1890,6 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() { result.second().stream().anyMatch(h -> h.getId() == 101L)); } - @Test - public void testListHostsForMigrationOfVMOvmHypervisor() { - // Test OVM hypervisor support - VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Ovm); - - Account caller = mockRootAdminAccount(); - Mockito.doReturn(caller).when(spy).getCaller(); - Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); - Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) - .thenReturn(null); - - HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Ovm); - Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); - - Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Ovm, null)) - .thenReturn(false); - - ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); - Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); - - VolumeVO volume = mockVolume(1L, 1L); - Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); - - DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); - Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); - - // Mock searchForServers for cluster-scoped search - HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Ovm); - HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Ovm); - List hosts = List.of(host1, host2); - Pair, Integer> hostsPair = new Pair<>(hosts, 2); - Mockito.doReturn(hostsPair).when(spy).searchForServers( - Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), - Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); - - setupMigrationMocks(vm, srcHost, hosts, volume); - - Ternary, Integer>, List, Map> result = - spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); - - // Verify OVM is in supported hypervisors list - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Ovm, null); - - // Verify response - Assert.assertNotNull(result); - Assert.assertEquals("Should return 2 total hosts for OVM", Integer.valueOf(2), result.first().second()); - Assert.assertEquals("Should have 2 suitable hosts for OVM", 2, result.second().size()); - Assert.assertTrue("Host 101 should be available", - result.second().stream().anyMatch(h -> h.getId() == 101L)); - Assert.assertTrue("Host 102 should be available", - result.second().stream().anyMatch(h -> h.getId() == 102L)); - } - - @Test - public void testListHostsForMigrationOfVMHypervHypervisor() { - // Test Hyperv hypervisor support - VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Hyperv); - - Account caller = mockRootAdminAccount(); - Mockito.doReturn(caller).when(spy).getCaller(); - Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); - Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) - .thenReturn(null); - - HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Hyperv); - Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); - - Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Hyperv, null)) - .thenReturn(false); - - ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); - Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); - - VolumeVO volume = mockVolume(1L, 1L); - Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); - - DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); - Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); - - // Mock searchForServers for cluster-scoped search - HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Hyperv); - HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Hyperv); - List hosts = List.of(host1, host2); - Pair, Integer> hostsPair = new Pair<>(hosts, 2); - Mockito.doReturn(hostsPair).when(spy).searchForServers( - Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), - Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); - - setupMigrationMocks(vm, srcHost, hosts, volume); - - Ternary, Integer>, List, Map> result = - spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); - - // Verify Hyperv is in supported hypervisors list - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Hyperv, null); - - // Verify response - Assert.assertNotNull(result); - Assert.assertEquals("Should return 2 total hosts for Hyperv", Integer.valueOf(2), result.first().second()); - Assert.assertEquals("Should have 2 suitable hosts for Hyperv", 2, result.second().size()); - Assert.assertTrue("Host 101 should be available", - result.second().stream().anyMatch(h -> h.getId() == 101L)); - Assert.assertTrue("Host 102 should be available", - result.second().stream().anyMatch(h -> h.getId() == 102L)); - } - - @Test - public void testListHostsForMigrationOfVMOvm3Hypervisor() { - // Test Ovm3 hypervisor support - VMInstanceVO vm = mockRunningVM(1L, HypervisorType.Ovm3); - - Account caller = mockRootAdminAccount(); - Mockito.doReturn(caller).when(spy).getCaller(); - Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); - Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) - .thenReturn(null); - - HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.Ovm3); - Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost); - - Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.Ovm3, null)) - .thenReturn(false); - - ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); - Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); - - VolumeVO volume = mockVolume(1L, 1L); - Mockito.when(volumeDao.findCreatedByInstance(vm.getId())).thenReturn(List.of(volume)); - - DiskOfferingVO diskOffering = mockSharedDiskOffering(1L); - Mockito.when(diskOfferingDao.findById(volume.getDiskOfferingId())).thenReturn(diskOffering); - - // Mock searchForServers for cluster-scoped search - HostVO host1 = mockHost(101L, 1L, 1L, 1L, HypervisorType.Ovm3); - HostVO host2 = mockHost(102L, 1L, 1L, 1L, HypervisorType.Ovm3); - List hosts = List.of(host1, host2); - Pair, Integer> hostsPair = new Pair<>(hosts, 2); - Mockito.doReturn(hostsPair).when(spy).searchForServers( - Mockito.anyLong(), Mockito.anyLong(), Mockito.isNull(), Mockito.any(Type.class), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.anyLong(), - Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), Mockito.isNull(), - Mockito.isNull(), Mockito.isNull(), Mockito.anyLong()); - - setupMigrationMocks(vm, srcHost, hosts, volume); - - Ternary, Integer>, List, Map> result = - spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); - - // Verify Ovm3 is in supported hypervisors list - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.Ovm3, null); - - // Verify response - Assert.assertNotNull(result); - Assert.assertEquals("Should return 2 total hosts for Ovm3", Integer.valueOf(2), result.first().second()); - Assert.assertEquals("Should have 2 suitable hosts for Ovm3", 2, result.second().size()); - Assert.assertTrue("Host 101 should be available", - result.second().stream().anyMatch(h -> h.getId() == 101L)); - Assert.assertTrue("Host 102 should be available", - result.second().stream().anyMatch(h -> h.getId() == 102L)); - } @Test public void testListHostsForMigrationOfVMWithNullKeyword() { @@ -2180,7 +2023,6 @@ private VMInstanceVO mockRunningVM(Long id, HypervisorType hypervisorType) { private VMInstanceVO mockVM(Long id, HypervisorType hypervisorType, State state) { VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); when(vm.getId()).thenReturn(id); - when(vm.getInstanceName()).thenReturn("VM-" + id); when(vm.getState()).thenReturn(state); when(vm.getHypervisorType()).thenReturn(hypervisorType); when(vm.getHostId()).thenReturn(100L); diff --git a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java index 01a02dd136c3..b76ef51c5234 100644 --- a/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/cluster/ClusterDrsServiceImplTest.java @@ -289,13 +289,11 @@ public void testGetDrsPlanWithSystemVMs() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); - // System VM should not be processed for compatibility check - Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(systemVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); } @Test @@ -331,13 +329,11 @@ public void testGetDrsPlanWithNonRunningVMs() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); - // Stopped VM should not be processed for compatibility check - Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(stoppedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); } @Test @@ -376,13 +372,11 @@ public void testGetDrsPlanWithSkipDrsFlag() throws ConfigurationException { Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); List> result = clusterDrsService.getDrsPlan(cluster, 5); assertEquals(0, result.size()); - // VM with SKIP_DRS flag should not be processed - Mockito.verify(managementServer, Mockito.never()).getTechnicallyCompatibleHosts(Mockito.eq(skippedVm), Mockito.anyLong(), Mockito.anyLong(), Mockito.any()); } @Test @@ -421,7 +415,7 @@ public void testGetDrsPlanWithNoCompatibleHosts() throws ConfigurationException Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); @@ -466,7 +460,7 @@ public void testGetDrsPlanWithExceptionInCompatibilityCheck() throws Configurati Mockito.when(hostDao.findByClusterId(1L)).thenReturn(hostList); Mockito.when(vmInstanceDao.listByClusterId(1L)).thenReturn(vmList); - Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(false); + Mockito.when(balancedAlgorithm.needsDrs(Mockito.any(), Mockito.anyList(), Mockito.anyList())).thenReturn(true); Mockito.when(serviceOfferingDao.findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong())).thenReturn(serviceOffering); Mockito.when(hostJoinDao.searchByIds(Mockito.any())).thenReturn(List.of(hostJoin1)); @@ -875,7 +869,6 @@ public void testGetBestMigration() throws ConfigurationException { Mockito.eq(destHost), Mockito.eq(hostCpuCapacityMap), Mockito.eq(hostMemoryCapacityMap), Mockito.any(Boolean.class), Mockito.any(Double.class), Mockito.any(double[].class), Mockito.any(Map.class)); - Pair bestMigration = clusterDrsService.getBestMigration(cluster, balancedAlgorithm, vmList, vmIdServiceOfferingMap, hostCpuCapacityMap, hostMemoryCapacityMap, vmToCompatibleHostsCache, vmToStorageMotionCache, vmToExcludesMap); From 56ab2c0e6e3c2e72768e922f5c7d176c305f785b Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Mon, 10 Nov 2025 16:36:37 +0530 Subject: [PATCH 6/6] Refactor ClusterDrsService --- .../cluster/ClusterDrsServiceImpl.java | 349 +++++++++++------- 1 file changed, 207 insertions(+), 142 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java index 3d08e9740286..7e1011ff9314 100644 --- a/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java +++ b/server/src/main/java/org/apache/cloudstack/cluster/ClusterDrsServiceImpl.java @@ -333,19 +333,14 @@ void generateDrsPlanForAllClusters() { * @throws ConfigurationException * If there is an error in the DRS configuration. */ - List> getDrsPlan(Cluster cluster, - int maxIterations) throws ConfigurationException { - List> migrationPlan = new ArrayList<>(); + List> getDrsPlan(Cluster cluster, int maxIterations) throws ConfigurationException { if (cluster.getAllocationState() == Disabled || maxIterations <= 0) { return Collections.emptyList(); } - ClusterDrsAlgorithm algorithm = getDrsAlgorithm(ClusterDrsAlgorithm.valueIn(cluster.getId())); List hostList = hostDao.findByClusterId(cluster.getId()); List vmList = new ArrayList<>(vmInstanceDao.listByClusterId(cluster.getId())); - int iteration = 0; - Map hostMap = hostList.stream().collect(Collectors.toMap(HostVO::getId, host -> host)); Map> hostVmMap = getHostVmMap(hostList, vmList); @@ -372,84 +367,33 @@ List> getDrsPlan(Cluster cluster, serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId())); } - // PHASE 1: Pre-compute suitable hosts (once per eligible VM - never changes) - // Use listHostsForMigrationOfVM to get hosts validated by getCapableSuitableHosts - // This ensures DRS uses the same validation as "find host for migration" command - Map> vmToCompatibleHostsCache = new HashMap<>(); - Map> vmToStorageMotionCache = new HashMap<>(); - - for (VirtualMachine vm : vmList) { - // Skip ineligible VMs - if (vm.getType().isUsedBySystem() || - vm.getState() != VirtualMachine.State.Running || - (MapUtils.isNotEmpty(vm.getDetails()) && - "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) { - continue; - } - - try { - // Use listHostsForMigrationOfVM to get suitable hosts (validated by getCapableSuitableHosts) - // This ensures the same validation as the "find host for migration" command - Ternary, Integer>, List, Map> hostsForMigration = - managementServer.listHostsForMigrationOfVM(vm, 0L, 500L, null, vmList); - - List suitableHosts = hostsForMigration.second(); // Get suitable hosts (validated by HostAllocator) - Map requiresStorageMotion = hostsForMigration.third(); + Pair>, Map>> hostCache = getCompatibleHostAndVmStorageMotionCache(vmList); + Map> vmToCompatibleHostsCache = hostCache.first(); + Map> vmToStorageMotionCache = hostCache.second(); - if (suitableHosts != null && !suitableHosts.isEmpty()) { - vmToCompatibleHostsCache.put(vm.getId(), suitableHosts); - vmToStorageMotionCache.put(vm.getId(), requiresStorageMotion); - } - } catch (Exception e) { - logger.debug("Could not get suitable hosts for VM {}: {}", vm, e.getMessage()); - } - } + Set vmsWithAffinityGroups = getVmsWithAffinityGroups(vmList, vmToCompatibleHostsCache); - // Pre-fetch affinity group mappings for all eligible VMs (once, before iterations) - // This allows us to skip expensive affinity processing for VMs without affinity groups - Set vmsWithAffinityGroups = new HashSet<>(); - for (VirtualMachine vm : vmList) { - if (vmToCompatibleHostsCache.containsKey(vm.getId())) { - // Check if VM has any affinity groups - if list is empty, VM has no affinity groups - List affinityGroupMappings = affinityGroupVMMapDao.listByInstanceId(vm.getId()); - if (CollectionUtils.isNotEmpty(affinityGroupMappings)) { - vmsWithAffinityGroups.add(vm.getId()); - } - } - } + return getMigrationPlans(maxIterations, cluster, hostMap, vmList, vmsWithAffinityGroups, vmToCompatibleHostsCache, + vmToStorageMotionCache, vmIdServiceOfferingMap, originalHostIdVmIdMap, hostVmMap, hostCpuMap, hostMemoryMap); + } - // PHASE 2: Iteration loop with affinity check per iteration + private List> getMigrationPlans( + long maxIterations, Cluster cluster, Map hostMap, List vmList, + Set vmsWithAffinityGroups, Map> vmToCompatibleHostsCache, + Map> vmToStorageMotionCache, Map vmIdServiceOfferingMap, + Map> originalHostIdVmIdMap, Map> hostVmMap, + Map> hostCpuMap, Map> hostMemoryMap + ) throws ConfigurationException { + ClusterDrsAlgorithm algorithm = getDrsAlgorithm(ClusterDrsAlgorithm.valueIn(cluster.getId())); + int iteration = 0; + List> migrationPlan = new ArrayList<>(); while (iteration < maxIterations && algorithm.needsDrs(cluster, new ArrayList<>(hostCpuMap.values()), new ArrayList<>(hostMemoryMap.values()))) { logger.debug("Starting DRS iteration {} for cluster {}", iteration + 1, cluster); // Re-evaluate affinity constraints with current (simulated) VM placements - Map vmToExcludesMap = new HashMap<>(); - for (VirtualMachine vm : vmList) { - if (vmToCompatibleHostsCache.containsKey(vm.getId())) { - Host srcHost = hostMap.get(vm.getHostId()); - if (srcHost != null) { - // Only call expensive applyAffinityConstraints for VMs with affinity groups - // For VMs without affinity groups, create minimal ExcludeList (just source host) - ExcludeList excludes; - if (vmsWithAffinityGroups.contains(vm.getId())) { - DataCenterDeployment plan = new DataCenterDeployment( - srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), - null, null, null); - VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, - vmIdServiceOfferingMap.get(vm.getId()), null, null); - - excludes = managementServer.applyAffinityConstraints( - vm, vmProfile, plan, vmList); - } else { - // VM has no affinity groups - create minimal ExcludeList (just source host) - excludes = new ExcludeList(); - excludes.addHost(vm.getHostId()); - } - vmToExcludesMap.put(vm.getId(), excludes); - } - } - } + Map vmToExcludesMap = getVmToExcludesMap(vmList, hostMap, vmsWithAffinityGroups, + vmToCompatibleHostsCache, vmIdServiceOfferingMap); logger.debug("Completed affinity evaluation for DRS iteration {} for cluster {}", iteration + 1, cluster); @@ -484,6 +428,106 @@ List> getDrsPlan(Cluster cluster, return migrationPlan; } + private Map getVmToExcludesMap(List vmList, Map hostMap, + Set vmsWithAffinityGroups, Map> vmToCompatibleHostsCache, + Map vmIdServiceOfferingMap) { + Map vmToExcludesMap = new HashMap<>(); + for (VirtualMachine vm : vmList) { + if (vmToCompatibleHostsCache.containsKey(vm.getId())) { + Host srcHost = hostMap.get(vm.getHostId()); + if (srcHost != null) { + // Only call expensive applyAffinityConstraints for VMs with affinity groups + // For VMs without affinity groups, create minimal ExcludeList (just source host) + ExcludeList excludes; + if (vmsWithAffinityGroups.contains(vm.getId())) { + DataCenterDeployment plan = new DataCenterDeployment( + srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), + null, null, null); + VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, + vmIdServiceOfferingMap.get(vm.getId()), null, null); + + excludes = managementServer.applyAffinityConstraints( + vm, vmProfile, plan, vmList); + } else { + // VM has no affinity groups - create minimal ExcludeList (just source host) + excludes = new ExcludeList(); + excludes.addHost(vm.getHostId()); + } + vmToExcludesMap.put(vm.getId(), excludes); + } + } + } + return vmToExcludesMap; + } + + + /** + * Pre-compute suitable hosts (once per eligible VM - never changes) + * Use listHostsForMigrationOfVM to get hosts validated by getCapableSuitableHosts + * This ensures DRS uses the same validation as "find host for migration" command + * + * @param vmList List of VMs to pre-compute suitable hosts for + * @return Pair of VM to compatible hosts map and VM to storage motion requirement map + */ + private Pair>, Map>> getCompatibleHostAndVmStorageMotionCache( + List vmList + ) { + Map> vmToCompatibleHostsCache = new HashMap<>(); + Map> vmToStorageMotionCache = new HashMap<>(); + + for (VirtualMachine vm : vmList) { + // Skip ineligible VMs + if (vm.getType().isUsedBySystem() || + vm.getState() != VirtualMachine.State.Running || + (MapUtils.isNotEmpty(vm.getDetails()) && + "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS)))) { + continue; + } + + try { + // Use listHostsForMigrationOfVM to get suitable hosts (validated by getCapableSuitableHosts) + // This ensures the same validation as the "find host for migration" command + Ternary, Integer>, List, Map> hostsForMigration = + managementServer.listHostsForMigrationOfVM(vm, 0L, 500L, null, vmList); + + List suitableHosts = hostsForMigration.second(); // Get suitable hosts (validated by HostAllocator) + Map requiresStorageMotion = hostsForMigration.third(); + + if (suitableHosts != null && !suitableHosts.isEmpty()) { + vmToCompatibleHostsCache.put(vm.getId(), suitableHosts); + vmToStorageMotionCache.put(vm.getId(), requiresStorageMotion); + } + } catch (Exception e) { + logger.debug("Could not get suitable hosts for VM {}: {}", vm, e.getMessage()); + } + } + return new Pair<>(vmToCompatibleHostsCache, vmToStorageMotionCache); + } + + /** + * Pre-fetch affinity group mappings for all eligible VMs (once, before iterations) + * This allows us to skip expensive affinity processing for VMs without affinity groups + * + * @param vmList List of VMs to check for affinity groups + * @param vmToCompatibleHostsCache Cached map of VM IDs to their compatible hosts + * @return Set of VM IDs that have affinity groups + */ + private Set getVmsWithAffinityGroups( + List vmList, Map> vmToCompatibleHostsCache + ) { + Set vmsWithAffinityGroups = new HashSet<>(); + for (VirtualMachine vm : vmList) { + if (vmToCompatibleHostsCache.containsKey(vm.getId())) { + // Check if VM has any affinity groups - if list is empty, VM has no affinity groups + List affinityGroupMappings = affinityGroupVMMapDao.listByInstanceId(vm.getId()); + if (CollectionUtils.isNotEmpty(affinityGroupMappings)) { + vmsWithAffinityGroups.add(vm.getId()); + } + } + } + return vmsWithAffinityGroups; + } + private ClusterDrsAlgorithm getDrsAlgorithm(String algoName) { if (drsAlgorithmMap.containsKey(algoName)) { return drsAlgorithmMap.get(algoName); @@ -548,9 +592,67 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm null); // Pre-calculate base metrics array once per iteration for optimized imbalance calculation - // This reduces complexity from O(V × H × H) to O(V × H) per iteration String metricType = getClusterDrsMetric(cluster.getId()); Map> baseMetricsMap = "cpu".equals(metricType) ? hostCpuCapacityMap : hostMemoryCapacityMap; + Pair> baseMetricsAndIndexMap = getBaseMetricsArrayAndHostIdIndexMap(cluster, baseMetricsMap); + double[] baseMetricsArray = baseMetricsAndIndexMap.first(); + Map hostIdToIndexMap = baseMetricsAndIndexMap.second(); + + double improvement = 0; + Pair bestMigration = new Pair<>(null, null); + + for (VirtualMachine vm : vmList) { + List compatibleHosts = vmToCompatibleHostsCache.get(vm.getId()); + Map requiresStorageMotion = vmToStorageMotionCache.get(vm.getId()); + ExcludeList excludes = vmToExcludesMap.get(vm.getId()); + + ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId()); + if (skipDrs(vm, compatibleHosts, serviceOffering)) { + continue; + } + + long vmCpu = (long) serviceOffering.getCpu() * serviceOffering.getSpeed(); + long vmMemory = serviceOffering.getRamSize() * 1024L * 1024L; + + for (Host destHost : compatibleHosts) { + Ternary metrics = getMetricsForMigration(cluster, algorithm, vm, vmCpu, + vmMemory, serviceOffering, destHost, hostCpuCapacityMap, hostMemoryCapacityMap, + requiresStorageMotion, preImbalance, baseMetricsArray, hostIdToIndexMap, excludes); + if (metrics == null) { + continue; + } + Double currentImprovement = metrics.first(); + Double cost = metrics.second(); + Double benefit = metrics.third(); + if (benefit > cost && (currentImprovement > improvement)) { + bestMigration = new Pair<>(vm, destHost); + improvement = currentImprovement; + } + } + } + return bestMigration; + } + + private boolean skipDrs(VirtualMachine vm, List compatibleHosts, ServiceOffering serviceOffering) { + if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running) { + return true; + } + if (MapUtils.isNotEmpty(vm.getDetails()) && + "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) { + return true; + } + if (CollectionUtils.isEmpty(compatibleHosts)) { + return true; + } + if (serviceOffering == null) { + return true; + } + return false; + } + + private Pair> getBaseMetricsArrayAndHostIdIndexMap( + Cluster cluster, Map> baseMetricsMap + ) { double[] baseMetricsArray = new double[baseMetricsMap.size()]; Map hostIdToIndexMap = new HashMap<>(); @@ -575,77 +677,40 @@ Pair getBestMigration(Cluster cluster, ClusterDrsAlgorithm System.arraycopy(baseMetricsArray, 0, trimmed, 0, index); baseMetricsArray = trimmed; } + return new Pair<>(baseMetricsArray, hostIdToIndexMap); + } - double improvement = 0; - Pair bestMigration = new Pair<>(null, null); - - for (VirtualMachine vm : vmList) { - if (vm.getType().isUsedBySystem() || vm.getState() != VirtualMachine.State.Running || - (MapUtils.isNotEmpty(vm.getDetails()) && - "true".equalsIgnoreCase(vm.getDetails().get(VmDetailConstants.SKIP_DRS))) - ) { - continue; - } - - // Use cached compatible hosts - List compatibleHosts = vmToCompatibleHostsCache.get(vm.getId()); - Map requiresStorageMotion = vmToStorageMotionCache.get(vm.getId()); - ExcludeList excludes = vmToExcludesMap.get(vm.getId()); - - if (CollectionUtils.isEmpty(compatibleHosts)) { - continue; - } - - ServiceOffering serviceOffering = vmIdServiceOfferingMap.get(vm.getId()); - if (serviceOffering == null) { - continue; - } - - // Pre-calculate VM resource requirements for quick capacity filtering - long vmCpu = (long) serviceOffering.getCpu() * serviceOffering.getSpeed(); - long vmMemory = serviceOffering.getRamSize() * 1024L * 1024L; - - for (Host destHost : compatibleHosts) { - if (cluster.getId() != destHost.getClusterId()) { - continue; - } - - // Check affinity constraints - if (excludes != null && excludes.shouldAvoid(destHost)) { - continue; - } - - // Quick capacity pre-filter: skip hosts that don't have enough capacity - Ternary destHostCpu = hostCpuCapacityMap.get(destHost.getId()); - Ternary destHostMemory = hostMemoryCapacityMap.get(destHost.getId()); - if (destHostCpu == null || destHostMemory == null) { - continue; - } + private Ternary getMetricsForMigration( + Cluster cluster, ClusterDrsAlgorithm algorithm, VirtualMachine vm, long vmCpu, long vmMemory, + ServiceOffering serviceOffering, Host destHost, Map> hostCpuCapacityMap, + Map> hostMemoryCapacityMap, Map requiresStorageMotion, + Double preImbalance, double[] baseMetricsArray, Map hostIdToIndexMap, ExcludeList excludes + ) throws ConfigurationException { + if (cluster.getId() != destHost.getClusterId()) { + return null; + } - long destHostAvailableCpu = (destHostCpu.third() - destHostCpu.second()) - destHostCpu.first(); - long destHostAvailableMemory = (destHostMemory.third() - destHostMemory.second()) - destHostMemory.first(); + // Check affinity constraints + if (excludes != null && excludes.shouldAvoid(destHost)) { + return null; + } - if (destHostAvailableCpu < vmCpu || destHostAvailableMemory < vmMemory) { - continue; // Skip hosts without sufficient capacity - } + // Quick capacity pre-filter: skip hosts that don't have enough capacity + Ternary destHostCpu = hostCpuCapacityMap.get(destHost.getId()); + Ternary destHostMemory = hostMemoryCapacityMap.get(destHost.getId()); + if (destHostCpu == null || destHostMemory == null) { + return null; + } - // getMetrics uses updated capacity maps - this is the real capacity check - // Pass pre-calculated pre-imbalance and base metrics array for optimized calculation - Ternary metrics = algorithm.getMetrics(cluster, vm, - serviceOffering, destHost, hostCpuCapacityMap, hostMemoryCapacityMap, - requiresStorageMotion.getOrDefault(destHost, false), preImbalance, - baseMetricsArray, hostIdToIndexMap); + long destHostAvailableCpu = (destHostCpu.third() - destHostCpu.second()) - destHostCpu.first(); + long destHostAvailableMemory = (destHostMemory.third() - destHostMemory.second()) - destHostMemory.first(); - Double currentImprovement = metrics.first(); - Double cost = metrics.second(); - Double benefit = metrics.third(); - if (benefit > cost && (currentImprovement > improvement)) { - bestMigration = new Pair<>(vm, destHost); - improvement = currentImprovement; - } - } + if (destHostAvailableCpu < vmCpu || destHostAvailableMemory < vmMemory) { + return null; // Skip hosts without sufficient capacity } - return bestMigration; + + return algorithm.getMetrics(cluster, vm, serviceOffering, destHost, hostCpuCapacityMap, hostMemoryCapacityMap, + requiresStorageMotion.getOrDefault(destHost, false), preImbalance, baseMetricsArray, hostIdToIndexMap); }