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

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

[Fiona Ebner]

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.

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

-- 
Generated by git-murpp 0.5.0


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

[pve-devel] [PATCH storage 1/9] api change log: improve style consistency a bit

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 ApiChangeLog | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index 1b06fa7..0984afb 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -22,12 +22,15 @@ Future changes should be documented in here.
   Feel free to request allowing more drivers or options on the pve-devel mailing list based on your
   needs.
 
-* Introduce rename_snapshot() plugin method
-    This method allow to rename a vm disk snapshot name to a different snapshot name.
+* Introduce `rename_snapshot()` plugin method
 
-* Introduce volume_qemu_snapshot_method() plugin method
-    This method declares how snapshots should be handled for *running* VMs.
-    This should return one of the following:
+  This method allow to rename a vm disk snapshot name to a different snapshot name.
+
+* Introduce `volume_qemu_snapshot_method()` plugin method
+
+  This method declares how snapshots should be handled for *running* VMs.
+
+  This should return one of the following:
     'qemu':
       Qemu must perform the snapshot. The storage plugin does nothing.
     'storage':
@@ -46,7 +49,6 @@ Future changes should be documented in here.
       NOTE: Storages must support using "current" as a special name in `rename_snapshot()` to
       cheaply convert a snapshot into the current disk state and back.
 
-
 ##  Version 11:
 
 * Allow declaring storage features via plugin data
@@ -56,7 +58,7 @@ Future changes should be documented in here.
   `backup-provider`, see below for more details. To declare support for this feature, return
   `features => { 'backup-provider' => 1 }` as part of the plugin data.
 
-* Introduce new_backup_provider() plugin method
+* Introduce `new_backup_provider()` plugin method
 
   Proxmox VE now supports a `Backup Provider API` that can be used to implement custom backup
   solutions tightly integrated in the Proxmox VE stack. See the `PVE::BackupProvider::Plugin::Base`
-- 
2.47.2



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

[pve-devel] [PATCH storage 2/9] plugin: add get_formats() method and use it instead of default_format()

[Fiona Ebner]

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.

The implementation for LVM is done in a separate commit.

Remove the now unused default_format() function from Plugin.pm.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 ApiChangeLog              |  8 ++++++
 src/PVE/Storage.pm        | 19 +++++++------
 src/PVE/Storage/Plugin.pm | 59 ++++++++++++++++++++++++++-------------
 3 files changed, 59 insertions(+), 27 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index 0984afb..d80bfb3 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -49,6 +49,14 @@ Future changes should be documented in here.
       NOTE: Storages must support using "current" as a special name in `rename_snapshot()` to
       cheaply convert a snapshot into the current disk state and back.
 
+* Introduce `get_formats()` plugin method
+
+  Get information about the supported formats and default format according to the current storage
+  configuration. The default implemenation is backwards-compatible with previous behavior and looks
+  at the definition given in the plugin data, as well as the `format` storage configuration option,
+  which can override the default format. Must be implemented when the supported formats or default
+  format depend on the storage configuration.
+
 ##  Version 11:
 
 * Allow declaring storage features via plugin data
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 6ca9f88..1faf893 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -857,10 +857,11 @@ my $volname_for_storage = sub {
 
     my $scfg = storage_config($cfg, $storeid);
 
-    my (undef, $valid_formats) = PVE::Storage::Plugin::default_format($scfg);
-    my $format_is_valid = grep { $_ eq $format } @$valid_formats;
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+    my $formats = $plugin->get_formats($scfg, $storeid);
     die "unsupported format '$format' for storage type $scfg->{type}\n"
-        if !$format_is_valid;
+        if !$formats->{valid}->{$format};
 
     (my $name_without_extension = $name) =~ s/\.$format$//;
 
@@ -1184,14 +1185,12 @@ sub vdisk_alloc {
 
     $vmid = parse_vmid($vmid);
 
-    my $defformat = PVE::Storage::Plugin::default_format($scfg);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    $fmt = $defformat if !$fmt;
+    $fmt = $plugin->get_formats($scfg, $storeid)->{default} if !$fmt;
 
     activate_storage($cfg, $storeid);
 
-    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-
     # lock shared storage
     return $plugin->cluster_lock_storage(
         $storeid,
@@ -1673,8 +1672,12 @@ sub storage_default_format {
     my ($cfg, $storeid) = @_;
 
     my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    return PVE::Storage::Plugin::default_format($scfg);
+    my $formats = $plugin->get_formats($scfg, $storeid);
+
+    return
+        wantarray ? ($formats->{default}, [sort keys $formats->{valid}->%*]) : $formats->{default};
 }
 
 sub vgroup_is_used {
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index ef04cb1..c3c1b63 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -287,23 +287,6 @@ sub storage_has_feature {
     return;
 }
 
-sub default_format {
-    my ($scfg) = @_;
-
-    my $type = $scfg->{type};
-    my $def = $defaultData->{plugindata}->{$type};
-
-    my $def_format = 'raw';
-    my $valid_formats = [$def_format];
-
-    if (defined($def->{format})) {
-        $def_format = $scfg->{format} || $def->{format}->[1];
-        $valid_formats = [sort keys %{ $def->{format}->[0] }];
-    }
-
-    return wantarray ? ($def_format, $valid_formats) : $def_format;
-}
-
 PVE::JSONSchema::register_format('pve-storage-path', \&verify_path);
 
 sub verify_path {
@@ -640,6 +623,44 @@ sub preallocation_cmd_opt {
 
 # Storage implementation
 
+=pod
+
+=head3 get_formats
+
+    my $formats = $plugin->get_formats($scfg);
+    my $default_format = $formats->{default};
+    my $is_valid = $formats->{valid}->{$format} ? 1 : 0;
+
+Get information about the supported formats and default format according to the current storage
+configuration C<$scfg>. The return value is a hash reference with C<default> mapping to the default
+format and C<valid> mapping to a hash reference, where each supported format is present as a key
+mapping to C<1>. For example:
+
+    {
+        default => 'raw',
+        valid => {
+            'qcow2 => 1,
+            'raw' => 1,
+        },
+    }
+
+=cut
+
+sub get_formats {
+    my ($class, $scfg, $storeid) = @_;
+
+    my $type = $scfg->{type};
+    my $plugin_data = $defaultData->{plugindata}->{$type};
+
+    return { default => 'raw', valid => { raw => 1 } } if !defined($plugin_data->{format});
+
+    return {
+        default => $scfg->{format} || $plugin_data->{format}->[1],
+        # copy rather than passing direct reference
+        valid => { $plugin_data->{format}->[0]->%* },
+    };
+}
+
 # called during addition of storage (before the new storage config got written)
 # die to abort addition if there are (grave) problems
 # NOTE: runs in a storage config *locked* context
@@ -1526,8 +1547,8 @@ sub list_images {
 
     my $imagedir = $class->get_subdir($scfg, 'images');
 
-    my ($defFmt, $vaidFmts) = default_format($scfg);
-    my $fmts = join('|', @$vaidFmts);
+    my $format_info = $class->get_formats($scfg, $storeid);
+    my $fmts = join('|', sort keys $format_info->{valid}->%*);
 
     my $res = [];
 
-- 
2.47.2



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

[pve-devel] [PATCH storage 3/9] lvm plugin: implement get_formats() method

[Fiona Ebner]

As the alloc_lvm_image() helper asserts, qcow2 cannot be used as a
format without the 'snapshot-as-volume-chain' configuration option.
Therefore it is necessary to implement get_formats() and distinguish
based on the storage configuration.

In case the 'snapshot-as-volume-chain' option is set, qcow2 is even
preferred and thus declared the default format.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 8be2a10..1690e97 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -405,6 +405,16 @@ sub options {
 
 # Storage implementation
 
+sub get_formats {
+    my ($class, $scfg, $storeid) = @_;
+
+    if ($scfg->{'snapshot-as-volume-chain'}) {
+        return { default => 'qcow2', valid => { 'qcow2' => 1, 'raw' => 1, } };
+    }
+
+    return { default => 'raw', valid => { 'raw' => 1 } };
+}
+
 sub on_add_hook {
     my ($class, $storeid, $scfg, %param) = @_;
 
-- 
2.47.2



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

[pve-devel] [PATCH storage 4/9] api: status: rely on get_formats() method for determining format-related info

[Fiona Ebner]

Rely on get_formats() rather than just the static plugin data in the
'status' API call. This removes the need for the special casing for
LVM storages without the 'snapshot-as-volume-chain' option. It also
fixes the issue that the 'format' storage configuration option to
override the default format was previously ignored there.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Storage/Status.pm | 6 ------
 src/PVE/Storage.pm             | 5 +++--
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 25933f8..c172073 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -220,12 +220,6 @@ __PACKAGE__->register_method({
                 $data->{used_fraction} = ($data->{used} // 0) / $data->{total};
             }
 
-            # TODO: add support to the storage plugin system to allow returing different supported
-            # formats depending on the storage config instead, this is just a stop gap!
-            if (lc($data->{type}) eq 'lvm') {
-                $data->{format}->[0]->{qcow2} = 0 if !$scfg->{'snapshot-as-volume-chain'};
-            }
-
             $res->{$storeid} = $data;
         }
 
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 1faf893..3286144 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1511,9 +1511,10 @@ sub storage_info {
 
         my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
         if ($includeformat) {
+            my $formats = $plugin->get_formats($scfg, $storeid);
+            $info->{$storeid}->{format} = [$formats->{valid}, $formats->{default}];
+
             my $pd = $plugin->plugindata();
-            $info->{$storeid}->{format} = $pd->{format}
-                if $pd->{format};
             $info->{$storeid}->{select_existing} = $pd->{select_existing}
                 if $pd->{select_existing};
         }
-- 
2.47.2



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

[pve-devel] [PATCH storage 5/9] introduce resolve_format_hint() helper

[Fiona Ebner]

Callers interested in the list of valid formats from
storage_default_format() actually want this functionality.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 3286144..24ca19f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1681,6 +1681,17 @@ sub storage_default_format {
         wantarray ? ($formats->{default}, [sort keys $formats->{valid}->%*]) : $formats->{default};
 }
 
+sub resolve_format_hint {
+    my ($cfg, $storeid, $format_hint) = @_;
+
+    my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+    my $formats = $plugin->get_formats($scfg, $storeid);
+    return $format_hint if $format_hint && $formats->{valid}->{$format_hint};
+    return $formats->{default};
+}
+
 sub vgroup_is_used {
     my ($cfg, $vgname) = @_;
 
-- 
2.47.2



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

[pve-devel] [PATCH storage 6/9] default format helper: only return default format

[Fiona Ebner]

Callers that required the valid formats are now using the
resolve_format_hint() helper instead.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Versioned breaks for qemu-server needed!

 src/PVE/Storage.pm | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 24ca19f..e2e6742 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1675,10 +1675,7 @@ sub storage_default_format {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    my $formats = $plugin->get_formats($scfg, $storeid);
-
-    return
-        wantarray ? ($formats->{default}, [sort keys $formats->{valid}->%*]) : $formats->{default};
+    return $plugin->get_formats($scfg, $storeid)->{default};
 }
 
 sub resolve_format_hint {
-- 
2.47.2



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

[pve-devel] [PATCH qemu-server 7/9] make tidy

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu/HMPPerms.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu/HMPPerms.pm b/src/PVE/API2/Qemu/HMPPerms.pm
index f6b32891..68213016 100644
--- a/src/PVE/API2/Qemu/HMPPerms.pm
+++ b/src/PVE/API2/Qemu/HMPPerms.pm
@@ -185,7 +185,7 @@ our $hmp_command_perms = {
 sub generate_description {
     my $cmd_by_priv = {};
     for my $cmd (sort keys $hmp_command_perms->%*) {
-        push $cmd_by_priv->{$hmp_command_perms->{$cmd}}->@*, $cmd;
+        push $cmd_by_priv->{ $hmp_command_perms->{$cmd} }->@*, $cmd;
     }
     my $none_cmds = delete($cmd_by_priv->{none})
         or die "internal error - no commands for 'none' found";
-- 
2.47.2



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

[pve-devel] [PATCH qemu-server 8/9] resolve destination disk format helper: drop unused variable

[Fiona Ebner]

The $scfg variable in resolve_dst_disk_format() has been unused since
commit 1f961e51 ("resolve destination disk format helper: use volume
format from storage layer").

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d7398648..60c390bd 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7964,7 +7964,6 @@ sub resolve_dst_disk_format {
     if (!$format) {
         # if no target format is specified, use the source disk format as hint
         if ($src_volid) {
-            my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
             $format = checked_volume_format($storecfg, $src_volid);
         } else {
             return $defFormat;
-- 
2.47.2



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

[pve-devel] [PATCH qemu-server 9/9] use storage layer's resolve_format_hint() helper where appropriate

[Fiona Ebner]

This also fixes an issue that the check_and_prepare_fleecing() helper
would always try 'raw' (which might not even be supported) rather than
the storage's default format.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-storage needed.

 src/PVE/QemuServer.pm        | 28 ++++++++--------------------
 src/PVE/VZDump/QemuServer.pm |  5 ++---
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 60c390bd..f8fb472a 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5337,8 +5337,7 @@ sub vm_migrate_alloc_nbd_disks {
         # order of precedence, filtered by whether storage supports it:
         # 1. explicit requested format
         # 2. default format of storage
-        my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
-        $format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
+        $format = PVE::Storage::resolve_format_hint($storecfg, $storeid, $format);
 
         my $size = $drive->{size} / 1024;
         my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, undef, $size);
@@ -6452,11 +6451,8 @@ my $restore_allocate_devices = sub {
         my $storeid = $d->{storeid};
         my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 
-        # test if requested format is supported
-        my ($defFormat, $validFormats) =
-            PVE::Storage::storage_default_format($storecfg, $storeid);
-        my $supported = grep { $_ eq $d->{format} } @$validFormats;
-        $d->{format} = $defFormat if !$supported;
+        # falls back to default format if requested format is not supported
+        $d->{format} = PVE::Storage::resolve_format_hint($storecfg, $storeid, $d->{format});
 
         my $name;
         if ($d->{is_cloudinit}) {
@@ -7959,21 +7955,13 @@ sub scsihw_infos {
 sub resolve_dst_disk_format {
     my ($storecfg, $storeid, $src_volid, $format) = @_;
 
-    my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
-
-    if (!$format) {
-        # if no target format is specified, use the source disk format as hint
-        if ($src_volid) {
-            $format = checked_volume_format($storecfg, $src_volid);
-        } else {
-            return $defFormat;
-        }
+    # if no target format is specified, use the source disk format as hint
+    if (!$format && $src_volid) {
+        $format = checked_volume_format($storecfg, $src_volid);
     }
 
-    # test if requested format is supported - else use default
-    my $supported = grep { $_ eq $format } @$validFormats;
-    $format = $defFormat if !$supported;
-    return $format;
+    # falls back to default format if requested format is not supported
+    return PVE::Storage::resolve_format_hint($storecfg, $storeid, $format);
 }
 
 sub generate_uuid {
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index eb85106c..5b94c369 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -698,9 +698,8 @@ my sub check_and_prepare_fleecing {
     if ($use_fleecing) {
         $self->query_block_node_sizes($vmid, $disks);
 
-        my ($default_format, $valid_formats) =
-            PVE::Storage::storage_default_format($self->{storecfg}, $fleecing_opts->{storage});
-        my $format = scalar(grep { $_ eq 'qcow2' } $valid_formats->@*) ? 'qcow2' : 'raw';
+        my $format = PVE::Storage::resolve_format_hint($self->{storecfg}, $fleecing_opts->{storage},
+            'qcow2');
 
         allocate_fleecing_images(
             $self,
-- 
2.47.2



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

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

[Max R. Carrara]

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

Re: [pve-devel] [PATCH storage 2/9] plugin: add get_formats() method and use it instead of default_format()

[Wolfgang Bumiller]

minor nit but the whole storage side LGTM

On Mon, Jul 21, 2025 at 02:10:47PM +0200, 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.
> 
> The implementation for LVM is done in a separate commit.
> 
> Remove the now unused default_format() function from Plugin.pm.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  ApiChangeLog              |  8 ++++++
>  src/PVE/Storage.pm        | 19 +++++++------
>  src/PVE/Storage/Plugin.pm | 59 ++++++++++++++++++++++++++-------------
>  3 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 0984afb..d80bfb3 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -49,6 +49,14 @@ Future changes should be documented in here.
>        NOTE: Storages must support using "current" as a special name in `rename_snapshot()` to
>        cheaply convert a snapshot into the current disk state and back.
>  
> +* Introduce `get_formats()` plugin method
> +
> +  Get information about the supported formats and default format according to the current storage
> +  configuration. The default implemenation is backwards-compatible with previous behavior and looks
> +  at the definition given in the plugin data, as well as the `format` storage configuration option,
> +  which can override the default format. Must be implemented when the supported formats or default
> +  format depend on the storage configuration.
> +
>  ##  Version 11:
>  
>  * Allow declaring storage features via plugin data
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 6ca9f88..1faf893 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -857,10 +857,11 @@ my $volname_for_storage = sub {
>  
>      my $scfg = storage_config($cfg, $storeid);
>  
> -    my (undef, $valid_formats) = PVE::Storage::Plugin::default_format($scfg);
> -    my $format_is_valid = grep { $_ eq $format } @$valid_formats;
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    my $formats = $plugin->get_formats($scfg, $storeid);
>      die "unsupported format '$format' for storage type $scfg->{type}\n"
> -        if !$format_is_valid;
> +        if !$formats->{valid}->{$format};
>  
>      (my $name_without_extension = $name) =~ s/\.$format$//;
>  
> @@ -1184,14 +1185,12 @@ sub vdisk_alloc {
>  
>      $vmid = parse_vmid($vmid);
>  
> -    my $defformat = PVE::Storage::Plugin::default_format($scfg);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> -    $fmt = $defformat if !$fmt;
> +    $fmt = $plugin->get_formats($scfg, $storeid)->{default} if !$fmt;
>  
>      activate_storage($cfg, $storeid);
>  
> -    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -
>      # lock shared storage
>      return $plugin->cluster_lock_storage(
>          $storeid,
> @@ -1673,8 +1672,12 @@ sub storage_default_format {
>      my ($cfg, $storeid) = @_;
>  
>      my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> -    return PVE::Storage::Plugin::default_format($scfg);
> +    my $formats = $plugin->get_formats($scfg, $storeid);
> +
> +    return
> +        wantarray ? ($formats->{default}, [sort keys $formats->{valid}->%*]) : $formats->{default};
>  }
>  
>  sub vgroup_is_used {
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index ef04cb1..c3c1b63 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -287,23 +287,6 @@ sub storage_has_feature {
>      return;
>  }
>  
> -sub default_format {
> -    my ($scfg) = @_;
> -
> -    my $type = $scfg->{type};
> -    my $def = $defaultData->{plugindata}->{$type};
> -
> -    my $def_format = 'raw';
> -    my $valid_formats = [$def_format];
> -
> -    if (defined($def->{format})) {
> -        $def_format = $scfg->{format} || $def->{format}->[1];
> -        $valid_formats = [sort keys %{ $def->{format}->[0] }];
> -    }
> -
> -    return wantarray ? ($def_format, $valid_formats) : $def_format;
> -}
> -
>  PVE::JSONSchema::register_format('pve-storage-path', \&verify_path);
>  
>  sub verify_path {
> @@ -640,6 +623,44 @@ sub preallocation_cmd_opt {
>  
>  # Storage implementation
>  
> +=pod
> +

↑ can be dropped

> +=head3 get_formats
> +
> +    my $formats = $plugin->get_formats($scfg);

↑ misses the `$storeid` parameter

> +    my $default_format = $formats->{default};
> +    my $is_valid = $formats->{valid}->{$format} ? 1 : 0;

↑ could just use !! instead of `? 1 : 0`

> +
> +Get information about the supported formats and default format according to the current storage
> +configuration C<$scfg>. The return value is a hash reference with C<default> mapping to the default
> +format and C<valid> mapping to a hash reference, where each supported format is present as a key
> +mapping to C<1>. For example:
> +
> +    {
> +        default => 'raw',
> +        valid => {
> +            'qcow2 => 1,
> +            'raw' => 1,
> +        },
> +    }
> +
> +=cut
> +
> +sub get_formats {
> +    my ($class, $scfg, $storeid) = @_;
> +
> +    my $type = $scfg->{type};
> +    my $plugin_data = $defaultData->{plugindata}->{$type};
> +
> +    return { default => 'raw', valid => { raw => 1 } } if !defined($plugin_data->{format});
> +
> +    return {
> +        default => $scfg->{format} || $plugin_data->{format}->[1],
> +        # copy rather than passing direct reference
> +        valid => { $plugin_data->{format}->[0]->%* },
> +    };
> +}
> +
>  # called during addition of storage (before the new storage config got written)
>  # die to abort addition if there are (grave) problems
>  # NOTE: runs in a storage config *locked* context
> @@ -1526,8 +1547,8 @@ sub list_images {
>  
>      my $imagedir = $class->get_subdir($scfg, 'images');
>  
> -    my ($defFmt, $vaidFmts) = default_format($scfg);
> -    my $fmts = join('|', @$vaidFmts);
> +    my $format_info = $class->get_formats($scfg, $storeid);
> +    my $fmts = join('|', sort keys $format_info->{valid}->%*);
>  
>      my $res = [];
>  
> -- 
> 2.47.2


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

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

[Wolfgang Bumiller]

applied series thanks

(fixed up the doc nits and dropped the make-tidy patch on qemu-server as
that was already fixed up in git)

On Mon, Jul 21, 2025 at 02:10:45PM +0200, 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.
> 
> 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(-)
> 
> -- 
> Generated by git-murpp 0.5.0


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

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

[Max R. Carrara]

On Tue Jul 22, 2025 at 3:03 PM CEST, Wolfgang Bumiller wrote:
> applied series thanks
>
> (fixed up the doc nits and dropped the make-tidy patch on qemu-server as
> that was already fixed up in git)

Is there a specific reason why my R-b & T-b trailers weren't added? :s


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

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

[Wolfgang Bumiller]

On Tue, Jul 22, 2025 at 04:17:23PM +0200, Max R. Carrara wrote:
> On Tue Jul 22, 2025 at 3:03 PM CEST, Wolfgang Bumiller wrote:
> > applied series thanks
> >
> > (fixed up the doc nits and dropped the make-tidy patch on qemu-server as
> > that was already fixed up in git)
> 
> Is there a specific reason why my R-b & T-b trailers weren't added? :s

No, I just used the wrong apply hotkey...


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