From 568f604bf64240ab0f946bc98b3b4876fbb8be51 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 23 Sep 2026 18:22:58 +0000 Subject: [PATCH 1/4] engine: do not force the destroy stop while the host may reconnect With vm.destroy.forcestop=true, destroying an instance whose host is briefly Disconnected releases its NICs, addresses and volumes without stopping it. The domain keeps running unmanaged and its address is handed to the next instance. A forced stop treats an unreachable host as proof the instance is stopped. That holds for a host that is gone, not for one that is Connecting, Disconnected, Alert or Rebalancing, as happens on every agent or management server restart. Add VirtualMachineManager.shouldForceStopOnDestroy(): the value of vm.destroy.forcestop, except while the host is in one of those states. The stop then fails, the instance stays Running and the destroy can be retried. Use it on all three destroy paths, and drop the unregistered duplicate of the ConfigKey in UserVmManagerImpl. Fixes #14232 Signed-off-by: Brad House --- .../com/cloud/vm/VirtualMachineManager.java | 6 +++ .../cloud/vm/VirtualMachineManagerImpl.java | 43 +++++++++++++++-- .../vm/VirtualMachineManagerImplTest.java | 48 +++++++++++++++++++ .../java/com/cloud/vm/UserVmManagerImpl.java | 5 +- 4 files changed, 95 insertions(+), 7 deletions(-) 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..734572343a43 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,12 @@ void orchestrateStart(String vmUuid, Map pa void destroy(String vmUuid, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException; + /** + * @return whether the stop that precedes destroying this instance should be forced: the value of + * vm.destroy.forcestop, except while the instance's host is in a state it may come back from. + */ + boolean shouldForceStopOnDestroy(VirtualMachine vm); + 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..68e122876d1a 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -31,6 +31,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.Date; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -481,7 +482,16 @@ 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. The stop is not forced while the instance's host is Connecting, " + + "Disconnected, Alert or Rebalancing: the destroy fails instead and can be retried once the host is back.", true); + + /** + * Host states from which the host may come back with its instances still running. A forced stop releases an + * instance's addresses and storage when the host cannot be reached, which is only safe when the host is known to + * be gone (Down, Removed, Error) or is Up and answers. + */ + protected static final Set HOST_STATES_THAT_MAY_RECONNECT = EnumSet.of(Status.Connecting, Status.Disconnected, + Status.Alert, Status.Rebalancing); static final ConfigKey ClusterDeltaSyncInterval = new ConfigKey("Advanced", Integer.class, "sync.interval", "60", "Cluster Delta sync interval in seconds", false); @@ -693,7 +703,7 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti _userVmDao.saveDetails(userVM); } - advanceStop(vm.getUuid(), VmDestroyForcestop.value()); + advanceStop(vm.getUuid(), shouldForceStopOnDestroy(vm)); vm = _vmDao.findByUuid(vm.getUuid()); try { @@ -2689,6 +2699,33 @@ public boolean stateTransitTo(final VirtualMachine vm1, final VirtualMachine.Eve return _stateMachine.transitTo(vm, e, new Pair<>(vm.getHostId(), hostId), _vmDao); } + /** + * vm.destroy.forcestop makes the stop that precedes a destroy a forced one, and a forced stop releases the + * instance's NICs, addresses and storage even when the host cannot be reached. That is right for a host that is + * gone, and wrong for one that is briefly disconnected, for example while its agent or the management server + * restarts: the domain keeps running, its address is handed to another instance and its volume is stranded. + * + * So the stop is not forced while the host is in a state it may come back from. The stop then fails, the instance + * stays Running and the destroy can be retried once the host has reconnected, or is Down and can be forced. + */ + @Override + public boolean shouldForceStopOnDestroy(final VirtualMachine vm) { + if (!VmDestroyForcestop.value()) { + return false; + } + final Long hostId = vm.getHostId(); + if (hostId == null) { + return true; + } + final HostVO host = _hostDao.findById(hostId); + if (host == null || !HOST_STATES_THAT_MAY_RECONNECT.contains(host.getStatus())) { + return true; + } + logger.warn("Not forcing the stop of {} for destroy: its host {} is {} and may still be running it. " + + "The destroy will fail if the host cannot be reached; retry once the host is Up or Down.", vm, host, host.getStatus()); + return false; + } + @Override public void destroy(final String vmUuid, final boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { VMInstanceVO vm = _vmDao.findByUuid(vmUuid); @@ -2699,7 +2736,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()); + advanceStop(vmUuid, shouldForceStopOnDestroy(vm)); deleteVMSnapshots(vm, expunge); 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..f770bfd3d377 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java @@ -126,6 +126,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; @@ -2049,4 +2050,51 @@ public void testUnmanageSuccessKvm() throws Exception { } } + private void assertForceStopOnDestroy(final boolean configured, final Long hostId, final Status hostStatus, final boolean expected) + throws NoSuchFieldException, IllegalAccessException { + final String previous = VirtualMachineManagerImpl.VmDestroyForcestop.defaultValue(); + try { + overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, String.valueOf(configured)); + when(vmInstanceMock.getHostId()).thenReturn(hostId); + when(hostMock.getStatus()).thenReturn(hostStatus); + assertEquals(expected, virtualMachineManagerImpl.shouldForceStopOnDestroy(vmInstanceMock)); + } finally { + overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, previous); + } + } + + @Test + public void shouldForceStopOnDestroyIsFalseWhenNotConfigured() throws Exception { + assertForceStopOnDestroy(false, hostMockId, Status.Up, false); + } + + @Test + public void shouldForceStopOnDestroyIsTrueWhenHostIsUp() throws Exception { + assertForceStopOnDestroy(true, hostMockId, Status.Up, true); + } + + @Test + public void shouldForceStopOnDestroyIsTrueWhenHostIsKnownToBeGone() throws Exception { + for (Status status : new Status[] {Status.Down, Status.Removed, Status.Error}) { + assertForceStopOnDestroy(true, hostMockId, status, true); + } + } + + @Test + public void shouldForceStopOnDestroyIsFalseWhileHostMayReconnect() throws Exception { + for (Status status : new Status[] {Status.Connecting, Status.Disconnected, Status.Alert, Status.Rebalancing}) { + assertForceStopOnDestroy(true, hostMockId, status, false); + } + } + + @Test + public void shouldForceStopOnDestroyIsTrueWithoutHost() throws Exception { + assertForceStopOnDestroy(true, null, Status.Disconnected, true); + } + + @Test + public void shouldForceStopOnDestroyIsTrueWhenHostRecordIsGone() throws Exception { + when(hostDaoMock.findById(hostMockId)).thenReturn(null); + assertForceStopOnDestroy(true, hostMockId, Status.Disconnected, true); + } } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 7636ef6b152c..bfa652469b6e 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -706,9 +706,6 @@ 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); - private static final ConfigKey VmDestroyForcestop = new ConfigKey<>("Advanced", Boolean.class, "vm.destroy.forcestop", "false", - "On destroy, force-stop takes this value ", true); - @Override public UserVmVO getVirtualMachine(long vmId) { return _vmDao.findById(vmId); @@ -3576,7 +3573,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C backupManager.checkAndRemoveBackupOfferingBeforeExpunge(vm); } - stopVirtualMachine(vmId, VmDestroyForcestop.value()); + stopVirtualMachine(vmId, _itMgr.shouldForceStopOnDestroy(vm)); // Detach all data disks from VM List dataVols = _volsDao.findByInstanceAndType(vmId, Volume.Type.DATADISK); From 9dfdaefc0eefce3fb2982321909c116b010eb474 Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 23 Sep 2026 18:49:06 +0000 Subject: [PATCH 2/4] engine: decide whether to release when the destroy stop fails Review of the previous commit: reading the host status before the stop misses hosts that are unreachable while still Up, such as those of a crashed management server until its peers mark them Disconnected, or an agent whose disconnect investigation is inconclusive. The decision was also baked into a job that may run much later. Decide where the StopCommand goes unanswered instead. A destroy now stops through VirtualMachineManager.advanceStopForDestroy(), which forces the stop according to vm.destroy.forcestop but, carried on VmWorkStop, releases the resources without the host's answer only when the host is gone: no host, no host record, Down or Removed. Otherwise it fails as an unforced stop does. The forced cleanup of instances that cannot enter Stopping gets the same check before it releases anything. The stop stays forced, so a host that answers still gets a hard stop. An explicit forced stop is unchanged: that is a caller stating the instance is to be treated as stopped, and it remains the way out for an instance on a host that stays Alert. Restore the vm.destroy.forcestop ConfigKey in UserVmManagerImpl, still needed for the force-stop permission check. Add tests through advanceStop() and for each destroy path. Signed-off-by: Brad House --- .../com/cloud/vm/VirtualMachineManager.java | 7 +- .../cloud/vm/VirtualMachineManagerImpl.java | 119 ++++++++------ .../main/java/com/cloud/vm/VmWorkStop.java | 13 ++ .../vm/VirtualMachineManagerImplTest.java | 146 +++++++++++++++--- .../java/com/cloud/vm/UserVmManagerImpl.java | 21 ++- .../com/cloud/vm/UserVmManagerImplTest.java | 26 +++- 6 files changed, 255 insertions(+), 77 deletions(-) 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 734572343a43..a48241273816 100644 --- a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java +++ b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java @@ -171,10 +171,11 @@ void orchestrateStart(String vmUuid, Map pa void destroy(String vmUuid, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException; /** - * @return whether the stop that precedes destroying this instance should be forced: the value of - * vm.destroy.forcestop, except while the instance's host is in a state it may come back from. + * Stop an instance ahead of destroying it. The stop is forced according to vm.destroy.forcestop, but when the + * host does not answer, the instance's resources are only released if the host is Down or Removed. Otherwise the + * stop fails as an unforced one does and the instance stays Running. */ - boolean shouldForceStopOnDestroy(VirtualMachine vm); + void advanceStopForDestroy(String vmUuid) throws ResourceUnavailableException, OperationTimedoutException, ConcurrentOperationException; void migrateAway(String vmUuid, long hostId) throws InsufficientServerCapacityException; 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 68e122876d1a..2edec1ebb2df 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -31,7 +31,6 @@ import java.util.Collections; import java.util.Comparator; import java.util.Date; -import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -482,16 +481,9 @@ 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. The stop is not forced while the instance's host is Connecting, " + - "Disconnected, Alert or Rebalancing: the destroy fails instead and can be retried once the host is back.", 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); - /** - * Host states from which the host may come back with its instances still running. A forced stop releases an - * instance's addresses and storage when the host cannot be reached, which is only safe when the host is known to - * be gone (Down, Removed, Error) or is Up and answers. - */ - protected static final Set HOST_STATES_THAT_MAY_RECONNECT = EnumSet.of(Status.Connecting, Status.Disconnected, - Status.Alert, Status.Rebalancing); static final ConfigKey ClusterDeltaSyncInterval = new ConfigKey("Advanced", Integer.class, "sync.interval", "60", "Cluster Delta sync interval in seconds", false); @@ -703,7 +695,7 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti _userVmDao.saveDetails(userVM); } - advanceStop(vm.getUuid(), shouldForceStopOnDestroy(vm)); + advanceStopForDestroy(vm.getUuid()); vm = _vmDao.findByUuid(vm.getUuid()); try { @@ -2374,6 +2366,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)) { @@ -2382,7 +2395,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()); @@ -2390,7 +2403,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"); @@ -2402,10 +2415,40 @@ 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. A host that is Up, Connecting, Disconnected, Alert or Rebalancing may + * still be running it, whether or not it answers right now. + */ + protected boolean isHostGone(final Long hostId) { + if (hostId == null) { + return true; + } + final HostVO host = _hostDao.findById(hostId); + 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 || isHostGone(vm.getHostId())) { + return true; + } + final HostVO host = _hostDao.findById(vm.getHostId()); + logger.warn("Not releasing the resources of {}: its host {} is {} and may still be running it. Retry once the host is Up, " + + "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) { @@ -2470,8 +2513,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); @@ -2521,7 +2564,8 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl throw new ConcurrentOperationException(String.format("%s is being operated on.", vm.toString())); } } catch (final NoTransitionException e1) { - if (!cleanUpEvenIfUnableToStop) { + // cleanup() releases the resources whether or not the host answers, so check before it runs + if (!mayReleaseWithoutHostConfirmation(vm, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone)) { throw new CloudRuntimeException("We cannot stop " + vm + " when it is in state " + vm.getState()); } final boolean doCleanup = true; @@ -2598,7 +2642,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,33 +2743,6 @@ public boolean stateTransitTo(final VirtualMachine vm1, final VirtualMachine.Eve return _stateMachine.transitTo(vm, e, new Pair<>(vm.getHostId(), hostId), _vmDao); } - /** - * vm.destroy.forcestop makes the stop that precedes a destroy a forced one, and a forced stop releases the - * instance's NICs, addresses and storage even when the host cannot be reached. That is right for a host that is - * gone, and wrong for one that is briefly disconnected, for example while its agent or the management server - * restarts: the domain keeps running, its address is handed to another instance and its volume is stranded. - * - * So the stop is not forced while the host is in a state it may come back from. The stop then fails, the instance - * stays Running and the destroy can be retried once the host has reconnected, or is Down and can be forced. - */ - @Override - public boolean shouldForceStopOnDestroy(final VirtualMachine vm) { - if (!VmDestroyForcestop.value()) { - return false; - } - final Long hostId = vm.getHostId(); - if (hostId == null) { - return true; - } - final HostVO host = _hostDao.findById(hostId); - if (host == null || !HOST_STATES_THAT_MAY_RECONNECT.contains(host.getStatus())) { - return true; - } - logger.warn("Not forcing the stop of {} for destroy: its host {} is {} and may still be running it. " - + "The destroy will fail if the host cannot be reached; retry once the host is Up or Down.", vm, host, host.getStatus()); - return false; - } - @Override public void destroy(final String vmUuid, final boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException { VMInstanceVO vm = _vmDao.findByUuid(vmUuid); @@ -2736,7 +2753,7 @@ public void destroy(final String vmUuid, final boolean expunge) throws AgentUnav logger.debug("Destroying vm {}, expunge flag {}", vm, (expunge ? "on" : "off")); - advanceStop(vmUuid, shouldForceStopOnDestroy(vm)); + advanceStopForDestroy(vmUuid); deleteVMSnapshots(vm, expunge); @@ -5742,6 +5759,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); @@ -5752,7 +5773,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); } @@ -6092,7 +6113,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..7d19d29e9693 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java @@ -21,6 +21,10 @@ 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 boolean releaseOnlyIfHostIsGone; + public VmWorkStop(long userId, long accountId, long vmId, String handlerName, boolean cleanup) { super(userId, accountId, vmId, handlerName); this.cleanup = cleanup; @@ -31,7 +35,16 @@ public VmWorkStop(VmWork vmWork, boolean cleanup) { this.cleanup = cleanup; } + public VmWorkStop(VmWork vmWork, boolean cleanup, boolean releaseOnlyIfHostIsGone) { + this(vmWork, 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 f770bfd3d377..c3b328f34b49 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; @@ -2050,51 +2052,151 @@ public void testUnmanageSuccessKvm() throws Exception { } } - private void assertForceStopOnDestroy(final boolean configured, final Long hostId, final Status hostStatus, final boolean expected) - throws NoSuchFieldException, IllegalAccessException { + private void withDestroyForcestop(final boolean value, final ThrowingRunnable body) throws Throwable { final String previous = VirtualMachineManagerImpl.VmDestroyForcestop.defaultValue(); try { - overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, String.valueOf(configured)); - when(vmInstanceMock.getHostId()).thenReturn(hostId); - when(hostMock.getStatus()).thenReturn(hostStatus); - assertEquals(expected, virtualMachineManagerImpl.shouldForceStopOnDestroy(vmInstanceMock)); + overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, String.valueOf(value)); + body.run(); } finally { overrideDefaultConfigValue(VirtualMachineManagerImpl.VmDestroyForcestop, previous); } } @Test - public void shouldForceStopOnDestroyIsFalseWhenNotConfigured() throws Exception { - assertForceStopOnDestroy(false, hostMockId, Status.Up, false); + public void isHostGoneForNoHostOrMissingRecord() { + assertTrue(virtualMachineManagerImpl.isHostGone(null)); + when(hostDaoMock.findById(hostMockId)).thenReturn(null); + assertTrue(virtualMachineManagerImpl.isHostGone(hostMockId)); } @Test - public void shouldForceStopOnDestroyIsTrueWhenHostIsUp() throws Exception { - assertForceStopOnDestroy(true, hostMockId, Status.Up, true); + 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 shouldForceStopOnDestroyIsTrueWhenHostIsKnownToBeGone() throws Exception { - for (Status status : new Status[] {Status.Down, Status.Removed, Status.Error}) { - assertForceStopOnDestroy(true, hostMockId, status, true); - } + 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 shouldForceStopOnDestroyIsFalseWhileHostMayReconnect() throws Exception { - for (Status status : new Status[] {Status.Connecting, Status.Disconnected, Status.Alert, Status.Rebalancing}) { - assertForceStopOnDestroy(true, hostMockId, status, false); + 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 shouldForceStopOnDestroyIsTrueWithoutHost() throws Exception { - assertForceStopOnDestroy(true, null, Status.Disconnected, true); + 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 shouldForceStopOnDestroyIsTrueWhenHostRecordIsGone() throws Exception { - when(hostDaoMock.findById(hostMockId)).thenReturn(null); - assertForceStopOnDestroy(true, hostMockId, Status.Disconnected, true); + 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()); } } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index bfa652469b6e..36b1e9c1b834 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -706,6 +706,9 @@ 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); + private static final ConfigKey VmDestroyForcestop = new ConfigKey<>("Advanced", Boolean.class, "vm.destroy.forcestop", "false", + "On destroy, force-stop takes this value ", true); + @Override public UserVmVO getVirtualMachine(long vmId) { return _vmDao.findById(vmId); @@ -3573,7 +3576,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C backupManager.checkAndRemoveBackupOfferingBeforeExpunge(vm); } - stopVirtualMachine(vmId, _itMgr.shouldForceStopOnDestroy(vm)); + stopVirtualMachineForDestroy(ctx.getCallingAccount(), vm); // Detach all data disks from VM List dataVols = _volsDao.findByInstanceAndType(vmId, Volume.Type.DATADISK); @@ -5661,6 +5664,22 @@ private void checkForceStopVmPermission(Account callingAccount) { } } + /** + * 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/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index cc2dc1cc9efa..0c88925a77c9 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,24 @@ 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); + } } From 29d02db0c68e4a691a8acad09564ec248ddeacbf Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 23 Sep 2026 18:49:06 +0000 Subject: [PATCH 3/4] server: do not expunge an instance still on its host during account cleanup Account cleanup expunges each instance even when destroying it failed. Expunge releases the instance's network resources before it stops it, so an instance whose stop fails loses its addresses while its domain keeps running. With the previous commit this is what a destroy on a briefly disconnected host now does. Skip the expunge when the destroy failed and the instance still has a host and is not stopped, and mark the account for another cleanup pass. Signed-off-by: Brad House --- .../com/cloud/user/AccountManagerImpl.java | 17 +++++++ .../cloud/user/AccountManagerImplTest.java | 44 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index b4c23beaef2e..3b06654855da 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; @@ -1006,6 +1016,13 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c _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; + continue; + } } } // no need to catch exception at this place as expunging vm diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 119704cc3971..deb514fefef1 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,47 @@ 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(_configMgr.releaseAccountSpecificVirtualRanges(account)).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).deleteWebhooksForAccount(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); + + Assert.assertTrue(accountManagerImpl.deleteUserAccount(42L)); + + Mockito.verify(_vmMgr, Mockito.never()).expunge(vm); + 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)); + } } From 6a27180f3df73354e37a74068e2a517085139d7e Mon Sep 17 00:00:00 2001 From: Brad House Date: Wed, 23 Sep 2026 19:23:16 +0000 Subject: [PATCH 4/4] engine, server: address the second review of the destroy stop An instance stalled in Starting or Migrating cannot enter Stopping, so its stop goes through the forced cleanup, which releases whatever the host answers. It was decided from the host status before any stop, and so refused even when an Up host would have answered. Try an ordinary stop first and run the cleanup once the host confirms it, or at once if the host is gone. Look the host up once when deciding whether to release. A host removed between two lookups made the log line throw in a finally, replacing the real error and leaving the instance in Stopping. Account cleanup went on to remove the security groups, networks and resource counts of an instance it had just left running. Stop after the instance loop in that case and let a later pass finish. Test the flag through the job queue: the queued VmWorkStop, the job handler, the in-job path and serialization, the stalled-instance paths and the force-stop permission check. Make the VmWorkStop field final. Signed-off-by: Brad House --- .../com/cloud/vm/VirtualMachineManager.java | 7 +- .../cloud/vm/VirtualMachineManagerImpl.java | 35 ++++-- .../main/java/com/cloud/vm/VmWorkStop.java | 9 +- .../vm/VirtualMachineManagerImplTest.java | 119 ++++++++++++++++++ .../com/cloud/user/AccountManagerImpl.java | 9 ++ .../java/com/cloud/vm/UserVmManagerImpl.java | 3 +- .../cloud/user/AccountManagerImplTest.java | 6 +- .../com/cloud/vm/UserVmManagerImplTest.java | 27 ++++ 8 files changed, 193 insertions(+), 22 deletions(-) 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 a48241273816..99e64a10b084 100644 --- a/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java +++ b/engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java @@ -171,9 +171,10 @@ 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 when the - * host does not answer, the instance's resources are only released if the host is Down or Removed. Otherwise the - * stop fails as an unforced one does and the instance stays Running. + * 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; 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 2edec1ebb2df..1b733fd10e0b 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -483,7 +483,6 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac static final ConfigKey VmDestroyForcestop = new ConfigKey("Advanced", Boolean.class, "vm.destroy.forcestop", "false", "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); @@ -2424,14 +2423,14 @@ private void orchestrateStop(final String vmUuid, final boolean cleanUpEvenIfUna /** * @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. A host that is Up, Connecting, Disconnected, Alert or Rebalancing may - * still be running it, whether or not it answers right now. + * 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) { - if (hostId == null) { - return true; - } - final HostVO host = _hostDao.findById(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; } @@ -2442,12 +2441,15 @@ protected boolean mayReleaseWithoutHostConfirmation(final VMInstanceVO vm, final if (!cleanUpEvenIfUnableToStop) { return false; } - if (!releaseOnlyIfHostIsGone || isHostGone(vm.getHostId())) { + if (!releaseOnlyIfHostIsGone || vm.getHostId() == null) { return true; } final HostVO host = _hostDao.findById(vm.getHostId()); - logger.warn("Not releasing the resources of {}: its host {} is {} and may still be running it. Retry once the host is Up, " - + "or stop the instance with forced=true if it is known to be gone.", vm, host, host.getStatus()); + 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; } @@ -2564,10 +2566,19 @@ protected void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUna throw new ConcurrentOperationException(String.format("%s is being operated on.", vm.toString())); } } catch (final NoTransitionException e1) { - // cleanup() releases the resources whether or not the host answers, so check before it runs - if (!mayReleaseWithoutHostConfirmation(vm, cleanUpEvenIfUnableToStop, releaseOnlyIfHostIsGone)) { + 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); 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 7d19d29e9693..fb20b960b825 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VmWorkStop.java @@ -23,20 +23,21 @@ public class VmWorkStop extends VmWork { // 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 boolean releaseOnlyIfHostIsGone; + 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) { - super(vmWork); - this.cleanup = cleanup; + this(vmWork, cleanup, false); } public VmWorkStop(VmWork vmWork, boolean cleanup, boolean releaseOnlyIfHostIsGone) { - this(vmWork, cleanup); + super(vmWork); + this.cleanup = cleanup; this.releaseOnlyIfHostIsGone = 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 c3b328f34b49..8506702384da 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java @@ -167,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; @@ -2199,4 +2200,122 @@ public void explicitForcedStopStillReleasesWhenTheHostIsDisconnected() throws Ex 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 3b06654855da..04ba81ef156e 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1010,6 +1010,7 @@ 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 { @@ -1021,6 +1022,7 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c // 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; } } @@ -1033,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 36b1e9c1b834..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); @@ -5657,7 +5658,7 @@ 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."); diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index deb514fefef1..a76f73521bac 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1744,18 +1744,20 @@ public void deleteUserAccountDoesNotExpungeAnInstanceStillOnItsHost() throws Exc 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(_configMgr.releaseAccountSpecificVirtualRanges(account)).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).deleteWebhooksForAccount(Mockito.anyLong()); 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)); } diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 0c88925a77c9..dbe1ea0ff19a 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -4632,4 +4632,31 @@ public void stopVirtualMachineForDestroyFailsWhenTheStopTimesOut() throws Except 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"); + } + } }