bhouse-nexthop commented on PR #14207:
URL: https://github.com/apache/cloudstack/pull/14207#issuecomment-5734948371
Thanks, this was a good catch on the `force` handling. Three changes pushed,
one item declined with reasoning.
**`force=true` on a missing report — you are right, fixed.** Confirmed in
`sendStop()`:
```java
} catch (final AgentUnavailableException | OperationTimedoutException e) {
if (!force) { return new Pair<>(false, errorMsg); }
}
return new Pair<>(true, null);
```
An unreachable or timing-out host answered success and the addresses were
released. The PR text claiming otherwise was wrong and is corrected. Now:
| report | force | unreachable host |
|--------------------|-------|--------------------------------------|
| PowerOff | true | unchanged, the host said it is down |
| PowerReportMissing | false | back off, keep the addresses |
Left `PowerOff` alone rather than changing its long-standing behaviour in a
bug fix.
**Alert on unknown instances — added**, `ALERT_TYPE_SYNC` alongside the
warn, on the same once-per-change rule. The opt-in setting to stop
`i-<acct>-<id>-VM` domains automatically is a sound idea but it is a new global
setting, so better as its own change than bolted onto a fix.
**Tests — added** for both branches you named: the `PowerReportMissing` path
(stop is unforced, a failed stop does not release resources) and
`ensureInstanceIsStoppedOnLastKnownHost` (External skipped, no host id,
`last_host_id` fallback, agent unavailable). 117 tests pass in the two classes.
**Aborting the expunge when the stop fails — declining, it would make things
worse here.** Two things in the path:
1. `UserVmManagerImpl.expunge()` calls `releaseNetworkResourcesOnExpunge()`
-> `_networkMgr.release(profile, false)` *before* `_itMgr.advanceExpunge()`.
The addresses are already released by the time the expunge-time StopCommand is
sent, so aborting there does not save the address.
2. A failed expunge is not a quiet retry. `expunge()` returns false,
`transitionExpungingToError()` moves the instance to **Error**, and a
`CloudRuntimeException` goes back to the caller. An Error instance is not
picked up by the expunge sweep, so it would sit with its addresses already
freed and its domain still running - worse than today, where it is at least
removed and shows up in the unknown-instance alert.
The ordering in `UserVmManagerImpl` is the real issue: the stop has to
happen before `_networkMgr.release()`. That is a change in a different class
and I would rather propose it separately than widen this PR. Noted as a known
limitation in the description.
Agreed on the collection-timestamp point - the wall-clock window is a proxy
for "this report predates the last state change", and stamping the report would
be exact. Also noted as a follow-up.
Fixed the description nits: the destroy is the provider reacting
automatically to the failed deploy, not a user action, and the added
StopCommand round trip on every expunge is now called out.
--
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]
