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

@@ -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;
+    }
 }

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);

engine/orchestration/test/com/cloud/vm/VirtualMachineManagerImplTest.java

@@ -466,12 +466,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);
     }
@@ -481,12 +481,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);
     }
@@ -500,7 +500,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);
     }

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);

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

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -3485,6 +3485,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);

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());

plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java

@@ -3628,6 +3628,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()) {