engine: do not force the destroy stop while the host may reconnect

engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java

@@ -170,6 +170,14 @@ void orchestrateStart(String vmUuid, Map<VirtualMachineProfile.Param, Object> 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;

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

@@ -481,7 +481,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
     static final ConfigKey<Long> VmOpCancelInterval = new ConfigKey<Long>("Advanced", Long.class, "vm.op.cancel.interval", "3600",
             "Time (in seconds) to wait before cancelling a operation", false);
     static final ConfigKey<Boolean> VmDestroyForcestop = new ConfigKey<Boolean>("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<Integer> ClusterDeltaSyncInterval = new ConfigKey<Integer>("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,15 +2394,15 @@ 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());
                 }
             }
 
         } else {
-            final Outcome<VirtualMachine> outcome = stopVmThroughJobQueue(vmUuid, cleanUpEvenIfUnableToStop);
+            final Outcome<VirtualMachine> 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<String, Boolean> vlanToPersistenceMap, NetworkVO networkVO) {
@@ -2460,8 +2515,8 @@ private Pair<String, Boolean> 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<Boolean, String> 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<VirtualMachine> startVmThroughJobQueue(final String vmUuid,
     }
 
     public Outcome<VirtualMachine> stopVmThroughJobQueue(final String vmUuid, final boolean cleanup) {
+        return stopVmThroughJobQueue(vmUuid, cleanup, false);
+    }
+
+    public Outcome<VirtualMachine> stopVmThroughJobQueue(final String vmUuid, final boolean cleanup, final boolean releaseOnlyIfHostIsGone) {
         String commandName = VmWorkStop.class.getName();
         Pair<VmWorkJobVO, Long> pendingWorkJob = retrievePendingWorkJob(null, vmUuid, null, commandName);
 
@@ -5715,7 +5784,7 @@ public Outcome<VirtualMachine> stopVmThroughJobQueue(final String vmUuid, final
             Pair<VmWorkJobVO, VmWork> 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<JobInfo.Status, String> 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);
     }
 

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