Skip to content

vmware: fix migrate vm with volume #5170

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

Merged
rohityadavcloud merged 3 commits into from
Jul 15, 2021
Merged

vmware: fix migrate vm with volume #5170

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e906f0
vmware: fix migrate vm with volume
shwstppr Jun 30, 2021
ad8e93b
Update plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/…
shwstppr Jul 5, 2021
6d663a6
fix
shwstppr Jul 5, 2021
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
31 changes: 15 additions & 16 deletions ...hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7337,20 +7337,20 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
if (targetHyperHost != null) {
ManagedObjectReference morTargetHostDc = targetHyperHost.getHyperHostDatacenter();
if (!morSourceHostDc.getValue().equalsIgnoreCase(morTargetHostDc.getValue())) {
String msg = "VM " + vmName + " cannot be migrated between different datacenter";
String msg = String.format("VM: %s cannot be migrated between different datacenter", vmName);
throw new CloudRuntimeException(msg);
}
}

// find VM through source host (VM is not at the target host yet)
vmMo = sourceHyperHost.findVmOnHyperHost(vmName);
if (vmMo == null) {
String msg = "VM " + vmName + " does not exist on host: " + sourceHyperHost.getHyperHostName();
String msg = String.format("VM: %s does not exist on host: %s", vmName, sourceHyperHost.getHyperHostName());
s_logger.warn(msg);
// find VM through source host (VM is not at the target host yet)
vmMo = dcMo.findVm(vmName);
if (vmMo == null) {
msg = "VM " + vmName + " does not exist on datacenter: " + dcMo.getName();
msg = String.format("VM: %s does not exist on datacenter: %s", vmName, dcMo.getName());
s_logger.error(msg);
throw new Exception(msg);
}
Expand All @@ -7364,11 +7364,9 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
if (StringUtils.isNotBlank(poolUuid)) {
VmwareHypervisorHost dsHost = targetHyperHost == null ? sourceHyperHost : targetHyperHost;
ManagedObjectReference morDatastore = null;
String msg;
morDatastore = getTargetDatastoreMOReference(poolUuid, dsHost);
if (morDatastore == null) {
msg = "Unable to find the target datastore: " + poolUuid + " on host: " + dsHost.getHyperHostName() +
" to execute migration";
String msg = String.format("Unable to find the target datastore: %s on host: %s to execute migration", poolUuid, dsHost.getHyperHostName());
s_logger.error(msg);
throw new CloudRuntimeException(msg);
}
Expand All @@ -7384,7 +7382,7 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
}
ManagedObjectReference morVolumeDatastore = getTargetDatastoreMOReference(filerTo.getUuid(), dsHost);
if (morVolumeDatastore == null) {
String msg = "Unable to find the target datastore: " + filerTo.getUuid() + " in datacenter: " + dcMo.getName() + " to execute migration";
String msg = String.format("Unable to find the target datastore: %s in datacenter: %s to execute migration", filerTo.getUuid(), dcMo.getName());
s_logger.error(msg);
throw new CloudRuntimeException(msg);
}
Expand Down Expand Up @@ -7445,8 +7443,7 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
mgr.prepareSecondaryStorageStore(secStoreUrl, secStoreId);
ManagedObjectReference morSecDs = prepareSecondaryDatastoreOnSpecificHost(secStoreUrl, targetHyperHost);
if (morSecDs == null) {
String msg = "Failed to prepare secondary storage on host, secondary store url: " + secStoreUrl;
throw new Exception(msg);
throw new Exception(String.format("Failed to prepare secondary storage on host, secondary store url: %s", secStoreUrl));
}
}

Expand All @@ -7455,15 +7452,15 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
if (!vmMo.changeDatastore(relocateSpec)) {
throw new Exception("Change datastore operation failed during storage migration");
} else {
s_logger.debug("Successfully migrated storage of VM " + vmName + " to target datastore(s)");
s_logger.debug(String.format("Successfully migrated storage of VM: %s to target datastore(s)", vmName));
}
// Migrate VM to target host.
if (targetHyperHost != null) {
ManagedObjectReference morPool = targetHyperHost.getHyperHostOwnerResourcePool();
if (!vmMo.migrate(morPool, targetHyperHost.getMor())) {
throw new Exception("VM migration to target host failed during storage migration");
} else {
s_logger.debug("Successfully migrated VM " + vmName + " from " + sourceHyperHost.getHyperHostName() + " to " + targetHyperHost.getHyperHostName());
s_logger.debug(String.format("Successfully migrated VM: %s from host %s to %s", vmName , sourceHyperHost.getHyperHostName(), targetHyperHost.getHyperHostName()));
}
}
} else {
Expand All @@ -7475,9 +7472,11 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
if (!vmMo.changeDatastore(relocateSpec)) {
throw new Exception("Change datastore operation failed during storage migration");
} else {
s_logger.debug("Successfully migrated VM " + vmName +
(hostInTargetCluster != null ? " from " + sourceHyperHost.getHyperHostName() + " to " + targetHyperHost.getHyperHostName() + " and " : " with ") +
"its storage to target datastore(s)");
String msg = String.format("Successfully migrated VM: %s with its storage to target datastore(s)", vmName);
if (targetHyperHost != null) {
msg = String.format("% from host %s to %s", msg, sourceHyperHost.getHyperHostName(), targetHyperHost.getHyperHostName());
}
s_logger.debug(msg);
}
}

Expand All @@ -7486,7 +7485,7 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
if (!vmMo.consolidateVmDisks()) {
s_logger.warn("VM disk consolidation failed after storage migration. Yet proceeding with VM migration.");
} else {
s_logger.debug("Successfully consolidated disks of VM " + vmName + ".");
s_logger.debug(String.format("Successfully consolidated disks of VM: %s", vmName));
}

if (MapUtils.isNotEmpty(volumeDeviceKey)) {
Expand All @@ -7501,7 +7500,7 @@ private List<VolumeObjectTO> relocateVirtualMachine(final VmwareHypervisorHost h
VolumeObjectTO newVol = new VolumeObjectTO();
newVol.setDataStoreUuid(entry.second().getUuid());
String newPath = vmMo.getVmdkFileBaseName(disk);
ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost, entry.second().getUuid());
ManagedObjectReference morDs = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(targetHyperHost != null ? targetHyperHost : sourceHyperHost, entry.second().getUuid());
Copy link
Member

@weizhouapache weizhouapache Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr might sourceHyperHost be null as well ?

Copy link
Contributor Author

@shwstppr shwstppr Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weizhouapache Method gets sourceHyperHost either from caller or the host which is processing the command,
https://github.com/apache/cloudstack/pull/5170/files#diff-77635c5f7eb9cef514aaf69e2fdc9217619afc9005bf8db9585c176e7401a88bR7329-R7331
Cannot be null if the command is being processed

Copy link
Member

@weizhouapache weizhouapache Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr great, thanks.

no idea if it breaks some features. at least the NPE is fixed by this pr.

Copy link
Contributor Author

@shwstppr shwstppr Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above ^^ is my understanding. Let me know if there should be additional checks @weizhouapache

Copy link
Contributor Author

@shwstppr shwstppr Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change was originally introduced here, d2ab350

Copy link
Contributor

@DaanHoogland DaanHoogland Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr if the origin is a merge commit than this should be fixed on the older branch and merged forward.

DatastoreMO dsMo = new DatastoreMO(getServiceContext(), morDs);
VirtualMachineDiskInfo diskInfo = diskInfoBuilder.getDiskInfoByBackingFileBaseName(newPath, dsMo.getName());
newVol.setId(volumeId);
Expand Down