kvm: delete primary-only RBD snapshots before the volume by weizhouapache · Pull Request #14132 · apache/cloudstack · GitHub

Description

This PR fixes #12002

On Ceph/RBD primary storage, deleting a volume also destroys any of its remaining snapshots: LibvirtStorageAdaptor#deleteVol passes VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS for RBD pools, so libvirt unprotects and removes them along with the image.

Run that same snapshots cleanup earlier: for a KVM + RBD volume, delete its primary-only snapshots before sending the volume delete command, while they can still be found and removed cleanly. This is the choke point shared by both the standalone DeleteVolume API and VM destroy/expunge, so it covers both affected flows. The existing post-delete cleanup in deleteVolumeCallback stays in place as a no-op safety net for anything this pass didn't find.

Verified live on a KVM + Ceph environment (RBD primary storage, snapshot.backup.to.secondary=false): destroying a VM with a primary-only snapshot now deletes it cleanly.

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

BLOCKER
Critical
Major
Minor
Trivial

Screenshots (if appropriate):

How Has This Been Tested?

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

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.93%. Comparing base (9018034) to head (f720421).
⚠️ Report is 26 commits behind head on 4.22.

Files with missing lines	Patch %	Lines
...e/cloudstack/storage/volume/VolumeServiceImpl.java	90.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.22   #14132   +/-   ##
=========================================
  Coverage     17.93%   17.93%           
- Complexity    16140    16149    +9     
=========================================
  Files          5928     5928           
  Lines        535174   535194   +20     
  Branches      65494    65497    +3     
=========================================
+ Hits          95968    96007   +39     
+ Misses       428280   428256   -24     
- Partials      10926    10931    +5

Flag	Coverage Δ
uitests	4.02% <ø> (ø)
unittests	19.00% <90.00%> (+<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.

@weizhouapache I see.. so your changes only run this for KVM + Ceph volumes, and it sits in the volume delete path instead of the storage cleanup loop. That fills the gap I had. Mine was running on every volume expunge no matter the storage type. I guess I will go ahead and close mine: #12813

Fixes apache#12002. On Ceph/RBD primary storage, deleting a volume also destroys any of its remaining snapshots: LibvirtStorageAdaptor#deleteVol passes VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS for RBD pools, so libvirt unprotects and removes them along with the image. Run that same snapshots cleanup earlier: for a KVM + RBD volume, delete its primary-only snapshots before sending the volume delete command, while they can still be found and removed cleanly. This is the choke point shared by both the standalone DeleteVolume API and VM destroy/expunge, so it covers both affected flows. The existing post-delete cleanup in deleteVolumeCallback stays in place as a no-op safety net for anything this pass didn't find. Verified live on a KVM + Ceph environment (RBD primary storage, snapshot.backup.to.secondary=false): destroying a VM with a primary-only snapshot now deletes it cleanly.

@blueorangutan package

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

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

@blueorangutan test

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[SF] Trillian test result (tid-16978)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 48513 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14132-t16978-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

what happens if the volume delete fails after this? looks like the snapshots are already gone by then

@Damans227
Yes, primary-only snapshots (those not backed up to secondary storage) will be gone even if the volume delete then fails. CloudStack Ceph users should be aware of this behaviour.

Deleting the snapshots before removing the volume has these benefits:

Resource account: the account's snapshot resource count is decremented.
Events: the SNAPSHOT.DELETE event and usage record are published.
Consistency: the database and the storage stay strongly consistent, because each snapshot is removed through the normal deletion workflow while it still exists on RBD.
Chain handling: snapshot chains and parent/child relationships are handled by the existing logic, with no duplicate code path.

The drawback is extra time, since each snapshot needs its own delete command to the agent. I think that cost is acceptable.

The alternative is to delete the volume first and then clean up the snapshots. It saves time, but if the deletion fails, we can't tell what state the snapshots are in without checking the volume.

makes sense, keeping the counts and events right is worth it. should we add a line in the docs so ceph users know the snapshots go first?

created doc PR apache/cloudstack-documentation#688
@Damans227

that covers it, thanks. can the note also say the snapshots stay deleted if the volume delete fails?

weizhouapache added this to the 4.22.2 milestone Sep 10, 2026

boring-cyborg Bot added the component:storage label Sep 10, 2026

weizhouapache mentioned this pull request Sep 10, 2026

Clean up primary-only snapshot DB records on volume expunge #12813

Closed

2 of 14 tasks

weizhouapache linked an issue Sep 10, 2026 that may be closed by this pull request

snapshot as Abnormal residue on rbd #12002

Open

weizhouapache force-pushed the 4.22-remove-volume-snapshot-before-remove-rbd-volume branch from 19944a4 to f720421 Compare September 11, 2026 12:42

weizhouapache marked this pull request as ready for review September 11, 2026 12:42

weizhouapache requested review from Damans227 and sureshanaparti September 12, 2026 08:55

weizhouapache linked an issue Sep 18, 2026 that may be closed by this pull request

blank volume snapshots list #13331

Open

Damans227 reviewed Oct 2, 2026

View reviewed changes

weizhouapache mentioned this pull request Oct 7, 2026

Add note on RBD volume snapshots when snapshot.backup.to.secondary is false apache/cloudstack-documentation#688

Open

weizhouapache added the component:ceph label Oct 7, 2026

weizhouapache mentioned this pull request Oct 13, 2026

[WIP] Ceph improvements shapeblue/cloudstack#147

Draft

3 of 14 tasks

nvazquez modified the milestones: 4.22.2, 4.22.3 Nov 9, 2026

                               future.complete(result);
                               return future;
                           }
+                          deletePrimaryOnlySnapshotsBeforeRbdVolumeDelete(vol);

kvm: delete primary-only RBD snapshots before the volume by weizhouapache · Pull Request #14132 · apache/cloudstack · GitHub

Repository navigation

kvm: delete primary-only RBD snapshots before the volume - #14132

Conversation

weizhouapache commented Sep 10, 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!

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

Uh oh!

Codecov Report

Uh oh!

Damans227 commented Sep 10, 2026

Uh oh!

weizhouapache commented Sep 11, 2026

Uh oh!

blueorangutan commented Sep 11, 2026

Uh oh!

blueorangutan commented Sep 11, 2026

Uh oh!

weizhouapache commented Sep 11, 2026

Uh oh!

blueorangutan commented Sep 11, 2026

Uh oh!

blueorangutan commented Sep 12, 2026

Uh oh!

Damans227 Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Oct 7, 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 Sep 10, 2026 •
edited
Loading