Skip to content

fix: handle failed storage VM snapshot cleanup in StorageVMSnapshotStrategy - #13976

Open
waterWang wants to merge 1 commit into
apache:mainfrom
waterWang:fix/failed-storage-vm-snapshot-handling
Open

waterWang wants to merge 1 commit into
apache:mainfrom
waterWang:fix/failed-storage-vm-snapshot-handling

Conversation

@waterWang

Copy link
Copy Markdown

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)

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.41%. Comparing base (158fe4f) to head (c804316).
⚠️ Report is 40 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (158fe4f) and HEAD (c804316). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (158fe4f) HEAD (c804316)
unittests 1 0
Additional details and impacted files
@@              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 Coverage Δ
uitests 3.41% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: conflict/waiting

Development

Successfully merging this pull request may close these issues.

3 participants