From: Fiona Ebner Date: Fri, 9 Dec 2022 10:30:41 +0000 (+0100) Subject: fix #4390: rbd: snapshot delete: avoid early return to fix handling TPM drive X-Git-Url: https://git.proxmox.com/?p=pve-storage.git;a=commitdiff_plain;h=0756b484d45cb19da09d46707df8e08e63f32fa7 fix #4390: rbd: snapshot delete: avoid early return to fix handling TPM drive The only caller where $running can even be truthy is QemuServer.pm's qemu_volume_snapshot_delete(). But there, a check if snapshots should be done with QEMU is already made and the storage function is only called if snapshots should not be done with QEMU (like for TPM drives which are not attached directly). So rely on the caller and do not return early to fix removing snapshots in such cases. Even if a stray call ends up here (can happen already by changing the krbd setting while a VM is running to flip the above-mentioned check and the early return check removed by this patch), it might not even be problematic. At least a quick test worked fine: 1. take snapshot via a monitor command in QEMU 2. remove snapshot via the storage layer 3. create a new file in the VM 4. take a snapshot with the same name via monitor command in QEMU 5. roll back to the snapshot 6. check that the file in the VM is as expected Using the storage layer to take the snapshots and QEMU to remove the snapshot also worked doing the same test. Even if it were problematic, the check in qemu-server should rather be improved then. (Trying to issue a snapshot mon command for a krbd-mapped image fails with an error on the other hand, but that is also not too bad and not relevant to the storage code. Again, it rather would be necessary to improve the check in qemu-server). The fact that the pve-container code didn't even pass $running is the reason removing snapshots worked for containers on a storage with krbd disabled (the pve-container code calls map_volume() explicitly, so containers can work regardless of the krbd setting in the storage configuration; see commit 841fba6 ("call map_volume before using volumes.") in pve-container). For volume_snapshot(), the early return with $running was already removed (or rather the relevant logic moved to QemuServer.pm) in 2015 by commit f5640e7 ("remove running from Storage and check it in QemuServer"), even before krbd support was added in RBDPlugin.pm. Signed-off-by: Fiona Ebner Reviewed-by: Fabian Grünbichler --- diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm index dc6e79d..9047504 100644 --- a/PVE/Storage/RBDPlugin.pm +++ b/PVE/Storage/RBDPlugin.pm @@ -767,8 +767,6 @@ sub volume_snapshot_rollback { sub volume_snapshot_delete { my ($class, $scfg, $storeid, $volname, $snap, $running) = @_; - return 1 if $running && !$scfg->{krbd}; # FIXME: ???? - $class->deactivate_volume($storeid, $scfg, $volname, $snap, {}); my ($vtype, $name, $vmid) = $class->parse_volname($volname);