From adce1f12b7f627d850e8a6344fbf72f1b02e1978 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:28:20 +0000 Subject: [PATCH 01/11] engine: do not report an instance missing right after its state changed A host VM state report lists only the instances running on the host at the moment the report was collected. An out-of-band report (sent by the agent on a libvirt lifecycle event) is processed with `force`, which skips both the graceful period and the outdated-report filter. Any instance absent from that report is immediately marked PowerReportMissing. That is wrong when the report was collected before a state change the management server made a moment later. Example: | time | event | |----------|----------------------------------------------------------| | 20:25:14 | another instance crashes; agent collects and sends report | | 20:25:16 | the starting instance's domain is created | | 20:25:17 | StartAnswer arrives, instance -> Running | | 20:25:18 | the 20:25:14 report is processed, instance not in it | | | -> PowerReportMissing, instance -> Stopped | The instance is running but is now recorded as stopped. On a busy host these forced reports are frequent, so the graceful period almost never applies. Fix: skip the missing-report verdict for an instance whose state changed within the graceful period, forced or not. A later report decides instead. An instance that has been Running for a while and then shuts itself down is unaffected, so out-of-band stop detection keeps working. Also make the log line state whether the verdict was forced. It previously said "has passed graceful period" even when `force` short-circuited the check, which is misleading when diagnosing this. Signed-off-by: Brad House --- .../vm/VirtualMachinePowerStateSyncImpl.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java index 475ed0f37bd2..da1c48ba6bf0 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java @@ -114,6 +114,21 @@ private List filterOutdatedFromMissingVmReport(List .collect(Collectors.toList()); } + /** + * A host report only lists the instances that were running on the host when the report was collected. When the + * management server changes an instance's state around that moment, an in-flight report may have been collected + * before the change and therefore says nothing about it. Treating such an instance as missing would undo the + * change that just happened, so instances whose state changed within the graceful period are left alone and + * judged by a later report instead. + */ + protected boolean hasRecentStateChange(VMInstanceVO instance, Date currentTime, long milliSecondsGracefulPeriod) { + Date lastStateChange = instance.getUpdateTime(); + if (lastStateChange == null) { + return false; + } + return currentTime.getTime() - lastStateChange.getTime() < milliSecondsGracefulPeriod; + } + private void processMissingVmReport(long hostId, Set vmIds, boolean force) { // any state outdates should be checked against the time before this list was retrieved Date startTime = DateUtil.currentGMTTime(); @@ -149,10 +164,16 @@ private void processMissingVmReport(long hostId, Set vmIds, boolean force) instance.getUuid(), VirtualMachine.PowerState.PowerReportMissing, DateUtil.getOutputString(vmStateUpdateTime)); + if (hasRecentStateChange(instance, currentTime, milliSecondsGracefulPeriod)) { + logger.debug("vm id: {} - state changed at {}, which is within the graceful period ({} ms); " + + "the report may have been collected before that change, skipping missing report", + instance.getId(), DateUtil.getOutputString(instance.getUpdateTime()), milliSecondsGracefulPeriod); + continue; + } long milliSecondsSinceLastStateUpdate = currentTime.getTime() - vmStateUpdateTime.getTime(); if (force || (milliSecondsSinceLastStateUpdate > milliSecondsGracefulPeriod)) { - logger.debug("vm id: {} - time since last state update({} ms) has passed graceful period", - instance.getId(), milliSecondsSinceLastStateUpdate); + logger.debug("vm id: {} - reporting missing (time since last state update: {} ms, graceful period: {} ms, forced: {})", + instance.getId(), milliSecondsSinceLastStateUpdate, milliSecondsGracefulPeriod, force); // this is where a race condition might have happened if we don't re-fetch the instance; // between the startime of this job and the currentTime of this missing-branch // an update might have occurred that we should not override in case of out of band migration From 6e55f5b42c7d09546cc00bcef353f8c46e47d2df Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:29:21 +0000 Subject: [PATCH 02/11] engine: stop the instance on its host before releasing resources on a missing report When a power report for a Running instance is missing, the instance is synced to Stopped and `releaseVmResources` frees its NICs and IP addresses. Unlike the PowerOff branch above it, no StopCommand is sent. A missing report only means the host did not list the instance. It is not proof the instance is gone. When it is still running: - its NIC and IP are marked free while it keeps using them - the IP is later assigned to another instance - two instances answer for the same address Fix: use the same path as PowerOff. Send the StopCommand first and release resources only if it succeeds. If the stop fails, keep the resources and let a later report retry, rather than freeing an address that is still in use. Signed-off-by: Brad House --- .../java/com/cloud/vm/VirtualMachineManagerImpl.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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..9475c9a64f97 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -5480,20 +5480,20 @@ private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { return; } - if (PowerState.PowerOff.equals(vm.getPowerState())) { + // A missing report is not proof that the instance is gone, only that the host did not list it. Stop it + // on the host before giving up its resources, otherwise a still-running instance keeps its NICs and IP + // addresses while the database says they are free and they get handed to another instance. + if (PowerState.PowerOff.equals(vm.getPowerState()) || PowerState.PowerReportMissing.equals(vm.getPowerState())) { final VirtualMachineGuru vmGuru = getVmGuru(vm); final VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); Pair result = sendStop(vmGuru, profile, true, true); if (!result.first()) { + logger.warn("Unable to stop VM {} on its host, not releasing its resources: {}", vm, result.second()); return; } else { // Release resources on StopCommand success releaseVmResources(profile, true); } - } else if (PowerState.PowerReportMissing.equals(vm.getPowerState())) { - final VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); - // VM will be sync-ed to Stopped state, release the resources - releaseVmResources(profile, true); } try { From 0b685ea23cd89368fb621979a9234f6b613cb8a6 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:30:18 +0000 Subject: [PATCH 03/11] engine: act on a power-on report for a destroyed or expunged instance When a host reports an instance as powered on and the database has it as Destroyed or Expunging, the report is only logged. The instance keeps running unmanaged: - CloudStack knows the host and the instance name at that moment - the instance's IP addresses and volumes have already been handed back - nothing else ever looks at it again Changes: | state | before | after | |----------------------|--------|---------------------------| | Destroyed, Expunging | log | send StopCommand to host | | Error | log | log and raise an alert | Error is only alerted, not stopped, because an instance in that state may still be wanted for inspection. Destroyed and Expunging are being deleted, so there is no case for leaving them running. Signed-off-by: Brad House --- .../cloud/vm/VirtualMachineManagerImpl.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) 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 9475c9a64f97..c76c54e54012 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -5437,6 +5437,7 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { case Destroyed: case Expunging: logger.info("Receive power on report when Instance is in destroyed or expunging state. Instance: {}, state: {}.", vm, vm.getState()); + stopUnmanagedInstanceOnReportingHost(vm); break; case Migrating: @@ -5452,10 +5453,45 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { case Error: default: logger.info("Receive power on report when Instance is in error or unexpected state. Instance: {}, state: {}.", vm, vm.getState()); + _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_SYNC, vm.getDataCenterId(), vm.getPodIdToDeployIn(), + VM_SYNC_ALERT_SUBJECT, String.format("Instance %s is reported running by host %s but is in %s state. " + + "It is not managed by CloudStack and may need to be stopped on the host.", + vm.getInstanceName(), vm.getPowerHostId(), vm.getState())); break; } } + /** + * The host reports an instance as powered on that the database considers destroyed or expunged. It will never be + * managed again, and its addresses and storage have already been handed back, so stop it on the host that + * reported it instead of leaving it running unmanaged. + */ + protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm) { + final Long powerHostId = vm.getPowerHostId(); + if (powerHostId == null) { + logger.warn("Instance {} is reported powered on but no reporting host is recorded, cannot stop it.", vm); + return; + } + try { + // checkBeforeCleanup must be false: the instance is known to be running, and that is exactly what + // has to be stopped. With it set, the host would refuse and answer "vm is still running on host". + final StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); + final Answer answer = _agentMgr.send(powerHostId, stop); + if (answer != null && answer.getResult()) { + logger.info("Stopped unmanaged instance {} on host {}.", vm, powerHostId); + return; + } + logger.warn("Unable to stop unmanaged instance {} on host {}: {}", vm, powerHostId, + answer == null ? "no answer from host" : answer.getDetails()); + } catch (final AgentUnavailableException | OperationTimedoutException e) { + logger.warn("Unable to stop unmanaged instance {} on host {}.", vm, powerHostId, e); + } + _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_SYNC, vm.getDataCenterId(), vm.getPodIdToDeployIn(), + VM_SYNC_ALERT_SUBJECT, String.format("Instance %s is reported running by host %s but is in %s state, " + + "and could not be stopped. It may need to be stopped on the host.", + vm.getInstanceName(), powerHostId, vm.getState())); + } + private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { switch (vm.getState()) { case Starting: From d43b25029b8b2ac019312a971354f56fe6d0b357 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:31:41 +0000 Subject: [PATCH 04/11] engine: stop the instance on its host before expunging it `advanceStop()` returns immediately, without sending anything to the host, when the database already has the instance in one of these states: if (state == State.Stopped) return; if (state == State.Destroyed || state == State.Expunging || state == State.Error) return; Expunge calls `advanceStop()` and then releases NICs and deletes volumes. So for an instance recorded as Stopped or Error, expunge never contacts the host at all. If the host is still running it, the instance keeps its addresses and its disks are deleted underneath it. `vm.destroy.forcestop` does not help: the early return happens before the host id is even looked at. Fix: after `advanceStop()`, send a StopCommand to the instance's host, or its last known host, regardless of the database state. It is a no-op when no domain is there. Signed-off-by: Brad House --- .../cloud/vm/VirtualMachineManagerImpl.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) 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 c76c54e54012..3db1b768b3c0 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -696,6 +696,11 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti advanceStop(vm.getUuid(), VmDestroyForcestop.value()); vm = _vmDao.findByUuid(vm.getUuid()); + // advanceStop() returns without contacting the host when the database already has the instance as Stopped, + // Error, Destroyed or Expunging. The host may still be running it. Expunging is about to release its + // addresses and delete its volumes, so make sure no domain is left behind for it. + ensureInstanceIsStoppedOnLastKnownHost(vm); + try { if (!stateTransitTo(vm, VirtualMachine.Event.ExpungeOperation, vm.getHostId())) { logger.debug("Unable to expunge the vm because it is not in the correct state: " + vm); @@ -5466,6 +5471,33 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { * managed again, and its addresses and storage have already been handed back, so stop it on the host that * reported it instead of leaving it running unmanaged. */ + /** + * Send a StopCommand for an instance to the last host it is known to have run on, whatever the database state + * says. Used before expunging, where the instance's addresses and volumes are about to be released and a domain + * left running on the host would keep using them. + */ + protected void ensureInstanceIsStoppedOnLastKnownHost(final VMInstanceVO vm) { + if (vm == null) { + return; + } + final Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); + if (hostId == null) { + return; + } + try { + // checkBeforeCleanup is false on purpose: a running domain is what has to be removed here. + final StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); + final Answer answer = _agentMgr.send(hostId, stop); + if (answer != null && answer.getResult()) { + return; + } + logger.warn("Unable to confirm instance {} is stopped on host {} before expunging: {}", vm, hostId, + answer == null ? "no answer from host" : answer.getDetails()); + } catch (final AgentUnavailableException | OperationTimedoutException e) { + logger.warn("Unable to confirm instance {} is stopped on host {} before expunging.", vm, hostId, e); + } + } + protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm) { final Long powerHostId = vm.getPowerHostId(); if (powerHostId == null) { From 7fcb94aad09e2a19e0d8a4af79d2864cd66d8a12 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:33:24 +0000 Subject: [PATCH 05/11] engine: warn when a host reports instances CloudStack does not know about A host report naming an instance with no record in the database means something is running there unmanaged, usually left behind by a deploy or an expunge that never reached the host. Today that produces one debug line per instance per report and nothing else, so it can go unnoticed indefinitely: Unable to find matched VM in CloudStack DB. name: i-2-3-VM powerstate: PowerOn Change: log the unknown instances for a host at warn level, and only when the set changes, so a standing condition is visible once instead of on every report. A host that stops reporting unknown instances logs one info line. Nothing is stopped automatically here. An unknown instance may be a domain an operator put on the host deliberately, so this only makes the condition visible; `listUnmanagedInstances` and import remain the way to act on it. Signed-off-by: Brad House --- .../vm/VirtualMachinePowerStateSyncImpl.java | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java index da1c48ba6bf0..f347625905d0 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java @@ -18,9 +18,11 @@ import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import javax.inject.Inject; @@ -49,6 +51,8 @@ public class VirtualMachinePowerStateSyncImpl implements VirtualMachinePowerStat @Inject HostDao hostDao; @Inject ManagementServiceConfiguration mgmtServiceConf; + private final Map> unknownInstancesByHost = new ConcurrentHashMap<>(); + private LazyCache vmCache; private LazyCache hostCache; @@ -66,14 +70,14 @@ public void resetHostSyncState(Host host) { @Override public void processHostVmStateReport(long hostId, Map report) { logger.debug("Process host VM state report. host: {}", hostCache.get(hostId)); - Map translatedInfo = convertVmStateReport(report); + Map translatedInfo = convertVmStateReport(hostId, report); processReport(hostId, translatedInfo, false); } @Override public void processHostVmStatePingReport(long hostId, Map report, boolean force) { logger.debug("Process host VM state report from ping process. host: {}", hostCache.get(hostId)); - Map translatedInfo = convertVmStateReport(report); + Map translatedInfo = convertVmStateReport(hostId, report); processReport(hostId, translatedInfo, force); } @@ -199,23 +203,54 @@ private void processReport(long hostId, Map tra logger.debug("Done with process of VM state report. host: {}", () -> hostCache.get(hostId)); } - public Map convertVmStateReport(Map states) { + public Map convertVmStateReport(long hostId, Map states) { final HashMap map = new HashMap<>(); if (MapUtils.isEmpty(states)) { + reportUnknownInstances(hostId, new HashSet<>()); return map; } + Set unknownInstanceNames = new HashSet<>(); Map nameIdMap = _instanceDao.getNameIdMapForVmInstanceNames(states.keySet()); for (Map.Entry entry : states.entrySet()) { Long id = nameIdMap.get(entry.getKey()); if (id != null) { map.put(id, entry.getValue().getState()); } else { + unknownInstanceNames.add(entry.getKey()); logger.debug("Unable to find matched VM in CloudStack DB. name: {} powerstate: {}", entry.getKey(), entry.getValue()); } } + reportUnknownInstances(hostId, unknownInstanceNames); return map; } + /** + * A host reporting an instance that CloudStack has no record of means something is running there unmanaged, + * usually left behind by a deploy or an expunge that did not reach the host. Previously this produced one debug + * line per instance per report and nothing else, so it could go unnoticed indefinitely. + * + * Logged at warn level, and only when the set of unknown instances on a host changes, so a standing condition + * does not repeat on every report. + * + * @return true when the set changed and was reported, false when there was nothing new to say. + */ + protected boolean reportUnknownInstances(long hostId, Set unknownInstanceNames) { + Set previous = unknownInstancesByHost.get(hostId); + if (unknownInstanceNames.equals(previous) || (CollectionUtils.isEmpty(unknownInstanceNames) && previous == null)) { + return false; + } + if (unknownInstanceNames.isEmpty()) { + unknownInstancesByHost.remove(hostId); + logger.info("No unknown instances are reported anymore. host: {}", () -> hostCache.get(hostId)); + return true; + } + unknownInstancesByHost.put(hostId, unknownInstanceNames); + logger.warn("Host reports {} instance(s) that do not exist in CloudStack DB, they are running unmanaged. " + + "host: {}, instances: [{}]", + unknownInstanceNames.size(), hostCache.get(hostId), String.join(", ", unknownInstanceNames)); + return true; + } + protected VMInstanceVO getVmFromId(long vmId) { return _instanceDao.findById(vmId); } From 5afd6043e49298e6623865ee77f07ef06a48ee61 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 17:35:45 +0000 Subject: [PATCH 06/11] engine: add tests for the power state sync changes Covers: | test | checks | |-----------------------------------------------|---------------------------------------------| | hasRecentStateChange, within/outside/null | the graceful period guard on state changes | | convertVmStateReport, known and unknown names | unknown instances are skipped, not mapped | | convertVmStateReport, empty report | no DB lookup for an empty report | | reportUnknownInstances, only reports changes | one report per change, quiet while unchanged| Signed-off-by: Brad House --- .../VirtualMachinePowerStateSyncImplTest.java | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java index 4df14fe22f3b..b7ac687d3d4e 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java @@ -18,10 +18,13 @@ import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.PublishScope; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -30,6 +33,7 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import com.cloud.agent.api.HostVmStateReportEntry; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.vm.dao.VMInstanceDao; @@ -104,4 +108,76 @@ public void test_updateAndPublishVmPowerStates_partialUpdated() { PublishScope.GLOBAL, 2L); } + + private VMInstanceVO instanceWithUpdateTime(Date updateTime) { + VMInstanceVO instance = Mockito.mock(VMInstanceVO.class); + Mockito.when(instance.getUpdateTime()).thenReturn(updateTime); + return instance; + } + + @Test + public void test_hasRecentStateChange_withinGracefulPeriod() { + Date now = new Date(); + VMInstanceVO instance = instanceWithUpdateTime(new Date(now.getTime() - 1000L)); + Assert.assertTrue(virtualMachinePowerStateSync.hasRecentStateChange(instance, now, 120000L)); + } + + @Test + public void test_hasRecentStateChange_outsideGracefulPeriod() { + Date now = new Date(); + VMInstanceVO instance = instanceWithUpdateTime(new Date(now.getTime() - 300000L)); + Assert.assertFalse(virtualMachinePowerStateSync.hasRecentStateChange(instance, now, 120000L)); + } + + @Test + public void test_hasRecentStateChange_nullUpdateTime() { + VMInstanceVO instance = instanceWithUpdateTime(null); + Assert.assertFalse(virtualMachinePowerStateSync.hasRecentStateChange(instance, new Date(), 120000L)); + } + + @Test + public void test_convertVmStateReport_mapsKnownAndSkipsUnknown() { + Map report = new HashMap<>(); + report.put("i-2-1-VM", new HostVmStateReportEntry(VirtualMachine.PowerState.PowerOn, "host")); + report.put("i-2-2-VM", new HostVmStateReportEntry(VirtualMachine.PowerState.PowerOn, "host")); + Map nameIdMap = new HashMap<>(); + nameIdMap.put("i-2-1-VM", 1L); + Mockito.when(instanceDao.getNameIdMapForVmInstanceNames(Mockito.anyCollection())).thenReturn(nameIdMap); + + Map result = virtualMachinePowerStateSync.convertVmStateReport(1L, report); + + Assert.assertEquals(1, result.size()); + Assert.assertEquals(VirtualMachine.PowerState.PowerOn, result.get(1L)); + } + + @Test + public void test_convertVmStateReport_emptyReport() { + Map result = + virtualMachinePowerStateSync.convertVmStateReport(1L, new HashMap<>()); + Assert.assertTrue(result.isEmpty()); + Mockito.verify(instanceDao, Mockito.never()).getNameIdMapForVmInstanceNames(Mockito.anyCollection()); + } + + @Test + public void test_reportUnknownInstances_onlyReportsChanges() { + Set unknown = new HashSet<>(); + unknown.add("i-2-3-VM"); + // first sighting is reported, an identical set afterwards is not + Assert.assertTrue(virtualMachinePowerStateSync.reportUnknownInstances(1L, unknown)); + Assert.assertFalse(virtualMachinePowerStateSync.reportUnknownInstances(1L, new HashSet<>(unknown))); + + // a new name in the set is a change and is reported again + Set grown = new HashSet<>(unknown); + grown.add("i-2-4-VM"); + Assert.assertTrue(virtualMachinePowerStateSync.reportUnknownInstances(1L, grown)); + + // clearing is reported once, then stays quiet + Assert.assertTrue(virtualMachinePowerStateSync.reportUnknownInstances(1L, new HashSet<>())); + Assert.assertFalse(virtualMachinePowerStateSync.reportUnknownInstances(1L, new HashSet<>())); + } + + @Test + public void test_reportUnknownInstances_noneEverSeen() { + Assert.assertFalse(virtualMachinePowerStateSync.reportUnknownInstances(2L, new HashSet<>())); + } } From 64d8b2dbae7ae3c4aafa283742a7850b24eea798 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 18:11:07 +0000 Subject: [PATCH 07/11] engine: build the new stop commands the same way sendStop does The two helpers added earlier in this branch called `new StopCommand(...)` directly. `sendStop()` does more than that, and skipping it has consequences: | not set | effect | |------------------------------|---------------------------------------------------------------| | `vlanToPersistenceMap` | `shouldDeleteBridge()` returns true for an empty map, so the host deletes bridges belonging to persistent networks | | external hypervisor details | `ExternalPathPayloadProvisioner` dereferences `cmd.getVirtualMachine()`, which is null -> NPE | | `controlIp` | the cmdline backup for system VMs does not happen | | `volumesToDisconnect` | volumes stay connected on the host | Fix: pull the command construction out of `sendStop()` into `buildStopCommand()` and use it in both helpers, so there is one place that knows how to build a StopCommand. `sendStop()` behaviour is unchanged. External instances are skipped in both helpers. Their teardown is done by their extension in `finalizeExpunge`, and a StopCommand issued from here would not carry what that path needs. Also removes a duplicated Javadoc block, which left `stopUnmanagedInstanceOnReportingHost` documented as the other method. Signed-off-by: Brad House --- .../cloud/vm/VirtualMachineManagerImpl.java | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) 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 3db1b768b3c0..3b79e2486c44 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -2214,8 +2214,13 @@ private List> getVolumesToDisconnect(VirtualMachine vm) { return volumesToDisconnect; } - protected Pair sendStop(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final boolean force, final boolean checkBeforeCleanup) { - final VirtualMachine vm = profile.getVirtualMachine(); + /** + * Build a StopCommand carrying everything the host needs to tear an instance down: the external hypervisor + * details, the VLAN persistence map that decides whether a bridge may be deleted, the control NIC address used + * for system VMs, and the volumes to disconnect. Callers that build a StopCommand without these will make the + * host delete bridges belonging to persistent networks and leave volumes connected. + */ + protected StopCommand buildStopCommand(final VirtualMachine vm, final VirtualMachineProfile profile, final boolean checkBeforeCleanup) { Map vlanToPersistenceMap = getVlanToPersistenceMapForVM(vm.getId()); StopCommand stpCmd = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), checkBeforeCleanup); updateStopCommandForExternalHypervisorType(vm.getHypervisorType(), profile, stpCmd); @@ -2224,7 +2229,12 @@ protected Pair sendStop(final VirtualMachineGuru guru, final Vi } stpCmd.setControlIp(getControlNicIpForVM(vm)); stpCmd.setVolumesToDisconnect(getVolumesToDisconnect(vm)); - final StopCommand stop = stpCmd; + return stpCmd; + } + + protected Pair sendStop(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final boolean force, final boolean checkBeforeCleanup) { + final VirtualMachine vm = profile.getVirtualMachine(); + final StopCommand stop = buildStopCommand(vm, profile, checkBeforeCleanup); try { Answer answer = null; if(vm.getHostId() != null) { @@ -5466,18 +5476,16 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { } } - /** - * The host reports an instance as powered on that the database considers destroyed or expunged. It will never be - * managed again, and its addresses and storage have already been handed back, so stop it on the host that - * reported it instead of leaving it running unmanaged. - */ /** * Send a StopCommand for an instance to the last host it is known to have run on, whatever the database state * says. Used before expunging, where the instance's addresses and volumes are about to be released and a domain * left running on the host would keep using them. + * + * External instances are skipped: their teardown is done by their extension in finalizeExpunge, and a + * StopCommand issued from here would not carry the details that path needs. */ protected void ensureInstanceIsStoppedOnLastKnownHost(final VMInstanceVO vm) { - if (vm == null) { + if (vm == null || HypervisorType.External.equals(vm.getHypervisorType())) { return; } final Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); @@ -5486,7 +5494,7 @@ protected void ensureInstanceIsStoppedOnLastKnownHost(final VMInstanceVO vm) { } try { // checkBeforeCleanup is false on purpose: a running domain is what has to be removed here. - final StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); + final StopCommand stop = buildStopCommand(vm, new VirtualMachineProfileImpl(vm), false); final Answer answer = _agentMgr.send(hostId, stop); if (answer != null && answer.getResult()) { return; @@ -5498,7 +5506,17 @@ protected void ensureInstanceIsStoppedOnLastKnownHost(final VMInstanceVO vm) { } } + /** + * The host reports an instance as powered on that the database considers destroyed or expunged. It will never be + * managed again, and its addresses and storage have already been handed back, so stop it on the host that + * reported it instead of leaving it running unmanaged. + * + * External instances are skipped, as in ensureInstanceIsStoppedOnLastKnownHost(). + */ protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm) { + if (HypervisorType.External.equals(vm.getHypervisorType())) { + return; + } final Long powerHostId = vm.getPowerHostId(); if (powerHostId == null) { logger.warn("Instance {} is reported powered on but no reporting host is recorded, cannot stop it.", vm); @@ -5507,7 +5525,7 @@ protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm) { try { // checkBeforeCleanup must be false: the instance is known to be running, and that is exactly what // has to be stopped. With it set, the host would refuse and answer "vm is still running on host". - final StopCommand stop = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), false); + final StopCommand stop = buildStopCommand(vm, new VirtualMachineProfileImpl(vm), false); final Answer answer = _agentMgr.send(powerHostId, stop); if (answer != null && answer.getResult()) { logger.info("Stopped unmanaged instance {} on host {}.", vm, powerHostId); From 191977e03e46d01d790a311eb28ea984ad19719a Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 18:11:18 +0000 Subject: [PATCH 08/11] engine: make the missing-report tests actually exercise the fix The tests added earlier stubbed `findByHostInStatesExcluding` with a single `Mockito.any()` for a varargs parameter that receives three states: findByHostInStatesExcluding(Long hostId, Collection excludingIds, State... states) The matcher never matched, so the mock returned its default empty list, `processMissingVmReport()` returned at the `isEmpty()` check, and the `never()` assertions passed without exercising anything. Fix: match the varargs explicitly. Verified by mutation - disabling the `hasRecentStateChange()` guard now fails `test_processMissingVmReport_forcedReportDoesNotOverrideRecentStateChange` with NeverWantedButInvoked. It did not before. Tests now cover: | test | checks | |-----------------------------------------------|--------------------------------------------------| | forcedReportDoesNotOverrideRecentStateChange | a forced report does not mark a just-changed instance missing | | forcedReportStillReportsSettledInstance | a settled instance is still reported, so out-of-band stop detection works | | unforcedReportHonoursGracefulPeriod | the graceful period still applies without force | | recordsUnknownInstances | an unknown name is recorded, not just logged | Also moves the "Detected missing VM" line below the guard, so a skipped instance is no longer logged as detected. Signed-off-by: Brad House --- .../vm/VirtualMachinePowerStateSyncImpl.java | 12 +-- .../VirtualMachinePowerStateSyncImplTest.java | 88 +++++++++++++++++++ 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java index f347625905d0..a412d0fe6b4c 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java @@ -162,18 +162,18 @@ private void processMissingVmReport(long hostId, Set vmIds, boolean force) vmStateUpdateTime = instance.getCreated(); } } - logger.debug("Detected missing VM. host: {}, vm id: {}({}), power state: {}, last state update: {}", - hostId, - instance.getId(), - instance.getUuid(), - VirtualMachine.PowerState.PowerReportMissing, - DateUtil.getOutputString(vmStateUpdateTime)); if (hasRecentStateChange(instance, currentTime, milliSecondsGracefulPeriod)) { logger.debug("vm id: {} - state changed at {}, which is within the graceful period ({} ms); " + "the report may have been collected before that change, skipping missing report", instance.getId(), DateUtil.getOutputString(instance.getUpdateTime()), milliSecondsGracefulPeriod); continue; } + logger.debug("Detected missing VM. host: {}, vm id: {}({}), power state: {}, last state update: {}", + hostId, + instance.getId(), + instance.getUuid(), + VirtualMachine.PowerState.PowerReportMissing, + DateUtil.getOutputString(vmStateUpdateTime)); long milliSecondsSinceLastStateUpdate = currentTime.getTime() - vmStateUpdateTime.getTime(); if (force || (milliSecondsSinceLastStateUpdate > milliSecondsGracefulPeriod)) { logger.debug("vm id: {} - reporting missing (time since last state update: {} ms, graceful period: {} ms, forced: {})", diff --git a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java index b7ac687d3d4e..189668b43627 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.vm; +import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -34,6 +36,7 @@ import org.mockito.junit.MockitoJUnitRunner; import com.cloud.agent.api.HostVmStateReportEntry; +import com.cloud.configuration.ManagementServiceConfiguration; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.vm.dao.VMInstanceDao; @@ -46,6 +49,8 @@ public class VirtualMachinePowerStateSyncImplTest { VMInstanceDao instanceDao; @Mock HostDao hostDao; + @Mock + ManagementServiceConfiguration mgmtServiceConf; @InjectMocks VirtualMachinePowerStateSyncImpl virtualMachinePowerStateSync = new VirtualMachinePowerStateSyncImpl(); @@ -180,4 +185,87 @@ public void test_reportUnknownInstances_onlyReportsChanges() { public void test_reportUnknownInstances_noneEverSeen() { Assert.assertFalse(virtualMachinePowerStateSync.reportUnknownInstances(2L, new HashSet<>())); } + + private VMInstanceVO missingInstance(long id, Date updateTime) { + VMInstanceVO instance = Mockito.mock(VMInstanceVO.class); + Mockito.lenient().when(instance.getId()).thenReturn(id); + Mockito.lenient().when(instance.getUuid()).thenReturn("uuid-" + id); + Mockito.lenient().when(instance.getPowerStateUpdateTime()).thenReturn(null); + Mockito.lenient().when(instance.getUpdateTime()).thenReturn(updateTime); + return instance; + } + + /** + * The instance changed state a moment ago, so an in-flight report that does not mention it proves nothing. + * This must hold even for a forced report, which is the bug being fixed: it fails if the + * hasRecentStateChange() call in processMissingVmReport() is removed. + */ + @Test + public void test_processMissingVmReport_forcedReportDoesNotOverrideRecentStateChange() { + VMInstanceVO instance = missingInstance(1L, new Date(System.currentTimeMillis() - 5000L)); + Mockito.when(mgmtServiceConf.getPingInterval()).thenReturn(60); + Mockito.when(instanceDao.findByHostInStatesExcluding(Mockito.anyLong(), Mockito.anyCollection(), + Mockito.eq(VirtualMachine.State.Running), Mockito.eq(VirtualMachine.State.Stopping), + Mockito.eq(VirtualMachine.State.Starting))).thenReturn(Collections.singletonList(instance)); + + virtualMachinePowerStateSync.processHostVmStatePingReport(1L, new HashMap<>(), true); + + Mockito.verify(instanceDao, Mockito.never()).updatePowerState(Mockito.anyMap(), Mockito.anyLong(), + Mockito.any(Date.class)); + } + + /** + * Same forced report, but the instance has been in its current state for longer than the graceful period. + * It is genuinely missing and must still be reported, so out-of-band stop detection keeps working. + */ + @Test + public void test_processMissingVmReport_forcedReportStillReportsSettledInstance() { + VMInstanceVO instance = missingInstance(1L, new Date(System.currentTimeMillis() - 600000L)); + Mockito.when(mgmtServiceConf.getPingInterval()).thenReturn(60); + Mockito.when(instanceDao.findByHostInStatesExcluding(Mockito.anyLong(), Mockito.anyCollection(), + Mockito.eq(VirtualMachine.State.Running), Mockito.eq(VirtualMachine.State.Stopping), + Mockito.eq(VirtualMachine.State.Starting))).thenReturn(Collections.singletonList(instance)); + Mockito.when(instanceDao.updatePowerState(Mockito.anyMap(), Mockito.anyLong(), + Mockito.any(Date.class))).thenReturn(new HashMap<>()); + + virtualMachinePowerStateSync.processHostVmStatePingReport(1L, new HashMap<>(), true); + + Mockito.verify(instanceDao, Mockito.times(1)).updatePowerState( + Mockito.argThat(states -> VirtualMachine.PowerState.PowerReportMissing.equals(states.get(1L))), + Mockito.eq(1L), Mockito.any(Date.class)); + } + + /** + * An unforced report must keep honouring the graceful period for a settled instance that is only briefly absent. + */ + @Test + public void test_processMissingVmReport_unforcedReportHonoursGracefulPeriod() { + Mockito.when(mgmtServiceConf.getPingInterval()).thenReturn(60); + VMInstanceVO instance = missingInstance(1L, new Date(System.currentTimeMillis() - 5000L)); + Mockito.when(instanceDao.findByHostInStatesExcluding(Mockito.anyLong(), Mockito.anyCollection(), + Mockito.eq(VirtualMachine.State.Running), Mockito.eq(VirtualMachine.State.Stopping), + Mockito.eq(VirtualMachine.State.Starting))).thenReturn(Collections.singletonList(instance)); + Mockito.lenient().when(instanceDao.isPowerStateUpToDate(instance)).thenReturn(true); + + virtualMachinePowerStateSync.processHostVmStatePingReport(1L, new HashMap<>(), false); + + Mockito.verify(instanceDao, Mockito.never()).updatePowerState(Mockito.anyMap(), Mockito.anyLong(), + Mockito.any(Date.class)); + } + + /** + * An unknown name in a report must be recorded, so a second sighting of the same set says nothing new. + */ + @Test + public void test_convertVmStateReport_recordsUnknownInstances() { + Map report = new HashMap<>(); + report.put("i-2-2-VM", new HostVmStateReportEntry(VirtualMachine.PowerState.PowerOn, "host")); + Mockito.when(instanceDao.getNameIdMapForVmInstanceNames(Mockito.anyCollection())) + .thenReturn(new HashMap<>()); + + virtualMachinePowerStateSync.convertVmStateReport(1L, report); + + Assert.assertFalse(virtualMachinePowerStateSync.reportUnknownInstances(1L, + new HashSet<>(Arrays.asList("i-2-2-VM")))); + } } From 4748babd667b341044d1e3fd9ef4a6af983a1b33 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 19:10:55 +0000 Subject: [PATCH 09/11] engine: do not force the stop for a missing power report `sendStop()` swallows `AgentUnavailableException` and `OperationTimedoutException` and reports success when `force` is true: } catch (final AgentUnavailableException | OperationTimedoutException e) { if (!force) { return new Pair<>(false, errorMsg); } } return new Pair<>(true, null); So a host that is unreachable or too slow to answer counts as a successful stop, and the caller goes on to release the NICs and IP addresses. A host too busy to answer is exactly the condition that produces the stale report this branch is reacting to, so that is the worst case to guess in. | report | force | on an unreachable host | |---------------------|-------|-------------------------------| | PowerOff | true | unchanged, host said it is down | | PowerReportMissing | false | back off, keep the addresses, let a later report decide | A PowerOff report is the host stating the instance is down. A missing report only means the host did not list it, which is not the same thing. Also makes `handlePowerOffReportWithNoPendingJobsOnVM()` protected so this branch can be tested. Signed-off-by: Brad House --- .../java/com/cloud/vm/VirtualMachineManagerImpl.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 3b79e2486c44..7da1bca4ed3e 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -5542,7 +5542,7 @@ protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm) { vm.getInstanceName(), powerHostId, vm.getState())); } - private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { + protected void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { switch (vm.getState()) { case Starting: case Stopping: @@ -5570,9 +5570,15 @@ private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) { // on the host before giving up its resources, otherwise a still-running instance keeps its NICs and IP // addresses while the database says they are free and they get handed to another instance. if (PowerState.PowerOff.equals(vm.getPowerState()) || PowerState.PowerReportMissing.equals(vm.getPowerState())) { + // force is false for a missing report. sendStop() swallows AgentUnavailableException and + // OperationTimedoutException and answers success when forced, and a host too busy to answer is + // exactly the condition that produced the stale report in the first place. Backing off and letting a + // later report decide is better than freeing an address on no evidence. A PowerOff report is the + // host stating the instance is down, so that path keeps its previous behaviour. + final boolean forceStop = PowerState.PowerOff.equals(vm.getPowerState()); final VirtualMachineGuru vmGuru = getVmGuru(vm); final VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); - Pair result = sendStop(vmGuru, profile, true, true); + Pair result = sendStop(vmGuru, profile, forceStop, true); if (!result.first()) { logger.warn("Unable to stop VM {} on its host, not releasing its resources: {}", vm, result.second()); return; From 2b38e6007aca0d2fe71e2c576780d2ecc8a1051e Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 19:11:05 +0000 Subject: [PATCH 10/11] engine: alert when a host reports instances CloudStack does not know about A warn line only helps someone already reading the log. Raise an ALERT_TYPE_SYNC alert as well, so the condition reaches whoever watches alerts. The alert follows the same rule as the log: raised when the set of unknown instances on a host changes, not on every report. Nothing is stopped automatically. An unknown instance may be a domain an operator put on the host deliberately. Signed-off-by: Brad House --- .../vm/VirtualMachinePowerStateSyncImpl.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java index a412d0fe6b4c..8a8fb7db6de5 100644 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java @@ -36,6 +36,7 @@ import org.apache.logging.log4j.Logger; import com.cloud.agent.api.HostVmStateReportEntry; +import com.cloud.alert.AlertManager; import com.cloud.configuration.ManagementServiceConfiguration; import com.cloud.host.Host; import com.cloud.host.HostVO; @@ -50,6 +51,9 @@ public class VirtualMachinePowerStateSyncImpl implements VirtualMachinePowerStat @Inject VMInstanceDao _instanceDao; @Inject HostDao hostDao; @Inject ManagementServiceConfiguration mgmtServiceConf; + @Inject AlertManager _alertMgr; + + protected static final String UNKNOWN_INSTANCES_ALERT_SUBJECT = "Instances running on a host that CloudStack has no record of"; private final Map> unknownInstancesByHost = new ConcurrentHashMap<>(); @@ -245,9 +249,18 @@ protected boolean reportUnknownInstances(long hostId, Set unknownInstanc return true; } unknownInstancesByHost.put(hostId, unknownInstanceNames); + HostVO host = hostCache.get(hostId); + String names = String.join(", ", unknownInstanceNames); logger.warn("Host reports {} instance(s) that do not exist in CloudStack DB, they are running unmanaged. " + "host: {}, instances: [{}]", - unknownInstanceNames.size(), hostCache.get(hostId), String.join(", ", unknownInstanceNames)); + unknownInstanceNames.size(), host, names); + if (host != null) { + _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_SYNC, host.getDataCenterId(), host.getPodId(), + UNKNOWN_INSTANCES_ALERT_SUBJECT, + String.format("Host %s reports %d instance(s) that do not exist in CloudStack: %s. They are " + + "running unmanaged and may still be holding addresses and storage.", + host.getName(), unknownInstanceNames.size(), names)); + } return true; } From 71e7a576497c561955d8da43c0966bffa2af0db7 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 19:11:05 +0000 Subject: [PATCH 11/11] engine: cover the stop paths that decide whether an address is freed These are the branches that decide whether a still-running instance keeps its addresses, and they had no tests. `VirtualMachineManagerImplTest`: | test | checks | |-----------------------------------------------|-----------------------------------------------| | missingReportUsesUnforcedStopAndKeepsResources | a missing report stops unforced, and a failed stop does not release resources | | powerOffKeepsForcedStop | a PowerOff report still stops forced | | ensure...skipsExternal | External instances are left to their extension | | ensure...noHostIdDoesNothing | nothing is sent when there is no host to send to | | ensure...fallsBackToLastHostId | the last known host is used when `host_id` is cleared | | ensure...agentUnavailableIsSwallowed | an unreachable host does not break the expunge | `VirtualMachinePowerStateSyncImplTest` gains the AlertManager mock the alert needs. Signed-off-by: Brad House --- .../vm/VirtualMachineManagerImplTest.java | 82 +++++++++++++++++++ .../VirtualMachinePowerStateSyncImplTest.java | 3 + 2 files changed, 85 insertions(+) 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..68abdcea18a8 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; @@ -2049,4 +2050,85 @@ public void testUnmanageSuccessKvm() throws Exception { } } + @Test + public void testEnsureInstanceIsStoppedOnLastKnownHost_skipsExternal() throws Exception { + when(vmInstanceMock.getHypervisorType()).thenReturn(HypervisorType.External); + virtualMachineManagerImpl.ensureInstanceIsStoppedOnLastKnownHost(vmInstanceMock); + verify(agentManagerMock, never()).send(anyLong(), any(StopCommand.class)); + } + + @Test + public void testEnsureInstanceIsStoppedOnLastKnownHost_noHostIdDoesNothing() throws Exception { + when(vmInstanceMock.getHypervisorType()).thenReturn(HypervisorType.KVM); + when(vmInstanceMock.getHostId()).thenReturn(null); + when(vmInstanceMock.getLastHostId()).thenReturn(null); + virtualMachineManagerImpl.ensureInstanceIsStoppedOnLastKnownHost(vmInstanceMock); + verify(agentManagerMock, never()).send(anyLong(), any(StopCommand.class)); + } + + @Test + public void testEnsureInstanceIsStoppedOnLastKnownHost_fallsBackToLastHostId() throws Exception { + StopCommand stopCommand = mock(StopCommand.class); + when(vmInstanceMock.getHypervisorType()).thenReturn(HypervisorType.KVM); + when(vmInstanceMock.getHostId()).thenReturn(null); + when(vmInstanceMock.getLastHostId()).thenReturn(7L); + doReturn(stopCommand).when(virtualMachineManagerImpl).buildStopCommand(any(), any(), eq(false)); + com.cloud.agent.api.Answer answer = mock(com.cloud.agent.api.Answer.class); + when(answer.getResult()).thenReturn(true); + when(agentManagerMock.send(eq(7L), any(StopCommand.class))).thenReturn(answer); + + virtualMachineManagerImpl.ensureInstanceIsStoppedOnLastKnownHost(vmInstanceMock); + + verify(agentManagerMock, times(1)).send(eq(7L), any(StopCommand.class)); + } + + @Test + public void testEnsureInstanceIsStoppedOnLastKnownHost_agentUnavailableIsSwallowed() throws Exception { + StopCommand stopCommand = mock(StopCommand.class); + when(vmInstanceMock.getHypervisorType()).thenReturn(HypervisorType.KVM); + when(vmInstanceMock.getHostId()).thenReturn(7L); + doReturn(stopCommand).when(virtualMachineManagerImpl).buildStopCommand(any(), any(), eq(false)); + when(agentManagerMock.send(anyLong(), any(StopCommand.class))) + .thenThrow(new AgentUnavailableException("down", 7L)); + + // must not propagate, expunge continues + virtualMachineManagerImpl.ensureInstanceIsStoppedOnLastKnownHost(vmInstanceMock); + + verify(agentManagerMock, times(1)).send(anyLong(), any(StopCommand.class)); + } + + /** + * A missing report is weak evidence, so the stop must not be forced: sendStop() answers success for an + * unreachable host when forced, which would free the addresses of an instance that is still running. + */ + @Test + public void testHandlePowerOffReport_missingReportUsesUnforcedStopAndKeepsResources() { + when(vmInstanceMock.getState()).thenReturn(VirtualMachine.State.Migrating); + when(vmInstanceMock.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerReportMissing); + doReturn(mock(VirtualMachineGuru.class)).when(virtualMachineManagerImpl).getVmGuru(any()); + doReturn(new Pair<>(false, "host did not answer")).when(virtualMachineManagerImpl) + .sendStop(any(), any(), eq(false), eq(true)); + + virtualMachineManagerImpl.handlePowerOffReportWithNoPendingJobsOnVM(vmInstanceMock); + + verify(virtualMachineManagerImpl, times(1)).sendStop(any(), any(), eq(false), eq(true)); + verify(virtualMachineManagerImpl, never()).releaseVmResources(any(), anyBoolean()); + } + + /** + * A PowerOff report is the host stating the instance is down, so that path keeps its forced stop. + */ + @Test + public void testHandlePowerOffReport_powerOffKeepsForcedStop() { + when(vmInstanceMock.getState()).thenReturn(VirtualMachine.State.Migrating); + when(vmInstanceMock.getPowerState()).thenReturn(VirtualMachine.PowerState.PowerOff); + doReturn(mock(VirtualMachineGuru.class)).when(virtualMachineManagerImpl).getVmGuru(any()); + doReturn(new Pair<>(false, "host did not answer")).when(virtualMachineManagerImpl) + .sendStop(any(), any(), eq(true), eq(true)); + + virtualMachineManagerImpl.handlePowerOffReportWithNoPendingJobsOnVM(vmInstanceMock); + + verify(virtualMachineManagerImpl, times(1)).sendStop(any(), any(), eq(true), eq(true)); + verify(virtualMachineManagerImpl, never()).releaseVmResources(any(), anyBoolean()); + } } diff --git a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java index 189668b43627..31371ae50328 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachinePowerStateSyncImplTest.java @@ -36,6 +36,7 @@ import org.mockito.junit.MockitoJUnitRunner; import com.cloud.agent.api.HostVmStateReportEntry; +import com.cloud.alert.AlertManager; import com.cloud.configuration.ManagementServiceConfiguration; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; @@ -51,6 +52,8 @@ public class VirtualMachinePowerStateSyncImplTest { HostDao hostDao; @Mock ManagementServiceConfiguration mgmtServiceConf; + @Mock + AlertManager alertManager; @InjectMocks VirtualMachinePowerStateSyncImpl virtualMachinePowerStateSync = new VirtualMachinePowerStateSyncImpl();