From f6c603b28db79b81bf689e0a6ce09f772a92a4e7 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Thu, 18 Jun 2026 17:17:30 -0300 Subject: [PATCH 1/2] Fix `findHostsForMigration` never returning hosts from other clusters --- .../cloud/server/ManagementServerImpl.java | 59 ++++++++++++------- .../server/ManagementServerImplTest.java | 57 ++++++++++++++---- 2 files changed, 83 insertions(+), 33 deletions(-) diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index bd4c311e3cd5..32034e1425aa 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1490,19 +1490,7 @@ Ternary, Integer>, List, Map hypervisorTypes = Arrays.asList(new HypervisorType[]{HypervisorType.VMware, HypervisorType.KVM}); - if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) { - canMigrateWithStorage = _hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(), srcHostVersion); - } + final String srcHostVersion = getHypervisorVersionOfHost(srcHost); // Check if the vm is using any disks on local storage. final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null); @@ -1517,6 +1505,7 @@ Ternary, Integer>, List, Map, Integer>, List, Map(allHostsPairResult, filteredHosts, requiresStorageMotion); } + protected boolean isStorageMigrationSupported(final VirtualMachine vm) { + final Host srcHost = _hostDao.findById(vm.getHostId()); + if (srcHost == null) { + throw new CloudRuntimeException(String.format("Unable to find the host where Instance [%s] is running.", vm.getInstanceName())); + } + + final List hypervisorTypes = Arrays.asList(HypervisorType.VMware, HypervisorType.KVM); + if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) { + final String srcHostVersion = getHypervisorVersionOfHost(srcHost); + return _hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(), srcHostVersion); + } + return false; + } + + protected String getHypervisorVersionOfHost(final Host host) { + final String version = host.getHypervisorVersion(); + if (version == null && HypervisorType.KVM.equals(host.getHypervisorType())) { + return ""; + } + return version; + } + /** * Apply affinity group constraints and other exclusion rules for VM migration. * This builds an ExcludeList based on affinity groups, DPDK requirements, and dedicated resources. @@ -1642,7 +1653,6 @@ public ExcludeList applyAffinityConstraints(VirtualMachine vm, VirtualMachinePro * @param plan The deployment plan * @param compatibleHosts List of technically compatible hosts * @param excludes ExcludeList with hosts to avoid - * @param srcHost Source host (for architecture filtering) * @return List of suitable hosts with capacity */ protected List getCapableSuitableHosts( @@ -1650,8 +1660,7 @@ protected List getCapableSuitableHosts( final VirtualMachineProfile vmProfile, final DataCenterDeployment plan, final List compatibleHosts, - final ExcludeList excludes, - final Host srcHost) { + final ExcludeList excludes) { List suitableHosts = new ArrayList<>(); @@ -1677,6 +1686,7 @@ protected List getCapableSuitableHosts( // Only list hosts of the same architecture as the source Host in a multi-arch zone if (!suitableHosts.isEmpty()) { + Host srcHost = _hostDao.findById(vm.getHostId()); List clusterArchs = ApiDBUtils.listZoneClustersArchs(vm.getDataCenterId()); if (CollectionUtils.isNotEmpty(clusterArchs) && clusterArchs.size() > 1) { suitableHosts = suitableHosts.stream().filter(h -> h.getArch() == srcHost.getArch()).collect(Collectors.toList()); @@ -1707,9 +1717,7 @@ public Ternary, Integer>, List, Map, Integer>, List, Map suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes, srcHost); + List suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes); final Pair, Integer> otherHosts = new Pair<>(allHostsPair.first(), allHostsPair.second()); return new Ternary<>(otherHosts, suitableHosts, requiresStorageMotion); } + protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm) { + final Host srcHost = _hostDao.findById(vm.getHostId()); + + final boolean canMigrateWithStorage = isStorageMigrationSupported(vm); + if (canMigrateWithStorage) { + return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), null, null, null, null); + } + + return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null); + } + /** * Add non DPDK enabled hosts to the avoid list */ diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index da2005f61368..412ed1dde09a 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -20,6 +20,7 @@ import com.cloud.dc.DataCenterVO; import com.cloud.dc.Vlan.VlanType; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.deploy.DataCenterDeployment; import com.cloud.deploy.DeploymentPlanningManager; import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.PermissionDeniedException; @@ -227,7 +228,7 @@ public void setup() throws IllegalAccessException, NoSuchFieldException { apiDBUtilsMock = Mockito.mockStatic(ApiDBUtils.class); // Return empty list to avoid architecture filtering in most tests apiDBUtilsMock.when(() -> ApiDBUtils.listZoneClustersArchs(Mockito.anyLong())) - .thenReturn(new ArrayList<>()); + .thenReturn(new ArrayList<>()); } @After @@ -246,7 +247,7 @@ private void overrideDefaultConfigValue(final ConfigKey configKey, final String } @Test(expected = InvalidParameterValueException.class) - public void testDuplicateRegistraitons(){ + public void testDuplicateRegistrations() { String accountName = "account"; String publicKeyString = "ssh-rsa very public"; String publicKeyMaterial = spy.getPublicKeyFromKeyKeyMaterial(publicKeyString); @@ -888,7 +889,7 @@ public void testListHostsForMigrationOfVMWithSystemVM() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify storage motion capability was checked - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null); // Verify result structure and data Assert.assertNotNull(result); @@ -952,7 +953,7 @@ public void testListHostsForMigrationOfVMWithDomainRouter() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify hypervisor capabilities were checked - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, ""); // Verify result contains expected hosts Assert.assertNotNull(result); @@ -1097,7 +1098,7 @@ public void testListHostsForMigrationOfVMKVMWithNullHypervisorVersion() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify KVM null version was converted to empty string - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, ""); // Verify result data Assert.assertNotNull(result); @@ -1416,7 +1417,7 @@ public void testListHostsForMigrationOfVMStorageMotionCapabilityCheck() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify storage motion capability was checked for User VM - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null); // Verify response data Assert.assertNotNull(result); @@ -1481,7 +1482,7 @@ public void testListHostsForMigrationOfVMWithAllSupportedHypervisors() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify hypervisor is in supported hypervisors list - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(hypervisorType, version); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(hypervisorType, version); // Verify validation passed for this hypervisor Assert.assertNotNull("Result should not be null for " + hypervisorType, result); @@ -1589,7 +1590,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForSystemVM() { 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); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null); // Verify response structure Assert.assertNotNull(result); @@ -1642,7 +1643,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForUserVM() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify User VM can migrate with storage (User VM type always checks) - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, ""); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, ""); // Verify response data Assert.assertNotNull(result); @@ -1695,7 +1696,7 @@ public void testListHostsForMigrationOfVMWithoutStorageMotionClusterScope() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify XenServer without storage motion was checked - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.XenServer, null); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).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), @@ -1845,14 +1846,14 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() { 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); + .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); + .thenReturn(true); ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class); Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering); @@ -1880,7 +1881,7 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() { spy.listHostsForMigrationOfVM(1L, 0L, 20L, null); // Verify VMware always checks storage motion (hypervisorTypes list includes VMware) - Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null); + Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null); // Verify response Assert.assertNotNull(result); @@ -2074,4 +2075,34 @@ private DiskOfferingVO mockSharedDiskOffering(Long id) { Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false); return diskOffering; } + + @Test + public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhenStorageMigrationIsSupported() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm); + + HostVO srcHost = mockHost(100L, 1L, 2L, 3L, HypervisorType.KVM); + Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong()); + + DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm); + + Assert.assertEquals(3L, deploymentPlan.getDataCenterId()); + Assert.assertEquals(2L, (long) deploymentPlan.getPodId()); + Assert.assertNull(deploymentPlan.getClusterId()); + } + + @Test + public void createDeploymentPlanForMigrationListingTestAllocatesInSourceClusterWhenStorageMigrationIsNotSupported() { + VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer); + Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm); + + HostVO srcHost = mockHost(200L, 4L, 5L, 6L, HypervisorType.XenServer); + Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong()); + + DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm); + + Assert.assertEquals(6L, deploymentPlan.getDataCenterId()); + Assert.assertEquals(5L, (long) deploymentPlan.getPodId()); + Assert.assertEquals(4L, (long) deploymentPlan.getClusterId()); + } } From 8a48f3da51f9f8e83b9f98149c88937575a921df Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Fri, 19 Jun 2026 09:24:33 -0300 Subject: [PATCH 2/2] Address reviews --- .../cloud/server/ManagementServerImpl.java | 45 ++++++++----------- .../server/ManagementServerImplTest.java | 20 ++++----- 2 files changed, 28 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 32034e1425aa..d8f075ed3a67 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1469,6 +1469,7 @@ protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDat */ Ternary, Integer>, List, Map> getTechnicallyCompatibleHosts( final VirtualMachine vm, + final Host srcHost, final Long startIndex, final Long pageSize, final String keyword) { @@ -1479,19 +1480,6 @@ Ternary, Integer>, List, Map(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>()); } - final long srcHostId = vm.getHostId(); - final Host srcHost = _hostDao.findById(srcHostId); - if (srcHost == null) { - if (logger.isDebugEnabled()) { - logger.debug("Unable to find the host with ID: " + srcHostId + " of this Instance: " + vm); - } - final InvalidParameterValueException ex = new InvalidParameterValueException("Unable to find the host (with specified ID) of instance with specified ID"); - ex.addProxyObject(String.valueOf(srcHostId), "hostId"); - ex.addProxyObject(vm.getUuid(), "vmId"); - throw ex; - } - final String srcHostVersion = getHypervisorVersionOfHost(srcHost); - // Check if the vm is using any disks on local storage. final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null); final List volumes = _volumeDao.findCreatedByInstance(vmProfile.getId()); @@ -1505,11 +1493,12 @@ Ternary, Integer>, List, Map, Integer> allHostsPair = null; List allHosts = null; @@ -1585,12 +1574,7 @@ Ternary, Integer>, List, Map(allHostsPairResult, filteredHosts, requiresStorageMotion); } - protected boolean isStorageMigrationSupported(final VirtualMachine vm) { - final Host srcHost = _hostDao.findById(vm.getHostId()); - if (srcHost == null) { - throw new CloudRuntimeException(String.format("Unable to find the host where Instance [%s] is running.", vm.getInstanceName())); - } - + protected boolean isStorageMigrationSupported(final VirtualMachine vm, final Host srcHost) { final List hypervisorTypes = Arrays.asList(HypervisorType.VMware, HypervisorType.KVM); if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) { final String srcHostVersion = getHypervisorVersionOfHost(srcHost); @@ -1702,9 +1686,19 @@ public Ternary, Integer>, List, Map, Integer>, List, Map> compatibilityResult = - getTechnicallyCompatibleHosts(vm, startIndex, pageSize, keyword); + getTechnicallyCompatibleHosts(vm, srcHost, startIndex, pageSize, keyword); Pair, Integer> allHostsPair = compatibilityResult.first(); List filteredHosts = compatibilityResult.second(); @@ -1717,7 +1711,7 @@ public Ternary, Integer>, List, Map, Integer>, List, Map(otherHosts, suitableHosts, requiresStorageMotion); } - protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm) { - final Host srcHost = _hostDao.findById(vm.getHostId()); - - final boolean canMigrateWithStorage = isStorageMigrationSupported(vm); + protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm, final Host srcHost) { + final boolean canMigrateWithStorage = isStorageMigrationSupported(vm, srcHost); if (canMigrateWithStorage) { return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), null, null, null, null); } - return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null); } diff --git a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java index 412ed1dde09a..2f3e97716a03 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerImplTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerImplTest.java @@ -827,9 +827,13 @@ public void testListHostsForMigrationOfVMLxcUserVM() { @Test public void testListHostsForMigrationOfVMGpuEnabled() { VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); + long hostId = vm.getHostId(); + HostVO srcHost = mockHost(hostId, 4L, 5L, 6L, HypervisorType.KVM); Account caller = mockRootAdminAccount(); + Mockito.doReturn(caller).when(spy).getCaller(); Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm); + Mockito.doReturn(srcHost).when(hostDao).findById(hostId); // Mock GPU detail Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString())) @@ -1509,8 +1513,6 @@ public void testListHostsForMigrationOfVMSourceHostNotFound() { 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); @@ -2079,12 +2081,11 @@ private DiskOfferingVO mockSharedDiskOffering(Long id) { @Test public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhenStorageMigrationIsSupported() { VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM); - Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm); + HostVO srcHost = mockHost(vm.getHostId(), 1L, 2L, 3L, HypervisorType.KVM); - HostVO srcHost = mockHost(100L, 1L, 2L, 3L, HypervisorType.KVM); - Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong()); + Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm, srcHost); - DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm); + DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost); Assert.assertEquals(3L, deploymentPlan.getDataCenterId()); Assert.assertEquals(2L, (long) deploymentPlan.getPodId()); @@ -2094,12 +2095,11 @@ public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhen @Test public void createDeploymentPlanForMigrationListingTestAllocatesInSourceClusterWhenStorageMigrationIsNotSupported() { VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer); - Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm); + HostVO srcHost = mockHost(vm.getHostId(), 4L, 5L, 6L, HypervisorType.XenServer); - HostVO srcHost = mockHost(200L, 4L, 5L, 6L, HypervisorType.XenServer); - Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong()); + Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm, srcHost); - DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm); + DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost); Assert.assertEquals(6L, deploymentPlan.getDataCenterId()); Assert.assertEquals(5L, (long) deploymentPlan.getPodId());