* [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough
@ 2025-12-16 13:02 Fiona Ebner
2025-12-16 13:02 ` [pve-devel] [PATCH storage 01/11] plugin: document volume_snapshot_info() method Fiona Ebner
` (10 more replies)
0 siblings, 11 replies; 12+ messages in thread
From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [pve-devel] [PATCH storage 01/11] plugin: document volume_snapshot_info() method 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 02/11] plugin: volume snapshot info: also return virtual size Fiona Ebner ` (9 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 02/11] plugin: volume snapshot info: also return virtual size 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 01/11] plugin: document volume_snapshot_info() method Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 03/11] plugin: document volume_resize() method Fiona Ebner ` (8 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 03/11] plugin: document volume_resize() method 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 01/11] plugin: document volume_snapshot_info() method Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 02/11] plugin: volume snapshot info: also return virtual size Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 04/11] plugin/storage: volume resize: add snapname parameter Fiona Ebner ` (7 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 04/11] plugin/storage: volume resize: add snapname parameter 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (2 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 03/11] plugin: document volume_resize() method Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 05/11] lvm plugin: snapshot delete: clarify comment about using commit Fiona Ebner ` (6 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 05/11] lvm plugin: snapshot delete: clarify comment about using commit 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (3 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 04/11] plugin/storage: volume resize: add snapname parameter Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 06/11] bump API age and version Fiona Ebner ` (5 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 06/11] bump API age and version 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (4 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 05/11] lvm plugin: snapshot delete: clarify comment about using commit Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 07/11] partially fix #7094: lvm: snapshot delete: ensure commit target is large enough Fiona Ebner ` (4 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 07/11] partially fix #7094: lvm: snapshot delete: ensure commit target is large enough 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (5 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 06/11] bump API age and version Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 08/11] plugin: volume snapshot info: don't set 'order' for internal snapshots Fiona Ebner ` (3 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 08/11] plugin: volume snapshot info: don't set 'order' for internal snapshots 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (6 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 07/11] partially fix #7094: lvm: snapshot delete: ensure commit target is large enough Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 09/11] plugin: volume snapshot info: correctly return internal snapshot information Fiona Ebner ` (2 subsequent siblings) 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 09/11] plugin: volume snapshot info: correctly return internal snapshot information 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (7 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 08/11] plugin: volume snapshot info: don't set 'order' for internal snapshots Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 10/11] plugin: volume snapshot info: do not set 'ext' property Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH qemu-server 11/11] partially fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH storage 10/11] plugin: volume snapshot info: do not set 'ext' property 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (8 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 09/11] plugin: volume snapshot info: correctly return internal snapshot information Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH qemu-server 11/11] partially fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [pve-devel] [PATCH qemu-server 11/11] partially fix #7094: external snapshot delete: ensure commit target is large enough 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner ` (9 preceding siblings ...) 2025-12-16 13:02 ` [pve-devel] [PATCH storage 10/11] plugin: volume snapshot info: do not set 'ext' property Fiona Ebner @ 2025-12-16 13:02 ` Fiona Ebner 10 siblings, 0 replies; 12+ messages in thread From: Fiona Ebner @ 2025-12-16 13:02 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-12-16 13:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-12-16 13:02 [pve-devel] [PATCH-SERIES storage/qemu-server 00/11] fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 01/11] plugin: document volume_snapshot_info() method Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 02/11] plugin: volume snapshot info: also return virtual size Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 03/11] plugin: document volume_resize() method Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 04/11] plugin/storage: volume resize: add snapname parameter Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 05/11] lvm plugin: snapshot delete: clarify comment about using commit Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 06/11] bump API age and version Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 07/11] partially fix #7094: lvm: snapshot delete: ensure commit target is large enough Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 08/11] plugin: volume snapshot info: don't set 'order' for internal snapshots Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 09/11] plugin: volume snapshot info: correctly return internal snapshot information Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH storage 10/11] plugin: volume snapshot info: do not set 'ext' property Fiona Ebner 2025-12-16 13:02 ` [pve-devel] [PATCH qemu-server 11/11] partially fix #7094: external snapshot delete: ensure commit target is large enough Fiona Ebner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.