bhouse-nexthop opened a new pull request, #14233: URL: https://github.com/apache/cloudstack/pull/14233
### Description With `vm.destroy.forcestop=true`, destroying an instance whose host is briefly disconnected 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 cannot reach the host treats the instance as stopped. That is right for a host that is gone, and wrong for one that is `Disconnected`, `Connecting`, `Alert` or `Rebalancing`, for example while its agent or a management server restarts. `vm.destroy.forcestop` applies that to every destroy. This change keeps `vm.destroy.forcestop` but does not force the stop while the instance's host is in one of those states. The stop is then an ordinary one: it fails, the instance goes back to `Running`, the destroy returns an error, and the caller can retry once the host is `Up`, or is `Down` and the stop can be forced. | host status | before | after | |-----------------------------------------------|--------------------------|-------------------------------| | Up | forced, as configured | unchanged | | Down, Removed, Error, or no host | forced, as configured | unchanged | | Connecting, Disconnected, Alert, Rebalancing | forced: resources released, domain left running | not forced: destroy fails, instance stays Running | The three destroy paths that read `vm.destroy.forcestop` now go through `VirtualMachineManager.shouldForceStopOnDestroy()`: - `UserVmManagerImpl.destroyVm(DestroyVMCmd)` - `VirtualMachineManagerImpl.destroy()` - `VirtualMachineManagerImpl.advanceExpunge()` The duplicate `vm.destroy.forcestop` ConfigKey in `UserVmManagerImpl` is removed. It was not registered there; the one in `VirtualMachineManagerImpl` is the registered key. Behaviour that does **not** change: - an explicit forced stop (`stopVirtualMachine forced=true`), HA, and host removal still force the stop regardless of host status. Those are a caller saying the instance is to be treated as stopped - with `vm.destroy.forcestop=false` nothing changes ### Known limitations - The host status is read before the stop is sent. A host that disconnects in between still gets a forced stop. That window is the round trip of one command, not the minute-long disconnect of an agent restart. - An instance on a host that stays `Disconnected` (agent down, host alive) can no longer be destroyed with `vm.destroy.forcestop` alone. Stop it with `forced=true` first, which is an explicit decision, then destroy it. ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] 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 - [x] Critical - [ ] Major - [ ] Minor - [ ] Trivial ### How Has This Been Tested? - unit tests for `shouldForceStopOnDestroy()`: setting off, host Up, host Down/Removed/Error, host Connecting/Disconnected/Alert/Rebalancing, no host, host record gone. `VirtualMachineManagerImplTest` passes - traced on a running 4.22 deployment: during one rolling agent and management server upgrade, 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 #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? - checked the unforced path when the host is unreachable: `advanceStop()` transitions the instance back with `OperationFailed` and throws, so the destroy fails before `destroyVm()` or the expunge release anything - checked the callers that force a stop for their own reasons (`stopVirtualMachine forced=true`, HA, host deletion) do not go through `shouldForceStopOnDestroy()` and are unaffected -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
