Fixup vm powerstate update (#8545)

Co-authored-by: Suresh Kumar Anaparti <suresh.anaparti@shapeblue.com>

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -213,8 +213,8 @@ import com.cloud.service.ServiceOfferingVO;
 import com.cloud.service.dao.ServiceOfferingDao;
 import com.cloud.storage.DiskOfferingVO;
 import com.cloud.storage.ScopeType;
-import com.cloud.storage.Storage.ImageFormat;
 import com.cloud.storage.Storage;
+import com.cloud.storage.Storage.ImageFormat;
 import com.cloud.storage.StorageManager;
 import com.cloud.storage.StoragePool;
 import com.cloud.storage.VMTemplateVO;
@@ -2208,9 +2208,11 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
 
             boolean result = stateTransitTo(vm, Event.OperationSucceeded, null);
             if (result) {
+                vm.setPowerState(PowerState.PowerOff);
+                _vmDao.update(vm.getId(), vm);
                 if (VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
                     ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
-                    resourceCountDecrement(vm.getAccountId(),new Long(offering.getCpu()), new Long(offering.getRamSize()));
+                    resourceCountDecrement(vm.getAccountId(), offering.getCpu().longValue(), offering.getRamSize().longValue());
                 }
             } else {
                 throw new CloudRuntimeException("unable to stop " + vm);
@@ -2761,6 +2763,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         }
 
         vm.setLastHostId(srcHostId);
+        _vmDao.resetVmPowerStateTracking(vm.getId());
         try {
             if (vm.getHostId() == null || vm.getHostId() != srcHostId || !changeState(vm, Event.MigrationRequested, dstHostId, work, Step.Migrating)) {
                 _networkMgr.rollbackNicForMigration(vmSrc, profile);

engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java

@@ -66,7 +66,7 @@ import com.cloud.vm.VirtualMachine.Type;
 public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implements VMInstanceDao {
 
     public static final Logger s_logger = Logger.getLogger(VMInstanceDaoImpl.class);
-    private static final int MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT = 3;
+    static final int MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT = 3;
 
     protected SearchBuilder<VMInstanceVO> VMClusterSearch;
     protected SearchBuilder<VMInstanceVO> LHVMClusterSearch;
@@ -897,17 +897,19 @@ public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implem
 
     @Override
     public boolean updatePowerState(final long instanceId, final long powerHostId, final VirtualMachine.PowerState powerState, Date wisdomEra) {
-        return Transaction.execute(new TransactionCallback<Boolean>() {
+        return Transaction.execute(new TransactionCallback<>() {
             @Override
             public Boolean doInTransaction(TransactionStatus status) {
                 boolean needToUpdate = false;
                 VMInstanceVO instance = findById(instanceId);
                 if (instance != null
-                &&  (null == instance.getPowerStateUpdateTime()
+                        && (null == instance.getPowerStateUpdateTime()
                         || instance.getPowerStateUpdateTime().before(wisdomEra))) {
                     Long savedPowerHostId = instance.getPowerHostId();
-                    if (instance.getPowerState() != powerState || savedPowerHostId == null
+                    if (instance.getPowerState() != powerState
-                            || savedPowerHostId.longValue() != powerHostId) {
+                            || savedPowerHostId == null
+                            || savedPowerHostId != powerHostId
+                            || !isPowerStateInSyncWithInstanceState(powerState, powerHostId, instance)) {
                         instance.setPowerState(powerState);
                         instance.setPowerHostId(powerHostId);
                         instance.setPowerStateUpdateCount(1);
@@ -929,6 +931,17 @@ public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implem
         });
     }
 
+    private boolean isPowerStateInSyncWithInstanceState(final VirtualMachine.PowerState powerState, final long powerHostId, final VMInstanceVO instance) {
+        State instanceState = instance.getState();
+        if ((powerState == VirtualMachine.PowerState.PowerOff && instanceState == State.Running)
+                || (powerState == VirtualMachine.PowerState.PowerOn && instanceState == State.Stopped)) {
+            s_logger.debug(String.format("VM id: %d on host id: %d and power host id: %d is in %s state, but power state is %s",
+                    instance.getId(), instance.getHostId(), powerHostId, instanceState, powerState));
+            return false;
+        }
+        return true;
+    }
+
     @Override
     public boolean isPowerStateUpToDate(final long instanceId) {
         VMInstanceVO instance = findById(instanceId);

engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDaoImplTest.java

@@ -17,22 +17,32 @@
 
 package com.cloud.vm.dao;
 
-import com.cloud.utils.Pair;
+import static com.cloud.vm.VirtualMachine.State.Running;
-import com.cloud.vm.VirtualMachine;
+import static com.cloud.vm.VirtualMachine.State.Stopped;
+import static com.cloud.vm.dao.VMInstanceDaoImpl.MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Date;
+
 import org.joda.time.DateTime;
 import org.junit.Before;
 import org.junit.Test;
-import org.junit.Assert;
 import org.mockito.Mock;
-
-import static com.cloud.vm.VirtualMachine.State.Running;
-import static com.cloud.vm.VirtualMachine.State.Stopped;
-
-import static org.mockito.Mockito.when;
-import com.cloud.vm.VMInstanceVO;
 import org.mockito.MockitoAnnotations;
 import org.mockito.Spy;
 
+import com.cloud.utils.Pair;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+
 /**
  * Created by sudharma_jain on 3/2/17.
  */
@@ -55,16 +65,130 @@ public class VMInstanceDaoImplTest {
     }
 
     @Test
-    public void testUpdateState() throws Exception {
+    public void testUpdateState() {
         Long destHostId = null;
-        Pair<Long, Long> opaqueMock = new Pair<Long, Long>(new Long(1), destHostId);
+        Pair<Long, Long> opaqueMock = new Pair<>(1L, destHostId);
         vmInstanceDao.updateState(Stopped, VirtualMachine.Event.FollowAgentPowerOffReport, Stopped, vm , opaqueMock);
     }
 
     @Test
-    public void testIfStateAndHostUnchanged() throws Exception {
+    public void testIfStateAndHostUnchanged() {
-        Assert.assertEquals(vmInstanceDao.ifStateUnchanged(Stopped, Stopped, null, null), true);
+        assertTrue(vmInstanceDao.ifStateUnchanged(Stopped, Stopped, null, null));
-        Assert.assertEquals(vmInstanceDao.ifStateUnchanged(Stopped, Running, null, null), false);
+        assertFalse(vmInstanceDao.ifStateUnchanged(Stopped, Running, null, null));
     }
 
+    @Test
+    public void testUpdatePowerStateDifferentPowerState() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
+        doReturn(vm).when(vmInstanceDao).findById(anyLong());
+        doReturn(true).when(vmInstanceDao).update(anyLong(), any());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());
+
+        verify(vm, times(1)).setPowerState(VirtualMachine.PowerState.PowerOff);
+        verify(vm, times(1)).setPowerHostId(1L);
+        verify(vm, times(1)).setPowerStateUpdateCount(1);
+        verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));
+
+        assertTrue(result);
+    }
+
+    @Test
+    public void testUpdatePowerStateVmNotFound() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
+        doReturn(null).when(vmInstanceDao).findById(anyLong());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());
+
+        verify(vm, never()).setPowerState(any());
+        verify(vm, never()).setPowerHostId(anyLong());
+        verify(vm, never()).setPowerStateUpdateCount(any(Integer.class));
+        verify(vm, never()).setPowerStateUpdateTime(any(Date.class));
+
+        assertFalse(result);
+    }
+
+    @Test
+    public void testUpdatePowerStateNoChangeFirstUpdate() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
+        when(vm.getState()).thenReturn(Running);
+        when(vm.getPowerStateUpdateCount()).thenReturn(1);
+        doReturn(vm).when(vmInstanceDao).findById(anyLong());
+        doReturn(true).when(vmInstanceDao).update(anyLong(), any());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());
+
+        verify(vm, never()).setPowerState(any());
+        verify(vm, never()).setPowerHostId(anyLong());
+        verify(vm, times(1)).setPowerStateUpdateCount(2);
+        verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));
+
+        assertTrue(result);
+    }
+
+    @Test
+    public void testUpdatePowerStateNoChangeMaxUpdatesValidState() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
+        when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
+        when(vm.getState()).thenReturn(Running);
+        doReturn(vm).when(vmInstanceDao).findById(anyLong());
+        doReturn(true).when(vmInstanceDao).update(anyLong(), any());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());
+
+        verify(vm, never()).setPowerState(any());
+        verify(vm, never()).setPowerHostId(anyLong());
+        verify(vm, never()).setPowerStateUpdateCount(any(Integer.class));
+        verify(vm, never()).setPowerStateUpdateTime(any(Date.class));
+
+        assertFalse(result);
+    }
+
+    @Test
+    public void testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmStopped() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOn);
+        when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
+        when(vm.getState()).thenReturn(Stopped);
+        doReturn(vm).when(vmInstanceDao).findById(anyLong());
+        doReturn(true).when(vmInstanceDao).update(anyLong(), any());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOn, new Date());
+
+        verify(vm, times(1)).setPowerState(any());
+        verify(vm, times(1)).setPowerHostId(anyLong());
+        verify(vm, times(1)).setPowerStateUpdateCount(1);
+        verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));
+
+        assertTrue(result);
+    }
+
+    @Test
+    public void testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmRunning() {
+        when(vm.getPowerStateUpdateTime()).thenReturn(null);
+        when(vm.getPowerHostId()).thenReturn(1L);
+        when(vm.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOff);
+        when(vm.getPowerStateUpdateCount()).thenReturn(MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT);
+        when(vm.getState()).thenReturn(Running);
+        doReturn(vm).when(vmInstanceDao).findById(anyLong());
+        doReturn(true).when(vmInstanceDao).update(anyLong(), any());
+
+        boolean result = vmInstanceDao.updatePowerState(1L, 1L, VirtualMachine.PowerState.PowerOff, new Date());
+
+        verify(vm, times(1)).setPowerState(any());
+        verify(vm, times(1)).setPowerHostId(anyLong());
+        verify(vm, times(1)).setPowerStateUpdateCount(1);
+        verify(vm, times(1)).setPowerStateUpdateTime(any(Date.class));
+
+        assertTrue(result);
+    }
 }