diff --git a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java index 702404546894..99e64a10b084 100644 --- a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java +++ b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java @@ -170,6 +170,14 @@ void orchestrateStart(String vmUuid, Map pa void destroy(String vmUuid, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException; + /** + * Stop an instance ahead of destroying it. The stop is forced according to vm.destroy.forcestop, but the + * instance's resources are only released without the host confirming the stop if the host is gone (no host, + * no host record, Down or Removed). Otherwise the stop fails as an unforced one does and the instance is left + * in the state it was in. + */ + void advanceStopForDestroy(String vmUuid) throws ResourceUnavailableException, OperationTimedoutException, ConcurrentOperationException; + void migrateAway(String vmUuid, long hostId) throws InsufficientServerCapacityException; void migrate(String vmUuid, long srcHostId, DeployDestination dest) throws ResourceUnavailableException, ConcurrentOperationException; diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java index c98391a654db..1b733fd10e0b 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -481,7 +481,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac static final ConfigKey VmOpCancelInterval = new ConfigKey("Advanced", Long.class, "vm.op.cancel.interval", "3600", "Time (in seconds) to wait before cancelling a operation", false); static final ConfigKey VmDestroyForcestop = new ConfigKey("Advanced", Boolean.class, "vm.destroy.forcestop", "false", - "On destroy, force-stop takes this value ", true); + "On destroy, force-stop takes this value. When the host cannot be reached, the instance's resources are only " + + "released if the host is Down or Removed; otherwise the destroy fails and can be retried once the host is back.", true); static final ConfigKey ClusterDeltaSyncInterval = new ConfigKey("Advanced", Integer.class, "sync.interval", "60", "Cluster Delta sync interval in seconds", false); @@ -693,7 +694,7 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti _userVmDao.saveDetails(userVM); } - advanceStop(vm.getUuid(), VmDestroyForcestop.value()); + advanceStopForDestroy(vm.getUuid()); vm = _vmDao.findByUuid(vm.getUuid()); try { @@ -2364,6 +2365,27 @@ protected void releaseVmResources(final VirtualMachineProfile profile, final boo @Override public void advanceStop(final String vmUuid, final boolean cleanUpEvenIfUnableToStop) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { + advanceStop(vmUuid, cleanUpEvenIfUnableToStop, false); + } + + /** + * vm.destroy.forcestop makes the stop that precedes a destroy a forced one. A forced stop that gets no answer from + * the host releases the instance's NICs, addresses and storage anyway. That is right for a host that is gone, and + * wrong for one that is only briefly unreachable, for example while its agent or a management server restarts: + * the domain keeps running, its address is handed to another instance and its volume is stranded. + * + * So a destroy's forced stop releases without the host's answer only when the host is gone. Otherwise it fails + * as an unforced stop would, the instance stays Running and the destroy can be retried. An explicit forced stop + * is not affected: that is a caller stating the instance is to be treated as stopped. + */ + @Override + public void advanceStopForDestroy(final String vmUuid) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { + final boolean force = VmDestroyForcestop.value(); + advanceStop(vmUuid, force, force); + } + + protected void advanceStop(final String vmUuid, final boolean cleanUpEvenIfUnableToStop, final boolean releaseOnlyIfHostIsGone) + throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { final AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { @@ -2372,7 +2394,7 @@ public void advanceStop(final String vmUuid, final boolean cleanUpEvenIfUnableTo final VirtualMachine vm = _vmDao.findByUuid(vmUuid); placeHolder = createPlaceHolderWork(vm.getId()); try { - orchestrateStop(vmUuid, cleanUpEvenIfUnableToStop); + orchestrateStop(vmUuid, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone); } finally { if (placeHolder != null) { _workJobDao.expunge(placeHolder.getId()); @@ -2380,7 +2402,7 @@ public void advanceStop(final String vmUuid, final boolean cleanUpEvenIfUnableTo } } else { - final Outcome outcome = stopVmThroughJobQueue(vmUuid, cleanUpEvenIfUnableToStop); + final Outcome outcome = stopVmThroughJobQueue(vmUuid, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone); retrieveVmFromJobOutcome(outcome, vmUuid, "stopVm"); @@ -2392,10 +2414,43 @@ public void advanceStop(final String vmUuid, final boolean cleanUpEvenIfUnableTo } } - private void orchestrateStop(final String vmUuid, final boolean cleanUpEvenIfUnableToStop) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { + private void orchestrateStop(final String vmUuid, final boolean cleanUpEvenIfUnableToStop, final boolean releaseOnlyIfHostIsGone) + throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { final VMInstanceVO vm = _vmDao.findByUuid(vmUuid); - advanceStop(vm, cleanUpEvenIfUnableToStop); + advanceStop(vm, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone); + } + + /** + * @return true when the host an instance was on cannot be running it anymore: there is no host, its record is + * gone, or it is Down or Removed. In any other status the host may still be running it, whether or not + * it answers right now. + */ + protected boolean isHostGone(final Long hostId) { + return hostId == null || isGone(_hostDao.findById(hostId)); + } + + private static boolean isGone(final HostVO host) { + return host == null || host.getStatus() == Status.Down || host.getStatus() == Status.Removed; + } + + /** + * Whether a stop may release an instance's resources without the host confirming the instance is stopped. + */ + protected boolean mayReleaseWithoutHostConfirmation(final VMInstanceVO vm, final boolean cleanUpEvenIfUnableToStop, final boolean releaseOnlyIfHostIsGone) { + if (!cleanUpEvenIfUnableToStop) { + return false; + } + if (!releaseOnlyIfHostIsGone || vm.getHostId() == null) { + return true; + } + final HostVO host = _hostDao.findById(vm.getHostId()); + if (isGone(host)) { + return true; + } + logger.warn("Not releasing the resources of {}: its host {} is {} and did not confirm the instance is stopped. " + + "Retry the destroy, or stop the instance with forced=true if it is known to be gone.", vm, host, host.getStatus()); + return false; } private void updatePersistenceMap(Map vlanToPersistenceMap, NetworkVO networkVO) { @@ -2460,8 +2515,8 @@ private Pair getVMNetworkDetails(NetworkVO networkVO, boolean i return null; } - private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnableToStop) throws AgentUnavailableException, OperationTimedoutException, - ConcurrentOperationException { + protected void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnableToStop, final boolean releaseOnlyIfHostIsGone) + throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { final State state = vm.getState(); if (state == State.Stopped) { logger.debug("VM is already stopped: {}", vm); @@ -2514,6 +2569,16 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl if (!cleanUpEvenIfUnableToStop) { throw new CloudRuntimeException("We cannot stop " + vm + " when it is in state " + vm.getState()); } + // cleanup() releases the resources whether or not the host answers. For a destroy on a host that is not + // gone, go ahead only once the host has confirmed the instance is stopped. + if (releaseOnlyIfHostIsGone && !isHostGone(vm.getHostId())) { + final Pair stopResult = sendStop(vmGuru, profile, false, false); + if (!stopResult.first()) { + logger.warn("Not releasing the resources of {} in state {}: its host did not confirm the instance is stopped.", vm, vm.getState()); + String errorDetails = stopResult.second() != null ? " due to " + stopResult.second() : ""; + throw new CloudRuntimeException("Unable to stop " + vm + " in state " + vm.getState() + errorDetails); + } + } final boolean doCleanup = true; logger.warn("Unable to transition the state but we're moving on because it's forced stop", e1); @@ -2588,7 +2653,7 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl logger.warn("Unable to stop {} due to [{}].", profile.toString(), e.toString(), e); } finally { if (!stopped) { - if (!cleanUpEvenIfUnableToStop) { + if (!mayReleaseWithoutHostConfirmation(vm, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone)) { logger.warn("Unable to stop vm {}", vm); try { stateTransitTo(vm, Event.OperationFailed, vm.getHostId()); @@ -2699,7 +2764,7 @@ public void destroy(final String vmUuid, final boolean expunge) throws AgentUnav logger.debug("Destroying vm {}, expunge flag {}", vm, (expunge ? "on" : "off")); - advanceStop(vmUuid, VmDestroyForcestop.value()); + advanceStopForDestroy(vmUuid); deleteVMSnapshots(vm, expunge); @@ -5705,6 +5770,10 @@ public Outcome startVmThroughJobQueue(final String vmUuid, } public Outcome stopVmThroughJobQueue(final String vmUuid, final boolean cleanup) { + return stopVmThroughJobQueue(vmUuid, cleanup, false); + } + + public Outcome stopVmThroughJobQueue(final String vmUuid, final boolean cleanup, final boolean releaseOnlyIfHostIsGone) { String commandName = VmWorkStop.class.getName(); Pair pendingWorkJob = retrievePendingWorkJob(null, vmUuid, null, commandName); @@ -5715,7 +5784,7 @@ public Outcome stopVmThroughJobQueue(final String vmUuid, final Pair newVmWorkJobAndInfo = createWorkJobAndWorkInfo(commandName, VmWorkJobVO.Step.Prepare, vmId); workJob = newVmWorkJobAndInfo.first(); - VmWorkStop workInfo = new VmWorkStop(newVmWorkJobAndInfo.second(), cleanup); + VmWorkStop workInfo = new VmWorkStop(newVmWorkJobAndInfo.second(), cleanup, releaseOnlyIfHostIsGone); setCmdInfoAndSubmitAsyncJob(workJob, workInfo, vmId); } @@ -6055,7 +6124,7 @@ private Pair orchestrateStop(final VmWorkStop work) thro throw new CloudRuntimeException(message); } - orchestrateStop(vm.getUuid(), work.isCleanup()); + orchestrateStop(vm.getUuid(), work.isCleanup(), work.isReleaseOnlyIfHostIsGone()); return new Pair<>(JobInfo.Status.SUCCEEDED, null); } diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java b/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java index 00ff7559cbaa..fb20b960b825 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java @@ -21,17 +21,31 @@ public class VmWorkStop extends VmWork { private final boolean cleanup; + // With cleanup, release the resources without the host's answer only when the host is gone. Absent from jobs + // queued before this field existed, which then read false and keep the previous behaviour. + private final boolean releaseOnlyIfHostIsGone; + public VmWorkStop(long userId, long accountId, long vmId, String handlerName, boolean cleanup) { super(userId, accountId, vmId, handlerName); this.cleanup = cleanup; + this.releaseOnlyIfHostIsGone = false; } public VmWorkStop(VmWork vmWork, boolean cleanup) { + this(vmWork, cleanup, false); + } + + public VmWorkStop(VmWork vmWork, boolean cleanup, boolean releaseOnlyIfHostIsGone) { super(vmWork); this.cleanup = cleanup; + this.releaseOnlyIfHostIsGone = releaseOnlyIfHostIsGone; } public boolean isCleanup() { return cleanup; } + + public boolean isReleaseOnlyIfHostIsGone() { + return releaseOnlyIfHostIsGone; + } } diff --git a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java index c9a404f9c89c..8506702384da 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java @@ -24,6 +24,7 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; @@ -81,6 +82,7 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.junit.function.ThrowingRunnable; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.InOrder; @@ -126,6 +128,7 @@ import com.cloud.exception.OperationTimedoutException; import com.cloud.host.Host; import com.cloud.host.HostVO; +import com.cloud.host.Status; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorGuruManager; @@ -164,6 +167,7 @@ import com.cloud.utils.Ternary; import com.cloud.utils.db.EntityManager; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.fsm.NoTransitionException; import com.cloud.utils.fsm.StateMachine2; import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.dao.NicDao; @@ -2049,4 +2053,269 @@ public void testUnmanageSuccessKvm() throws Exception { } } + private void withDestroyForcestop(final boolean value, final ThrowingRunnable body) throws Throwable { + final String previous = VirtualMachineManagerImpl.VmDestroyForcestop.defaultValue(); + try { + overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, String.valueOf(value)); + body.run(); + } finally { + overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, previous); + } + } + + @Test + public void isHostGoneForNoHostOrMissingRecord() { + assertTrue(virtualMachineManagerImpl.isHostGone(null)); + when(hostDaoMock.findById(hostMockId)).thenReturn(null); + assertTrue(virtualMachineManagerImpl.isHostGone(hostMockId)); + } + + @Test + public void isHostGoneOnlyForDownOrRemoved() { + for (Status status : Status.values()) { + when(hostMock.getStatus()).thenReturn(status); + assertEquals(status.toString(), status == Status.Down || status == Status.Removed, + virtualMachineManagerImpl.isHostGone(hostMockId)); + } + } + + @Test + public void mayReleaseWithoutHostConfirmationNeverForAnUnforcedStop() { + assertFalse(virtualMachineManagerImpl.mayReleaseWithoutHostConfirmation(vmInstanceMock, false, false)); + assertFalse(virtualMachineManagerImpl.mayReleaseWithoutHostConfirmation(vmInstanceMock, false, true)); + verify(hostDaoMock, never()).findById(anyLong()); + } + + @Test + public void mayReleaseWithoutHostConfirmationForAnExplicitForcedStopWhateverTheHost() { + assertTrue(virtualMachineManagerImpl.mayReleaseWithoutHostConfirmation(vmInstanceMock, true, false)); + verify(hostDaoMock, never()).findById(anyLong()); + } + + @Test + public void mayReleaseWithoutHostConfirmationForADestroyOnlyWhenTheHostIsGone() { + when(vmInstanceMock.getHostId()).thenReturn(hostMockId); + for (Status status : new Status[] {Status.Up, Status.Connecting, Status.Disconnected, Status.Alert, Status.Rebalancing}) { + when(hostMock.getStatus()).thenReturn(status); + assertFalse(status.toString(), virtualMachineManagerImpl.mayReleaseWithoutHostConfirmation(vmInstanceMock, true, true)); + } + for (Status status : new Status[] {Status.Down, Status.Removed}) { + when(hostMock.getStatus()).thenReturn(status); + assertTrue(status.toString(), virtualMachineManagerImpl.mayReleaseWithoutHostConfirmation(vmInstanceMock, true, true)); + } + } + + @Test + public void advanceStopForDestroyForcesOnlyWhenConfiguredAndThenOnlyIfTheHostIsGone() throws Throwable { + doNothing().when(virtualMachineManagerImpl).advanceStop(anyString(), anyBoolean(), anyBoolean()); + withDestroyForcestop(true, () -> virtualMachineManagerImpl.advanceStopForDestroy("vm-uuid")); + verify(virtualMachineManagerImpl).advanceStop("vm-uuid", true, true); + withDestroyForcestop(false, () -> virtualMachineManagerImpl.advanceStopForDestroy("vm-uuid")); + verify(virtualMachineManagerImpl).advanceStop("vm-uuid", false, false); + } + + @Test + public void explicitAdvanceStopDoesNotRequireTheHostToBeGone() throws Exception { + doNothing().when(virtualMachineManagerImpl).advanceStop(anyString(), anyBoolean(), anyBoolean()); + virtualMachineManagerImpl.advanceStop("vm-uuid", true); + verify(virtualMachineManagerImpl).advanceStop("vm-uuid", true, false); + } + + @Test + public void destroyStopsThroughTheDestroyStop() throws Exception { + when(vmInstanceDaoMock.findByUuid("vm-uuid")).thenReturn(vmInstanceMock); + when(vmInstanceMock.getState()).thenReturn(State.Running); + doThrow(new CloudRuntimeException("host may still be running it")).when(virtualMachineManagerImpl).advanceStopForDestroy("vm-uuid"); + + assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.destroy("vm-uuid", true)); + + verify(virtualMachineManagerImpl).advanceStopForDestroy("vm-uuid"); + verify(virtualMachineManagerImpl, never()).advanceStop(anyString(), anyBoolean()); + } + + @Test + public void advanceExpungeStopsThroughTheDestroyStop() throws Exception { + when(vmInstanceMock.getUuid()).thenReturn("vm-uuid"); + when(vmInstanceMock.getRemoved()).thenReturn(null); + when(vmInstanceMock.getHypervisorType()).thenReturn(HypervisorType.KVM); + doThrow(new CloudRuntimeException("host may still be running it")).when(virtualMachineManagerImpl).advanceStopForDestroy("vm-uuid"); + + assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceExpunge(vmInstanceMock)); + + verify(virtualMachineManagerImpl).advanceStopForDestroy("vm-uuid"); + verify(virtualMachineManagerImpl, never()).advanceStop(anyString(), anyBoolean()); + } + + /** + * A Running instance on a host whose StopCommand cannot be delivered. State changes are applied to the instance + * so advanceStop() sees Stopping after StopRequested, as it would with the real state machine. + */ + private VMInstanceVO runningVmWhoseStopCannotBeDelivered(final Status hostStatus) throws Exception { + VMInstanceVO vm = new VMInstanceVO(); + ReflectionTestUtils.setField(vm, "id", 1L); + ReflectionTestUtils.setField(vm, "uuid", "vm-uuid"); + ReflectionTestUtils.setField(vm, "instanceName", "i-2-1-VM"); + ReflectionTestUtils.setField(vm, "hostId", hostMockId); + ReflectionTestUtils.setField(vm, "type", VirtualMachine.Type.User); + ReflectionTestUtils.setField(vm, "hypervisorType", HypervisorType.KVM); + ReflectionTestUtils.setField(vm, "state", State.Running); + when(hostMock.getStatus()).thenReturn(hostStatus); + doReturn(guru).when(virtualMachineManagerImpl).getVmGuru(vm); + Mockito.doAnswer(invocation -> { + VirtualMachine.Event event = invocation.getArgument(1); + ReflectionTestUtils.setField(vm, "state", event == VirtualMachine.Event.StopRequested ? State.Stopping : State.Running); + return true; + }).when(virtualMachineManagerImpl).stateTransitTo(eq(vm), any(VirtualMachine.Event.class), any()); + when(agentManagerMock.send(anyLong(), any(StopCommand.class))).thenThrow(new AgentUnavailableException("Disconnected", hostMockId)); + return vm; + } + + @Test + public void destroyStopDoesNotReleaseWhenTheHostMayStillBeRunningTheInstance() throws Exception { + VMInstanceVO vm = runningVmWhoseStopCannotBeDelivered(Status.Disconnected); + + assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true)); + + verify(virtualMachineManagerImpl, never()).releaseVmResources(any(), anyBoolean()); + verify(virtualMachineManagerImpl).stateTransitTo(vm, VirtualMachine.Event.OperationFailed, hostMockId); + assertEquals(State.Running, vm.getState()); + } + + @Test + public void destroyStopReleasesWhenTheHostIsDown() throws Exception { + VMInstanceVO vm = runningVmWhoseStopCannotBeDelivered(Status.Down); + doThrow(new CloudRuntimeException("released")).when(virtualMachineManagerImpl).releaseVmResources(any(), eq(true)); + + CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true)); + + assertEquals("released", e.getMessage()); + } + + @Test + public void explicitForcedStopStillReleasesWhenTheHostIsDisconnected() throws Exception { + VMInstanceVO vm = runningVmWhoseStopCannotBeDelivered(Status.Disconnected); + doThrow(new CloudRuntimeException("released")).when(virtualMachineManagerImpl).releaseVmResources(any(), eq(true)); + + CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, false)); + + assertEquals("released", e.getMessage()); + } + + /** + * An instance in Starting cannot take StopRequested, so advanceStop() goes through the forced cleanup, which + * releases the resources whatever the host answers. + */ + private VMInstanceVO startingVmOnHost(final Status hostStatus) throws Exception { + VMInstanceVO vm = new VMInstanceVO(); + ReflectionTestUtils.setField(vm, "id", 1L); + ReflectionTestUtils.setField(vm, "uuid", "vm-uuid"); + ReflectionTestUtils.setField(vm, "instanceName", "i-2-1-VM"); + ReflectionTestUtils.setField(vm, "hostId", hostMockId); + ReflectionTestUtils.setField(vm, "type", VirtualMachine.Type.User); + ReflectionTestUtils.setField(vm, "hypervisorType", HypervisorType.KVM); + ReflectionTestUtils.setField(vm, "state", State.Starting); + when(hostMock.getStatus()).thenReturn(hostStatus); + doReturn(guru).when(virtualMachineManagerImpl).getVmGuru(vm); + doThrow(new NoTransitionException("no StopRequested from Starting")).when(virtualMachineManagerImpl) + .stateTransitTo(vm, VirtualMachine.Event.StopRequested, hostMockId); + doThrow(new CloudRuntimeException("cleaned up")).when(virtualMachineManagerImpl) + .cleanup(any(), any(), any(), any(), eq(true)); + return vm; + } + + @Test + public void destroyStopOfAStalledInstanceFailsWhenTheHostDoesNotConfirm() throws Exception { + VMInstanceVO vm = startingVmOnHost(Status.Disconnected); + doReturn(new Pair<>(false, "host did not answer")).when(virtualMachineManagerImpl).sendStop(any(), any(), eq(false), eq(false)); + + CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true)); + + assertTrue(e.getMessage().contains("host did not answer")); + verify(virtualMachineManagerImpl, never()).cleanup(any(), any(), any(), any(), anyBoolean()); + } + + @Test + public void destroyStopOfAStalledInstanceCleansUpOnceAnUpHostConfirms() throws Exception { + VMInstanceVO vm = startingVmOnHost(Status.Up); + doReturn(new Pair<>(true, null)).when(virtualMachineManagerImpl).sendStop(any(), any(), eq(false), eq(false)); + + CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true)); + + assertEquals("cleaned up", e.getMessage()); + } + + @Test + public void destroyStopOfAStalledInstanceCleansUpWithoutAskingAHostThatIsDown() throws Exception { + VMInstanceVO vm = startingVmOnHost(Status.Down); + + CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true)); + + assertEquals("cleaned up", e.getMessage()); + verify(virtualMachineManagerImpl, never()).sendStop(any(), any(), anyBoolean(), anyBoolean()); + } + + @Test + public void advanceStopQueuesTheFlagOnTheStopJob() throws Exception { + final AsyncJobExecutionContext jobContext = mock(AsyncJobExecutionContext.class); + final VmWorkJobVO workJob = mock(VmWorkJobVO.class); + final String commandName = VmWorkStop.class.getName(); + doReturn(new Pair(null, 1L)).when(virtualMachineManagerImpl) + .retrievePendingWorkJob(Mockito.isNull(), eq("vm-uuid"), Mockito.isNull(), eq(commandName)); + doReturn(new Pair(workJob, new VmWork(1L, 1L, 1L, "VirtualMachineManagerImpl"))).when(virtualMachineManagerImpl) + .createWorkJobAndWorkInfo(commandName, VmWorkJobVO.Step.Prepare, 1L); + final ArgumentCaptor queued = ArgumentCaptor.forClass(VmWork.class); + doThrow(new CloudRuntimeException("queued")).when(virtualMachineManagerImpl).setCmdInfoAndSubmitAsyncJob(eq(workJob), queued.capture(), eq(1L)); + + try (MockedStatic ignored = Mockito.mockStatic(AsyncJobExecutionContext.class)) { + ignored.when(AsyncJobExecutionContext::getCurrentExecutionContext).thenReturn(jobContext); + when(jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)).thenReturn(false); + + assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop("vm-uuid", true, true)); + } + + final VmWorkStop work = (VmWorkStop) queued.getValue(); + assertTrue(work.isCleanup()); + assertTrue(work.isReleaseOnlyIfHostIsGone()); + } + + @Test + public void advanceStopInsideAVmWorkJobPassesTheFlagOn() throws Exception { + final AsyncJobExecutionContext jobContext = mock(AsyncJobExecutionContext.class); + when(vmInstanceDaoMock.findByUuid("vm-uuid")).thenReturn(vmInstanceMock); + doNothing().when(virtualMachineManagerImpl).advanceStop(any(VMInstanceVO.class), anyBoolean(), anyBoolean()); + + try (MockedStatic ignored = Mockito.mockStatic(AsyncJobExecutionContext.class)) { + ignored.when(AsyncJobExecutionContext::getCurrentExecutionContext).thenReturn(jobContext); + when(jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)).thenReturn(true); + + virtualMachineManagerImpl.advanceStop("vm-uuid", true, true); + } + + verify(virtualMachineManagerImpl).advanceStop(vmInstanceMock, true, true); + } + + @Test + public void theStopJobHandlerPassesTheFlagOn() throws Exception { + when(_entityMgr.findById(VMInstanceVO.class, 1L)).thenReturn(vmInstanceMock); + when(vmInstanceMock.getUuid()).thenReturn("vm-uuid"); + when(vmInstanceDaoMock.findByUuid("vm-uuid")).thenReturn(vmInstanceMock); + doNothing().when(virtualMachineManagerImpl).advanceStop(any(VMInstanceVO.class), anyBoolean(), anyBoolean()); + + ReflectionTestUtils.invokeMethod(virtualMachineManagerImpl, "orchestrateStop", + new VmWorkStop(new VmWork(1L, 1L, 1L, "VirtualMachineManagerImpl"), true, true)); + + verify(virtualMachineManagerImpl).advanceStop(vmInstanceMock, true, true); + } + + @Test + public void theFlagSurvivesJobSerialization() { + final VmWork base = new VmWork(1L, 1L, 1L, "VirtualMachineManagerImpl"); + VmWorkStop work = VmWorkSerializer.deserialize(VmWorkStop.class, VmWorkSerializer.serialize(new VmWorkStop(base, true, true))); + assertTrue(work.isCleanup()); + assertTrue(work.isReleaseOnlyIfHostIsGone()); + + work = VmWorkSerializer.deserialize(VmWorkStop.class, VmWorkSerializer.serialize(new VmWorkStop(base, true))); + assertTrue(work.isCleanup()); + assertFalse(work.isReleaseOnlyIfHostIsGone()); + } } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index b4c23beaef2e..04ba81ef156e 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -911,6 +911,16 @@ protected void cleanupPluginsResourcesIfNeeded(Account account) { } } + /** + * @return true when the instance still has a host and is not stopped, so its domain may be running there. + */ + protected boolean isStillOnItsHost(long vmId) { + UserVmVO vm = _userVmDao.findById(vmId); + return vm != null && vm.getHostId() != null && vm.getState() != VirtualMachine.State.Stopped + && vm.getState() != VirtualMachine.State.Destroyed && vm.getState() != VirtualMachine.State.Expunging + && vm.getState() != VirtualMachine.State.Error; + } + protected boolean cleanupAccount(AccountVO account, long callerUserId, Account caller) { long accountId = account.getId(); boolean accountCleanupNeeded = false; @@ -1000,12 +1010,21 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c logger.debug("Expunging # of Instances (Account={}): {}", account, vms.size()); } + boolean instanceLeftOnItsHost = false; for (UserVmVO vm : vms) { if (vm.getState() != VirtualMachine.State.Destroyed && vm.getState() != VirtualMachine.State.Expunging) { try { _vmMgr.destroyVm(vm.getId(), false); } catch (Exception e) { logger.warn("Failed destroying instance {} as part of account deletion.", vm, e); + if (isStillOnItsHost(vm.getId())) { + // Expunging releases the instance's addresses before it stops it, so a stop that fails + // there leaves a running domain without them. Leave it for the next cleanup of this account. + logger.warn("Not expunging instance {}, it may still be running on its host.", vm); + accountCleanupNeeded = true; + instanceLeftOnItsHost = true; + continue; + } } } // no need to catch exception at this place as expunging vm @@ -1016,6 +1035,13 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c } } + if (instanceLeftOnItsHost) { + // The rest of the cleanup would take away its security groups, networks and resource counts while it + // may still be running. Finish on a later pass, once it is gone. + logger.warn("Deferring the rest of the cleanup of account {}: it has instances that may still be running.", account); + return true; + } + // Mark the account's volumes as destroyed List volumes = _volumeDao.findDetachedByAccount(accountId); for (VolumeVO volume : volumes) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 7636ef6b152c..a7be8c8b1238 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -706,6 +706,7 @@ public void setKubernetesServiceHelpers(final List kube private static final ConfigKey VmwareAdditionalConfigAllowList = new ConfigKey<>(String.class, "allow.additional.vm.configuration.list.vmware", "Advanced", "", "Comma separated list of allowed additional configuration options.", true, ConfigKey.Scope.Global, null, null, EnableAdditionalVmConfig.key(), null, null, ConfigKey.Kind.CSV, null); + // Registered by VirtualMachineManagerImpl; both read the same setting. private static final ConfigKey VmDestroyForcestop = new ConfigKey<>("Advanced", Boolean.class, "vm.destroy.forcestop", "false", "On destroy, force-stop takes this value ", true); @@ -3576,7 +3577,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C backupManager.checkAndRemoveBackupOfferingBeforeExpunge(vm); } - stopVirtualMachine(vmId, VmDestroyForcestop.value()); + stopVirtualMachineForDestroy(ctx.getCallingAccount(), vm); // Detach all data disks from VM List dataVols = _volsDao.findByInstanceAndType(vmId, Volume.Type.DATADISK); @@ -5657,13 +5658,29 @@ protected void updateVncPasswordIfItHasChanged(String originalVncPassword, Strin public void finalizeExpunge(VirtualMachine vm) { } - private void checkForceStopVmPermission(Account callingAccount) { + protected void checkForceStopVmPermission(Account callingAccount) { if (!AllowUserForceStopVm.valueIn(callingAccount.getId())) { logger.error("Parameter [{}] can only be passed by Admin accounts or when the allow.user.force.stop.vm config is true for the account.", ApiConstants.FORCED); throw new PermissionDeniedException("Account does not have the permission to force stop the vm."); } } + /** + * Stop an instance ahead of destroying it. See VirtualMachineManager.advanceStopForDestroy(): with + * vm.destroy.forcestop the stop is forced, but it does not release the instance's resources while its host may + * still be running it. + */ + protected void stopVirtualMachineForDestroy(Account caller, UserVmVO vm) throws ResourceUnavailableException, ConcurrentOperationException { + if (VmDestroyForcestop.value()) { + checkForceStopVmPermission(caller); + } + try { + _itMgr.advanceStopForDestroy(vm.getUuid()); + } catch (OperationTimedoutException e) { + throw new CloudRuntimeException("Unable to contact the agent to stop the virtual machine " + vm, e); + } + } + @Override @ActionEvent(eventType = EventTypes.EVENT_VM_STOP, eventDescription = "stopping Vm", async = true) public UserVm stopVirtualMachine(long vmId, boolean forced) throws ConcurrentOperationException { diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 119704cc3971..a76f73521bac 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -75,6 +75,7 @@ import com.cloud.vm.UserVmManagerImpl; import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; import com.cloud.vm.snapshot.VMSnapshotVO; public class AccountManagerImplTest extends AccountManagentImplTestBase { @@ -1731,4 +1732,49 @@ public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws accountManagerImpl.checkRoleEscalation(caller, requested); } + + @Test + public void deleteUserAccountDoesNotExpungeAnInstanceStillOnItsHost() throws Exception { + AccountVO account = new AccountVO(); + account.setId(42L); + DomainVO domain = new DomainVO(); + UserVmVO vm = Mockito.mock(UserVmVO.class); + Mockito.when(vm.getId()).thenReturn(7L); + Mockito.when(vm.getState()).thenReturn(VirtualMachine.State.Running); + Mockito.when(_accountDao.findById(42L)).thenReturn(account); + Mockito.doNothing().when(accountManagerImpl).checkAccess(Mockito.any(Account.class), Mockito.isNull(), Mockito.anyBoolean(), Mockito.any(Account.class)); + Mockito.when(_accountDao.remove(42L)).thenReturn(true); + Mockito.when(_userVmDao.listByAccountId(42L)).thenReturn(Arrays.asList(vm)); + Mockito.when(_vmMgr.destroyVm(7L, false)).thenThrow(new CloudRuntimeException("host is Disconnected")); + Mockito.doReturn(true).when(accountManagerImpl).isStillOnItsHost(7L); + Mockito.lenient().when(_domainMgr.getDomain(Mockito.anyLong())).thenReturn(domain); + Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), Mockito.any(Domain.class))).thenReturn(true); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); + + Assert.assertTrue(accountManagerImpl.deleteUserAccount(42L)); + + Mockito.verify(_vmMgr, Mockito.never()).expunge(vm); + // the rest of the account is left alone while the instance may still be running + Mockito.verify(_volumeDao, Mockito.never()).findDetachedByAccount(42L); + Mockito.verify(_configMgr, Mockito.never()).releaseAccountSpecificVirtualRanges(account); + Mockito.verify(accountManagerImpl, Mockito.never()).deleteWebhooksForAccount(42L); + Mockito.verify(_accountDao, Mockito.atLeastOnce()).markForCleanup(Mockito.eq(42L)); + } + + @Test + public void isStillOnItsHostOnlyForAnInstanceWithAHostThatIsNotStopped() { + UserVmVO vm = Mockito.mock(UserVmVO.class); + Mockito.when(_userVmDao.findById(7L)).thenReturn(vm); + Mockito.when(vm.getHostId()).thenReturn(3L); + for (VirtualMachine.State state : VirtualMachine.State.values()) { + Mockito.when(vm.getState()).thenReturn(state); + boolean stopped = state == VirtualMachine.State.Stopped || state == VirtualMachine.State.Destroyed + || state == VirtualMachine.State.Expunging || state == VirtualMachine.State.Error; + Assert.assertEquals(state.toString(), !stopped, accountManagerImpl.isStillOnItsHost(7L)); + } + Mockito.when(vm.getHostId()).thenReturn(null); + Assert.assertFalse(accountManagerImpl.isStillOnItsHost(7L)); + Mockito.when(_userVmDao.findById(7L)).thenReturn(null); + Assert.assertFalse(accountManagerImpl.isStillOnItsHost(7L)); + } } diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index cc2dc1cc9efa..dbe1ea0ff19a 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -131,6 +131,7 @@ import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InsufficientServerCapacityException; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.OperationTimedoutException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; import com.cloud.exception.ResourceUnavailableException; @@ -3831,7 +3832,7 @@ public void testDestroyVm() throws ResourceUnavailableException { when(volumeApiService.destroyVolume(volumeId, CallContext.current().getCallingAccount(), expunge, false)).thenReturn(vol); - doReturn(vm).when(userVmManagerImpl).stopVirtualMachine(anyLong(), anyBoolean()); + doNothing().when(userVmManagerImpl).stopVirtualMachineForDestroy(any(), any()); doReturn(vm).when(userVmManagerImpl).destroyVm(vmId, expunge); doReturn(true).when(userVmManagerImpl).expunge(vm); @@ -3841,7 +3842,8 @@ public void testDestroyVm() throws ResourceUnavailableException { assertNotNull(result); assertEquals(vm, result); - Mockito.verify(userVmManagerImpl).stopVirtualMachine(vmId, false); + Mockito.verify(userVmManagerImpl).stopVirtualMachineForDestroy(any(), eq(vm)); + Mockito.verify(userVmManagerImpl, never()).stopVirtualMachine(anyLong(), anyBoolean()); Mockito.verify(backupManager).checkAndRemoveBackupOfferingBeforeExpunge(vm); } } @@ -4610,4 +4612,51 @@ public void getRootVolumeSizeForVmRestoreAppliesMaxIopsToTheMaxIopsField() { Assert.assertEquals(Long.valueOf(500L), volume.getMinIops()); Assert.assertEquals(Long.valueOf(2000L), volume.getMaxIops()); } + + @Test + public void stopVirtualMachineForDestroyUsesTheDestroyStop() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getUuid()).thenReturn("vm-uuid"); + + userVmManagerImpl.stopVirtualMachineForDestroy(callerAccount, vm); + + Mockito.verify(virtualMachineManager).advanceStopForDestroy("vm-uuid"); + Mockito.verify(virtualMachineManager, never()).advanceStop(anyString(), anyBoolean()); + } + + @Test(expected = CloudRuntimeException.class) + public void stopVirtualMachineForDestroyFailsWhenTheStopTimesOut() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getUuid()).thenReturn("vm-uuid"); + Mockito.doThrow(new OperationTimedoutException(null, 1L, 1L, 1, false)).when(virtualMachineManager).advanceStopForDestroy("vm-uuid"); + + userVmManagerImpl.stopVirtualMachineForDestroy(callerAccount, vm); + } + + private void setDestroyForcestop(String value) throws Exception { + java.lang.reflect.Field key = UserVmManagerImpl.class.getDeclaredField("VmDestroyForcestop"); + key.setAccessible(true); + java.lang.reflect.Field defaultValue = ConfigKey.class.getDeclaredField("_defaultValue"); + defaultValue.setAccessible(true); + defaultValue.set(key.get(null), value); + } + + @Test + public void stopVirtualMachineForDestroyChecksTheForceStopPermissionOnlyWhenForced() throws Exception { + UserVmVO vm = mock(UserVmVO.class); + when(vm.getUuid()).thenReturn("vm-uuid"); + try { + setDestroyForcestop("true"); + doNothing().when(userVmManagerImpl).checkForceStopVmPermission(callerAccount); + userVmManagerImpl.stopVirtualMachineForDestroy(callerAccount, vm); + Mockito.verify(userVmManagerImpl).checkForceStopVmPermission(callerAccount); + + Mockito.clearInvocations(userVmManagerImpl); + setDestroyForcestop("false"); + userVmManagerImpl.stopVirtualMachineForDestroy(callerAccount, vm); + Mockito.verify(userVmManagerImpl, never()).checkForceStopVmPermission(any()); + } finally { + setDestroyForcestop("false"); + } + } }