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

[bhouse-nexthop]

Description

With vm.destroy.forcestop=true, destroying an instance whose host is briefly unreachable releases the instance's NICs, addresses and volumes without stopping it. The domain keeps running with no record in CloudStack, and its address is handed to the next instance.

Fixes #14232

A forced stop that gets no answer from the host treats the instance as stopped. That is right for a host that is gone, and wrong for one that is only unreachable for a while: an agent or management server restart, a management server crash before its peers take over its hosts, or an inconclusive disconnect investigation. vm.destroy.forcestop applies that to every destroy.

This change keeps vm.destroy.forcestop and keeps the stop forced, so a host that answers still gets a hard stop. What changes is what a destroy does when the host does not confirm the stop. The decision is made at that point, not before the stop is queued:

when a destroy's stop is not confirmed by the host	before	after
host is none, record gone, Down or Removed	resources released	unchanged
any other host status, StopCommand unanswered	resources released, domain left running	stop fails, instance left as it was, destroy fails and can be retried
any other host status, host answers that it could not stop	resources released, domain left running	same as above
instance stalled in Starting or Migrating	released whatever the host answered	an ordinary stop is tried first; released only once the host confirms it, or if the host is gone

commit	change
1	first version: decided from the host status before the stop. Superseded by 2
2	VirtualMachineManager.advanceStopForDestroy(): the stop is forced per vm.destroy.forcestop, and a releaseOnlyIfHostIsGone flag on VmWorkStop makes advanceStop() release without the host's confirmation only when the host is gone
3	account cleanup no longer expunges an instance whose destroy failed while it is still on its host
4	second review: stalled instances try an ordinary stop before the forced cleanup instead of deciding from host status; account cleanup defers the rest of the account while such an instance remains; one host lookup in the release decision; tests for the flag through the job queue and serialization

Commit 1 is kept so the review history makes sense; it can be squashed on merge.

Every path that stops an instance to destroy it now goes through advanceStopForDestroy():

• UserVmManagerImpl.destroyVm(DestroyVMCmd)
• VirtualMachineManagerImpl.destroy(), also used by host deletion with forcedestroylocalstorage
• VirtualMachineManagerImpl.advanceExpunge(), which also expunges virtual routers (network restart with cleanup, network deletion, network GC), CPVM/SSVM and internal load balancer VMs

HA destroy is not affected: it stops the instance with an explicit forced stop before destroying it.

Behaviour that does not change:

• an explicit forced stop (stopVirtualMachine forced=true, HA) still releases regardless of the host. That is a caller stating the instance is to be treated as stopped
• with vm.destroy.forcestop=false nothing changes
• a VmWorkStop job queued before the upgrade deserializes with the flag false and keeps the previous behaviour; one queued by an upgraded management server and run by an old one is read without the flag, as before

Known limitations

• An instance on a host that stays unreachable without being marked Down, for example Alert after an inconclusive investigation, can no longer be destroyed with vm.destroy.forcestop alone. Stop it with forced=true first, which is an explicit decision (and needs allow.user.force.stop.vm for non-admins), then destroy it. The same applies to routers: restartNetwork cleanup=true and network GC fail on such a host until the router is stopped with forced=true.
• A hung agent on an Up host now makes the destroy fail rather than release. That is intended: the host may still be running the domain.
• A stop request joins a stop job already pending for the same instance, whatever flags that job was queued with. This is existing behaviour for the cleanup flag too. An explicit forced stop that joins a destroy's stop inherits its caution, and a destroy that joins an explicit forced stop inherits the force.
• Account cleanup defers an instance whose destroy failed for any reason while it still has a host, not only when the host is unreachable, and the rest of that account's cleanup waits for it. The account is retried every account.cleanup.interval.
• destroyVm(DestroyVMCmd) with expunge=true removes the backup offering before the stop. If the stop now fails, the instance stays without it. That ordering predates this 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 through advanceStop(): a destroy stop that cannot reach a Disconnected host fails, does not call releaseVmResources() and leaves the instance Running; with the host Down it releases; an explicit forced stop still releases on a Disconnected host
• unit tests for a stalled (Starting) instance: no cleanup when the host does not confirm, cleanup once an Up host confirms, cleanup without asking a Down host
• unit tests that the flag reaches the queued VmWorkStop, that the job handler and the in-job path pass it on, and that it survives VmWorkSerializer (a job without it reads false)
• unit tests for isHostGone() over every host status, mayReleaseWithoutHostConfirmation(), advanceStopForDestroy(), the three destroy paths, the force-stop permission check, and the account cleanup guard and deferral
• mutation-tested: dropping the flag at each step of the job queue, dropping the stalled-instance stop, restoring the old release check, or treating Up or Disconnected as gone each fail a test
• VirtualMachineManagerImplTest (115), UserVmManagerImplTest and AccountManagerImplTest pass
• traced on a running 4.22 deployment: during two rolling agent and management server upgrades, 31 instances were left running unmanaged, each preceded by Unable to actually stop ... but continue with release because it's a force stop with AgentUnavailableException ... Disconnected for a destroy. 8 of them were destroyed by management servers that already had engine: do not orphan an instance on a forced power report during start #14207, whose expunge-time StopCommand also could not reach the host

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

Two adversarial reviews, each checked against the code:

• the first found that deciding from the host status before the stop misses hosts that are unreachable while still Up, and that the decision was baked into a job that may run much later. Commit 2 moves the decision to where the stop goes unconfirmed. It also found that account cleanup expunges an instance whose destroy failed, and expunge releases network resources before it stops the instance (commit 3)
• the second found that stalled instances were still decided from host status, refusing even when an Up host would answer; that account cleanup went on to remove the security groups, networks and resource counts of an instance it had left running; a race where a host removed between two lookups caused an NPE in a finally; and that the job-queue plumbing of the flag was untested. Commit 4 addresses all four
• checked the flag across mixed-version clusters: VmWorkStop is Java-serialized with an unchanged serialVersionUID, and a stream without the field reads false

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.01%. Comparing base (ac8d69c) to head (6a27180).

Files with missing lines	Patch %	Lines
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java	81.81%	5 Missing and 3 partials ⚠️
...tration/src/main/java/com/cloud/vm/VmWorkStop.java	85.71%	1 Missing ⚠️
...c/main/java/com/cloud/user/AccountManagerImpl.java	93.33%	0 Missing and 1 partial ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	90.90%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14233      +/-   ##
============================================
+ Coverage     17.98%   18.01%   +0.02%     
- Complexity    16195    16234      +39     
============================================
  Files          5930     5930              
  Lines        535649   535714      +65     
  Branches      65590    65600      +10     
============================================
+ Hits          96343    96486     +143     
+ Misses       428330   428226     -104     
- Partials      10976    11002      +26     

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

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.