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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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
> 
> 
>