bhouse-nexthop commented on PR #14207: URL: https://github.com/apache/cloudstack/pull/14207#issuecomment-5734225546
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. -- 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]
