DaanHoogland commented on a change in pull request #4895:
URL: https://github.com/apache/cloudstack/pull/4895#discussion_r607726405
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -4413,9 +4418,17 @@ protected Answer execute(MigrateVmToPoolCommand cmd) {
}
}
- private Answer migrateAndAnswer(VirtualMachineMO vmMo, String poolUuid,
VmwareHypervisorHost hyperHost, Command cmd) throws Exception {
- ManagedObjectReference morDs = getTargetDatastoreMOReference(poolUuid,
hyperHost);
-
+ private Answer migrateAndAnswer(VirtualMachineMO vmMo, String poolUuid,
VmwareHypervisorHost sourceHyperHost,
Review comment:
please restructure this method (at least extract the new pieces)
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -4627,7 +4640,7 @@ protected Answer execute(MigrateWithStorageCommand cmd) {
morDc = srcHyperHost.getHyperHostDatacenter();
morDcOfTargetHost = tgtHyperHost.getHyperHostDatacenter();
if
(!morDc.getValue().equalsIgnoreCase(morDcOfTargetHost.getValue())) {
- String msg = "Source host & target host are in different
datacentesr";
+ String msg = "Source host & target host are in different
datacenter";
Review comment:
:+1:
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
##########
@@ -209,16 +209,35 @@ protected VMwareGuru() {
return vmwareVmImplementer.implement(vm, toVirtualMachineTO(vm),
getClusterId(vm.getId()));
}
- long getClusterId(long vmId) {
- long clusterId;
- Long hostId;
-
- hostId = _vmDao.findById(vmId).getHostId();
- if (hostId == null) {
+ Long getClusterId(long vmId) {
+ Long clusterId = null;
+ Long hostId = null;
+ VMInstanceVO vm = _vmDao.findById(vmId);
+ if (vm != null) {
+ hostId = _vmDao.findById(vmId).getHostId();
+ }
+ if (vm != null && hostId == null) {
// If VM is in stopped state then hostId would be undefined. Hence
read last host's Id instead.
hostId = _vmDao.findById(vmId).getLastHostId();
}
- clusterId = _hostDao.findById(hostId).getClusterId();
+ HostVO host = null;
+ if (hostId != null) {
+ host = _hostDao.findById(hostId);
+ }
+ if (host != null) {
+ clusterId = host.getClusterId();
+ } else {
+ List<VolumeVO> volumes = _volumeDao.findByInstanceAndType(vmId,
Volume.Type.ROOT);
+ if (CollectionUtils.isNotEmpty(volumes)) {
+ VolumeVO rootVolume = volumes.get(0);
+ if (rootVolume.getPoolId() != null) {
+ StoragePoolVO pool =
_storagePoolDao.findById(rootVolume.getPoolId());
+ if (pool != null && pool.getClusterId() != null) {
+ clusterId = pool.getClusterId();
+ }
+ }
+ }
Review comment:
please extract into a separate method. i.e. `Long
getPoolFromRootVolume(...)`
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -4839,6 +4852,11 @@ private Answer migrateVolume(MigrateVolumeCommand cmd) {
String path = cmd.getVolumePath();
VmwareHypervisorHost hyperHost = getHyperHost(getServiceContext());
+ VmwareHypervisorHost hyperHostInTargetCluster = null;
+ if (cmd.getHostGuidInTargetCluster() != null) {
+ hyperHostInTargetCluster =
VmwareHelper.getHostMOFromHostName(getServiceContext(),
cmd.getHostGuidInTargetCluster());
+ }
+ VmwareHypervisorHost targetDSHost = hyperHostInTargetCluster != null ?
hyperHostInTargetCluster : hyperHost;
Review comment:
this merrits a call to a specific method
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -4854,23 +4872,32 @@ private Answer migrateVolume(MigrateVolumeCommand cmd) {
// we need to spawn a worker VM to attach the volume to and move it
morSourceDS =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
cmd.getSourcePool().getUuid());
dsMo = new DatastoreMO(hyperHost.getContext(), morSourceDS);
- morDestintionDS =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost,
cmd.getTargetPool().getUuid());
- destinationDsMo = new DatastoreMO(hyperHost.getContext(),
morDestintionDS);
+ morDestintionDS =
HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetDSHost,
cmd.getTargetPool().getUuid());
+ destinationDsMo = new DatastoreMO(targetDSHost.getContext(),
morDestintionDS);
vmName = getWorkerName(getServiceContext(), cmd, 0, dsMo);
if (destinationDsMo.getDatastoreType().equalsIgnoreCase("VVOL")) {
isvVolsInvolved = true;
vmName = getWorkerName(getServiceContext(), cmd, 0,
destinationDsMo);
}
+ String hardwareVersion = null;
+ if (hyperHostInTargetCluster != null) {
+ Integer sourceHardwareVersion =
HypervisorHostHelper.getHostHardwareVersion(hyperHost);
+ Integer destinationHardwareVersion =
HypervisorHostHelper.getHostHardwareVersion(hyperHostInTargetCluster);
+ if (sourceHardwareVersion != null &&
destinationHardwareVersion != null &&
!sourceHardwareVersion.equals(destinationHardwareVersion)) {
+ hardwareVersion =
String.valueOf(Math.min(sourceHardwareVersion, destinationHardwareVersion));
+ }
+ }
Review comment:
please extract
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
##########
@@ -1066,14 +1087,29 @@ private VirtualDisk getAttachedDisk(VirtualMachineMO
vmMo, String diskPath) thro
VolumeTO vol = new VolumeTO(volume, destination);
vols.add(vol);
}
- MigrateVmToPoolCommand migrateVmToPoolCommand = new
MigrateVmToPoolCommand(vm.getInstanceName(), vols, destination.getUuid(), true);
- commands.add(migrateVmToPoolCommand);
- // OfflineVmwareMigration: cleanup if needed
final Long destClusterId = destination.getClusterId();
final Long srcClusterId = getClusterId(vm.getId());
+ final boolean isInterClusterMigration = srcClusterId != null &&
destClusterId != null && ! srcClusterId.equals(destClusterId);
+ Host hostInTargetCluster = null;
+ if (isInterClusterMigration) {
+ // Without host vMotion might fail between non-shared storages
with error similar to,
+ // https://kb.vmware.com/s/article/1003795
+ // As this is offline migration VM won't be started on this host
+ List<HostVO> hosts =
_hostDao.findHypervisorHostInCluster(destClusterId);
+ if (CollectionUtils.isNotEmpty(hosts)) {
+ hostInTargetCluster = hosts.get(0);
+ }
+ if (hostInTargetCluster == null) {
+ throw new CloudRuntimeException("Migration failed, unable to
find suitable target host for VM placement while migrating between storage
pools of different clusters without shared storages");
+ }
+ }
+ MigrateVmToPoolCommand migrateVmToPoolCommand = new
MigrateVmToPoolCommand(vm.getInstanceName(),
+ vols, destination.getUuid(), hostInTargetCluster == null ?
null : hostInTargetCluster.getGuid(), true);
+ commands.add(migrateVmToPoolCommand);
Review comment:
please restructure this in smaller methods
##########
File path:
plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java
##########
@@ -188,20 +177,42 @@ public void copyAsync(DataObject srcData, DataObject
destData, Host destHost, As
throw new UnsupportedOperationException();
}
StoragePool sourcePool = (StoragePool) srcData.getDataStore();
+ ScopeType sourceScopeType =
srcData.getDataStore().getScope().getScopeType();
StoragePool targetPool = (StoragePool) destData.getDataStore();
+ ScopeType targetScopeType =
destData.getDataStore().getScope().getScopeType();
+ Long hostId = null;
+ String hostGuidInTargetCluster = null;
+ if (ScopeType.CLUSTER.equals(sourceScopeType)) {
+ // Find Volume source cluster and select any Vmware hypervisor
host to attach worker VM
+ hostId =
findSuitableHostIdForWorkerVmPlacement(sourcePool.getClusterId());
+ if (hostId == null) {
+ throw new CloudRuntimeException("Offline Migration failed,
unable to find suitable host for worker VM placement in the cluster of storage
pool: " + sourcePool.getName());
+ }
+ if (ScopeType.CLUSTER.equals(targetScopeType) &&
!sourcePool.getClusterId().equals(targetPool.getClusterId())) {
+ // Without host vMotion might fail between non-shared storages
with error similar to,
+ // https://kb.vmware.com/s/article/1003795
+ List<HostVO> hosts =
hostDao.findHypervisorHostInCluster(targetPool.getClusterId());
+ if (CollectionUtils.isNotEmpty(hosts)) {
+ hostGuidInTargetCluster = hosts.get(0).getGuid();
+ }
+ if (hostGuidInTargetCluster == null) {
+ throw new CloudRuntimeException("Offline Migration failed,
unable to find suitable target host for worker VM placement while migrating
between storage pools of different cluster without shared storages");
+ }
+ }
+ } else if (ScopeType.CLUSTER.equals(targetScopeType)) {
+ hostId =
findSuitableHostIdForWorkerVmPlacement(targetPool.getClusterId());
+ if (hostId == null) {
+ throw new CloudRuntimeException("Offline Migration failed,
unable to find suitable host for worker VM placement in the cluster of storage
pool: " + targetPool.getName());
+ }
+ }
Review comment:
please extract `getTargetScopeType(..)` and possibly more methods from
that.
--
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]
