Skip to content

vmware: don't use redundant worker VM to extract volume #3218

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
DaanHoogland merged 1 commit into from
May 23, 2019
Merged

vmware: don't use redundant worker VM to extract volume #3218

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
41 changes: 24 additions & 17 deletions ...rs/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,12 +930,12 @@ private String backupSnapshotToSecondaryStorage(VirtualMachineMO vmMo, long acco
String prevSnapshotUuid, String prevBackupUuid, String workerVmName, Integer nfsVersion) throws Exception {

String backupUuid = UUID.randomUUID().toString();
exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, getSnapshotRelativeDirInSecStorage(accountId, volumeId), backupUuid, workerVmName, nfsVersion);
exportVolumeToSecondaryStorage(vmMo, volumePath, secStorageUrl, getSnapshotRelativeDirInSecStorage(accountId, volumeId), backupUuid, workerVmName, nfsVersion, true);
return backupUuid + "/" + backupUuid;
}

private void exportVolumeToSecondaryStroage(VirtualMachineMO vmMo, String volumePath, String secStorageUrl, String secStorageDir, String exportName, String workerVmName,
Integer nfsVersion) throws Exception {
private void exportVolumeToSecondaryStorage(VirtualMachineMO vmMo, String volumePath, String secStorageUrl, String secStorageDir, String exportName, String workerVmName,
Integer nfsVersion, boolean clonedWorkerVMNeeded) throws Exception {

String secondaryMountPoint = _mountService.getMountPoint(secStorageUrl, nfsVersion);
String exportPath = secondaryMountPoint + "/" + secStorageDir + "/" + exportName;
Expand All @@ -961,16 +961,19 @@ private void exportVolumeToSecondaryStroage(VirtualMachineMO vmMo, String volume
throw new Exception(msg);
}

// 4 MB is the minimum requirement for VM memory in VMware
vmMo.cloneFromCurrentSnapshot(workerVmName, 0, 4, volumeDeviceInfo.second(), VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()));
clonedVm = vmMo.getRunningHost().findVmOnHyperHost(workerVmName);
if (clonedVm == null) {
String msg = "Unable to create dummy VM to export volume. volume path: " + volumePath;
s_logger.error(msg);
throw new Exception(msg);
if (clonedWorkerVMNeeded) {
// 4 MB is the minimum requirement for VM memory in VMware
vmMo.cloneFromCurrentSnapshot(workerVmName, 0, 4, volumeDeviceInfo.second(), VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()));
clonedVm = vmMo.getRunningHost().findVmOnHyperHost(workerVmName);
if (clonedVm == null) {
String msg = "Unable to create dummy VM to export volume. volume path: " + volumePath;
s_logger.error(msg);
throw new Exception(msg);
}
clonedVm.exportVm(exportPath, exportName, false, false); //Note: volss: not to create ova.
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 26, 2019

Choose a reason for hiding this comment

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

how about unifying both calls by assigning the clonedVm or vmMo to a local var, so it is obvious where the real work is done in both cases?

Copy link
Member Author

@yadvr yadvr May 23, 2019

Choose a reason for hiding this comment

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

It's because, if we need to clone i.e. require a worker VM clonedVm, then in finally we need to detach and destroy that worker VM. This is why we cannot refactor them to use a single variable.

Copy link
Contributor

@DaanHoogland DaanHoogland May 23, 2019

Choose a reason for hiding this comment

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

ok, as said it is a matter of style but we have that boolean to decide: clonedWorkerVMNeeded

} else {
vmMo.exportVm(exportPath, exportName, false, false);
}

clonedVm.exportVm(exportPath, exportName, false, false); //Note: volss: not to create ova.
} finally {
if (clonedVm != null) {
clonedVm.detachAllDisks();
Expand Down Expand Up @@ -998,6 +1001,7 @@ private Pair<String, String> copyVolumeToSecStorage(VmwareHostService hostServic
throw new Exception(msg);
}

boolean clonedWorkerVMNeeded = true;
vmMo = hyperHost.findVmOnHyperHost(vmName);
if (vmMo == null) {
// create a dummy worker vm for attaching the volume
Expand All @@ -1014,16 +1018,19 @@ private Pair<String, String> copyVolumeToSecStorage(VmwareHostService hostServic
String datastoreVolumePath = getVolumePathInDatastore(dsMo, volumePath + ".vmdk", searchExcludedFolders);
workerVm.attachDisk(new String[] {datastoreVolumePath}, morDs);
vmMo = workerVm;
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 26, 2019

Choose a reason for hiding this comment

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

if we move clonedWorkerVMNeeded outside the try workerVm is not needed.

Copy link
Member Author

@yadvr yadvr May 23, 2019

Choose a reason for hiding this comment

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

Same reason as before, using different instances/variable, it's easier to track is we need to destroy the worker vm after we're done. If we re-use vmMo, we might accidently destroy the user-vm.

clonedWorkerVMNeeded = false;
} else {
vmMo.createSnapshot(exportName, "Temporary snapshot for copy-volume command", false, false);
}

vmMo.createSnapshot(exportName, "Temporary snapshot for copy-volume command", false, false);

exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, "volumes/" + volumeFolder, exportName, hostService.getWorkerName(hyperHost.getContext(), cmd, 1),
nfsVersion);
exportVolumeToSecondaryStorage(vmMo, volumePath, secStorageUrl, "volumes/" + volumeFolder, exportName, hostService.getWorkerName(hyperHost.getContext(), cmd, 1),
nfsVersion, clonedWorkerVMNeeded);
return new Pair<String, String>(volumeFolder, exportName);

} finally {
vmMo.removeSnapshot(exportName, false);
if (vmMo != null && vmMo.getSnapshotMor(exportName) != null) {
vmMo.removeSnapshot(exportName, false);
}
if (workerVm != null) {
//detach volume and destroy worker vm
workerVm.detachAllDisks();
Expand Down
38 changes: 23 additions & 15 deletions ...s/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@ private Pair<String, String> copyVolumeToSecStorage(VmwareHostService hostServic
throw new Exception(msg);
}

boolean clonedWorkerVMNeeded = true;
vmMo = hyperHost.findVmOnHyperHost(vmName);
if (vmMo == null || VmwareResource.getVmState(vmMo) == PowerState.PowerOff) {
// create a dummy worker vm for attaching the volume
Expand All @@ -999,15 +1000,18 @@ private Pair<String, String> copyVolumeToSecStorage(VmwareHostService hostServic
String datastoreVolumePath = getVolumePathInDatastore(dsMo, volumePath + ".vmdk", searchExcludedFolders);
workerVm.attachDisk(new String[] {datastoreVolumePath}, morDs);
vmMo = workerVm;
clonedWorkerVMNeeded = false;
} else {
vmMo.createSnapshot(exportName, "Temporary snapshot for copy-volume command", false, false);
}

vmMo.createSnapshot(exportName, "Temporary snapshot for copy-volume command", false, false);

exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, destVolumePath, exportName, hostService.getWorkerName(hyperHost.getContext(), cmd, 1), _nfsVersion);
exportVolumeToSecondaryStorage(vmMo, volumePath, secStorageUrl, destVolumePath, exportName, hostService.getWorkerName(hyperHost.getContext(), cmd, 1), _nfsVersion, clonedWorkerVMNeeded);
return new Pair<>(destVolumePath, exportName);

} finally {
vmMo.removeSnapshot(exportName, false);
if (vmMo != null && vmMo.getSnapshotMor(exportName) != null) {
vmMo.removeSnapshot(exportName, false);
}
if (workerVm != null) {
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 26, 2019

Choose a reason for hiding this comment

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

same here as in VmwareStorageManagerImpl.java#1020; if we move clonedWorkerVMNeeded outside the try block we don't need this extra workerVm

//detach volume and destroy worker vm
workerVm.detachAllDisks();
Expand Down Expand Up @@ -1657,8 +1661,8 @@ public Answer createTemplateFromSnapshot(CopyCommand cmd) {
}

// return Pair<String(divice bus name), String[](disk chain)>
private Pair<String, String[]> exportVolumeToSecondaryStroage(VirtualMachineMO vmMo, String volumePath, String secStorageUrl, String secStorageDir,
String exportName, String workerVmName, Integer nfsVersion) throws Exception {
private Pair<String, String[]> exportVolumeToSecondaryStorage(VirtualMachineMO vmMo, String volumePath, String secStorageUrl, String secStorageDir,
String exportName, String workerVmName, Integer nfsVersion, boolean clonedWorkerVMNeeded) throws Exception {

String secondaryMountPoint = mountService.getMountPoint(secStorageUrl, nfsVersion);
String exportPath = secondaryMountPoint + "/" + secStorageDir + "/" + exportName;
Expand All @@ -1684,14 +1688,18 @@ private Pair<String, String[]> exportVolumeToSecondaryStroage(VirtualMachineMO v
throw new Exception(msg);
}

// 4 MB is the minimum requirement for VM memory in VMware
Pair<VirtualMachineMO, String[]> cloneResult =
vmMo.cloneFromCurrentSnapshot(workerVmName, 0, 4, volumeDeviceInfo.second(), VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()));
clonedVm = cloneResult.first();
String disks[] = cloneResult.second();

clonedVm.exportVm(exportPath, exportName, false, false);
return new Pair<>(volumeDeviceInfo.second(), disks);
String diskDevice = volumeDeviceInfo.second();
String disks[] = vmMo.getCurrentSnapshotDiskChainDatastorePaths(diskDevice);
if (clonedWorkerVMNeeded) {
// 4 MB is the minimum requirement for VM memory in VMware
Pair<VirtualMachineMO, String[]> cloneResult =
vmMo.cloneFromCurrentSnapshot(workerVmName, 0, 4, diskDevice, VmwareHelper.getDiskDeviceDatastore(volumeDeviceInfo.first()));
clonedVm = cloneResult.first();
clonedVm.exportVm(exportPath, exportName, false, false);
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 26, 2019

Choose a reason for hiding this comment

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

also here, it would be nice to see just one call for doing the 'real' work.

Copy link
Member Author

@yadvr yadvr May 23, 2019

Choose a reason for hiding this comment

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

In the current mechanism, we need a worker VM to take snapshot and export the disk; without at least one worker VM I'm not sure how we can export template/disk?

} else {
vmMo.exportVm(exportPath, exportName, false, false);
}
return new Pair<>(diskDevice, disks);
} finally {
if (clonedVm != null) {
clonedVm.detachAllDisks();
Expand All @@ -1706,7 +1714,7 @@ private Ternary<String, String, String[]> backupSnapshotToSecondaryStorage(Virtu
Integer nfsVersion) throws Exception {

String backupUuid = UUID.randomUUID().toString();
Pair<String, String[]> snapshotInfo = exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, installPath, backupUuid, workerVmName, nfsVersion);
Pair<String, String[]> snapshotInfo = exportVolumeToSecondaryStorage(vmMo, volumePath, secStorageUrl, installPath, backupUuid, workerVmName, nfsVersion, true);
return new Ternary<>(backupUuid, snapshotInfo.first(), snapshotInfo.second());
}

Expand Down