engine: keep a migrating instance while its source host may run it

A Migrating instance has its hostId set to the destination and its
lastHostId to the source, and either host may be running it until the
migration settles. The destroy stop only checked hostId, so with the
destination Down it released the resources even though the source host
was Up and could still be running the domain.

For a destroy, refuse the forced cleanup of a Migrating instance while
the source host is not gone. An explicit forced stop is unchanged.

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

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

@@ -2434,6 +2434,23 @@ private static boolean isGone(final HostVO host) {
         return host == null || host.getStatus() == Status.Down || host.getStatus() == Status.Removed;
     }
 
+    /**
+     * A Migrating instance has its hostId set to the destination and its lastHostId to the source. Until the
+     * migration settles either host may be running it, and a StopCommand only goes to the destination.
+     *
+     * @return the source host of a migration in progress when that host is not gone, otherwise null
+     */
+    protected Long migrationSourceThatMayBeRunningIt(final VMInstanceVO vm) {
+        if (vm.getState() != State.Migrating) {
+            return null;
+        }
+        final Long sourceHostId = vm.getLastHostId();
+        if (sourceHostId == null || sourceHostId.equals(vm.getHostId()) || isHostGone(sourceHostId)) {
+            return null;
+        }
+        return sourceHostId;
+    }
+
     /**
      * Whether a stop may release an instance's resources without the host confirming the instance is stopped.
      */
@@ -2571,6 +2588,13 @@ protected void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUna
             }
             // 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.
+            final Long migrationSourceHostId = releaseOnlyIfHostIsGone ? migrationSourceThatMayBeRunningIt(vm) : null;
+            if (migrationSourceHostId != null) {
+                logger.warn("Not releasing the resources of {} in state {}: the host it is migrating from, {}, may still be running it.",
+                        vm, vm.getState(), migrationSourceHostId);
+                throw new CloudRuntimeException("Unable to stop " + vm + " in state " + vm.getState()
+                        + ": the host it is migrating from may still be running it");
+            }
             if (releaseOnlyIfHostIsGone && !isHostGone(vm.getHostId())) {
                 final Pair<Boolean, String> stopResult = sendStop(vmGuru, profile, false, false);
                 if (!stopResult.first()) {

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

@@ -2254,6 +2254,60 @@ public void destroyStopOfAStalledInstanceCleansUpWithoutAskingAHostThatIsDown()
         verify(virtualMachineManagerImpl, never()).sendStop(any(), any(), anyBoolean(), anyBoolean());
     }
 
+    /**
+     * An instance stuck in Migrating: hostId is the destination, lastHostId the source. StopRequested is not a
+     * transition from Migrating, so advanceStop() goes through the forced cleanup.
+     */
+    private VMInstanceVO migratingVm(final Status destinationStatus, final Status sourceStatus) throws Exception {
+        final long sourceHostId = hostMockId + 1;
+        final HostVO sourceHost = mock(HostVO.class);
+        when(sourceHost.getStatus()).thenReturn(sourceStatus);
+        when(hostDaoMock.findById(sourceHostId)).thenReturn(sourceHost);
+        VMInstanceVO vm = startingVmOnHost(destinationStatus);
+        ReflectionTestUtils.setField(vm, "state", State.Migrating);
+        ReflectionTestUtils.setField(vm, "lastHostId", sourceHostId);
+        return vm;
+    }
+
+    @Test
+    public void destroyStopOfAMigratingInstanceFailsWhileTheSourceHostMayStillRunIt() throws Exception {
+        for (Status source : new Status[] {Status.Up, Status.Disconnected, Status.Alert}) {
+            VMInstanceVO vm = migratingVm(Status.Down, source);
+
+            CloudRuntimeException e = assertThrows(source.toString(), CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true));
+
+            assertTrue(e.getMessage().contains("migrating from"));
+        }
+        verify(virtualMachineManagerImpl, never()).sendStop(any(), any(), anyBoolean(), anyBoolean());
+        verify(virtualMachineManagerImpl, never()).cleanup(any(), any(), any(), any(), anyBoolean());
+    }
+
+    @Test
+    public void destroyStopOfAMigratingInstanceCleansUpWhenBothHostsAreGone() throws Exception {
+        VMInstanceVO vm = migratingVm(Status.Down, Status.Removed);
+
+        CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, true));
+
+        assertEquals("cleaned up", e.getMessage());
+    }
+
+    @Test
+    public void explicitForcedStopOfAMigratingInstanceIgnoresTheSourceHost() throws Exception {
+        VMInstanceVO vm = migratingVm(Status.Down, Status.Up);
+
+        CloudRuntimeException e = assertThrows(CloudRuntimeException.class, () -> virtualMachineManagerImpl.advanceStop(vm, true, false));
+
+        assertEquals("cleaned up", e.getMessage());
+    }
+
+    @Test
+    public void migrationSourceIsOnlyConsideredForAMigratingInstance() throws Exception {
+        VMInstanceVO vm = migratingVm(Status.Up, Status.Up);
+        assertEquals(Long.valueOf(hostMockId + 1), virtualMachineManagerImpl.migrationSourceThatMayBeRunningIt(vm));
+        ReflectionTestUtils.setField(vm, "state", State.Running);
+        assertNull(virtualMachineManagerImpl.migrationSourceThatMayBeRunningIt(vm));
+    }
+
     @Test
     public void advanceStopQueuesTheFlagOnTheStopJob() throws Exception {
         final AsyncJobExecutionContext jobContext = mock(AsyncJobExecutionContext.class);