Clean up primary-only snapshot DB records on volume expunge#12813
Clean up primary-only snapshot DB records on volume expunge#12813Damans227 wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12813 +/- ##
============================================
- Coverage 17.61% 17.60% -0.01%
- Complexity 15662 15665 +3
============================================
Files 5917 5917
Lines 531415 531434 +19
Branches 64973 64978 +5
============================================
- Hits 93588 93580 -8
- Misses 427271 427296 +25
- Partials 10556 10558 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm, but a concern remains (might be mute): Are we sure, when marking the snapshots records, there are really no data files left on primary for those?
I do not see any code ensuring that.
There was a problem hiding this comment.
Pull request overview
This PR addresses orphaned primary-only snapshot database records during volume expunge, especially for KVM + Ceph/RBD when snapshots are not backed up to secondary storage.
Changes:
- Adds snapshot cleanup before volume expunge in
StorageManagerImpl.cleanupStorage(). - Introduces helper logic to expunge primary-only snapshot store refs and mark snapshots destroyed.
- Adds unit tests for cleanup, secondary-copy skip, and destroyed-snapshot skip cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/src/main/java/com/cloud/storage/StorageManagerImpl.java |
Adds primary-only snapshot record cleanup during storage garbage collection. |
server/src/test/java/com/cloud/storage/StorageManagerImplTest.java |
Adds unit tests covering the new cleanup helper behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snapshot.setState(Snapshot.State.Destroyed); | ||
| _snapshotDao.update(snapshot.getId(), snapshot); |
| } | ||
|
|
||
| try { | ||
| cleanupSnapshotRecordsInPrimaryStorageOnly(vol); |
Description
When
snapshot.backup.to.secondary=false(KVM + Ceph) and a VM is expunged, Ceph destroys the RBD snapshots along with the volume image, but the DB records (snapshots, snapshot_store_ref) are left behind as undeletable orphans.Fix
In
StorageManagerImpl.cleanupStorage(), clean up primary-only snapshot records before the volume is expunged from storage.Fixes: #12002
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested on KVM + Ceph (RBD) with
snapshot.backup.to.secondary=false:How did you try to break this feature and the system with this change?
Tested with snapshots having both primary and secondary refs, and with already-destroyed snapshots, both worked correctly skipped.