You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "bhouse-nexthop (via GitHub)" <gi...@apache.org> on 2026/09/23 18:23:05 UTC
[PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on code in PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#discussion_r4274254051
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2514,6 +2569,16 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
if (!cleanUpEvenIfUnableToStop) {
throw new CloudRuntimeException("We cannot stop " + vm + " when it is in state " + vm.getState());
}
+ // cleanup() releases the resources whether or not the host answers. For a destroy on a host that is not
+ // gone, go ahead only once the host has confirmed the instance is stopped.
+ if (releaseOnlyIfHostIsGone && !isHostGone(vm.getHostId())) {
Review Comment:
Good catch, you're right. While Migrating, `hostId` is the destination and `lastHostId` the source (`migrate()` sets `vm.setLastHostId(srcHostId)` then moves to Migrating with `dstHostId`), and the check only looked at `hostId`. So with the destination Down and the source Up, it released.
Fixed in 8e3c815411: the destroy stop now refuses for a Migrating instance while the source host isn't gone:
```java
final Long migrationSourceHostId = releaseOnlyIfHostIsGone ? migrationSourceThatMayBeRunningIt(vm) : null;
if (migrationSourceHostId != null) {
throw new CloudRuntimeException("Unable to stop " + vm + " in state " + vm.getState()
+ ": the host it is migrating from may still be running it");
}
```
It only releases once both hosts are Down/Removed. An explicit `forced=true` stop works as before. Tests cover source Up/Disconnected/Alert (refused), both gone (released) and the explicit forced stop.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6423230818
> @DaanHoogland @weizhouapache @sureshanaparti gentle ping. This has been open since 09-23 and is on the 4.22.2 milestone. @Damans227 approved it on 10-14, so it still needs a committer review and a Trillian run. Could one of you take a look?
It also needs 3rd party testing @bhouse-nexthop
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6342327447
@DaanHoogland @weizhouapache could one of you review this for 4.22.2 and add it to the milestone? @Damans227 approved it on 10-14; it still needs a committer review and a test run. I understand 4.22.2 is due to go out Monday.
Heads up on merge order: this and #14207 both add tests at the end of `VirtualMachineManagerImplTest`, so whichever merges second will conflict there. I'll rebase that one as soon as the first is in.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-5800624428
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14233?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.02%. Comparing base ([`ac8d69c`](https://app.codecov.io/gh/apache/cloudstack/commit/ac8d69c5c1d7d2a30b9e49e8f484a365123557d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`568f604`](https://app.codecov.io/gh/apache/cloudstack/commit/568f604bf64240ab0f946bc98b3b4876fbb8be51?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (ac8d69c) and HEAD (568f604). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (ac8d69c) | HEAD (568f604) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #14233 +/- ##
=============================================
- Coverage 17.98% 4.02% -13.97%
=============================================
Files 5930 449 -5481
Lines 535649 38239 -497410
Branches 65590 7082 -58508
=============================================
- Hits 96343 1538 -94805
+ Misses 428330 36489 -391841
+ Partials 10976 212 -10764
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14233/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.02% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14233?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#discussion_r4274199506
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2514,6 +2569,16 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
if (!cleanUpEvenIfUnableToStop) {
throw new CloudRuntimeException("We cannot stop " + vm + " when it is in state " + vm.getState());
}
+ // cleanup() releases the resources whether or not the host answers. For a destroy on a host that is not
+ // gone, go ahead only once the host has confirmed the instance is stopped.
+ if (releaseOnlyIfHostIsGone && !isHostGone(vm.getHostId())) {
Review Comment:
for an instance stuck in migrating, this only checks the target host. what happens if the target is down but the old host is still running it? looks like it still gets released
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6425838812
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19757
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-5932046601
@vladimirpetrov @sureshanaparti could you take a look at this one? We'd like this fix to make it into the upcoming 4.22.2 release.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6438340050
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#discussion_r4274294353
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -2514,6 +2569,16 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
if (!cleanUpEvenIfUnableToStop) {
throw new CloudRuntimeException("We cannot stop " + vm + " when it is in state " + vm.getState());
}
+ // cleanup() releases the resources whether or not the host answers. For a destroy on a host that is not
+ // gone, go ahead only once the host has confirmed the instance is stopped.
+ if (releaseOnlyIfHostIsGone && !isHostGone(vm.getHostId())) {
Review Comment:
that covers it, thanks
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6423289922
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6438353079
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "bhouse-nexthop (via GitHub)" <gi...@apache.org>.
bhouse-nexthop commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6415668838
@DaanHoogland @weizhouapache @sureshanaparti gentle ping. This has been open since 09-23 and is on the 4.22.2 milestone. @Damans227 approved it on 10-14, so it still needs a committer review and a Trillian run. Could one of you take a look?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6423248404
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] engine: do not force the destroy stop while the host may reconnect [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14233:
URL: https://github.com/apache/cloudstack/pull/14233#issuecomment-6460947903
<b>[SF] Trillian test result (tid-17281)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 65537 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14233-t17281-kvm-ol8.zip
Smoke tests completed. 147 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_11_destroy_ssvm | `Error` | 960.58 | test_ssvm.py
ContextSuite context=TestVMWareStoragePolicies>:setup | `Error` | 0.00 | test_storage_policy.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org