DaanHoogland commented on a change in pull request #4534:
URL: https://github.com/apache/cloudstack/pull/4534#discussion_r541026313
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5812,7 +5814,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId,
Host destinationHost) thr
// If no suitable destination found then throw exception
if (dest == null) {
- throw new RuntimeException("Unable to find suitable destination to
migrate VM " + vm.getInstanceName());
+ throw new CloudRuntimeException("Unable to find suitable
destination to migrate VM " + vm.getInstanceName());
Review comment:
👍
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5787,6 +5789,50 @@ public VirtualMachine migrateVirtualMachine(Long vmId,
Host destinationHost) thr
// check if migrating to same host
long srcHostId = vm.getHostId();
+
+ DeployDestination dest = null;
+ if (destinationHost == null) {
+ vm.setLastHostId(null); // Do not check last host
+ final VirtualMachineProfile profile = new
VirtualMachineProfileImpl(vm);
+ final Host host = _hostDao.findById(srcHostId);
+ final DataCenterDeployment plan = new
DataCenterDeployment(host.getDataCenterId(), null, null, null, null, null);
+ ExcludeList excludes = new ExcludeList();
+ excludes.addHost(srcHostId);
+ try {
+ dest = _planningMgr.planDeployment(profile, plan, excludes,
null);
+ } catch (final AffinityConflictException e2) {
+ s_logger.warn("Unable to create deployment, affinity rules
associted to the VM conflict", e2);
+ throw new CloudRuntimeException("Unable to create deployment,
affinity rules associted to the VM conflict");
+ } catch (final InsufficientServerCapacityException e3) {
+ throw new CloudRuntimeException("Unable to find a server to
migrate the vm to");
+ }
+ } else {
+ dest = checkVmMigrationDestination(vm, srcHostId, destinationHost);
+ }
+
+ // If no suitable destination found then throw exception
+ if (dest == null) {
+ throw new RuntimeException("Unable to find suitable destination to
migrate VM " + vm.getInstanceName());
+ }
+
+ UserVmVO uservm = _vmDao.findById(vmId);
+ if (uservm != null) {
+ collectVmDiskStatistics(uservm);
+ collectVmNetworkStatistics(uservm);
+ }
+ _itMgr.migrate(vm.getUuid(), srcHostId, dest);
+ VMInstanceVO vmInstance = _vmInstanceDao.findById(vmId);
+ if (vmInstance.getType().equals(VirtualMachine.Type.User)) {
+ return _vmDao.findById(vmId);
+ } else {
+ return vmInstance;
+ }
Review comment:
I know i sound like a broken record , but I see four methods here. Can
you extract these pieces of code please?
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -1262,7 +1263,29 @@ private boolean doMaintain(final long hostId) {
return true;
}
- final List<HostVO> hosts =
listAllUpAndEnabledHosts(Host.Type.Routing, host.getClusterId(),
host.getPodId(), host.getDataCenterId());
+ List<HostVO> hosts = listAllUpAndEnabledHosts(Host.Type.Routing,
host.getClusterId(), host.getPodId(), host.getDataCenterId());
+ if (hosts == null || hosts.isEmpty()) {
+ s_logger.warn("Unable to find a host for vm migration in
cluster: " + host.getClusterId());
+ if (MIGRATE_VM_ACROSS_CLUSTERS.value()) {
+ s_logger.info("Looking for hosts across different clusters
in zone: " + host.getDataCenterId());
+ hosts = listAllUpAndEnabledHosts(Host.Type.Routing, null,
null, host.getDataCenterId());
+ if (hosts == null || hosts.isEmpty()) {
+ s_logger.warn("Unable to find a host for vm migration
in zone: " + host.getDataCenterId());
+ return false;
+ }
+ // Dont migrate vm if it has volumes on cluster-wide pool
+ for (final VMInstanceVO vm : vms) {
+ if (_vmMgr.checkIfVmHasClusterWideVolumes(vm.getId()))
{
+ s_logger.warn("Unable to migrate vm " +
vm.getInstanceName() + " as it has volumes on cluster-wide pool");
+ return false;
+ }
+ }
+ } else {
+ s_logger.warn("Not migrating VM across cluster since " +
MIGRATE_VM_ACROSS_CLUSTERS.key() + " is false");
+ return false;
+ }
+ }
Review comment:
sugestion:
`if (! clusterWideMigrationSupported(...)) return false;`
and extract this code to
`boolean clusterWideMigrationSupported(...)`
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3268,6 +3226,22 @@ public boolean checkIfVmHasClusterWideVolumes(Long vmId)
{
}
+ private DataCenterDeployment getMigrationDeployment(final Long vmId, final
Host host, final Long poolId, final ExcludeList excludes) {
+ if (MIGRATE_VM_ACROSS_CLUSTERS.value() &&
!checkIfVmHasClusterWideVolumes(vmId)) {
+ s_logger.info("Searching for hosts in the zone for vm migration");
+ List<Long> clustersToExcluded =
_clusterDao.listAllClusters(host.getDataCenterId());
Review comment:
sugest you name this `clustersToExclude` without the d on the end
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
