Skip to content

Commit ec2d3ea

Browse files
winterhazelwinterhazel
authored
Fix findHostsForMigration never returning hosts from other clusters (#13452)
1 parent a4b102f commit ec2d3ea

2 files changed

Lines changed: 86 additions & 44 deletions

File tree

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

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,6 +1469,7 @@ protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDat
14691469
*/
14701470
Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boolean>> getTechnicallyCompatibleHosts(
14711471
final VirtualMachine vm,
1472+
final Host srcHost,
14721473
final Long startIndex,
14731474
final Long pageSize,
14741475
final String keyword) {
@@ -1479,31 +1480,6 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
14791480
return new Ternary<>(new Pair<>(new ArrayList<>(), 0), new ArrayList<>(), new HashMap<>());
14801481
}
14811482

1482-
final long srcHostId = vm.getHostId();
1483-
final Host srcHost = _hostDao.findById(srcHostId);
1484-
if (srcHost == null) {
1485-
if (logger.isDebugEnabled()) {
1486-
logger.debug("Unable to find the host with ID: " + srcHostId + " of this Instance: " + vm);
1487-
}
1488-
final InvalidParameterValueException ex = new InvalidParameterValueException("Unable to find the host (with specified ID) of instance with specified ID");
1489-
ex.addProxyObject(String.valueOf(srcHostId), "hostId");
1490-
ex.addProxyObject(vm.getUuid(), "vmId");
1491-
throw ex;
1492-
}
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-
}
1506-
15071483
// Check if the vm is using any disks on local storage.
15081484
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
15091485
final List<VolumeVO> volumes = _volumeDao.findCreatedByInstance(vmProfile.getId());
@@ -1517,10 +1493,12 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15171493
}
15181494
}
15191495

1496+
boolean canMigrateWithStorage = isStorageMigrationSupported(vm, srcHost);
15201497
if (!canMigrateWithStorage && usesLocal) {
15211498
throw new InvalidParameterValueException("Unsupported operation, instance uses Local storage, cannot migrate");
15221499
}
15231500

1501+
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
15241502
final Type hostType = srcHost.getType();
15251503
Pair<List<HostVO>, Integer> allHostsPair = null;
15261504
List<HostVO> allHosts = null;
@@ -1596,6 +1574,23 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15961574
return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion);
15971575
}
15981576

1577+
protected boolean isStorageMigrationSupported(final VirtualMachine vm, final Host srcHost) {
1578+
final List<HypervisorType> hypervisorTypes = Arrays.asList(HypervisorType.VMware, HypervisorType.KVM);
1579+
if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) {
1580+
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
1581+
return _hypervisorCapabilitiesDao.isStorageMotionSupported(srcHost.getHypervisorType(), srcHostVersion);
1582+
}
1583+
return false;
1584+
}
1585+
1586+
protected String getHypervisorVersionOfHost(final Host host) {
1587+
final String version = host.getHypervisorVersion();
1588+
if (version == null && HypervisorType.KVM.equals(host.getHypervisorType())) {
1589+
return "";
1590+
}
1591+
return version;
1592+
}
1593+
15991594
/**
16001595
* Apply affinity group constraints and other exclusion rules for VM migration.
16011596
* This builds an ExcludeList based on affinity groups, DPDK requirements, and dedicated resources.
@@ -1692,9 +1687,19 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
16921687

16931688
validateVmForHostMigration(vm);
16941689

1690+
final long srcHostId = vm.getHostId();
1691+
final Host srcHost = _hostDao.findById(srcHostId);
1692+
if (srcHost == null) {
1693+
logger.debug("Unable to find the host with ID: {} of this Instance: {}", srcHostId, vm);
1694+
final InvalidParameterValueException ex = new InvalidParameterValueException("Unable to find the host (with specified ID) of instance with specified ID");
1695+
ex.addProxyObject(String.valueOf(srcHostId), "hostId");
1696+
ex.addProxyObject(vm.getUuid(), "vmId");
1697+
throw ex;
1698+
}
1699+
16951700
// Get technically compatible hosts (storage, hypervisor, UEFI)
16961701
Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boolean>> compatibilityResult =
1697-
getTechnicallyCompatibleHosts(vm, startIndex, pageSize, keyword);
1702+
getTechnicallyCompatibleHosts(vm, srcHost, startIndex, pageSize, keyword);
16981703

16991704
Pair<List<? extends Host>, Integer> allHostsPair = compatibilityResult.first();
17001705
List<? extends Host> filteredHosts = compatibilityResult.second();
@@ -1707,9 +1712,7 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17071712
}
17081713

17091714
// 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);
1715+
final DataCenterDeployment plan = createDeploymentPlanForMigrationListing(vm, srcHost);
17131716
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(
17141717
vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
17151718

@@ -1723,6 +1726,14 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17231726
return new Ternary<>(otherHosts, suitableHosts, requiresStorageMotion);
17241727
}
17251728

1729+
protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm, final Host srcHost) {
1730+
final boolean canMigrateWithStorage = isStorageMigrationSupported(vm, srcHost);
1731+
if (canMigrateWithStorage) {
1732+
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), null, null, null, null);
1733+
}
1734+
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
1735+
}
1736+
17261737
/**
17271738
* Add non DPDK enabled hosts to the avoid list
17281739
*/

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

Lines changed: 46 additions & 15 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);
@@ -826,9 +827,13 @@ public void testListHostsForMigrationOfVMLxcUserVM() {
826827
@Test
827828
public void testListHostsForMigrationOfVMGpuEnabled() {
828829
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM);
830+
long hostId = vm.getHostId();
831+
HostVO srcHost = mockHost(hostId, 4L, 5L, 6L, HypervisorType.KVM);
829832
Account caller = mockRootAdminAccount();
833+
830834
Mockito.doReturn(caller).when(spy).getCaller();
831835
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
836+
Mockito.doReturn(srcHost).when(hostDao).findById(hostId);
832837

833838
// Mock GPU detail
834839
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
@@ -888,7 +893,7 @@ public void testListHostsForMigrationOfVMWithSystemVM() {
888893
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
889894

890895
// Verify storage motion capability was checked
891-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.VMware, null);
896+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.VMware, null);
892897

893898
// Verify result structure and data
894899
Assert.assertNotNull(result);
@@ -952,7 +957,7 @@ public void testListHostsForMigrationOfVMWithDomainRouter() {
952957
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
953958

954959
// Verify hypervisor capabilities were checked
955-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.KVM, "");
960+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.KVM, "");
956961

957962
// Verify result contains expected hosts
958963
Assert.assertNotNull(result);
@@ -1097,7 +1102,7 @@ public void testListHostsForMigrationOfVMKVMWithNullHypervisorVersion() {
10971102
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
10981103

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

11021107
// Verify result data
11031108
Assert.assertNotNull(result);
@@ -1416,7 +1421,7 @@ public void testListHostsForMigrationOfVMStorageMotionCapabilityCheck() {
14161421
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
14171422

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

14211426
// Verify response data
14221427
Assert.assertNotNull(result);
@@ -1481,7 +1486,7 @@ public void testListHostsForMigrationOfVMWithAllSupportedHypervisors() {
14811486
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
14821487

14831488
// Verify hypervisor is in supported hypervisors list
1484-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(hypervisorType, version);
1489+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(hypervisorType, version);
14851490

14861491
// Verify validation passed for this hypervisor
14871492
Assert.assertNotNull("Result should not be null for " + hypervisorType, result);
@@ -1508,8 +1513,6 @@ public void testListHostsForMigrationOfVMSourceHostNotFound() {
15081513
Account caller = mockRootAdminAccount();
15091514
Mockito.doReturn(caller).when(spy).getCaller();
15101515
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
1511-
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
1512-
.thenReturn(null);
15131516
Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(null);
15141517

15151518
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
@@ -1589,7 +1592,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForSystemVM() {
15891592
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
15901593

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

15941597
// Verify response structure
15951598
Assert.assertNotNull(result);
@@ -1642,7 +1645,7 @@ public void testListHostsForMigrationOfVMStorageMotionCheckForUserVM() {
16421645
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
16431646

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

16471650
// Verify response data
16481651
Assert.assertNotNull(result);
@@ -1695,7 +1698,7 @@ public void testListHostsForMigrationOfVMWithoutStorageMotionClusterScope() {
16951698
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
16961699

16971700
// Verify XenServer without storage motion was checked
1698-
Mockito.verify(hypervisorCapabilitiesDao).isStorageMotionSupported(HypervisorType.XenServer, null);
1701+
Mockito.verify(hypervisorCapabilitiesDao, Mockito.atLeastOnce()).isStorageMotionSupported(HypervisorType.XenServer, null);
16991702
// Verify cluster-scoped search was used (not zone-wide)
17001703
Mockito.verify(spy).searchForServers(
17011704
Mockito.eq(0L), Mockito.eq(20L), Mockito.isNull(), Mockito.any(Type.class),
@@ -1845,14 +1848,14 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() {
18451848
Mockito.doReturn(caller).when(spy).getCaller();
18461849
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
18471850
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
1848-
.thenReturn(null);
1851+
.thenReturn(null);
18491852

18501853
HostVO srcHost = mockHost(100L, 1L, 1L, 1L, HypervisorType.VMware);
18511854
Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(srcHost);
18521855

18531856
// VMware with DomainRouter should still check storage motion
18541857
Mockito.when(hypervisorCapabilitiesDao.isStorageMotionSupported(HypervisorType.VMware, null))
1855-
.thenReturn(true);
1858+
.thenReturn(true);
18561859

18571860
ServiceOfferingVO offering = Mockito.mock(ServiceOfferingVO.class);
18581861
Mockito.when(offeringDao.findById(vm.getId(), vm.getServiceOfferingId())).thenReturn(offering);
@@ -1880,7 +1883,7 @@ public void testListHostsForMigrationOfVMVmwareStorageMotionCheck() {
18801883
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
18811884

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

18851888
// Verify response
18861889
Assert.assertNotNull(result);
@@ -2074,4 +2077,32 @@ private DiskOfferingVO mockSharedDiskOffering(Long id) {
20742077
Mockito.when(diskOffering.isUseLocalStorage()).thenReturn(false);
20752078
return diskOffering;
20762079
}
2080+
2081+
@Test
2082+
public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhenStorageMigrationIsSupported() {
2083+
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM);
2084+
HostVO srcHost = mockHost(vm.getHostId(), 1L, 2L, 3L, HypervisorType.KVM);
2085+
2086+
Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm, srcHost);
2087+
2088+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost);
2089+
2090+
Assert.assertEquals(3L, deploymentPlan.getDataCenterId());
2091+
Assert.assertEquals(2L, (long) deploymentPlan.getPodId());
2092+
Assert.assertNull(deploymentPlan.getClusterId());
2093+
}
2094+
2095+
@Test
2096+
public void createDeploymentPlanForMigrationListingTestAllocatesInSourceClusterWhenStorageMigrationIsNotSupported() {
2097+
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer);
2098+
HostVO srcHost = mockHost(vm.getHostId(), 4L, 5L, 6L, HypervisorType.XenServer);
2099+
2100+
Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm, srcHost);
2101+
2102+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost);
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)