Skip to content

Commit 8a48f3d

Browse files
winterhazelwinterhazel
committed
Address reviews
1 parent f6c603b commit 8a48f3d

2 files changed

Lines changed: 28 additions & 37 deletions

File tree

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

Lines changed: 18 additions & 27 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,19 +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-
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
1494-
14951483
// Check if the vm is using any disks on local storage.
14961484
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
14971485
final List<VolumeVO> volumes = _volumeDao.findCreatedByInstance(vmProfile.getId());
@@ -1505,11 +1493,12 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15051493
}
15061494
}
15071495

1508-
boolean canMigrateWithStorage = isStorageMigrationSupported(vm);
1496+
boolean canMigrateWithStorage = isStorageMigrationSupported(vm, srcHost);
15091497
if (!canMigrateWithStorage && usesLocal) {
15101498
throw new InvalidParameterValueException("Unsupported operation, instance uses Local storage, cannot migrate");
15111499
}
15121500

1501+
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
15131502
final Type hostType = srcHost.getType();
15141503
Pair<List<HostVO>, Integer> allHostsPair = null;
15151504
List<HostVO> allHosts = null;
@@ -1585,12 +1574,7 @@ Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boo
15851574
return new Ternary<>(allHostsPairResult, filteredHosts, requiresStorageMotion);
15861575
}
15871576

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-
1577+
protected boolean isStorageMigrationSupported(final VirtualMachine vm, final Host srcHost) {
15941578
final List<HypervisorType> hypervisorTypes = Arrays.asList(HypervisorType.VMware, HypervisorType.KVM);
15951579
if (VirtualMachine.Type.User.equals(vm.getType()) || hypervisorTypes.contains(vm.getHypervisorType())) {
15961580
final String srcHostVersion = getHypervisorVersionOfHost(srcHost);
@@ -1702,9 +1686,19 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17021686

17031687
validateVmForHostMigration(vm);
17041688

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

17091703
Pair<List<? extends Host>, Integer> allHostsPair = compatibilityResult.first();
17101704
List<? extends Host> filteredHosts = compatibilityResult.second();
@@ -1717,7 +1711,7 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17171711
}
17181712

17191713
// Create deployment plan and VM profile
1720-
final DataCenterDeployment plan = createDeploymentPlanForMigrationListing(vm);
1714+
final DataCenterDeployment plan = createDeploymentPlanForMigrationListing(vm, srcHost);
17211715
final VirtualMachineProfile vmProfile = new VirtualMachineProfileImpl(
17221716
vm, null, _offeringDao.findById(vm.getId(), vm.getServiceOfferingId()), null, null);
17231717

@@ -1731,14 +1725,11 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
17311725
return new Ternary<>(otherHosts, suitableHosts, requiresStorageMotion);
17321726
}
17331727

1734-
protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm) {
1735-
final Host srcHost = _hostDao.findById(vm.getHostId());
1736-
1737-
final boolean canMigrateWithStorage = isStorageMigrationSupported(vm);
1728+
protected DataCenterDeployment createDeploymentPlanForMigrationListing(final VirtualMachine vm, final Host srcHost) {
1729+
final boolean canMigrateWithStorage = isStorageMigrationSupported(vm, srcHost);
17381730
if (canMigrateWithStorage) {
17391731
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), null, null, null, null);
17401732
}
1741-
17421733
return new DataCenterDeployment(srcHost.getDataCenterId(), srcHost.getPodId(), srcHost.getClusterId(), null, null, null);
17431734
}
17441735

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -827,9 +827,13 @@ public void testListHostsForMigrationOfVMLxcUserVM() {
827827
@Test
828828
public void testListHostsForMigrationOfVMGpuEnabled() {
829829
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM);
830+
long hostId = vm.getHostId();
831+
HostVO srcHost = mockHost(hostId, 4L, 5L, 6L, HypervisorType.KVM);
830832
Account caller = mockRootAdminAccount();
833+
831834
Mockito.doReturn(caller).when(spy).getCaller();
832835
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
836+
Mockito.doReturn(srcHost).when(hostDao).findById(hostId);
833837

834838
// Mock GPU detail
835839
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
@@ -1509,8 +1513,6 @@ public void testListHostsForMigrationOfVMSourceHostNotFound() {
15091513
Account caller = mockRootAdminAccount();
15101514
Mockito.doReturn(caller).when(spy).getCaller();
15111515
Mockito.when(vmInstanceDao.findById(1L)).thenReturn(vm);
1512-
Mockito.when(serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(), GPU.Keys.pciDevice.toString()))
1513-
.thenReturn(null);
15141516
Mockito.when(hostDao.findById(vm.getHostId())).thenReturn(null);
15151517

15161518
spy.listHostsForMigrationOfVM(1L, 0L, 20L, null);
@@ -2079,12 +2081,11 @@ private DiskOfferingVO mockSharedDiskOffering(Long id) {
20792081
@Test
20802082
public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhenStorageMigrationIsSupported() {
20812083
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.KVM);
2082-
Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm);
2084+
HostVO srcHost = mockHost(vm.getHostId(), 1L, 2L, 3L, HypervisorType.KVM);
20832085

2084-
HostVO srcHost = mockHost(100L, 1L, 2L, 3L, HypervisorType.KVM);
2085-
Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong());
2086+
Mockito.doReturn(true).when(spy).isStorageMigrationSupported(vm, srcHost);
20862087

2087-
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm);
2088+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost);
20882089

20892090
Assert.assertEquals(3L, deploymentPlan.getDataCenterId());
20902091
Assert.assertEquals(2L, (long) deploymentPlan.getPodId());
@@ -2094,12 +2095,11 @@ public void createDeploymentPlanForMigrationListingTestAllocatesInAnyClusterWhen
20942095
@Test
20952096
public void createDeploymentPlanForMigrationListingTestAllocatesInSourceClusterWhenStorageMigrationIsNotSupported() {
20962097
VMInstanceVO vm = mockRunningVM(1L, HypervisorType.XenServer);
2097-
Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm);
2098+
HostVO srcHost = mockHost(vm.getHostId(), 4L, 5L, 6L, HypervisorType.XenServer);
20982099

2099-
HostVO srcHost = mockHost(200L, 4L, 5L, 6L, HypervisorType.XenServer);
2100-
Mockito.doReturn(srcHost).when(hostDao).findById(Mockito.anyLong());
2100+
Mockito.doReturn(false).when(spy).isStorageMigrationSupported(vm, srcHost);
21012101

2102-
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm);
2102+
DataCenterDeployment deploymentPlan = spy.createDeploymentPlanForMigrationListing(vm, srcHost);
21032103

21042104
Assert.assertEquals(6L, deploymentPlan.getDataCenterId());
21052105
Assert.assertEquals(5L, (long) deploymentPlan.getPodId());

0 commit comments

Comments
 (0)