Fix instance snapshots with KvmFileBasedStorageVmSnapshotStrategy by sureshanaparti · Pull Request #14515 · apache/cloudstack

sureshanaparti · 2026-10-28T12:17:58Z

Description

This PR fixes the instance snapshots with KvmFileBasedStorageVmSnapshotStrategy.

KvmFileBasedStorageVmSnapshotStrategy.canHandle() rejected any snapshot past Allocated that lacked the kvmFileBasedStorageSnapshot detail, but that detail is only persisted after a successful create. A failed create (e.g. quiescevm=true on a guest without a running agent) therefore left an Error row no strategy would claim once DefaultVMSnapshotStrategy was excluded. deleteVMSnapshot then failed, and deleteAllVMSnapshots made destroyVirtualMachine fail too, leaving the instance undeletable.
The strategy now tags every snapshot it starts creating with a kvmFileBasedStorageSnapshotStrategy detail, so it recognises its own rows in any later state regardless of whether the VM is still eligible for disk-only snapshots. Error rows that predate the marker fall back to the creation eligibility check.
Deleting a row that never recorded volume snapshots only removes its metadata: nothing reached primary storage and no create usage events were published, so no host command or delete usage events are issued.

Fixes #14481

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

The new metadata-only deletion path is selected solely based on the absence of… · New
The new metadata-only deletion path is selected solely based on the absence of… · New
Given the new branching around ‘volume snapshot details’ vs ‘strategy marker’, it would be valuable… · New
The tests introduced several string literals for snapshot detail keys… · New
The tests introduced several string literals for snapshot detail keys… · New
The tests introduced several string literals for snapshot detail keys… · New

What changed in this PR

Fixes KVM file-based VM snapshot lifecycle issues where failed snapshot creates could leave unclaimed Error rows and block instance deletion, by tagging snapshots early and adjusting handling/deletion behavior.

Changes:

Persist a strategy marker detail early during snapshot creation so later Error rows are still recognized by this strategy.
Relax canHandle() logic to claim snapshots based on either volume-snapshot details or the new strategy marker, with an eligibility fallback for legacy Error rows.
Add a “metadata-only” deletion path for snapshots that never recorded volume snapshot details.

File	Description
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java	Adds strategy marker persistence, updates `canHandle()`, and introduces metadata-only deletion for detail-less snapshots.
engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java	Adds unit tests for strategy marker persistence, updated `canHandle()` behavior, and metadata-only deletion behavior.

_{Review effort: Lite}

    public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) {
        logger.info("Starting VM snapshot delete process for snapshot [{}].", vmSnapshot.getUuid());
-        UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId());
        VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot;
+        if (!hasVolumeSnapshotDetails(vmSnapshotBeingDeleted)) {
+            return deleteVMSnapshotWithoutVolumeSnapshots(vmSnapshotBeingDeleted);
+        }
+
+        UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId());
        Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete");
        validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete");


+    protected boolean hasVolumeSnapshotDetails(VMSnapshot vmSnapshot) {
+        return CollectionUtils.isNotEmpty(vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT));
+    }


    public StrategyPriority canHandle(VMSnapshot vmSnapshot) {
-        if (!VMSnapshot.State.Allocated.equals(vmSnapshot.getState())) {
-            List<VMSnapshotDetailsVO> vmSnapshotDetails = vmSnapshotDetailsDao.findDetails(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT);
-            if (CollectionUtils.isEmpty(vmSnapshotDetails)) {
+        VMSnapshot.State state = vmSnapshot.getState();
+        if (!VMSnapshot.State.Allocated.equals(state)) {
+            if (hasVolumeSnapshotDetails(vmSnapshot) || isCreatedByThisStrategy(vmSnapshot)) {
+                return StrategyPriority.HIGHEST;
+            }
+            if (!VMSnapshot.State.Error.equals(state)) {
                logger.debug("KVM file based storage VM snapshot strategy cannot handle [{}] as it is not a KVM file based storage VM snapshot.",
                        vmSnapshot.getUuid());
                return StrategyPriority.CANT_HANDLE;
            }
-            return StrategyPriority.HIGHEST;
+            // Error rows created before the strategy marker existed carry no detail at all; fall back to the creation eligibility check.
+            logger.debug("VM snapshot [{}] is in Error state without any KVM file based storage details; checking whether this strategy would have created it.",
+                    vmSnapshot.getUuid());
        }


+
+        InOrder inOrder = inOrder(vmSnapshotHelper, vmSnapshotDetailsDao);
+        inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.CreateRequested);
+        inOrder.verify(vmSnapshotDetailsDao).addDetail(vmSnapshotId, "kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false);


+        when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot"))
+                .thenReturn(List.of(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshot", "30", true)));


+        when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotStrategy"))
+                .thenReturn(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false));


codecov · 2026-10-28T12:45:38Z

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 18.32%. Comparing base (c4c9731) to head (c8da86e).

Files with missing lines	Patch %	Lines
...napshot/KvmFileBasedStorageVmSnapshotStrategy.java	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14515      +/-   ##
============================================
+ Coverage     18.31%   18.32%   +0.01%     
- Complexity    16633    16652      +19     
============================================
  Files          5939     5939              
  Lines        537748   537772      +24     
  Branches      65924    65927       +3     
============================================
+ Hits          98472    98558      +86     
+ Misses       428070   428003      -67     
- Partials      11206    11211       +5

Flag	Coverage Δ
uitests	`4.08% <ø> (ø)`
unittests	`19.41% <96.55%> (+0.01%)`	⬆️

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.

sureshanaparti · 2026-10-28T13:08:54Z

@blueorangutan package

blueorangutan · 2026-10-28T13:10:07Z

@sureshanaparti 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.

blueorangutan · 2026-10-28T14:34:28Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19803

JoaoJandre · 2026-10-28T17:54:11Z

+            if (!VMSnapshot.State.Error.equals(state)) {
                logger.debug("KVM file based storage VM snapshot strategy cannot handle [{}] as it is not a KVM file based storage VM snapshot.",
                        vmSnapshot.getUuid());
                return StrategyPriority.CANT_HANDLE;
            }
-            return StrategyPriority.HIGHEST;
+            // Error rows created before the strategy marker existed carry no detail at all; fall back to the creation eligibility check.
+            logger.debug("VM snapshot [{}] is in Error state without any KVM file based storage details; checking whether this strategy would have created it.",
+                    vmSnapshot.getUuid());


I'm not sure this fallback is good approach. Aren't the other changes enough?
My fear is that we eventually try to operate a VM snapshot which is not from this strategy.

Fix instance snapshot with KvmFileBasedStorageVmSnapshotStrategy

a768e89

boring-cyborg Bot added the component:storage label Oct 28, 2026

sureshanaparti linked an issue Oct 28, 2026 that may be closed by this pull request

Instance snapshot failures when using 'KvmFileBasedStorageVmSnapshotStrategy' #14481

Open

sureshanaparti added this to the 4.22.3 milestone Oct 28, 2026

sureshanaparti requested a lite review from Copilot October 28, 2026 12:19

Copilot AI reviewed Oct 28, 2026

View reviewed changes

Copilot stopped reviewing on behalf of sureshanaparti due to an error October 28, 2026 12:41
The job was not acquired by a runner even after multiple attempts.

winterhazel requested a review from JoaoJandre October 28, 2026 12:45

review changes

c8da86e

sureshanaparti added the status:needs-testing label Oct 28, 2026

sureshanaparti requested a review from DaanHoogland October 28, 2026 13:08

JoaoJandre reviewed Oct 28, 2026

View reviewed changes

Navigation Menu

Repository navigation

Fix instance snapshots with KvmFileBasedStorageVmSnapshotStrategy - #14515

Fix instance snapshots with KvmFileBasedStorageVmSnapshotStrategy#14515
sureshanaparti wants to merge 2 commits into
apache:4.22from
shapeblue:vm-snapshot-kvm-fixes

sureshanaparti commented Oct 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Oct 28, 2026 •

edited

Loading

Uh oh!

sureshanaparti commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

JoaoJandre Oct 28, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		when(vmSnapshotDetailsDao.findDetails(20L, "kvmFileBasedStorageSnapshot"))
		.thenReturn(List.of(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshot", "30", true)));

		when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotStrategy"))
		.thenReturn(new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotStrategy", Boolean.TRUE.toString(), false));

Conversation

sureshanaparti commented Oct 28, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

codecov Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sureshanaparti commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

blueorangutan commented Oct 28, 2026

Uh oh!

JoaoJandre Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 28, 2026 •

edited

Loading