Skip to content

Fix findHostsForMigration never returning hosts from other clusters #13452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
winterhazel wants to merge 1 commit into
base: 4.20
Choose a base branch
from
+83 −33
Open

Fix findHostsForMigration never returning hosts from other clusters #13452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 39 additions & 20 deletions server/src/main/java/com/cloud/server/ManagementServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,19 +1490,7 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
ex.addProxyObject(vm.getUuid(), "vmId");
throw ex;
}

String srcHostVersion = srcHost.getHypervisorVersion();
if (HypervisorType.KVM.equals(srcHost.getHypervisorType()) && srcHostVersion == null) {
srcHostVersion = "";
}

// Check if the vm can be migrated with storage.
boolean canMigrateWithStorage = false;

List<HypervisorType> 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);
Expand All @@ -1517,6 +1505,7 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
}
}

boolean canMigrateWithStorage = isStorageMigrationSupported(vm);
if (!canMigrateWithStorage && usesLocal) {
throw new InvalidParameterValueException("Unsupported operation, instance uses Local storage, cannot migrate");
}
Expand Down Expand Up @@ -1596,6 +1585,28 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
return new Ternary<>(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<HypervisorType> 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.
Expand Down Expand Up @@ -1642,16 +1653,14 @@ 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<Host> getCapableSuitableHosts(
final VirtualMachine vm,
final VirtualMachineProfile vmProfile,
final DataCenterDeployment plan,
final List<? extends Host> compatibleHosts,
final ExcludeList excludes,
final Host srcHost) {
final ExcludeList excludes) {

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

Expand All @@ -1677,6 +1686,7 @@ protected List<Host> 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<CPU.CPUArch> clusterArchs = ApiDBUtils.listZoneClustersArchs(vm.getDataCenterId());
if (CollectionUtils.isNotEmpty(clusterArchs) && clusterArchs.size() > 1) {
suitableHosts = suitableHosts.stream().filter(h -> h.getArch() == srcHost.getArch()).collect(Collectors.toList());
Expand Down Expand Up @@ -1707,22 +1717,31 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
}

// 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 DataCenterDeployment plan = createDeploymentPlanForMigrationListing(vm);
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<Host> suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes, srcHost);
List<Host> suitableHosts = getCapableSuitableHosts(vm, vmProfile, plan, filteredHosts, excludes);

final Pair<List<? extends Host>, 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
*/
Expand Down
57 changes: 44 additions & 13 deletions server/src/test/java/com/cloud/server/ManagementServerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
}
}
Loading