Skip to content

Commit cffdf71

Browse files
winterhazelwinterhazel
committed
Fix findHostsForMigration never returning hosts from other clusters
1 parent 4244c2c commit cffdf71

2 files changed

Lines changed: 83 additions & 33 deletions

File tree

‎server/src/main/java/com/cloud/server/ManagementServerImpl.java‎

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,19 +1490,7 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
14901490
ex.addProxyObject(vm.getUuid(), "vmId");
14911491
throw ex;
14921492
}
1493-
1494-
String srcHostVersion = srcHost.getHypervisorVersion();
1495-
if (HypervisorType.KVM.equals(srcHost.getHypervisorType()) && srcHostVersion == null) {
1496-
srcHostVersion = "";
1497-
}
1498-
1499-
// Check if the vm can be migrated with storage.
1500-
boolean canMigrateWithStorage = false;
1501-
1502-
List<HypervisorType> hypervisorTypes = Arrays.asList(new HypervisorType[]{HypervisorType.VMware, HypervisorType.KVM});
1503-
if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) {
1504-
canMigrateWithStorage = _hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(), srcHostVersion);
1505-
}
1493+
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
15061494

15071495
// Check if the vm is using any disks on local storage.
15081496
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
@@ -1517,6 +1505,7 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15171505
}
15181506
}
15191507

1508+
boolean canMigrateWithStorage = isStorageMigrationSupported(vm);
15201509
if (!canMigrateWithStorage && usesLocal) {
15211510
throw new InvalidParameterValueException("Unsupported operation, instance uses Local storage, cannot migrate");
15221511
}
@@ -1596,6 +1585,28 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15961585
return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion);
15971586
}
15981587

1588+
protected boolean isStorageMigrationSupported(final VirtualMachine vm) {
1589+
final Host srcHost = _hostDao.findById(vm.getHostId());
1590+
if (srcHost == null) {
1591+
throw new CloudRuntimeException(String.format("Unable to find the host where Instance [%s] is running.", vm.getInstanceName()));
1592+
}
1593+
1594+
final List<HypervisorType> hypervisorTypes = Arrays.asList(HypervisorType.VMware, HypervisorType.KVM);
1595+
if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) {
1596+
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
1597+
return _hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(), srcHostVersion);
1598+
}
1599+
return false;
1600+
}
1601+
1602+
protected String getHypervisorVersionOfHost(final Host host) {
1603+
final String version = host.getHypervisorVersion();
1604+
if (version == null && HypervisorType.KVM.equals(host.getHypervisorType())) {
1605+
return "";
1606+
}
1607+
return version;
1608+
}
1609+
15991610
/**
16001611
* Apply affinity group constraints and other exclusion rules for VM migration.
16011612
* This builds an ExcludeList based on affinity groups, DPDK requirements, and dedicated resources.
@@ -1642,16 +1653,14 @@ public ExcludeList applyAffinityConstraints(VirtualMachine vm, VirtualMachinePro
16421653
* @param plan The deployment plan
16431654
* @param compatibleHosts List of technically compatible hosts
16441655
* @param excludes ExcludeList with hosts to avoid
1645-
* @param srcHost Source host (for architecture filtering)
16461656
* @return List of suitable hosts with capacity
16471657
*/
16481658
protected List<Host> getCapableSuitableHosts(
16491659
final VirtualMachine vm,
16501660
final VirtualMachineProfile vmProfile,
16511661
final DataCenterDeployment plan,
16521662
final List<? extends Host> compatibleHosts,
1653-
final ExcludeList excludes,
1654-
final Host srcHost) {
1663+
final ExcludeList excludes) {
16551664

16561665
List<Host> suitableHosts = new ArrayList<>();
16571666

@@ -1677,6 +1686,7 @@ protected List<Host> getCapableSuitableHosts(
16771686

16781687
// Only list hosts of the same architecture as the source Host in a multi-arch zone
16791688
if (!suitableHosts.isEmpty()) {
1689+
Host srcHost = _hostDao.findById(vm.getHostId());
16801690
List<CPU.CPUArch> clusterArchs = ApiDBUtils.listZoneClustersArchs(vm.getDataCenterId());
16811691
if (CollectionUtils.isNotEmpty(clusterArchs) && clusterArchs.size() > 1) {
16821692
suitableHosts = suitableHosts.stream().filter(h -> h.getArch() == srcHost.getArch()).collect(Collectors.toList());
@@ -1707,22 +1717,31 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17071717
}
17081718

17091719
// Create deployment plan and VM profile
1710-
final Host srcHost = _hostDao.findById(vm.getHostId());
1711-
final DataCenterDeployment plan = new DataCenterDeployment(
1712-
srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
1720+
final DataCenterDeployment plan = createDeploymentPlanForMigrationListing(vm);
17131721
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(
17141722
vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
17151723

17161724
// Apply affinity constraints
17171725
final ExcludeList excludes = applyAffinityConstraints(vm, vmProfile, plan, vmList);
17181726

17191727
// Get hosts with capacity
1720-
List<Host> suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes, srcHost);
1728+
List<Host> suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes);
17211729

17221730
final Pair<List<? extends Host>, Integer> otherHosts = new Pair<>(allHostsPair.first(), allHostsPair.second());
17231731
return new Ternary<>(otherHosts, suitableHosts, requiresStorageMotion);
17241732
}
17251733

1734+
protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm) {
1735+
final Host srcHost = _hostDao.findById(vm.getHostId());
1736+
1737+
final boolean canMigrateWithStorage = isStorageMigrationSupported(vm);
1738+
if (canMigrateWithStorage) {
1739+
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), null, null, null, null);
1740+
}
1741+
1742+
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
1743+
}
1744+
17261745
/**
17271746
* Add non DPDK enabled hosts to the avoid list
17281747
*/

‎server/src/test/java/com/cloud/server/ManagementServerImplTest.java‎

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.cloud.dc.DataCenterVO;
2121
import com.cloud.dc.Vlan.VlanType;
2222
import com.cloud.dc.dao.DataCenterDao;
23+
import com.cloud.deploy.DataCenterDeployment;
2324
import com.cloud.deploy.DeploymentPlanningManager;
2425
import com.cloud.exception.InvalidParameterValueException;
2526
import com.cloud.exception.PermissionDeniedException;
@@ -227,7 +228,7 @@ public void setup() throws IllegalAccessException, NoSuchFieldException {
227228
apiDBUtilsMock = Mockito.mockStatic(ApiDBUtils.class);
228229
// Return empty list to avoid architecture filtering in most tests
229230
apiDBUtilsMock.when(() -> ApiDBUtils.listZoneClustersArchs(Mockito.anyLong()))
230-
.thenReturn(new ArrayList<>());
231+
.thenReturn(new ArrayList<>());
231232
}
232233

233234
@After
@@ -246,7 +247,7 @@ private void overrideDefaultConfigValue(final ConfigKey configKey, final String
246247
}
247248

248249
@Test(expected = InvalidParameterValueException.class)
249-
public void testDuplicateRegistraitons(){
250+
public void testDuplicateRegistrations() {
250251
String accountName = "account";
251252
String publicKeyString = "ssh-rsa very public";
252253
String publicKeyMaterial = spy.getPublicKeyFromKeyKeyMaterial(publicKeyString);
@@ -888,7 +889,7 @@ public void testListHostsForMigrationOfVMWithSystemVM() {
888889
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
889890

890891
// Verify storage motion capability was checked
891-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null);
892+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null);
892893

893894
// Verify result structure and data
894895
Assert.assertNotNull(result);
@@ -952,7 +953,7 @@ public void testListHostsForMigrationOfVMWithDomainRouter() {
952953
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
953954

954955
// Verify hypervisor capabilities were checked
955-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, "");
956+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, "");
956957

957958
// Verify result contains expected hosts
958959
Assert.assertNotNull(result);
@@ -1097,7 +1098,7 @@ public void testListHostsForMigrationOfVMKVMWithNullHypervisorVersion() {
10971098
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
10981099

10991100
// Verify KVM null version was converted to empty string
1100-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, "");
1101+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, "");
11011102

11021103
// Verify result data
11031104
Assert.assertNotNull(result);
@@ -1416,7 +1417,7 @@ public void testListHostsForMigrationOfVMStorageMotionCapabilityCheck() {
14161417
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
14171418

14181419
// Verify storage motion capability was checked for User VM
1419-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null);
1420+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null);
14201421

14211422
// Verify response data
14221423
Assert.assertNotNull(result);
@@ -1481,7 +1482,7 @@ public void testListHostsForMigrationOfVMWithAllSupportedHypervisors() {
14811482
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
14821483

14831484
// Verify hypervisor is in supported hypervisors list
1484-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(hypervisorType, version);
1485+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(hypervisorType, version);
14851486

14861487
// Verify validation passed for this hypervisor
14871488
Assert.assertNotNull("Result should not be null for " + hypervisorType, result);
@@ -1589,7 +1590,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForSystemVM() {
15891590
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
15901591

15911592
// Verify that storage motion capability was checked for system VM (VMware is in hypervisorTypes list)
1592-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null);
1593+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null);
15931594

15941595
// Verify response structure
15951596
Assert.assertNotNull(result);
@@ -1642,7 +1643,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForUserVM() {
16421643
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
16431644

16441645
// Verify User VM can migrate with storage (User VM type always checks)
1645-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, "");
1646+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, "");
16461647

16471648
// Verify response data
16481649
Assert.assertNotNull(result);
@@ -1695,7 +1696,7 @@ public void testListHostsForMigrationOfVMWithoutStorageMotionClusterScope() {
16951696
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
16961697

16971698
// Verify XenServer without storage motion was checked
1698-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.XenServer, null);
1699+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.XenServer, null);
16991700
// Verify cluster-scoped search was used (not zone-wide)
17001701
Mockito.verify(spy).searchForServers(
17011702
Mockito.eq(0L), Mockito.eq(20L), Mockito.isNull(), Mockito.any(Type.class),
@@ -1845,14 +1846,14 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() {
18451846
Mockito.doReturn(caller).when(spy).getCaller();
18461847
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
18471848
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
1848-
.thenReturn(null);
1849+
.thenReturn(null);
18491850

18501851
HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware);
18511852
Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost);
18521853

18531854
// VMware with DomainRouter should still check storage motion
18541855
Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null))
1855-
.thenReturn(true);
1856+
.thenReturn(true);
18561857

18571858
ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class);
18581859
Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering);
@@ -1880,7 +1881,7 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() {
18801881
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
18811882

18821883
// Verify VMware always checks storage motion (hypervisorTypes list includes VMware)
1883-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null);
1884+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null);
18841885

18851886
// Verify response
18861887
Assert.assertNotNull(result);
@@ -2074,4 +2075,34 @@ private DiskOfferingVO mockSharedDiskOffering(Long id) {
20742075
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);
20752076
return diskOffering;
20762077
}
2078+
2079+
@Test
2080+
public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhenStorageMigrationIsSupported() {
2081+
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM);
2082+
Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm);
2083+
2084+
HostVO srcHost = mockHost(100L, 1L, 2L, 3L, HypervisorType.KVM);
2085+
Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong());
2086+
2087+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm);
2088+
2089+
Assert.assertEquals(3L, deploymentPlan.getDataCenterId());
2090+
Assert.assertEquals(Long.valueOf(2L), deploymentPlan.getPodId());
2091+
Assert.assertNull(deploymentPlan.getClusterId());
2092+
}
2093+
2094+
@Test
2095+
public void createDeploymentPlanForMigrationListingTestAllocatesInSourceClusterWhenStorageMigrationIsNotSupported() {
2096+
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer);
2097+
Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm);
2098+
2099+
HostVO srcHost = mockHost(200L, 4L, 5L, 6L, HypervisorType.XenServer);
2100+
Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong());
2101+
2102+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm);
2103+
2104+
Assert.assertEquals(6L, deploymentPlan.getDataCenterId());
2105+
Assert.assertEquals(5L, (long) deploymentPlan.getPodId());
2106+
Assert.assertEquals(4L, (long) deploymentPlan.getClusterId());
2107+
}
20772108
}

0 commit comments

Comments
 (0)