public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Fiona Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [PATCH qemu-server 4/7] snapshot: add module for snapshot-related functionality
Date: Fri, 25 Sep 2026 14:26:28 +0200	[thread overview]
Message-ID: <20260925122646.139215-5-f.ebner@proxmox.com> (raw)
In-Reply-To: <20260925122646.139215-1-f.ebner@proxmox.com>

Initially, the 'Snapshot' module contains the functions for creating
and removing a snapshot from a volume.

Drops two more calls from QemuConfig back to QemuServer, making
progress towards getting rid of that cyclic dependency.

This was the only usage of the qmp_cmd() helper in QemuServer, so
remove the import.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuConfig.pm          |   5 +-
 src/PVE/QemuServer.pm          | 150 +-----------------------------
 src/PVE/QemuServer/Makefile    |   1 +
 src/PVE/QemuServer/Snapshot.pm | 162 +++++++++++++++++++++++++++++++++
 src/test/snapshot-test.pm      |   7 +-
 5 files changed, 170 insertions(+), 155 deletions(-)
 create mode 100644 src/PVE/QemuServer/Snapshot.pm

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 39e88366..c84f10f8 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -18,6 +18,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer;
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
+use PVE::QemuServer::Snapshot;
 use PVE::RESTEnvironment qw(log_warn);
 use PVE::Storage;
 use PVE::Tools;
@@ -401,7 +402,7 @@ sub __snapshot_create_vol_snapshot {
 
     print "snapshotting '$device' ($drive->{file})\n";
 
-    PVE::QemuServer::qemu_volume_snapshot($vmid, $device, $storecfg, $drive, $snapname);
+    PVE::QemuServer::Snapshot::qemu_volume_snapshot($vmid, $device, $storecfg, $drive, $snapname);
 }
 
 sub __snapshot_delete_remove_drive {
@@ -458,7 +459,7 @@ sub __snapshot_delete_vol_snapshot {
         );
     }
 
-    PVE::QemuServer::qemu_volume_snapshot_delete(
+    PVE::QemuServer::Snapshot::qemu_volume_snapshot_delete(
         $vmid, $storecfg, $drive, $snapname, $attached_deviceid, $running,
     );
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index e76a6ec4..322ad7fa 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -83,7 +83,7 @@ use PVE::QemuServer::DriveDevice qw(print_drivedevice_full scsihw_infos);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory qw(get_current_memory);
 use PVE::QemuServer::MetaInfo;
-use PVE::QemuServer::Monitor qw(mon_cmd qmp_cmd vm_qmp_peer);
+use PVE::QemuServer::Monitor qw(mon_cmd vm_qmp_peer);
 use PVE::QemuServer::Network;
 use PVE::QemuServer::OVMF;
 use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr parse_hostpci get_pci_bridges);
@@ -4332,134 +4332,6 @@ sub qemu_cpu_hotplug {
     }
 }
 
-sub qemu_volume_snapshot {
-    my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
-
-    my $volid = $drive->{file};
-    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
-
-    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $deviceid, $running);
-
-    if ($do_snapshots_type eq 'internal') {
-        print "internal qemu snapshot\n";
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        qmp_cmd($qmp_peer, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
-    } elsif ($do_snapshots_type eq 'external') {
-        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
-        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
-            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
-                . " QEMU machine version >= 10.0. See"
-                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
-        }
-        my $storeid = (PVE::Storage::parse_volume_id($volid))[0];
-        my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-        print "external qemu snapshot\n";
-        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
-        my $parent_snap = $snapshots->{'current'}->{parent};
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        PVE::QemuServer::VolumeChain::blockdev_external_snapshot(
-            $storecfg, $qmp_peer, $machine_version, $deviceid, $drive, $snap, $parent_snap,
-        );
-    } elsif ($do_snapshots_type eq 'storage') {
-        PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
-    }
-}
-
-sub qemu_volume_snapshot_delete {
-    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
-
-    my $volid = $drive->{file};
-
-    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
-
-    if ($do_snapshots_type eq 'internal') {
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-        qmp_cmd(
-            $qmp_peer,
-            'blockdev-snapshot-delete-internal-sync',
-            device => $attached_deviceid,
-            name => $snap,
-        );
-    } elsif ($do_snapshots_type eq 'external') {
-        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
-        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
-            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
-                . " QEMU machine version >= 10.0. See"
-                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
-        }
-
-        print "delete qemu external snapshot\n";
-
-        my $path = PVE::Storage::path($storecfg, $volid);
-        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
-
-        die "could not find snapshot '$snap' for volume '$volid'\n"
-            if !defined($snapshots->{$snap});
-
-        my $parentsnap = $snapshots->{$snap}->{parent};
-        my $childsnap = $snapshots->{$snap}->{child};
-
-        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
-
-        # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
-        # 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::VolumeChain::blockdev_commit(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $childsnap,
-                $snap,
-            );
-
-            PVE::Storage::rename_snapshot($storecfg, $volid, $snap, $childsnap);
-
-            PVE::QemuServer::VolumeChain::blockdev_replace(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $snap,
-                $childsnap,
-                $snapshots->{$childsnap}->{child},
-            );
-        } else {
-            #intermediate snapshot, we always stream the snapshot to child snapshot
-            print "stream intermediate snapshot $snap to $childsnap\n";
-            PVE::QemuServer::VolumeChain::blockdev_stream(
-                $storecfg,
-                $qmp_peer,
-                $machine_version,
-                $attached_deviceid,
-                $drive,
-                $snap,
-                $parentsnap,
-                $childsnap,
-            );
-        }
-    } elsif ($do_snapshots_type eq 'storage') {
-        PVE::Storage::volume_snapshot_delete(
-            $storecfg,
-            $volid,
-            $snap,
-            $attached_deviceid ? 1 : undef,
-        );
-    }
-}
-
 sub foreach_volid {
     my ($conf, $func, @param) = @_;
 
@@ -7810,26 +7682,6 @@ sub restore_tar_archive {
     warn $@ if $@;
 }
 
-sub do_snapshots_type {
-    my ($storecfg, $drive, $deviceid, $running) = @_;
-
-    #we use storage snapshot if vm is not running or if disk is unused;
-    return 'storage' if !$running || !$deviceid;
-
-    if (
-        $deviceid eq 'drive-tpmstate0'
-        && !PVE::QemuServer::Drive::drive_uses_qsd_fuse($storecfg, $drive)
-    ) {
-        return 'storage';
-    }
-
-    if (my $method = PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file})) {
-        return 'internal' if $method eq 'qemu';
-        return 'external' if $method eq 'mixed';
-    }
-    return 'storage';
-}
-
 =head3 template_create($vmid, $conf [, $disk])
 
 Converts all used disk volumes for the VM with the identifier C<$vmid> and
diff --git a/src/PVE/QemuServer/Makefile b/src/PVE/QemuServer/Makefile
index 060fac23..61ed1a4e 100644
--- a/src/PVE/QemuServer/Makefile
+++ b/src/PVE/QemuServer/Makefile
@@ -27,6 +27,7 @@ SOURCES=Agent.pm	\
 	QSD.pm		\
 	RNG.pm		\
 	RunState.pm	\
+	Snapshot.pm	\
 	StateFile.pm	\
 	USB.pm		\
 	Virtiofs.pm	\
diff --git a/src/PVE/QemuServer/Snapshot.pm b/src/PVE/QemuServer/Snapshot.pm
new file mode 100644
index 00000000..a0d4c239
--- /dev/null
+++ b/src/PVE/QemuServer/Snapshot.pm
@@ -0,0 +1,162 @@
+package PVE::QemuServer::Snapshot;
+
+use strict;
+use warnings;
+
+use PVE::Storage;
+
+use PVE::QemuServer::Drive;
+use PVE::QemuServer::Helpers;
+use PVE::QemuServer::Machine;
+use PVE::QemuServer::Monitor qw(qmp_cmd);
+use PVE::QemuServer::VolumeChain;
+
+sub do_snapshots_type {
+    my ($storecfg, $drive, $deviceid, $running) = @_;
+
+    #we use storage snapshot if vm is not running or if disk is unused;
+    return 'storage' if !$running || !$deviceid;
+
+    if (
+        $deviceid eq 'drive-tpmstate0'
+        && !PVE::QemuServer::Drive::drive_uses_qsd_fuse($storecfg, $drive)
+    ) {
+        return 'storage';
+    }
+
+    if (my $method = PVE::Storage::volume_qemu_snapshot_method($storecfg, $drive->{file})) {
+        return 'internal' if $method eq 'qemu';
+        return 'external' if $method eq 'mixed';
+    }
+    return 'storage';
+}
+
+sub qemu_volume_snapshot {
+    my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
+
+    my $volid = $drive->{file};
+    my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
+
+    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $deviceid, $running);
+
+    if ($do_snapshots_type eq 'internal') {
+        print "internal qemu snapshot\n";
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        qmp_cmd($qmp_peer, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
+    } elsif ($do_snapshots_type eq 'external') {
+        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
+            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
+                . " QEMU machine version >= 10.0. See"
+                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
+        }
+        my $storeid = (PVE::Storage::parse_volume_id($volid))[0];
+        my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+        print "external qemu snapshot\n";
+        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+        my $parent_snap = $snapshots->{'current'}->{parent};
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        PVE::QemuServer::VolumeChain::blockdev_external_snapshot(
+            $storecfg, $qmp_peer, $machine_version, $deviceid, $drive, $snap, $parent_snap,
+        );
+    } elsif ($do_snapshots_type eq 'storage') {
+        PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
+    }
+}
+
+sub qemu_volume_snapshot_delete {
+    my ($vmid, $storecfg, $drive, $snap, $attached_deviceid, $running) = @_;
+
+    my $volid = $drive->{file};
+
+    my $do_snapshots_type = do_snapshots_type($storecfg, $drive, $attached_deviceid, $running);
+
+    if ($do_snapshots_type eq 'internal') {
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+        qmp_cmd(
+            $qmp_peer,
+            'blockdev-snapshot-delete-internal-sync',
+            device => $attached_deviceid,
+            name => $snap,
+        );
+    } elsif ($do_snapshots_type eq 'external') {
+        my $machine_version = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+        if (!PVE::QemuServer::Machine::is_machine_version_at_least($machine_version, 10, 0)) {
+            die "storage for '$volid' is configured for snapshots as a volume chain - this requires"
+                . " QEMU machine version >= 10.0. See"
+                . " https://pve.proxmox.com/wiki/QEMU_Machine_Version_Upgrade\n";
+        }
+
+        print "delete qemu external snapshot\n";
+
+        my $path = PVE::Storage::path($storecfg, $volid);
+        my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+
+        die "could not find snapshot '$snap' for volume '$volid'\n"
+            if !defined($snapshots->{$snap});
+
+        my $parentsnap = $snapshots->{$snap}->{parent};
+        my $childsnap = $snapshots->{$snap}->{child};
+
+        my $qmp_peer = PVE::QemuServer::Drive::drive_qmp_peer($storecfg, $vmid, $drive);
+
+        # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
+        # 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::VolumeChain::blockdev_commit(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $childsnap,
+                $snap,
+            );
+
+            PVE::Storage::rename_snapshot($storecfg, $volid, $snap, $childsnap);
+
+            PVE::QemuServer::VolumeChain::blockdev_replace(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $snap,
+                $childsnap,
+                $snapshots->{$childsnap}->{child},
+            );
+        } else {
+            #intermediate snapshot, we always stream the snapshot to child snapshot
+            print "stream intermediate snapshot $snap to $childsnap\n";
+            PVE::QemuServer::VolumeChain::blockdev_stream(
+                $storecfg,
+                $qmp_peer,
+                $machine_version,
+                $attached_deviceid,
+                $drive,
+                $snap,
+                $parentsnap,
+                $childsnap,
+            );
+        }
+    } elsif ($do_snapshots_type eq 'storage') {
+        PVE::Storage::volume_snapshot_delete(
+            $storecfg,
+            $volid,
+            $snap,
+            $attached_deviceid ? 1 : undef,
+        );
+    }
+}
+
+1;
diff --git a/src/test/snapshot-test.pm b/src/test/snapshot-test.pm
index 0c5715c5..44e55d6a 100644
--- a/src/test/snapshot-test.pm
+++ b/src/test/snapshot-test.pm
@@ -404,10 +404,6 @@ sub set_migration_caps { } # ignored
 
 # BEGIN redefine PVE::QemuServer methods
 
-sub do_snapshots_type {
-    return 'storage';
-}
-
 sub vm_start {
     my ($storecfg, $vmid, $params, $migrate_opts) = @_;
 
@@ -463,6 +459,9 @@ $qemu_config_module->mock('has_feature', \&has_feature);
 $qemu_config_module->mock('__snapshot_save_vmstate', \&__snapshot_save_vmstate);
 $qemu_config_module->mock('assert_config_exists_on_node', \&assert_config_exists_on_node);
 
+my $qemu_snapshot_module = Test::MockModule->new('PVE::QemuServer::Snapshot');
+$qemu_snapshot_module->mock('do_snapshots_type' => sub { return 'storage'; });
+
 # ignore existing replication config
 my $repl_config_module = Test::MockModule->new('PVE::ReplicationConfig');
 $repl_config_module->mock('new' => sub { return bless {}, "PVE::ReplicationConfig" });
-- 
2.47.3





  parent reply	other threads:[~2026-09-25 12:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-25 12:26 [PATCH-SERIES qemu-server 0/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 1/7] volume snapshot (delete): avoid using deprecated check_running() helper Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 2/7] volume snapshot delete: move getting attached device ID to caller Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 3/7] fix #8071: volume snapshot delete: use current drive information to fix SAVC handling Fiona Ebner
2026-09-25 12:26 ` Fiona Ebner [this message]
2026-09-25 12:26 ` [PATCH qemu-server 5/7] snapshot: use v5.36 and subroutine signatures Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 6/7] volume chain: blockdev delete: remove superfluous parse_volume_id() call Fiona Ebner
2026-09-25 12:26 ` [PATCH qemu-server 7/7] volume chain: blockdev delete: pass volume ID instead of drive and mark private Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260925122646.139215-5-f.ebner@proxmox.com \
    --to=f.ebner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal