[pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough

[pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough

[Fiona Ebner]

For LVM storages using 'snapshot-as-volume-chain', it's necessary to
ensure that the size for the LV of the target snapshot volume is large
enough before doing a commit operation. This affects both the offline
and the online case, which have different code paths.

To make this possible via the storage (plugin) API, the
volume_resize() method gains a new 'snapname' parameter and the
'virtual-size' of each snapshot is returned in the result of
volume_snapshot_info().

The series also adds documentation for volume_resize() and
volume_snapshot_info(), the latter of which is a bit hairy, since it
was previously only used for ZFS replication and then re-used for
snapshot-as-volume-chain which returns quite different information.
It's not too bad though.

Lastly, there's some cleanups for the volume_snapshot_info() method.


Storage plugin API age and version are bumped in the series.
Dependency bump from qemu-server to libpve-storage-perl needed!


storage:

Fiona Ebner (10):
  plugin: document volume_snapshot_info() method
  plugin: volume snapshot info: also return virtual size
  plugin: document volume_resize() method
  plugin/storage: volume resize: add snapname parameter
  lvm plugin: snapshot delete: clarify comment about using commit
  bump API age and version
  partially fix #7094: lvm: snapshot delete: ensure commit target is
    large enough
  plugin: volume snapshot info: don't set 'order' for internal snapshots
  plugin: volume snapshot info: correctly return internal snapshot
    information
  plugin: volume snapshot info: do not set 'ext' property

 ApiChangeLog                         |  20 ++++++
 src/PVE/Storage.pm                   |   8 +--
 src/PVE/Storage/BTRFSPlugin.pm       |   3 +-
 src/PVE/Storage/ESXiPlugin.pm        |   2 +-
 src/PVE/Storage/ISCSIDirectPlugin.pm |   2 +-
 src/PVE/Storage/ISCSIPlugin.pm       |   2 +-
 src/PVE/Storage/LVMPlugin.pm         |  21 ++++--
 src/PVE/Storage/LvmThinPlugin.pm     |   8 ++-
 src/PVE/Storage/PBSPlugin.pm         |   2 +-
 src/PVE/Storage/Plugin.pm            | 103 +++++++++++++++++++++------
 src/PVE/Storage/RBDPlugin.pm         |   4 +-
 src/PVE/Storage/ZFSPlugin.pm         |   5 +-
 src/PVE/Storage/ZFSPoolPlugin.pm     |   4 +-
 13 files changed, 145 insertions(+), 39 deletions(-)


qemu-server:

Fiona Ebner (1):
  partially fix #7094: external snapshot delete: ensure commit target is
    large enough

 src/PVE/QemuServer.pm | 9 +++++++++
 1 file changed, 9 insertions(+)


Summary over all repositories:
  14 files changed, 154 insertions(+), 39 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 01/11] plugin: document volume_snapshot_info() method

[Fiona Ebner]

The method was originally only used for ZFS replication and then it
got re-used for snapshot-as-volume-chain which returns quite different
information. Document this and the possible values in each case.

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

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6f2e434..5122495 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1812,10 +1812,53 @@ sub status {
     return ($res->{total}, $res->{avail}, $res->{used}, 1);
 }
 
-# Returns a hash with the snapshot names as keys and the following data:
-# id        - Unique id to distinguish different snapshots even if the have the same name.
-# timestamp - Creation time of the snapshot (seconds since epoch).
-# Returns an empty hash if the volume does not exist.
+=head3 volume_snapshot_info
+
+    my $snapshots = $plugin->volume_snapshot_info($scfg, $storeid, $volname);
+    my $childsnap = $snapshots->{$snap}->{child};
+    my $childpath = $snapshots->{$childsnap}->{file};
+
+Query information about snapshots for C<$volname>. The result is a hash reference with snapshot
+names as keys and additional information as values. Different information is required for supporting
+the C<snapshot-as-volume-chain> storage configuration option and for supporting replication (note
+that replication is currently limited to native ZFS plugins).
+
+For C<snapshot-as-volume-chain> support, an entry with key C<current> exists for the current state.
+Required values are:
+
+=over
+
+=item C<file>: The image filename of the external snapshot volume.
+
+=item C<volname>: The volume name of the external snapshot volume.
+
+=item C<volid>: The volume ID of the external snapshot volume.
+
+=item C<child>: The name of the child snapshot in the volume chain. Not set if there is no child.
+
+=item C<parent>: The name of the parent snapshot in the volume chain. Not set if there is no parent.
+
+=item C<order>: Number that determines the position in the backing chain. C<0> for the current
+image, one more for each step further back in the volume chain.
+
+=item C<ext>: May be set if the snapshot is external when internal snapshots are also supported by
+the storage.
+
+=back
+
+For replication support, returns an empty hash if the volume does not exist. Required values are:
+
+=over
+
+=item C<id>: Unique identifier of the snapshot. Must be the same for the replicated instance. Must
+be different for different snapshots even if they share the same name.
+
+=item C<timestamp>: Creation time of the snapshot (seconds since epoch).
+
+=back
+
+=cut
+
 sub volume_snapshot_info {
     my ($class, $scfg, $storeid, $volname) = @_;
 
-- 
2.47.3



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

[pve-devel] [PATCH storage 02/11] plugin: volume snapshot info: also return virtual size

[Fiona Ebner]

For LVM storages using 'snapshot-as-volume-chain', it's necessary to
check that the size for the LV of the target snapshot volume is large
enough before doing a commit operation. Returning the virtual size of
the volume at the time the snapshot was taken in the result of
volume_snapshot_info() makes this easy to check.

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 102cf22..aa472f5 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -909,6 +909,7 @@ sub volume_snapshot_info {
         $info->{$snapname}->{file} = $snapfile;
         $info->{$snapname}->{volname} = "$snapvolname";
         $info->{$snapname}->{volid} = "$storeid:$snapvolname";
+        $info->{$snapname}->{'virtual-size'} = $snap->{'virtual-size'};
 
         my $parentfile = $snap->{'backing-filename'};
         if ($parentfile) {
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 5122495..260c259 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1834,6 +1834,8 @@ Required values are:
 
 =item C<volid>: The volume ID of the external snapshot volume.
 
+=item C<virtual-size>: The virtual size of the volume recorded by the snapshot.
+
 =item C<child>: The name of the child snapshot in the volume chain. Not set if there is no child.
 
 =item C<parent>: The name of the parent snapshot in the volume chain. Not set if there is no parent.
@@ -1913,6 +1915,7 @@ sub volume_snapshot_info {
             $info->{$snapname}->{file} = $snapfile;
             $info->{$snapname}->{volname} = "$snapvolname";
             $info->{$snapname}->{volid} = "$storeid:$snapvolname";
+            $info->{$snapname}->{'virtual-size'} = $snap->{'virtual-size'};
             $info->{$snapname}->{ext} = 1;
 
             my $parentfile = $snap->{'backing-filename'};
-- 
2.47.3



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

[pve-devel] [PATCH storage 03/11] plugin: document volume_resize() method

[Fiona Ebner]

Based on Max's submission [0] and Fabian's reply there. Also document
that the size is a multiple of 1024 and that padding to a larger size
is allowed.

[0]: https://lore.proxmox.com/pve-devel/20250326142059.261938-8-m.carrara@proxmox.com/

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

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 260c259..b0d3bbf 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1340,6 +1340,24 @@ sub volume_size_info {
 
 }
 
+=head3 volume_resize
+
+    $plugin->volume_resize(\%scfg, $storeid, $volname, $size, $running);
+
+Resize a volume to the new C<$size> in bytes. The size is guaranteed to be a multiple of C<1024>.
+The implementation may pad to a larger size if required. In case of virtual machines, C<$running>
+indicates that the VM is currently running and the call will be followed by a C<block_resize> QMP
+command in QEMU. If resizing is supported natively via QEMU (for example, when using librbd), then
+the plugin should simply return if the VM is running. For containers, C<$running> will always be
+C<0>.
+
+C<die>s in case of errors, or if the underlying storage implementation or the volume's format
+doesn't support resizing.
+
+This function should not return any value.
+
+=cut
+
 sub volume_resize {
     my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
 
-- 
2.47.3



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

[pve-devel] [PATCH storage 04/11] plugin/storage: volume resize: add snapname parameter

[Fiona Ebner]

For LVM storages using 'snapshot-as-volume-chain', it's necessary to
ensure that the size for the LV of the target snapshot volume is large
enough before doing a commit operation. Allow resizing a snapshot via
the storage (plugin) API to prepare this.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage.pm                   |  4 ++--
 src/PVE/Storage/BTRFSPlugin.pm       |  3 ++-
 src/PVE/Storage/ESXiPlugin.pm        |  2 +-
 src/PVE/Storage/ISCSIDirectPlugin.pm |  2 +-
 src/PVE/Storage/ISCSIPlugin.pm       |  2 +-
 src/PVE/Storage/LVMPlugin.pm         |  5 +++--
 src/PVE/Storage/LvmThinPlugin.pm     |  8 +++++++-
 src/PVE/Storage/PBSPlugin.pm         |  2 +-
 src/PVE/Storage/Plugin.pm            | 12 ++++++++----
 src/PVE/Storage/RBDPlugin.pm         |  4 +++-
 src/PVE/Storage/ZFSPlugin.pm         |  5 +++--
 src/PVE/Storage/ZFSPoolPlugin.pm     |  4 +++-
 12 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 6e87bac..c45d35b 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -393,7 +393,7 @@ sub volume_size_info {
 }
 
 sub volume_resize {
-    my ($cfg, $volid, $size, $running) = @_;
+    my ($cfg, $volid, $size, $running, $snapname) = @_;
 
     my $padding = (1024 - $size % 1024) % 1024;
     $size = $size + $padding;
@@ -402,7 +402,7 @@ sub volume_resize {
     if ($storeid) {
         my $scfg = storage_config($cfg, $storeid);
         my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-        return $plugin->volume_resize($scfg, $storeid, $volname, $size, $running);
+        return $plugin->volume_resize($scfg, $storeid, $volname, $size, $running, $snapname);
     } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
         die "resize file/device '$volid' is not possible\n";
     } else {
diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index e68d2bf..fb47aa0 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -496,10 +496,11 @@ sub volume_size_info {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
 
     my $format = ($class->parse_volname($volname))[6];
     if ($format eq 'subvol') {
+        die "resizing a snapshot is not supported for format 'subvol' in $class\n" if $snapname;
         # NOTE: `btrfs send/recv` actually drops quota information so supporting subvolumes with
         # quotas doesn't play nice with send/recv.
         die "cannot resize subvolume - btrfs quotas are currently not supported\n";
diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm
index f89e427..19f23bb 100644
--- a/src/PVE/Storage/ESXiPlugin.pm
+++ b/src/PVE/Storage/ESXiPlugin.pm
@@ -556,7 +556,7 @@ sub volume_import {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
 
     die "resizing volumes is not supported for $class\n";
 }
diff --git a/src/PVE/Storage/ISCSIDirectPlugin.pm b/src/PVE/Storage/ISCSIDirectPlugin.pm
index 62e9026..f976c31 100644
--- a/src/PVE/Storage/ISCSIDirectPlugin.pm
+++ b/src/PVE/Storage/ISCSIDirectPlugin.pm
@@ -227,7 +227,7 @@ sub volume_size_info {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
     die "volume resize is not possible on iscsi device\n";
 }
 
diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
index 30f4178..6472b2f 100644
--- a/src/PVE/Storage/ISCSIPlugin.pm
+++ b/src/PVE/Storage/ISCSIPlugin.pm
@@ -627,7 +627,7 @@ sub check_connection {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
     die "volume resize is not possible on iscsi device";
 }
 
diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index aa472f5..a134334 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -987,7 +987,7 @@ sub deactivate_volume {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
 
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
         $class->parse_volname($volname);
@@ -995,7 +995,8 @@ sub volume_resize {
     my $lvmsize = calculate_lvm_size($size / 1024, $format);
     $lvmsize = "${lvmsize}k";
 
-    my $path = $class->path($scfg, $volname);
+    my $path = $class->path($scfg, $volname, $storeid, $snapname);
+
     my $cmd = ['/sbin/lvextend', '-L', $lvmsize, $path];
 
     $class->cluster_lock_storage(
diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 2797d9e..1463335 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -355,7 +355,13 @@ sub create_base {
     return $newvolname;
 }
 
-# sub volume_resize {} reuse code from parent class
+sub volume_resize {
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
+
+    die "resizing a snapshot is not supported for $class\n" if $snapname;
+
+    return $class->SUPER::volume_resize($scfg, $storeid, $volname, $size, $running, $snapname);
+}
 
 sub volume_snapshot {
     my ($class, $scfg, $storeid, $volname, $snap) = @_;
diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 17e285a..9e5a5ec 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -977,7 +977,7 @@ sub volume_size_info {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
     die "volume resize is not possible on pbs device";
 }
 
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index b0d3bbf..221c872 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1342,14 +1342,15 @@ sub volume_size_info {
 
 =head3 volume_resize
 
-    $plugin->volume_resize(\%scfg, $storeid, $volname, $size, $running);
+    $plugin->volume_resize(\%scfg, $storeid, $volname, $size, $running, $snapname);
 
 Resize a volume to the new C<$size> in bytes. The size is guaranteed to be a multiple of C<1024>.
 The implementation may pad to a larger size if required. In case of virtual machines, C<$running>
 indicates that the VM is currently running and the call will be followed by a C<block_resize> QMP
 command in QEMU. If resizing is supported natively via QEMU (for example, when using librbd), then
 the plugin should simply return if the VM is running. For containers, C<$running> will always be
-C<0>.
+C<0>. If a snapshot name is specified via C<$snapname>, then the snapshot is the target of the
+resize operation.
 
 C<die>s in case of errors, or if the underlying storage implementation or the volume's format
 doesn't support resizing.
@@ -1359,13 +1360,16 @@ This function should not return any value.
 =cut
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
 
     die "can't resize this image format\n" if $volname !~ m/\.(raw|qcow2)$/;
 
     return 1 if $running;
 
-    my $path = $class->filesystem_path($scfg, $volname);
+    die "resizing a snapshot is not supported for $class without 'snapshot-as-volume-chain'\n"
+        if !$scfg->{'snapshot-as-volume-chain'} && $snapname;
+
+    my $path = $class->filesystem_path($scfg, $volname, $snapname);
 
     my $format = ($class->parse_volname($volname))[6];
 
diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 7d3e7ab..b537425 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -895,7 +895,9 @@ sub volume_size_info {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
+
+    die "resizing a snapshot is not supported for $class\n" if $snapname;
 
     return 1 if $running && !$scfg->{krbd}; # FIXME???
 
diff --git a/src/PVE/Storage/ZFSPlugin.pm b/src/PVE/Storage/ZFSPlugin.pm
index 99d8c8f..74e0a08 100644
--- a/src/PVE/Storage/ZFSPlugin.pm
+++ b/src/PVE/Storage/ZFSPlugin.pm
@@ -392,11 +392,12 @@ sub free_image {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
 
     $volname = ($class->parse_volname($volname))[1];
 
-    my $new_size = $class->SUPER::volume_resize($scfg, $storeid, $volname, $size, $running);
+    my $new_size =
+        $class->SUPER::volume_resize($scfg, $storeid, $volname, $size, $running, $snapname);
 
     $class->zfs_resize_lu($scfg, $volname, $new_size);
 
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index 3b3456b..8319344 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -742,7 +742,9 @@ sub create_base {
 }
 
 sub volume_resize {
-    my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+    my ($class, $scfg, $storeid, $volname, $size, $running, $snapname) = @_;
+
+    die "resizing a snapshot is not supported for $class\n" if $snapname;
 
     my $new_size = int($size / 1024);
 
-- 
2.47.3



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

[pve-devel] [PATCH storage 05/11] lvm plugin: snapshot delete: clarify comment about using commit

[Fiona Ebner]

The volume size of the child might actually be bigger if there was a
resize. However, in terms of actual data that needs to be merged, the
child is usually smaller.

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index a134334..0200666 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1201,7 +1201,8 @@ sub volume_snapshot_delete {
     my $childvolname = $snapshots->{$childsnap}->{volname};
 
     my $err = undef;
-    #if first snapshot,as it should be bigger,  we merge child, and rename the snapshot to child
+    # if first snapshot, as it should be bigger in terms of actual data, we merge child, and rename
+    # the snapshot to child
     if (!$parentsnap) {
         print "$volname: deleting snapshot '$snap' by commiting snapshot '$childsnap'\n";
         print "running 'qemu-img commit $childpath'\n";
-- 
2.47.3



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

[pve-devel] [PATCH storage 06/11] bump API age and version

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 ApiChangeLog       | 20 ++++++++++++++++++++
 src/PVE/Storage.pm |  4 ++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index de41192..5a78ca4 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -6,6 +6,26 @@ without breaking anything unaware of it.)
 
 Future changes should be documented in here.
 
+## Version 14:
+
+* Add new `$snapname` parameter to the `volume_resize()` plugin method
+
+  If specified, the snapshot is the target of the resize operation. This is currently only used in
+  combination with the `snapshot-as-volume-chain` configuration option before a commit operation.
+  Plugins that do not support the `snapshot-as-volume-chain` configuration option may either die
+  when `$snapname` is set or must implement resizing snapshots.
+
+* Add `virtual-size` to the information returned by the `volume_snapshot_info()` plugin method
+
+  This is the virtual size of the volume at the time the snapshot was taken. This information is
+  required for storages with the `snapshot-as-volume-chain` configuration option, to make snapshot
+  removal work correctly in combination with resizing images.
+
+* Properly document `volume_snapshot_info()` plugin method
+
+  If you already implement this method, check that the returned information is in line with the
+  documentation.
+
 ## Version 13:
 
 * Add new parameter $hints to the `activate_volume()` and `map_volume()` plugin methods
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index c45d35b..24d35a8 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::BTRFSPlugin;
 use PVE::Storage::ESXiPlugin;
 
 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 13;
+use constant APIVER => 14;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 4;
+use constant APIAGE => 5;
 
 our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
 
-- 
2.47.3



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

[pve-devel] [PATCH storage 07/11] partially fix #7094: lvm: snapshot delete: ensure commit target is large enough

[Fiona Ebner]

For LVM storages using 'snapshot-as-volume-chain', it's necessary to
ensure that the size for the LV of the target snapshot volume is large
enough before doing a commit operation. This fixes the offline case.

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

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 0200666..65e0ffa 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1205,6 +1205,14 @@ sub volume_snapshot_delete {
     # the snapshot to child
     if (!$parentsnap) {
         print "$volname: deleting snapshot '$snap' by commiting snapshot '$childsnap'\n";
+
+        my $snap_size = $snapshots->{$snap}->{'virtual-size'};
+        my $child_size = $snapshots->{$childsnap}->{'virtual-size'};
+        if (defined($child_size) && defined($snap_size) && $child_size > $snap_size) {
+            print "resize '$snap' ($snap_size bytes) to match '$childsnap' ($child_size bytes)\n";
+            $class->volume_resize($scfg, $storeid, $volname, $child_size, $running, $snap);
+        }
+
         print "running 'qemu-img commit $childpath'\n";
         #can't use -d here, as it's an lvm volume
         $cmd = ['/usr/bin/qemu-img', 'commit', $childpath];
-- 
2.47.3



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

[pve-devel] [PATCH storage 08/11] plugin: volume snapshot info: don't set 'order' for internal snapshots

[Fiona Ebner]

The 'order' property is only used for external snapshots. For internal
qcow2 snapshots, when a new snapshot is taken, the ID is one more than
the highest currently present ID. Thus, the order is currently
reversed compared to the one for the external snapshot backing chain,
where images further back in the chain have higher IDs. Just drop the
information, since it is not used. The qcow2 ID cannot be used for the
'id' property for replication either, because two snapshots with the
same name might end up with the same ID, which violates a requirement.

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

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 221c872..1fc2b8f 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1916,7 +1916,6 @@ sub volume_snapshot_info {
         my $snapshots = $json_decode->{snapshots};
         for my $snap (@$snapshots) {
             my $snapname = $snap->{name};
-            $info->{$snapname}->{order} = $snap->{id};
             $info->{$snapname}->{timestamp} = $snap->{'date-sec'};
 
         }
-- 
2.47.3



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

[pve-devel] [PATCH storage 09/11] plugin: volume snapshot info: correctly return internal snapshot information

[Fiona Ebner]

If the $follow_backing_files parameter is set for qemu_img_info(), it
will always return an array reference.

For the implementation in Plugin.pm it will thus be detected as a
volume chain even if the 'snapshot-as-volume-chain' storage
configuration option is not used. Fix this, by relying on the storage
configuration option. Flip the branches to avoid the need to negate
the condition.

For the implementation in LVMPlugin.pm, do not call qemu-img if the
'snapshot-as-volume-chain' storage configuration option is not used.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm |  4 ++--
 src/PVE/Storage/Plugin.pm    | 23 ++++++++++++-----------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 65e0ffa..1e52df3 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -868,6 +868,8 @@ sub status {
 sub volume_snapshot_info {
     my ($class, $scfg, $storeid, $volname) = @_;
 
+    return {} if !$scfg->{'snapshot-as-volume-chain'};
+
     my $short_volname = ($class->parse_volname($volname))[1];
 
     my $get_snapname_from_path = sub {
@@ -892,9 +894,7 @@ sub volume_snapshot_info {
     }
     my $info = {};
     my $order = 0;
-    return $info if ref($json_decode) ne 'ARRAY';
 
-    #no snapshot or external  snapshots is an arrayref
     my $snapshots = $json_decode;
     for my $snap (@$snapshots) {
         my $snapfile = $snap->{filename};
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 1fc2b8f..6fb4b98 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1903,7 +1903,8 @@ sub volume_snapshot_info {
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
         $class->parse_volname($volname);
 
-    my $json = PVE::Storage::Common::qemu_img_info($path, undef, 10, 1);
+    my $json =
+        PVE::Storage::Common::qemu_img_info($path, undef, 10, $scfg->{'snapshot-as-volume-chain'});
     die "failed to query file information with qemu-img\n" if !$json;
     my $json_decode = eval { decode_json($json) };
     if ($@) {
@@ -1911,16 +1912,8 @@ sub volume_snapshot_info {
     }
     my $info = {};
     my $order = 0;
-    if (ref($json_decode) eq 'HASH') {
-        #internal snapshots is a hashref
-        my $snapshots = $json_decode->{snapshots};
-        for my $snap (@$snapshots) {
-            my $snapname = $snap->{name};
-            $info->{$snapname}->{timestamp} = $snap->{'date-sec'};
-
-        }
-    } elsif (ref($json_decode) eq 'ARRAY') {
-        #no snapshot or external  snapshots is an arrayref
+    if ($scfg->{'snapshot-as-volume-chain'}) {
+        # calling qemu_img_info() with $follow_backing_files gives an array reference
         my $snapshots = $json_decode;
         for my $snap (@$snapshots) {
             my $snapfile = $snap->{filename};
@@ -1947,6 +1940,14 @@ sub volume_snapshot_info {
             }
             $order++;
         }
+    } else {
+        # calling qemu_img_info() without $follow_backing_files gives a single hash reference
+        my $snapshots = $json_decode->{snapshots};
+        for my $snap (@$snapshots) {
+            my $snapname = $snap->{name};
+            $info->{$snapname}->{timestamp} = $snap->{'date-sec'};
+
+        }
     }
 
     return $info;
-- 
2.47.3



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

[pve-devel] [PATCH storage 10/11] plugin: volume snapshot info: do not set 'ext' property

[Fiona Ebner]

The 'snapshot-as-volume-chain' storage configuration option determines
whether handling for external snapshots is done or not. The 'ext'
marker returned by volume_snapshot_info() doesn't add any information.
Also, the LVM plugin didn't return the 'ext' marker, making it
inconsistent. Remove the marker.

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

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6fb4b98..d60e023 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1154,7 +1154,6 @@ sub free_image {
             ) {
                 my $snap = $snapshots->{$snapid};
                 next if $snapid eq 'current';
-                next if !$snap->{ext};
                 eval { free_snap_image($class, $storeid, $scfg, $volname, $snapid); };
                 warn $@ if $@;
             }
@@ -1865,9 +1864,6 @@ Required values are:
 =item C<order>: Number that determines the position in the backing chain. C<0> for the current
 image, one more for each step further back in the volume chain.
 
-=item C<ext>: May be set if the snapshot is external when internal snapshots are also supported by
-the storage.
-
 =back
 
 For replication support, returns an empty hash if the volume does not exist. Required values are:
@@ -1930,7 +1926,6 @@ sub volume_snapshot_info {
             $info->{$snapname}->{volname} = "$snapvolname";
             $info->{$snapname}->{volid} = "$storeid:$snapvolname";
             $info->{$snapname}->{'virtual-size'} = $snap->{'virtual-size'};
-            $info->{$snapname}->{ext} = 1;
 
             my $parentfile = $snap->{'backing-filename'};
             if ($parentfile) {
-- 
2.47.3



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

[pve-devel] [PATCH qemu-server 11/11] partially fix #7094: external snapshot delete: ensure commit target is large enough

[Fiona Ebner]

For LVM storages using 'snapshot-as-volume-chain', it's necessary to
ensure that the size for the LV of the target snapshot volume is large
enough before doing a commit operation. Note that the change here also
affects file-based storages; previously resize would happen as part of
the commit operation automatically for them. This fixes the online
case.

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index d634251b..6ecb0bc4 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -4442,6 +4442,15 @@ sub qemu_volume_snapshot_delete {
         # improve-me: if firstsnap > child : commit, if firstsnap < child do a stream.
         if (!$parentsnap) {
             print "delete first snapshot $snap\n";
+
+            my $snap_size = $snapshots->{$snap}->{'virtual-size'};
+            my $child_size = $snapshots->{$childsnap}->{'virtual-size'};
+            if (defined($child_size) && defined($snap_size) && $child_size > $snap_size) {
+                print
+                    "resize '$snap' ($snap_size bytes) to match '$childsnap' ($child_size bytes)\n";
+                PVE::Storage::volume_resize($storecfg, $volid, $child_size, $running, $snap);
+            }
+
             PVE::QemuServer::Blockdev::blockdev_commit(
                 $storecfg,
                 $vmid,
-- 
2.47.3



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