Copilot commented on code in PR #12813:
URL: https://github.com/apache/cloudstack/pull/12813#discussion_r3257267515
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2292,6 +2293,34 @@ private void deleteSnapshot(SnapshotVO snapshot,
SnapshotDataStoreVO snapshotDat
}
}
+ /**
+ * Cleans up snapshot DB records for snapshots that exist only on primary
storage (no secondary copy).
+ * This handles the case where snapshot.backup.to.secondary=false and the
volume
+ * is being deleted during VM expunge — the RBD snapshots will be
destroyed along with the image,
+ * so the DB records need to be cleaned up to avoid orphaned entries.
+ */
+ protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume)
{
+ List<SnapshotVO> snapshots =
_snapshotDao.listByVolumeId(volume.getId());
+ if (CollectionUtils.isEmpty(snapshots)) {
+ return;
+ }
+ for (SnapshotVO snapshot : snapshots) {
+ if (Snapshot.State.Destroyed.equals(snapshot.getState())) {
+ continue;
+ }
+ List<SnapshotDataStoreVO> snapshotsOnPrimaryStorage =
_snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(),
DataStoreRole.Primary);
+ List<SnapshotDataStoreVO> snapshotsOnSecondaryStorage =
_snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(),
DataStoreRole.Image);
+ if (CollectionUtils.isNotEmpty(snapshotsOnPrimaryStorage) &&
CollectionUtils.isEmpty(snapshotsOnSecondaryStorage)) {
+ logger.info("Cleaning up snapshot {} (primary-only, no
secondary copy) as volume {} is being deleted", snapshot, volume);
+ for (SnapshotDataStoreVO snapshotOnPrimaryStorage :
snapshotsOnPrimaryStorage) {
+
_snapshotStoreDao.expunge(snapshotOnPrimaryStorage.getId());
+ }
+ snapshot.setState(Snapshot.State.Destroyed);
+ _snapshotDao.update(snapshot.getId(), snapshot);
Review Comment:
This path marks primary-only snapshots as destroyed without applying the
resource-accounting side effects of normal snapshot deletion.
`SnapshotManagerImpl.deleteSnapshot` decrements the account's snapshot resource
count when the snapshot becomes destroyed, so these cleanup-only deletions can
leave snapshot quota consumed even though the UI no longer shows the snapshot;
please decrement the snapshot resource count (and any storage usage that
applies to primary-only snapshots) when this cleanup succeeds, or route through
a deletion helper that performs the same accounting without touching the
already-removed RBD snapshot.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -2145,6 +2145,7 @@ public void cleanupStorage(boolean recurring) {
}
try {
+ cleanupSnapshotRecordsInPrimaryStorageOnly(vol);
Review Comment:
This removes the snapshot metadata before the asynchronous volume expunge
has actually succeeded. If `ensureVolumeIsExpungeReady` or `expungeVolumeAsync`
fails, or if the async delete later returns failure, the RBD image and
snapshots may still exist but CloudStack has already expunged their
`snapshot_store_ref` rows and marked the snapshots destroyed, leaving users
unable to manage valid snapshots. Move this cleanup to the successful
volume-delete callback path (or otherwise gate it on confirmed volume deletion
success) while avoiding the storage-side snapshot delete that currently causes
the orphan issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
