slavkap commented on a change in pull request #6057:
URL: https://github.com/apache/cloudstack/pull/6057#discussion_r821527386
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account
caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
Review comment:
@weizhouapache, this is not applied for the ROOT volumes. If the
`snapshot.backup.to.secondary` is disabled and you have snapshots on the ROOT
volume when you expunge the VM the snapshots will be in a `BackedUp` state in
the DB table `snapshots`, but with no records in the `snapshot_store_ref`
table, it is removed.
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -1583,12 +1583,27 @@ public Volume destroyVolume(long volumeId, Account
caller, boolean expunge, bool
s_logger.warn("Failed to expunge volume: " + volumeId);
return null;
}
+ markVolumeSnapshotsAsDestroyed(volume);
removeVolume(volume.getId());
}
return volume;
}
+ private void markVolumeSnapshotsAsDestroyed(VolumeVO volume) {
+ if (!Storage.ImageFormat.QCOW2.equals(volume.getFormat())) {
Review comment:
If you have a VM with Ceph's volumes, for example, and
`snapshot.backup.to.secondary` is disabled when you expunge the VM, all
snapshots remain active in UI, but there are no records in the
`snapshot_store_ref` table.
##########
File path:
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
##########
@@ -448,9 +448,9 @@ public Void
deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
volDao.remove(vo.getId());
}
- SnapshotDataStoreVO snapStoreVo =
_snapshotStoreDao.findByVolume(vo.getId(), DataStoreRole.Primary);
+ List<SnapshotDataStoreVO> snapStoreVOs =
_snapshotStoreDao.listAllByVolumeAndDataStore(vo.getId(),
DataStoreRole.Primary);
- if (snapStoreVo != null) {
+ for (SnapshotDataStoreVO snapStoreVo : snapStoreVOs) {
Review comment:
It's only my opinion, but we should handle this in each storage plugin.
Each storage has to decide whether it wants to keep the snapshots on the
primary, like they handle deleting or keeping the snapshots when a volume is
deleted.
--
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]
