On Wed, Jul 09, 2025 at 06:21:58PM +0200, Alexandre Derumier via pve-devel
wrote:
> From: Alexandre Derumier <alexandre.derum...@groupe-cyllene.com>
> To: pve-devel@lists.proxmox.com
> Subject: [PATCH pve-storage 09/13] storage: add volume_support_qemu_snapshot
> Date: Wed, 9 Jul 2025 18:21:58 +0200
> Message-Id: <20250709162202.2952597-14-alexandre.derum...@groupe-cyllene.com>
> X-Mailer: git-send-email 2.39.5
>
> Returns if the volume is supporting qemu snapshot:
> 'internal' : do the snapshot with qemu internal snapshot
> 'external' : do the snapshot with qemu external snapshot
> undef : does not support qemu snapshot
>
> Signed-off-by: Alexandre Derumier <alexandre.derum...@groupe-cyllene.com>
> ---
> ApiChangeLog | 8 ++++++++
> src/PVE/Storage.pm | 15 +++++++++++++++
> src/PVE/Storage/DirPlugin.pm | 10 ++++++++++
> src/PVE/Storage/LVMPlugin.pm | 7 +++++++
> src/PVE/Storage/Plugin.pm | 20 ++++++++++++++++++++
> src/PVE/Storage/RBDPlugin.pm | 6 ++++++
> 6 files changed, 66 insertions(+)
>
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 12eef1f..68e94fd 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -29,6 +29,14 @@ Future changes should be documented in here.
> * Introduce rename_snapshot() plugin method
> This method allow to rename a vm disk snapshot name to a different
> snapshot name.
>
> +* Introduce volume_support_qemu_snapshot() plugin method
> + This method is used to known if the a snapshot need to be done by qemu
> + or by the storage api.
> + returned values are :
> + 'internal' : support snapshot with qemu internal snapshot
> + 'external' : support snapshot with qemu external snapshot
> + undef : don't support qemu snapshot
The naming, description and returned values seem a bit of a mixed bag
here.
Also because "internal", from the POV of the storage, happens
"outside" of the storage.
I'd also argue this doesn't *specifically* have anything to do with qemu
itself (we could technically hook qcow2 files with ublk/fuse+loopdev to
use for containers as well...).
Let me try to layout my thoughts...
- "internal" means they are completely isolated within the volume
itself, the storage has nothing to do with it. (sort of).
- "external" means the storage is used to allocate volumes, but *also*
handles the creation of the *format*
- undef = only the storage itself can make snapshots (which is
technically incorrect because qemu could still just use *qcow2
snapshots*...
- The "internal" vs `undef` case is just a matter of "is the format
snapshot-capable", which makes me wonder whether this method should
even have 3 cases.
Conceptually, what we want is a way of creating a new volume with a
backing volume, specifically.
If we cannot do this, either the "user" needs to be able to create
snapshots *inside* the volume, or there is no snapshot support.
Also consider that technically the *beginning* of a snapshot chain can
even have a raw format, so I also wouldn't specifically pin the ability
to create a new volume using a backing volume on the current *volume*.
As a side note: Looking at the LVM plugin, it now reports "qcow2"
support, but refuses it unless an option is set. The list of which
formats are supported is currently not storage-config dependent, which
is a bit unfortunate there.
How about one of these (a bit long but bear with me):
supports_snapshots_as_backed_volumes($scfg)
The storage:
- Can create volumes with backing volumes (required for NOT-running
VMs)
- Allows renaming a volume into a snapshot such that calling
`volume_snapshot` then recreates the original name... (BUT! See
note [1] below for why I think this should not be a requirement!)
(for running VMs)
combined with
is_format_available($scfg, "qcow2")
(to directly check for the optional support as in the LVM plugin early)
qemu-server's do_snapshot_type() then does:
"storage" if
is tpmstate
OR is not running
OR format isn't qcow2 (this case would be new in qemu-server)
"external" if
supports_snapshots_as_backed_volumes() is true
AND is_format_available($scfg, "qcow2")
"internal" otherwise
Notes:
[1] Do we really need to rename the volume *outside* of the storage?
Why does eg. the LVM plugin need to distinguish between running and not
running at all? All it does is shove some `/dev/` names around, after
all. We should be able to go through the change-file/reopen code in
qemu-server regardless of when/who/where renames the files, no?
Taking a snapshot of a running VM does:
1. rename volume "into" snapshot from out of qemu-server
2. tell qemu to reopen the files under the new names
3. call volume_snapshot which:
- creates volume with the original name and the snapshot as backing
4. have qemu open the disk by the original name for blockdev-snapshot
Can't we make this:
1. tell qemu to reopen the original files with different *node ids*
(since the clashing of those is the only issue we run into when
simply dropping the $running case and skipping the early rename...)
2. call volume_snapshot without a $running parameter
3. continue as before (point 4 above)
> +
> ## Version 11:
>
> * Allow declaring storage features via plugin data
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index e0b79fa..b796908 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -2370,6 +2370,21 @@ sub rename_snapshot {
> );
> }
>
> +sub volume_support_qemu_snapshot {
> + my ($cfg, $volid) = @_;
> +
> + my ($storeid, $volname) = parse_volume_id($volid, 1);
> +
> + if ($storeid) {
> + my $scfg = storage_config($cfg, $storeid);
> +
> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> + return $plugin->volume_support_qemu_snapshot($storeid, $scfg,
> $volname);
> + }
> + return undef;
> +}
> +
> # Various io-heavy operations require io/bandwidth limits which can be
> # configured on multiple levels: The global defaults in datacenter.cfg, and
> # per-storage overrides. When we want to do a restore from storage A to
> storage
> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
> index 10e4f70..3e92383 100644
> --- a/src/PVE/Storage/DirPlugin.pm
> +++ b/src/PVE/Storage/DirPlugin.pm
> @@ -314,4 +314,14 @@ sub get_import_metadata {
> };
> }
>
> +sub volume_support_qemu_snapshot {
> + my ($class, $storeid, $scfg, $volname) = @_;
> +
> + my $format = ($class->parse_volname($volname))[6];
> + return if $format ne 'qcow2';
> +
> + my $type = $scfg->{'external-snapshots'} ? 'external' : 'internal';
> + return $type;
> +}
> +
> 1;
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 2441e59..be411e5 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -861,4 +861,11 @@ sub rename_snapshot {
> die "rename_snapshot is not implemented for $class";
> }
>
> +sub volume_support_qemu_snapshot {
> + my ($class, $storeid, $scfg, $volname) = @_;
> +
> + my $format = ($class->parse_volname($volname))[6];
> + return 'external' if $format eq 'qcow2';
> +}
> +
> 1;
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 6b2dc32..aab2024 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -2262,6 +2262,26 @@ sub new_backup_provider {
> die "implement me if enabling the feature 'backup-provider' in
> plugindata()->{features}\n";
> }
>
> +=pod
> +
> +=head3 volume_support_qemu_snapshot
> +
> + $blockdev = $plugin->volume_support_qemu_snapshot($storeid, $scfg,
> $volname)
> +
> +Returns a string with the type of snapshot that qemu can do for a specific
> volume
> +
> +'internal' : support snapshot with qemu internal snapshot
> +'external' : support snapshot with qemu external snapshot
> +undef : don't support qemu snapshot
> +=cut
> +
> +sub volume_support_qemu_snapshot {
> + my ($class, $storeid, $scfg, $volname) = @_;
> +
> + my $format = ($class->parse_volname($volname))[6];
> + return 'internal' if $format eq 'qcow2';
> +}
> +
> sub config_aware_base_mkdir {
> my ($class, $scfg, $path) = @_;
>
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index ee33006..45f8a7f 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -1061,4 +1061,10 @@ sub rename_snapshot {
> die "rename_snapshot is not implemented for $class";
> }
>
> +sub volume_support_qemu_snapshot {
> + my ($class, $storeid, $scfg, $volname) = @_;
> +
> + return 'internal' if !$scfg->{krbd};
> +}
> +
> 1;
> --
> 2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
