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

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

[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@proxmox.com>
---
 PVE/Storage/RBDPlugin.pm | 2 --
 1 file changed, 2 deletions(-)

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);
-- 
2.30.2

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

[Fabian Grünbichler]

On December 9, 2022 11:30 am, Fiona Ebner wrote:
> 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.

given the last two paragraphs, and the following:
- some plugins don't even have $running as parameter in the signature list
- qemu-server is not calling this code for librbd, qcow2 or qed files
- that (guarded) call site in qemu-server is the only one that actually sets
  $running to anything other than undef or 0 (AFAICT)
- only the RBDPlugin (this patch) and the DirPlugin and its descendants (which
  only supports qcow2 and qed for snapshotting, which are only supported by
  qemu-server, and handled directly by Qemu for running VMs) are handling
  $running at all

we should probably mark this parameter for dropping with 8.0 (both in the plugin
interface and in PVE::Storage itself)?

external plugins already cannot hook into the "use qemu for snapshotting" logic
(which could of course be changed, although I am not sure if there's much
benefit), and only leaving this parameter in for snapshot deletion doesn't bring
any benefit. I kinda doubt that any external plugins are using this parameter,
but technically removing it earlier could be considered a breaking change..

other than this, consider this

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 2 --
>  1 file changed, 2 deletions(-)
> 
> 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);
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[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@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?

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

[Fiona Ebner]

Am 11.01.23 um 16:44 schrieb Thomas Lamprecht:
> Can you please try to keep track of removing the $running param
> altogether with the future Proxmox VE 8.x?

Sure. I added it to my TODO list for PVE 8 and a reminder comment in the
code.