Skip to content

CLOUDSTACK-8609: [VMware] VM is not accessible after migration across clusters #2091

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
yadvr merged 5 commits into from Aug 21, 2018
Merged

CLOUDSTACK-8609: [VMware] VM is not accessible after migration across clusters #2091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3c7644
CLOUDSTACK-8609 [VMware] VM is not accessible after migration across …
May 9, 2017
18fdd6c
Merge branch '4.11' into CLOUDSTACK-8609_MigrationAcrossClusters
yadvr May 11, 2018
82cfd39
refactor common changes to a method
yadvr Aug 14, 2018
43697f1
removing trailing whitespace
yadvr Aug 14, 2018
d274689
github editor boo boo - throw exception
yadvr Aug 14, 2018
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
35 changes: 26 additions & 9 deletions plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1665,6 +1665,7 @@ protected StartAnswer execute(StartCommand cmd) {
String existingVmName = null;
VirtualMachineFileInfo existingVmFileInfo = null;
VirtualMachineFileLayoutEx existingVmFileLayout = null;
List<DatastoreMO> existingDatastores = new ArrayList<DatastoreMO>();

Pair<String, String> names = composeVmNames(vmSpec);
String vmInternalCSName = names.first();
Expand Down Expand Up @@ -1787,6 +1788,7 @@ protected StartAnswer execute(StartCommand cmd) {
existingVmName = existingVmInDc.getName();
existingVmFileInfo = existingVmInDc.getFileInfo();
existingVmFileLayout = existingVmInDc.getFileLayout();
existingDatastores = existingVmInDc.getAllDatastores();
existingVmInDc.unregisterVm();
}
Pair<ManagedObjectReference, DatastoreMO> rootDiskDataStoreDetails = null;
Expand Down Expand Up @@ -2253,7 +2255,18 @@ protected StartAnswer execute(StartCommand cmd) {

// Since VM was successfully powered-on, if there was an existing VM in a different cluster that was unregistered, delete all the files associated with it.
if (existingVmName != null && existingVmFileLayout != null) {
Copy link
Member

@rafaelweingartner rafaelweingartner May 11, 2018

Choose a reason for hiding this comment

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

Can you extract this IF body to a method?
This execute method is huge.

Copy link
Contributor Author

@sureshanaparti sureshanaparti May 11, 2018

Choose a reason for hiding this comment

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

@rafaelweingartner Sure. Will update.

Copy link
Member

@rafaelweingartner rafaelweingartner Jun 20, 2018

Choose a reason for hiding this comment

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

Perhaps you forgot to push the commit?

deleteUnregisteredVmFiles(existingVmFileLayout, dcMo, true);
List<String> vmDatastoreNames = new ArrayList<String>();
for (DatastoreMO vmDatastore : vmMo.getAllDatastores()) {
vmDatastoreNames.add(vmDatastore.getName());
}
// Don't delete files that are in a datastore that is being used by the new VM as well (zone-wide datastore).
List<String> skipDatastores = new ArrayList<String>();
for (DatastoreMO existingDatastore : existingDatastores) {
if (vmDatastoreNames.contains(existingDatastore.getName())) {
skipDatastores.add(existingDatastore.getName());
}
}
deleteUnregisteredVmFiles(existingVmFileLayout, dcMo, true, skipDatastores);
}

return startAnswer;
Expand Down Expand Up @@ -2941,7 +2954,14 @@ private void postDiskConfigBeforeStart(VirtualMachineMO vmMo, VirtualMachineTO v
}
}

private void deleteUnregisteredVmFiles(VirtualMachineFileLayoutEx vmFileLayout, DatacenterMO dcMo, boolean deleteDisks) throws Exception {
private void checkAndDeleteDatastoreFile(String filePath, List<String> skipDatastores, DatastoreMO dsMo, DatacenterMO dcMo) throws Exception {
if (dsMo != null && dcMo != null && (skipDatastores == null || !skipDatastores.contains(dsMo.getName()))) {
s_logger.debug("Deleting file: " + filePath);
dsMo.deleteFile(filePath, dcMo.getMor(), true);
}
}

private void deleteUnregisteredVmFiles(VirtualMachineFileLayoutEx vmFileLayout, DatacenterMO dcMo, boolean deleteDisks, List<String> skipDatastores) throws Exception {
s_logger.debug("Deleting files associated with an existing VM that was unregistered");
DatastoreFile vmFolder = null;
try {
Expand All @@ -2954,23 +2974,20 @@ private void deleteUnregisteredVmFiles(VirtualMachineFileLayoutEx vmFileLayout,
else if (file.getType().equals("config"))
vmFolder = new DatastoreFile(fileInDatastore.getDatastoreName(), fileInDatastore.getDir());
DatastoreMO dsMo = new DatastoreMO(dcMo.getContext(), dcMo.findDatastore(fileInDatastore.getDatastoreName()));
s_logger.debug("Deleting file: " + file.getName());
dsMo.deleteFile(file.getName(), dcMo.getMor(), true, VmwareManager.s_vmwareSearchExcludeFolder.value());
checkAndDeleteDatastoreFile(file.getName(), skipDatastores, dsMo, dcMo);
}
// Delete files that are present in the VM folder - this will take care of the VM disks as well.
DatastoreMO vmFolderDsMo = new DatastoreMO(dcMo.getContext(), dcMo.findDatastore(vmFolder.getDatastoreName()));
String[] files = vmFolderDsMo.listDirContent(vmFolder.getPath());
if (deleteDisks) {
for (String file : files) {
String vmDiskFileFullPath = String.format("%s/%s", vmFolder.getPath(), file);
s_logger.debug("Deleting file: " + vmDiskFileFullPath);
vmFolderDsMo.deleteFile(vmDiskFileFullPath, dcMo.getMor(), true, VmwareManager.s_vmwareSearchExcludeFolder.value());
checkAndDeleteDatastoreFile(vmDiskFileFullPath, skipDatastores, vmFolderDsMo, dcMo);
}
}
// Delete VM folder
if (deleteDisks || files.length == 0) {
s_logger.debug("Deleting folder: " + vmFolder.getPath());
vmFolderDsMo.deleteFolder(vmFolder.getPath(), dcMo.getMor());
checkAndDeleteDatastoreFile(vmFolder.getPath(), skipDatastores, vmFolderDsMo, dcMo);
}
} catch (Exception e) {
String message = "Failed to delete files associated with an existing VM that was unregistered due to " + VmwareHelper.getExceptionMessage(e);
Expand Down Expand Up @@ -4905,7 +4922,7 @@ protected Answer execute(UnregisterVMCommand cmd) {
VirtualMachineFileLayoutEx vmFileLayout = vmMo.getFileLayout();
context.getService().unregisterVM(vmMo.getMor());
if (cmd.getCleanupVmFiles()) {
deleteUnregisteredVmFiles(vmFileLayout, dataCenterMo, false);
deleteUnregisteredVmFiles(vmFileLayout, dataCenterMo, false, null);
Copy link
Member

@rafaelweingartner rafaelweingartner May 11, 2018

Choose a reason for hiding this comment

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

So, if there is an exception, you do delete the files, even if you shouldn't?

}
return new Answer(cmd, true, "unregister succeeded");
} catch (Exception e) {
Expand Down
33 changes: 33 additions & 0 deletions vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.Future;

import org.apache.commons.collections.CollectionUtils;
import org.apache.log4j.Logger;

import com.google.gson.Gson;
Expand Down Expand Up @@ -932,6 +933,38 @@ else if (prop.getName().startsWith("value[")) {
return networks;
}

public List<DatastoreMO> getAllDatastores() throws Exception {
Copy link
Member

@rafaelweingartner rafaelweingartner May 11, 2018

Choose a reason for hiding this comment

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

This "Exception" here. Is it from a VMware JDK method?

Copy link
Contributor Author

@sureshanaparti sureshanaparti May 11, 2018

Choose a reason for hiding this comment

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

@rafaelweingartner Correct. VMware SDK exception.

Copy link
Contributor

@DaanHoogland DaanHoogland May 14, 2018

Choose a reason for hiding this comment

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

@sureshanaparti is it worth handling it locally and/or throwing a more specific exception up?

Copy link
Member

@GabrielBrascher GabrielBrascher May 28, 2018

Choose a reason for hiding this comment

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

IMHO, the code would benefit from a more specific exception.

Copy link
Member

@rafaelweingartner rafaelweingartner Jun 20, 2018

Choose a reason for hiding this comment

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

@GabrielBrascher it is not possible. The methods from VMware SDK are throwing Exception.

Copy link
Member

@GabrielBrascher GabrielBrascher Jun 21, 2018

Choose a reason for hiding this comment

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

Thanks for pointing it.

PropertySpec pSpec = new PropertySpec();
pSpec.setType("Datastore");
pSpec.getPathSet().add("name");

TraversalSpec vmDatastoreTraversal = new TraversalSpec();
vmDatastoreTraversal.setType("VirtualMachine");
vmDatastoreTraversal.setPath("datastore");
vmDatastoreTraversal.setName("vmDatastoreTraversal");

ObjectSpec oSpec = new ObjectSpec();
oSpec.setObj(_mor);
oSpec.setSkip(Boolean.TRUE);
oSpec.getSelectSet().add(vmDatastoreTraversal);

PropertyFilterSpec pfSpec = new PropertyFilterSpec();
pfSpec.getPropSet().add(pSpec);
pfSpec.getObjectSet().add(oSpec);
List<PropertyFilterSpec> pfSpecArr = new ArrayList<PropertyFilterSpec>();
pfSpecArr.add(pfSpec);

List<ObjectContent> ocs = _context.getService().retrieveProperties(_context.getPropertyCollector(), pfSpecArr);

List<DatastoreMO> datastores = new ArrayList<DatastoreMO>();
if (CollectionUtils.isNotEmpty(ocs)) {
for (ObjectContent oc : ocs) {
datastores.add(new DatastoreMO(_context, oc.getObj()));
}
}
return datastores;
}

/**
* Retrieve path info to access VM files via vSphere web interface
* @return [0] vm-name, [1] data-center-name, [2] datastore-name
Expand Down