From bde008ed7eb82a7dd0b1e11c93fd313294cb6bc6 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 | 33 ++++++++--------- .../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, 68 insertions(+), 35 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 b88117b4301..4fbf3a6679a 100644 --- a/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java +++ b/core/src/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java @@ -174,7 +174,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)) { @@ -203,7 +203,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { } snapshotBackupUuid = backupSnapshotToSecondaryStorage(vmMo, accountId, volumeId, cmd.getVolumePath(), snapshotUuid, secondaryStorageUrl, prevSnapshotUuid, prevBackupUuid, - UUID.randomUUID().toString().replace("-", "")); + hostService.getWorkerName(context, cmd, 1)); success = (snapshotBackupUuid != null); if (success) { @@ -238,8 +238,6 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { return new BackupSnapshotAnswer(cmd, success, details, snapshotBackupUuid, true); } - - @Override public Answer execute(VmwareHostService hostService, CreatePrivateTemplateFromVolumeCommand cmd) { String secondaryStoragePoolURL = cmd.getSecondaryStorageUrl(); @@ -268,7 +266,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(), @@ -331,10 +329,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(); @@ -393,8 +391,7 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { DatastoreMO primaryDsMo = new DatastoreMO(hyperHost.getContext(), morPrimaryDs); details = createVolumeFromSnapshot(hyperHost, primaryDsMo, - newVolumeName, accountId, volumeId, - secondaryStorageUrl, backedUpSnapshotUuid); + newVolumeName, accountId, volumeId, secondaryStorageUrl, backedUpSnapshotUuid); if (details == null) { success = true; } @@ -738,8 +735,10 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { return "Failed to delete snapshot backup file, backupUuid: " + backupUuid; } - 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; @@ -756,12 +755,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()); @@ -780,7 +778,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); @@ -795,7 +793,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 a9b3b56cafe..cf47d92c3cd 100755 --- a/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/core/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -2885,7 +2885,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 { @@ -2969,7 +2969,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) { @@ -3968,7 +3968,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 0e69084ab28..5396d0d80ac 100644 --- a/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java +++ b/core/src/com/cloud/storage/resource/VmwareSecondaryStorageResourceHandler.java @@ -66,12 +66,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; } @@ -219,9 +225,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 bda525e2edf..6babfa30b70 100644 --- a/server/src/com/cloud/hypervisor/guru/VMwareGuru.java +++ b/server/src/com/cloud/hypervisor/guru/VMwareGuru.java @@ -266,6 +266,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 c9f54f40549..3421f300c58 100755 --- a/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java +++ b/server/src/com/cloud/hypervisor/vmware/VmwareManagerImpl.java @@ -749,6 +749,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 {