From 00629b61c648208ece29fc00b78863a725c83175 Mon Sep 17 00:00:00 2001 From: Kelven Yang Date: Fri, 6 Jan 2012 18:56:14 -0800 Subject: [PATCH] bug 12787: improve worker VM cleanup in snapshot operation --- .../vmware/manager/VmwareHostService.java | 2 +- .../manager/VmwareStorageManagerImpl.java | 62 +++++++++---------- .../vmware/resource/VmwareResource.java | 6 +- ...VmwareSecondaryStorageResourceHandler.java | 14 ++++- .../com/cloud/hypervisor/guru/VMwareGuru.java | 6 ++ .../hypervisor/vmware/VmwareManagerImpl.java | 5 ++ .../vmware/mo/VirtualMachineMO.java | 37 +++++++---- 7 files changed, 82 insertions(+), 50 deletions(-) diff --git a/core/src/com/cloud/hypervisor/vmware/manager/VmwareHostService.java b/core/src/com/cloud/hypervisor/vmware/manager/VmwareHostService.java index 5ea35118525..3464e01ce45 100644 --- a/core/src/com/cloud/hypervisor/vmware/manager/VmwareHostService.java +++ b/core/src/com/cloud/hypervisor/vmware/manager/VmwareHostService.java @@ -13,5 +13,5 @@ public interface VmwareHostService { void invalidateServiceContext(VmwareContext context); VmwareHypervisorHost getHyperHost(VmwareContext context, Command cmd); - String getWorkerName(VmwareContext context, Command cmd); + String getWorkerName(VmwareContext context, Command cmd, int workerSequence); } diff --git a/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java b/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java index b549d0736de..a4ad5c319d9 100644 --- a/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java +++ b/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java @@ -178,7 +178,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { if(vmMo == null) { dsMo = new DatastoreMO(hyperHost.getContext(), morDs); - workerVMName = hostService.getWorkerName(context, cmd); + workerVMName = hostService.getWorkerName(context, cmd, 0); // attach a volume to dummay wrapper VM for taking snapshot and exporting the VM for backup if (!hyperHost.createBlankVm(workerVMName, 1, 512, 0, false, 4, 0, VirtualMachineGuestOsIdentifier._otherGuest.toString(), morDs, false)) { @@ -206,11 +206,8 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { } } - snapshotBackupUuid = backupSnapshotToSecondaryStorage(vmMo, accountId, - volumeId, cmd.getVolumePath(), snapshotUuid, - secondaryStoragePoolURL, prevSnapshotUuid, - prevBackupUuid, - UUID.randomUUID().toString().replace("-", "")); + snapshotBackupUuid = backupSnapshotToSecondaryStorage(vmMo, accountId, volumeId, cmd.getVolumePath(), snapshotUuid, secondaryStoragePoolURL, prevSnapshotUuid, prevBackupUuid, + hostService.getWorkerName(context, cmd, 1)); success = (snapshotBackupUuid != null); if (success) { @@ -330,7 +327,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { Ternary result = createTemplateFromVolume(vmMo, accountId, templateId, cmd.getTemplateName(), secondaryStoragePoolURL, volumePath, - hostService.getWorkerName(context, cmd)); + hostService.getWorkerName(context, cmd, 0)); return new CreatePrivateTemplateAnswer(cmd, true, null, result.first(), result.third(), result.second(), @@ -393,10 +390,10 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { Pair result; if (cmd.toSecondaryStorage()) { - result = copyVolumeToSecStorage( - hyperHost, vmName, volumeId, cmd.getPool().getUuid(), volumePath, + result = copyVolumeToSecStorage(hostService, + hyperHost, cmd, vmName, volumeId, cmd.getPool().getUuid(), volumePath, secondaryStorageURL, - hostService.getWorkerName(context, cmd)); + hostService.getWorkerName(context, cmd, 0)); } else { StorageFilerTO poolTO = cmd.getPool(); @@ -455,8 +452,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { DatastoreMO primaryDsMo = new DatastoreMO(hyperHost.getContext(), morPrimaryDs); details = createVolumeFromSnapshot(hyperHost, primaryDsMo, - newVolumeName, accountId, volumeId, - secondaryStoragePoolURL, backedUpSnapshotUuid); + newVolumeName, accountId, volumeId, secondaryStoragePoolURL, backedUpSnapshotUuid); if (details == null) { success = true; } @@ -798,22 +794,24 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { } return "Failed to delete snapshot backup file, backupUuid: " + backupUuid; - } + } + + private String deleteSnapshotDirOnSecondaryStorage(long accountId, long volumeId, String secStorageUrl) throws Exception { + String secondaryMountPoint = _mountService.getMountPoint(secStorageUrl); + String snapshotMountRoot = secondaryMountPoint + "/" + getSnapshotRelativeDirInSecStorage(accountId, volumeId); + + synchronized(snapshotMountRoot.intern()) { + Script command = new Script(false, "rm", _timeout, s_logger); + command.add("-rf"); + command.add(snapshotMountRoot); + return command.execute(); + } + } - private String deleteSnapshotDirOnSecondaryStorage(long accountId, long volumeId, String secStorageUrl) throws Exception { - String secondaryMountPoint = _mountService.getMountPoint(secStorageUrl); - String snapshotMountRoot = secondaryMountPoint + "/" + getSnapshotRelativeDirInSecStorage(accountId, volumeId); - - synchronized(snapshotMountRoot.intern()) { - Script command = new Script(false, "rm", _timeout, s_logger); - command.add("-rf"); - command.add(snapshotMountRoot); - return command.execute(); - } - } - - private Pair copyVolumeToSecStorage(VmwareHypervisorHost hyperHost, String vmName, long volumeId, String poolId, String volumePath, - String secStorageUrl, String workerVmName) throws Exception { + private Pair copyVolumeToSecStorage(VmwareHostService hostService, VmwareHypervisorHost hyperHost, CopyVolumeCommand cmd, + String vmName, long volumeId, String poolId, String volumePath, + String secStorageUrl, String workerVmName) throws Exception { + String volumeFolder = String.valueOf(volumeId) + "/"; VirtualMachineMO workerVm=null; VirtualMachineMO vmMo=null; @@ -830,12 +828,11 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { vmMo = hyperHost.findVmOnHyperHost(vmName); if (vmMo == null) { - //create a dummy worker vm for attaching the volume + // create a dummy worker vm for attaching the volume DatastoreMO dsMo = new DatastoreMO(hyperHost.getContext(), morDs); //restrict VM name to 32 chars, (else snapshot descriptor file name will be truncated to 32 chars of vm name) - String workerVMName = UUID.randomUUID().toString().replaceAll("-", ""); VirtualMachineConfigSpec vmConfig = new VirtualMachineConfigSpec(); - vmConfig.setName(workerVMName); + vmConfig.setName(workerVmName); vmConfig.setMemoryMB((long) 4); vmConfig.setNumCPUs(1); vmConfig.setGuestId(VirtualMachineGuestOsIdentifier._otherGuest.toString()); @@ -854,7 +851,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { vmConfig.setDeviceChange(new VirtualDeviceConfigSpec[] { scsiControllerSpec }); hyperHost.createVm(vmConfig); - workerVm = hyperHost.findVmOnHyperHost(workerVMName); + workerVm = hyperHost.findVmOnHyperHost(workerVmName); if (workerVm == null) { String msg = "Unable to create worker VM to execute CopyVolumeCommand"; s_logger.error(msg); @@ -869,7 +866,8 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { vmMo.createSnapshot(exportName, "Temporary snapshot for copy-volume command", false, false); - exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, "volumes/" + volumeFolder, exportName, workerVmName); + exportVolumeToSecondaryStroage(vmMo, volumePath, secStorageUrl, "volumes/" + volumeFolder, exportName, + hostService.getWorkerName(hyperHost.getContext(), cmd, 1)); return new Pair(volumeFolder, exportName); } finally { diff --git a/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 3187ee563c2..f8be5a7691d 100755 --- a/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -2874,7 +2874,7 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa if (cmd.getDiskCharacteristics().getType() == Volume.Type.ROOT) { if (cmd.getTemplateUrl() == null) { // create a root volume for blank VM - String dummyVmName = getWorkerName(context, cmd); + String dummyVmName = getWorkerName(context, cmd, 0); VirtualMachineMO vmMo = null; try { @@ -2958,7 +2958,7 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa VirtualMachineMO vmMo = null; String volumeUuid = UUID.randomUUID().toString().replace("-", ""); String volumeDatastorePath = String.format("[%s] %s.vmdk", dsMo.getName(), volumeUuid); - String dummyVmName = getWorkerName(context, cmd); + String dummyVmName = getWorkerName(context, cmd, 0); try { vmMo = prepareVolumeHostDummyVm(hyperHost, dsMo, dummyVmName); if (vmMo == null) { @@ -3950,7 +3950,7 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa @Override @DB - public String getWorkerName(VmwareContext context, Command cmd) { + public String getWorkerName(VmwareContext context, Command cmd, int workerSequence) { VmwareManager mgr = context.getStockObject(VmwareManager.CONTEXT_STOCK_NAME); String vmName = mgr.composeWorkerName(); diff --git a/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java b/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java index 8f5ccf45386..53db3a1da29 100644 --- a/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java +++ b/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java @@ -72,12 +72,18 @@ public class VmwareSecondaryStorageResourceHandler implements SecondaryStorageRe answer = _resource.defaultAction(cmd); } + // special handling to pass-back context info for cleanups if(cmd.getContextParam("execid") != null) { answer.setContextParam("execid", cmd.getContextParam("execid")); } + if(cmd.getContextParam("checkpoint") != null) { answer.setContextParam("checkpoint", cmd.getContextParam("checkpoint")); } + + if(cmd.getContextParam("checkpoint2") != null) { + answer.setContextParam("checkpoint2", cmd.getContextParam("checkpoint2")); + } return answer; } @@ -241,9 +247,13 @@ public class VmwareSecondaryStorageResourceHandler implements SecondaryStorageRe } @Override - public String getWorkerName(VmwareContext context, Command cmd) { + public String getWorkerName(VmwareContext context, Command cmd, int workerSequence) { assert(cmd.getContextParam("worker") != null); - return cmd.getContextParam("worker"); + assert(workerSequence < 2); + + if(workerSequence == 0) + return cmd.getContextParam("worker"); + return cmd.getContextParam("worker2"); } @Override diff --git a/server/src/com/cloud/hypervisor/guru/VMwareGuru.java b/server/src/com/cloud/hypervisor/guru/VMwareGuru.java index 4078c2d031e..1c703b67c76 100644 --- a/server/src/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/server/src/com/cloud/hypervisor/guru/VMwareGuru.java @@ -270,6 +270,12 @@ public class VMwareGuru extends HypervisorGuruBase implements HypervisorGuru { long checkPointId = _checkPointMgr.pushCheckPoint(new VmwareCleanupMaid(hostDetails.get("guid"), workerName)); cmd.setContextParam("worker", workerName); cmd.setContextParam("checkpoint", String.valueOf(checkPointId)); + + // some commands use 2 workers + String workerName2 = _vmwareMgr.composeWorkerName(); + long checkPointId2 = _checkPointMgr.pushCheckPoint(new VmwareCleanupMaid(hostDetails.get("guid"), workerName2)); + cmd.setContextParam("worker2", workerName2); + cmd.setContextParam("checkpoint2", String.valueOf(checkPointId2)); } return cmdTarget.first().getId(); diff --git a/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java b/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java index 88b56a2a2c6..3c5234653c5 100755 --- a/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java +++ b/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java @@ -747,6 +747,11 @@ public class VmwareManagerImpl implements VmwareManager, VmwareStorageMount, Lis if(checkPointIdStr != null) { _checkPointMgr.popCheckPoint(Long.parseLong(checkPointIdStr)); } + + checkPointIdStr = answer.getContextParam("checkpoint2"); + if(checkPointIdStr != null) { + _checkPointMgr.popCheckPoint(Long.parseLong(checkPointIdStr)); + } } } diff --git a/vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java b/vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java index 7fac9932019..20f8149551c 100755 --- a/vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java +++ b/vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java @@ -1411,19 +1411,32 @@ public class VirtualMachineMO extends BaseMO { HostMO hostMo = getRunningHost(); VirtualMachineConfigInfo vmConfigInfo = getConfigInfo(); - hostMo.createBlankVm(clonedVmName, 1, cpuSpeedMHz, 0, false, memoryMb, 0, vmConfigInfo.getGuestId(), morDs, false); - VirtualMachineMO clonedVmMo = hostMo.findVmOnHyperHost(clonedVmName); + if(!hostMo.createBlankVm(clonedVmName, 1, cpuSpeedMHz, 0, false, memoryMb, 0, vmConfigInfo.getGuestId(), morDs, false)) + throw new Exception("Unable to create a blank VM"); - VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); - VirtualDeviceConfigSpec[] deviceConfigSpecArray = new VirtualDeviceConfigSpec[1]; - deviceConfigSpecArray[0] = new VirtualDeviceConfigSpec(); - - VirtualDevice device = VmwareHelper.prepareDiskDevice(clonedVmMo, -1, disks, morDs, -1, 1); - - deviceConfigSpecArray[0].setDevice(device); - deviceConfigSpecArray[0].setOperation(VirtualDeviceConfigSpecOperation.add); - vmConfigSpec.setDeviceChange(deviceConfigSpecArray); - clonedVmMo.configureVm(vmConfigSpec); + VirtualMachineMO clonedVmMo = hostMo.findVmOnHyperHost(clonedVmName); + if(clonedVmMo == null) + throw new Exception("Unable to find just-created blank VM"); + + boolean bSuccess = false; + try { + VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); + VirtualDeviceConfigSpec[] deviceConfigSpecArray = new VirtualDeviceConfigSpec[1]; + deviceConfigSpecArray[0] = new VirtualDeviceConfigSpec(); + + VirtualDevice device = VmwareHelper.prepareDiskDevice(clonedVmMo, -1, disks, morDs, -1, 1); + + deviceConfigSpecArray[0].setDevice(device); + deviceConfigSpecArray[0].setOperation(VirtualDeviceConfigSpecOperation.add); + vmConfigSpec.setDeviceChange(deviceConfigSpecArray); + clonedVmMo.configureVm(vmConfigSpec); + bSuccess = true; + } finally { + if(!bSuccess) { + clonedVmMo.detachAllDisks(); + clonedVmMo.destroy(); + } + } } public void plugDevice(VirtualDevice device) throws Exception {