You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "waterWang (via GitHub)" <gi...@apache.org> on 2026/08/26 04:47:08 UTC
[PR] fix: handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy [cloudstack]
waterWang opened a new pull request, #13976:
URL: https://github.com/apache/cloudstack/pull/13976
## Fix: Handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy
### Problem
When a KVM disk-only VM snapshot (created via `StorageVMSnapshotStrategy`) fails during creation — for example, when the QEMU guest agent is not connected and the freeze operation fails — the snapshot enters the `Error` state. The `STORAGE_SNAPSHOT` detail is removed during the creation rollback.
When the user later tries to delete this failed snapshot, `StorageVMSnapshotStrategy.canHandle()` returns `CANT_HANDLE` because the `STORAGE_SNAPSHOT` detail is absent. The deletion falls through to `DefaultVMSnapshotStrategy`, which sends a `DeleteVMSnapshotCommand` to the KVM agent. For a stopped VM with RAW/RBD volumes, the libvirt domain does not exist, so the command fails with:
```
Delete Instance Snapshot failed due to org.libvirt.LibvirtException:
Domain not found: no domain with matching name 'i-2-43-VM'
```
This is a RAW/RBD variant of issue #11673. PR #11687 fixed a similar case for QCOW2/stopped VMs, but the fallback only handles QCOW2 volumes.
### Fix
Two changes in `StorageVMSnapshotStrategy`:
1. **`canHandle(VMSnapshot)`**: Allow `Error` state snapshots (that would otherwise be handled by `StorageVMSnapshotStrategy`) to skip the `STORAGE_SNAPSHOT` detail check. Since the detail is removed during the creation rollback, failed snapshots would otherwise be rejected.
2. **`deleteVMSnapshot` override**: For `Error` state storage snapshots, skip the hypervisor `DeleteVMSnapshotCommand` (which would fail on a stopped VM) and clean up the database record directly via `deleteVMSnapshotFromDB`. This is safe because the `takeVMSnapshot` rollback already removed any underlying storage snapshots that were created before the failure.
### FSM validation
The `Error → Event.ExpungeRequested → Expunging` transition is valid (confirmed in `VMSnapshot.State`).
### Testing
- `Error` state snapshots with KVM + Disk type + `VmStorageSnapshotKvm` enabled are now handled by `StorageVMSnapshotStrategy` instead of `DefaultVMSnapshotStrategy`
- For `Error` state, deletion bypasses the hypervisor command and cleans up the DB directly
- `Ready` state snapshots continue to use the inherited `deleteVMSnapshot` flow (unchanged behavior)
--
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] fix: handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13976:
URL: https://github.com/apache/cloudstack/pull/13976#issuecomment-5602528885
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13976?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 3.41%. Comparing base ([`158fe4f`](https://app.codecov.io/gh/apache/cloudstack/commit/158fe4fad5328d5a6f42fabf30b9b2e0738d3949?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c804316`](https://app.codecov.io/gh/apache/cloudstack/commit/c8043166813c1d0eca57ae7a08bbbd867038812a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 40 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (158fe4f) and HEAD (c804316). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (158fe4f) | HEAD (c804316) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13976 +/- ##
=============================================
- Coverage 19.74% 3.41% -16.33%
=============================================
Files 6371 487 -5884
Lines 575784 41863 -533921
Branches 70478 7912 -62566
=============================================
- Hits 113665 1429 -112236
+ Misses 449765 40234 -409531
+ Partials 12354 200 -12154
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13976/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/13976/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13976/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/13976?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