fix: handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy - #13976
Open
waterWang wants to merge 1 commit into
Open
fix: handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy#13976waterWang wants to merge 1 commit into
waterWang wants to merge 1 commit into
Conversation
DaanHoogland
requested changes
Aug 26, 2026
DaanHoogland
left a comment
Contributor
There was a problem hiding this comment.
too high comment/code ratio. please refactor sensible comments to the javadoc of the relevant interfaces and clarify remaining code with using good noun/verbs for identifiers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 theErrorstate. TheSTORAGE_SNAPSHOTdetail is removed during the creation rollback.When the user later tries to delete this failed snapshot,
StorageVMSnapshotStrategy.canHandle()returnsCANT_HANDLEbecause theSTORAGE_SNAPSHOTdetail is absent. The deletion falls through toDefaultVMSnapshotStrategy, which sends aDeleteVMSnapshotCommandto the KVM agent. For a stopped VM with RAW/RBD volumes, the libvirt domain does not exist, so the command fails with: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:canHandle(VMSnapshot): AllowErrorstate snapshots (that would otherwise be handled byStorageVMSnapshotStrategy) to skip theSTORAGE_SNAPSHOTdetail check. Since the detail is removed during the creation rollback, failed snapshots would otherwise be rejected.deleteVMSnapshotoverride: ForErrorstate storage snapshots, skip the hypervisorDeleteVMSnapshotCommand(which would fail on a stopped VM) and clean up the database record directly viadeleteVMSnapshotFromDB. This is safe because thetakeVMSnapshotrollback already removed any underlying storage snapshots that were created before the failure.FSM validation
The
Error → Event.ExpungeRequested → Expungingtransition is valid (confirmed inVMSnapshot.State).Testing
Errorstate snapshots with KVM + Disk type +VmStorageSnapshotKvmenabled are now handled byStorageVMSnapshotStrategyinstead ofDefaultVMSnapshotStrategyErrorstate, deletion bypasses the hypervisor command and cleans up the DB directlyReadystate snapshots continue to use the inheriteddeleteVMSnapshotflow (unchanged behavior)