engine: do not orphan an instance on a forced power report during start

[bhouse-nexthop]

Description

An instance can end up running on a KVM host with no record in CloudStack. Its IP address is released and later handed to another instance, so two instances answer for the same address. Its root volume stays in Destroy and cannot be deleted while the domain holds it.

Fixes: #14206

The trigger is the out-of-band ping the KVM agent sends when another instance on the same host shuts down or crashes. That ping is processed with force, which skips the graceful period. The KVM report lists only powered-on domains, so an instance that is still starting is absent from it and is marked PowerReportMissing.

Sequence:

time	what happens
20:23:5x	DeployVM starts, instance -> Starting, StartCommand sent
20:25:14	another instance crashes; agent collects a report and sends it with force
20:25:16	the new domain is created on the host
20:25:17	StartAnswer arrives, instance -> Running
20:25:18	the 20:25:14 report is processed; instance is not in it -> PowerReportMissing
	NICs and IP released, instance -> Stopped. No StopCommand is sent
20:25:19	the deploy job finds it Stopped, logs an error, instance -> Error, job fails
20:25:28	the failed deploy is followed by an automatic destroy with expunge; advanceStop() returns at once because state is Error
20:25:39	expunged and removed. No StopCommand was ever sent

One commit per finding, so they can be reviewed independently.

commit	change
1	do not report an instance missing when its state changed within the graceful period, forced or not
2	send a StopCommand before releasing resources on a missing report, as the PowerOff branch already does
3	on a power-on report for a Destroyed or Expunging instance, stop it on the reporting host; alert for Error
4	send a StopCommand to the last known host before expunging, whatever the database state says
5	log instances a host reports that CloudStack has no record of at warn level, once per change
6	unit tests
7	build the new stop commands the way sendStop() does, so the host gets the VLAN persistence map, external details, control IP and volumes to disconnect
8	make the missing-report tests actually exercise the fix
9	do not force the stop for a missing report, so an unreachable host does not cause an address to be freed
10	raise an alert, not just a log line, for unknown instances
11	tests for the branches that decide whether an address is freed

Commit 1 is the root cause. Commits 2 to 4 are independent backstops, each of which would have prevented the end result on its own.

Behaviour that does not change:

• an instance that has been Running for a while and then shuts itself down is still detected immediately, so out-of-band stop detection keeps working
• nothing is stopped automatically for an unknown instance, it is only made visible, since an operator may have put a domain on the host deliberately

Known limitations

• The guard compares wall-clock elapsed time against 2 x ping.interval. The exact invariant is "this report was collected before the last state change". Stamping the report with its collection time and comparing against update_time would be precise; that needs a field on PingRoutingCommand and is better done separately.
• For an out-of-band migration, a successful StopAnswer from the old host proves the domain is not on that host, not that the instance is gone. Not a regression: the previous code released the resources unconditionally.
• UserVmManagerImpl.expunge() calls _networkMgr.release() before advanceExpunge(), so by the time the expunge-time StopCommand is sent the addresses have already been released. Moving the stop ahead of that release would close the remaining window, in a separate change.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

• unit tests added for the sync changes, VirtualMachinePowerStateSyncImplTest passes (11 tests)
• the condition was traced on a running 4.22 deployment: agent logs show the out-of-band pings lining up one for one with the management server processing each report, including one collected two seconds before the domain existed and processed one second after the start completed
• on the affected hosts the forced pings arrived every few seconds, so the graceful period almost never applied

How did you try to break this feature and the system with this change?

• checked that checkBeforeCleanup is set correctly for each new StopCommand. It makes the host refuse to stop a running domain, so it is false where a running domain is what must be removed (commits 3 and 4) and true where the point is to back off if the instance turns out to be alive (commit 2)
• checked the missing-report change does not weaken out-of-band stop detection. The guard only applies within the graceful period of a state change, and an instance that has been Running is well outside it
• sendStop(..., force=true, ...) swallows AgentUnavailableException and OperationTimedoutException and answers success, so an unreachable host would have had its addresses released. Commit 9 passes force=false for a missing report; PowerOff keeps force=true, which is its existing behaviour
• checked what an expunge costs now: it adds a StopCommand round trip for every instance, including ones already stopped. KVM answers "Domain not found" and still runs disk and interface cleanup, so it is correct but visible on busy hosts

[codecov]

Codecov Report

❌ Patch coverage is 62.76596% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.00%. Comparing base (a8c8c18) to head (71e7a57).

Files with missing lines	Patch %	Lines
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java	40.00%	30 Missing and 3 partials ⚠️
...com/cloud/vm/VirtualMachinePowerStateSyncImpl.java	94.87%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14207      +/-   ##
============================================
+ Coverage     17.97%   18.00%   +0.03%     
- Complexity    16184    16219      +35     
============================================
  Files          5930     5930              
  Lines        535615   535695      +80     
  Branches      65582    65594      +12     
============================================
+ Hits          96271    96465     +194     
+ Misses       428377   428241     -136     
- Partials      10967    10989      +22     

Flag	Coverage Δ
uitests	4.02% <ø> (ø)
unittests	19.08% <62.76%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bhouse-nexthop]

Self-review follow-up. Two defects found in the new stop paths, both fixed.

1. The new StopCommands were built by hand and missed what sendStop() sets.

sendStop() enriches the command with the external hypervisor details, the VLAN persistence map, the control NIC address and the volumes to disconnect. The two new helpers called new StopCommand(...) directly and set none of it. Consequences:

omission	effect
vlanToPersistenceMap	LibvirtComputingResource.shouldDeleteBridge() returns true for an empty map, so the host deletes bridges belonging to persistent networks
external hypervisor details	ExternalPathPayloadProvisioner dereferences cmd.getVirtualMachine(), which would be null -> NPE on every External expunge
controlIp	the cmdline backup for system VMs silently does not happen
volumesToDisconnect	volumes left connected on the host

Fixed by extracting buildStopCommand() out of sendStop() and using it in both helpers, so there is one place that knows how to build a StopCommand. External instances are now skipped in both helpers: their teardown is done by the extension in finalizeExpunge.

2. The new tests did not test the fix.

The processMissingVmReport stub used a single Mockito.any() for a varargs parameter that receives three states, so it never matched. findByHostInStatesExcluding returned Mockito's default empty list, the method returned early, and the never() assertions passed without exercising anything.

Fixed by matching the varargs explicitly. Verified by mutation: disabling the hasRecentStateChange guard now makes test_processMissingVmReport_forcedReportDoesNotOverrideRecentStateChange fail, which is the point of the test.

Also in this push:

• the "Detected missing VM" debug line was logged before the skip; moved below it
• removed a duplicated Javadoc block left by a rebase

Known limitations, deliberately not addressed here:

• The guard assumes an in-flight report is at most 2 x ping.interval stale. If management-server queueing delays processing beyond that, the race can still occur. The robust fix is a collection timestamp on PingRoutingCommand, which is a larger change and better done separately.
• For an out-of-band migration, a successful StopAnswer from the old host proves the domain is not there, not that the instance is gone. Not a regression, the previous code released resources unconditionally.

[bhouse-nexthop]

Three further changes pushed.

1. The stop for a missing report was forced. sendStop() swallows the agent exceptions and answers success when forced:

} catch (final AgentUnavailableException | OperationTimedoutException e) {
    if (!force) { return new Pair<>(false, errorMsg); }
}
return new Pair<>(true, null);

So an unreachable or slow host counted as a successful stop and the addresses were released. A host too busy to answer is the condition that produces the stale report in the first place.

report	force	unreachable host
PowerOff	true	unchanged, the host said it is down
PowerReportMissing	false	back off, keep the addresses

2. Unknown instances now raise an ALERT_TYPE_SYNC alert, not only a log line, on the same once-per-change rule. Nothing is stopped automatically.

3. Tests for the branches that decide whether an address is freed: the PowerReportMissing path (unforced stop, failed stop does not release resources) and ensureInstanceIsStoppedOnLastKnownHost (External skipped, no host id, last_host_id fallback, agent unavailable).

Not changed: aborting the expunge when the stop fails. UserVmManagerImpl.expunge() calls _networkMgr.release() before advanceExpunge(), so the addresses are already released by then, and a failed expunge sends the instance to Error via transitionExpungingToError(), where the expunge sweep does not pick it up. That would be worse than the current behaviour. The ordering in UserVmManagerImpl is the real fix and belongs in its own change; it is listed under Known limitations in the description, along with the report-collection-timestamp follow-up.