[pve-devel] applied: [PATCH storage] fix #4390: rbd: snapshot delete: avoid early return to fix handling TPM drive

[Thomas Lamprecht]

Am 09/12/2022 um 11:30 schrieb Fiona Ebner:
> 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 <f.ebner at proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 2 --
>  1 file changed, 2 deletions(-)
> 
>

applied, with Fabian's R-b, thanks!

Can you please try to keep track of removing the $running param
altogether with the future Proxmox VE 8.x?