Re: [pve-devel] [PATCH-SERIES storage/qemu-server 0/9] storage configuration-dependent format info

["Max R. Carrara" <m.carrara@proxmox.com>]

On Mon Jul 21, 2025 at 2:10 PM CEST, Fiona Ebner wrote:
> The LVM plugin can only use qcow2 format when the
> 'snapshot-as-volume-chain' configuration option is set. The format
> information is currently only recorded statically in the plugin data.
> This causes issues, for example, restoring a guest volume that uses
> qcow2 as a format hint on an LVM storage without the option set will
> fail, because the plugin data indicates that qcow2 is supported.
> Introduce a dedicated method, so that plugins can indicate what
> actually is supported according to the storage configuration.
>
> In case the 'snapshot-as-volume-chain' option is set for the LVM
> storage, qcow2 is even preferred and thus declared the default format.
>
> As a second step, a dedicated resolve_format_hint() helper is
> introduced.
>
> That second part requires a dependency and a breaks between
> qemu-server and pve-storage.

Review
------

No surprises here in terms of the code itself; the commits are easy to
follow as well. Can't spot anything that's off. I'm also always happy to
see a docstring being added ;P

I've also double-checked whether the removal of Plugin::default_format()
was fine; a quick `rg` through all my local repositories didn't spot
anything at least.

Testing
-------

Built this series by building pve-storage first, installing it, and then
building qemu-server. (Took me a hot minute to find that out though,
since I had skimmed over your note regarding dependency updates / breaks
above, woops.)

Tested this by configuring a fresh LVM (thick) storage with
storage-managed snapshots, installing a new Debian VM, and then running
a snapshot as well as a rollback after making a couple changes inside
the VM. 

Haven't noticed anything off in regards to this series overall.

Summary
-------

Seems to do what it says on the tin, so... LGTM.

Consider:

Reviewed-by: Max R. Carrara <m.carrara@proxmox.com>
Tested-by: Max R. Carrara <m.carrara@proxmox.com>

>
> storage:
>
> Fiona Ebner (6):
>   api change log: improve style consistency a bit
>   plugin: add get_formats() method and use it instead of
>     default_format()
>   lvm plugin: implement get_formats() method
>   api: status: rely on get_formats() method for determining
>     format-related info
>   introduce resolve_format_hint() helper
>   default format helper: only return default format
>
>  ApiChangeLog                   | 22 +++++++++----
>  src/PVE/API2/Storage/Status.pm |  6 ----
>  src/PVE/Storage.pm             | 32 ++++++++++++------
>  src/PVE/Storage/LVMPlugin.pm   | 10 ++++++
>  src/PVE/Storage/Plugin.pm      | 59 +++++++++++++++++++++++-----------
>  5 files changed, 88 insertions(+), 41 deletions(-)
>
>
> qemu-server:
>
> Fiona Ebner (3):
>   make tidy
>   resolve destination disk format helper: drop unused variable
>   use storage layer's resolve_format_hint() helper where appropriate
>
>  src/PVE/API2/Qemu/HMPPerms.pm |  2 +-
>  src/PVE/QemuServer.pm         | 29 ++++++++---------------------
>  src/PVE/VZDump/QemuServer.pm  |  5 ++---
>  3 files changed, 11 insertions(+), 25 deletions(-)
>
>
> Summary over all repositories:
>   8 files changed, 99 insertions(+), 66 deletions(-)



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel