From 800e9dbac5f1a2ccdb8b202d9061239ee71879fb Mon Sep 17 00:00:00 2001 From: Koushik Das Date: Mon, 14 Apr 2014 16:12:35 +0530 Subject: [PATCH] CLOUDSTACK-6402: Fix StopCommand so that VMs are not removed accidentally as part of vmsync Added a new flag 'checkBeforeCleanup' to StopCommand based on which check is done to see if VM is running in HV host. If VM is running then in this case it is not stopped and the operation bails out. Also modified the MS code to call the StopCommand with appropriate value for the flag based on the context. Currently it is only set to 'true' when called from the new vmsync logic based on powerstate of VM. For rest it is set to 'false' meaning no change in behaviour. --- core/src/com/cloud/agent/api/StopCommand.java | 14 ++++++++--- .../cloud/vm/VirtualMachineManagerImpl.java | 24 +++++++++---------- .../vm/VirtualMachineManagerImplTest.java | 10 ++++---- .../HypervResourceController.cs | 11 +++++++++ .../HypervResourceController1Test.cs | 2 +- .../resource/LibvirtComputingResource.java | 12 ++++++++++ .../vmware/resource/VmwareResource.java | 12 ++++++++++ .../xen/resource/CitrixResourceBase.java | 6 +++++ 8 files changed, 70 insertions(+), 21 deletions(-) diff --git a/core/src/com/cloud/agent/api/StopCommand.java b/core/src/com/cloud/agent/api/StopCommand.java index 00d7f5fff6c..61d13e0f8e9 100755 --- a/core/src/com/cloud/agent/api/StopCommand.java +++ b/core/src/com/cloud/agent/api/StopCommand.java @@ -25,26 +25,30 @@ public class StopCommand extends RebootCommand { private String publicConsoleProxyIpAddress = null; boolean executeInSequence = false; private GPUDeviceTO gpuDevice; + boolean checkBeforeCleanup = false; protected StopCommand() { } - public StopCommand(VirtualMachine vm, boolean isProxy, String urlPort, String publicConsoleProxyIpAddress, boolean executeInSequence) { + public StopCommand(VirtualMachine vm, boolean isProxy, String urlPort, String publicConsoleProxyIpAddress, boolean executeInSequence, boolean checkBeforeCleanup) { super(vm); this.isProxy = isProxy; this.urlPort = urlPort; this.publicConsoleProxyIpAddress = publicConsoleProxyIpAddress; this.executeInSequence = executeInSequence; + this.checkBeforeCleanup = checkBeforeCleanup; } - public StopCommand(VirtualMachine vm, boolean executeInSequence) { + public StopCommand(VirtualMachine vm, boolean executeInSequence, boolean checkBeforeCleanup) { super(vm); this.executeInSequence = executeInSequence; + this.checkBeforeCleanup = checkBeforeCleanup; } - public StopCommand(String vmName, boolean executeInSequence) { + public StopCommand(String vmName, boolean executeInSequence, boolean checkBeforeCleanup) { super(vmName); this.executeInSequence = executeInSequence; + this.checkBeforeCleanup = checkBeforeCleanup; } @Override @@ -71,4 +75,8 @@ public class StopCommand extends RebootCommand { public void setGpuDevice(GPUDeviceTO gpuDevice) { this.gpuDevice = gpuDevice; } + + public boolean checkBeforeCleanup() { + return this.checkBeforeCleanup; + } } diff --git a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java index a8aae92f5c2..07c45586677 100755 --- a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java @@ -1047,7 +1047,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac s_logger.info("The guru did not like the answers so stopping " + vm); } - StopCommand cmd = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType())); + StopCommand cmd = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); StopAnswer answer = (StopAnswer) _agentMgr.easySend(destHostId, cmd); if ( answer != null ) { @@ -1225,9 +1225,9 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac } } - protected boolean sendStop(VirtualMachineGuru guru, VirtualMachineProfile profile, boolean force) { + protected boolean sendStop(VirtualMachineGuru guru, VirtualMachineProfile profile, boolean force, boolean checkBeforeCleanup) { VirtualMachine vm = profile.getVirtualMachine(); - StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType())); + StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), checkBeforeCleanup); try { StopAnswer answer = (StopAnswer)_agentMgr.send(vm.getHostId(), stop); if (answer != null) { @@ -1282,7 +1282,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac if (step == Step.Started || step == Step.Starting || step == Step.Release) { if (vm.getHostId() != null) { - if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop)) { + if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) { s_logger.warn("Failed to stop vm " + vm + " in " + State.Starting + " state as a part of cleanup process"); return false; } @@ -1295,26 +1295,26 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac } } else if (state == State.Stopping) { if (vm.getHostId() != null) { - if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop)) { + if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) { s_logger.warn("Failed to stop vm " + vm + " in " + State.Stopping + " state as a part of cleanup process"); return false; } } } else if (state == State.Migrating) { if (vm.getHostId() != null) { - if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop)) { + if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) { s_logger.warn("Failed to stop vm " + vm + " in " + State.Migrating + " state as a part of cleanup process"); return false; } } if (vm.getLastHostId() != null) { - if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop)) { + if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) { s_logger.warn("Failed to stop vm " + vm + " in " + State.Migrating + " state as a part of cleanup process"); return false; } } } else if (state == State.Running) { - if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop)) { + if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) { s_logger.warn("Failed to stop vm " + vm + " in " + State.Running + " state as a part of cleanup process"); return false; } @@ -1488,7 +1488,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac vmGuru.prepareStop(profile); - StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType())); + StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); boolean stopped = false; StopAnswer answer = null; @@ -2464,11 +2464,11 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac } public Command cleanup(VirtualMachine vm) { - return new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType())); + return new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); } public Command cleanup(String vmName) { - return new StopCommand(vmName, getExecuteInSequence(null)); + return new StopCommand(vmName, getExecuteInSequence(null), false); } @@ -4257,7 +4257,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac VirtualMachineGuru vmGuru = getVmGuru(vm); VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); - sendStop(vmGuru, profile, true); + sendStop(vmGuru, profile, true, true); try { stateTransitTo(vm, VirtualMachine.Event.FollowAgentPowerOffReport, null); diff --git a/engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java b/engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java index 49b2fc5fb11..a8cf6c01504 100644 --- a/engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java +++ b/engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java @@ -465,12 +465,12 @@ public class VirtualMachineManagerImplTest { VirtualMachineGuru guru = mock(VirtualMachineGuru.class); VirtualMachine vm = mock(VirtualMachine.class); VirtualMachineProfile profile = mock(VirtualMachineProfile.class); - StopAnswer answer = new StopAnswer(new StopCommand(vm, false), "ok", true); + StopAnswer answer = new StopAnswer(new StopCommand(vm, false, false), "ok", true); when(profile.getVirtualMachine()).thenReturn(vm); when(vm.getHostId()).thenReturn(1L); when(_agentMgr.send(anyLong(), (Command)any())).thenReturn(answer); - boolean actual = _vmMgr.sendStop(guru, profile, false); + boolean actual = _vmMgr.sendStop(guru, profile, false, false); Assert.assertTrue(actual); } @@ -480,12 +480,12 @@ public class VirtualMachineManagerImplTest { VirtualMachineGuru guru = mock(VirtualMachineGuru.class); VirtualMachine vm = mock(VirtualMachine.class); VirtualMachineProfile profile = mock(VirtualMachineProfile.class); - StopAnswer answer = new StopAnswer(new StopCommand(vm, false), "fail", false); + StopAnswer answer = new StopAnswer(new StopCommand(vm, false, false), "fail", false); when(profile.getVirtualMachine()).thenReturn(vm); when(vm.getHostId()).thenReturn(1L); when(_agentMgr.send(anyLong(), (Command)any())).thenReturn(answer); - boolean actual = _vmMgr.sendStop(guru, profile, false); + boolean actual = _vmMgr.sendStop(guru, profile, false, false); Assert.assertFalse(actual); } @@ -499,7 +499,7 @@ public class VirtualMachineManagerImplTest { when(vm.getHostId()).thenReturn(1L); when(_agentMgr.send(anyLong(), (Command)any())).thenReturn(null); - boolean actual = _vmMgr.sendStop(guru, profile, false); + boolean actual = _vmMgr.sendStop(guru, profile, false, false); Assert.assertFalse(actual); } diff --git a/plugins/hypervisors/hyperv/DotNet/ServerResource/HypervResource/HypervResourceController.cs b/plugins/hypervisors/hyperv/DotNet/ServerResource/HypervResource/HypervResourceController.cs index 4f4c14e15b1..ca3c2067999 100644 --- a/plugins/hypervisors/hyperv/DotNet/ServerResource/HypervResource/HypervResourceController.cs +++ b/plugins/hypervisors/hyperv/DotNet/ServerResource/HypervResource/HypervResourceController.cs @@ -1135,7 +1135,18 @@ namespace HypervResource logger.Info(CloudStackTypes.StopCommand + Utils.CleanString(cmd.ToString())); string details = null; bool result = false; + bool checkBeforeCleanup = cmd.checkBeforeCleanup; + String vmName = cmd.vmName; + if (checkBeforeCleanup == true) + { + ComputerSystem vm = wmiCallsV2.GetComputerSystem(vmName); + if (vm == null || vm.EnabledState == 2) + { + // VM is not available or vm in running state + return ReturnCloudStackTypedJArray(new { result = false, details = "VM is running on host, bailing out", vm = vmName, contextMap = contextMap }, CloudStackTypes.StopAnswer); + } + } try { wmiCallsV2.DestroyVm(cmd); diff --git a/plugins/hypervisors/hyperv/DotNet/ServerResource/ServerResource.Tests/HypervResourceController1Test.cs b/plugins/hypervisors/hyperv/DotNet/ServerResource/ServerResource.Tests/HypervResourceController1Test.cs index fd0a4bf2f7e..87a12c977d2 100644 --- a/plugins/hypervisors/hyperv/DotNet/ServerResource/ServerResource.Tests/HypervResourceController1Test.cs +++ b/plugins/hypervisors/hyperv/DotNet/ServerResource/ServerResource.Tests/HypervResourceController1Test.cs @@ -281,7 +281,7 @@ namespace ServerResource.Tests HypervResourceController rsrcServer = new HypervResourceController(); HypervResourceController.wmiCallsV2 = wmiCallsV2; - String sampleStop = "{\"isProxy\":false,\"vmName\":\"i-2-17-VM\",\"contextMap\":{},\"wait\":0}"; + String sampleStop = "{\"isProxy\":false,\"vmName\":\"i-2-17-VM\",\"contextMap\":{},\"checkBeforeCleanup\":false,\"wait\":0}"; dynamic jsonStopCmd = JsonConvert.DeserializeObject(sampleStop); // Act diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index beb5e10fd34..34c1b6d7ca6 100755 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -3484,6 +3484,18 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv protected Answer execute(StopCommand cmd) { final String vmName = cmd.getVmName(); + if (cmd.checkBeforeCleanup()) { + try { + Connect conn = LibvirtConnection.getConnectionByVmName(vmName); + Domain vm = conn.domainLookupByName(cmd.getVmName()); + if (vm != null && vm.getInfo().state == DomainInfo.DomainState.VIR_DOMAIN_RUNNING) { + return new StopAnswer(cmd, "vm is still running on host", false); + } + } catch (Exception e) { + s_logger.debug("Failed to get vm status in case of checkboforecleanup is true", e); + } + } + State state = null; synchronized (_vms) { state = _vms.get(vmName); diff --git a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 4f7d4506348..e89777d1363 100755 --- a/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -2740,6 +2740,18 @@ public class VmwareResource implements StoragePoolResource, ServerResource, Vmwa try { VirtualMachineMO vmMo = hyperHost.findVmOnHyperHost(cmd.getVmName()); if (vmMo != null) { + if (cmd.checkBeforeCleanup()) { + if (getVmPowerState(vmMo) != PowerState.PowerOff) { + String msg = "StopCommand is sent for cleanup and VM " + cmd.getVmName() + " is current running. ignore it."; + s_logger.warn(msg); + return new StopAnswer(cmd, msg, false); + } else { + String msg = "StopCommand is sent for cleanup and VM " + cmd.getVmName() + " is indeed stopped already."; + s_logger.info(msg); + return new StopAnswer(cmd, msg, true); + } + } + State state = null; synchronized (_vms) { state = _vms.get(cmd.getVmName()); diff --git a/plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java b/plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java index 55b9d45f445..1702bc9b51c 100644 --- a/plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java +++ b/plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java @@ -3627,6 +3627,12 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe return new StopAnswer(cmd, msg, platformstring, false); } + if (cmd.checkBeforeCleanup() && vmr.powerState == VmPowerState.RUNNING) { + String msg = "Vm " + vmName + " is running on host and checkBeforeCleanup flag is set, so bailing out"; + s_logger.debug(msg); + return new StopAnswer(cmd, msg, false); + } + State state = s_vms.getState(_cluster, vmName); synchronized (_cluster.intern()) {