[pve-devel] [PATCH-SERIES v2 guest-common/qemu-server/container] activate storages for snapshot operations

[pve-devel] [PATCH-SERIES v2 guest-common/qemu-server/container] activate storages for snapshot operations

[Fabian Ebner]

to make it work when the storage is just not active yet, and have
early errors when the storage cannot be activated. Also prohibits
snapshot operations when an involved storage is disabled, but
otherwise available.


Both qemu-server and pve-container build-depend upon pve-guest-common
for the added tests.


Changes from v1:
    * Rebase on current master.


guest-common:

Fabian Ebner (2):
  config: remove unused variable
  config: activate affected storages for snapshot operations

 src/PVE/AbstractConfig.pm | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)


qemu-server:

Fabian Ebner (1):
  snapshot: implement __snapshot_activate_storages

 PVE/QemuConfig.pm                             | 19 +++++++++++
 .../create/qemu-server/303.conf               | 13 +++++++
 .../delete/qemu-server/204.conf               | 33 ++++++++++++++++++
 .../rollback/qemu-server/303.conf             | 34 +++++++++++++++++++
 .../create/qemu-server/303.conf               | 13 +++++++
 .../delete/qemu-server/204.conf               | 33 ++++++++++++++++++
 .../rollback/qemu-server/303.conf             | 34 +++++++++++++++++++
 test/snapshot-test.pm                         | 32 +++++++++++++++++
 8 files changed, 211 insertions(+)
 create mode 100644 test/snapshot-expected/create/qemu-server/303.conf
 create mode 100644 test/snapshot-expected/delete/qemu-server/204.conf
 create mode 100644 test/snapshot-expected/rollback/qemu-server/303.conf
 create mode 100644 test/snapshot-input/create/qemu-server/303.conf
 create mode 100644 test/snapshot-input/delete/qemu-server/204.conf
 create mode 100644 test/snapshot-input/rollback/qemu-server/303.conf


container:

Fabian Ebner (3):
  config: snapshot_delete_remove_drive: check for parsed value
  config: parse_volume: don't die when noerr is set
  snapshot: implement __snapshot_activate_storages

 src/PVE/LXC/Config.pm                         | 25 +++++++++++++--
 .../snapshot-expected/create/lxc/204.conf     | 10 ++++++
 .../snapshot-expected/delete/lxc/204.conf     | 25 +++++++++++++++
 .../snapshot-expected/rollback/lxc/209.conf   | 29 +++++++++++++++++
 src/test/snapshot-input/create/lxc/204.conf   | 10 ++++++
 src/test/snapshot-input/delete/lxc/204.conf   | 25 +++++++++++++++
 src/test/snapshot-input/rollback/lxc/209.conf | 29 +++++++++++++++++
 src/test/snapshot-test.pm                     | 32 +++++++++++++++++++
 8 files changed, 183 insertions(+), 2 deletions(-)
 create mode 100644 src/test/snapshot-expected/create/lxc/204.conf
 create mode 100644 src/test/snapshot-expected/delete/lxc/204.conf
 create mode 100644 src/test/snapshot-expected/rollback/lxc/209.conf
 create mode 100644 src/test/snapshot-input/create/lxc/204.conf
 create mode 100644 src/test/snapshot-input/delete/lxc/204.conf
 create mode 100644 src/test/snapshot-input/rollback/lxc/209.conf

-- 
2.30.2

[pve-devel] [PATCH v2 guest-common 1/2] config: remove unused variable

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/AbstractConfig.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 4ea6c80..0c40062 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -877,8 +877,6 @@ my $snapshot_delete_assert_not_needed_by_replication = sub {
 sub snapshot_delete {
     my ($class, $vmid, $snapname, $force, $drivehash) = @_;
 
-    my $prepare = 1;
-
     my $unused = [];
 
     my $conf = $class->load_config($vmid);
-- 
2.30.2

[pve-devel] [PATCH v2 guest-common 2/2] config: activate affected storages for snapshot operations

[Fabian Ebner]

For snapshot creation, the storage for the vmstate file is activated
via vdisk_alloc when the state file is created.

Do not activate the volumes themselves, as that has unnecessary side
effects (e.g. waiting for zvol device link for ZFS, mapping the volume
for RBD). If a storage can only do snapshot operations on a volume
that has been activated, it needs to activate the volume itself.

The actual implementation will be in the plugins to be able to skip
CD ROM drives and bind-mounts, etc.

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

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 0c40062..2d15388 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -786,6 +786,13 @@ sub __snapshot_commit {
     $class->lock_config($vmid, $updatefn);
 }
 
+# Activates the storages affected by the snapshot operations.
+sub __snapshot_activate_storages {
+    my ($class, $conf, $include_vmstate) = @_;
+
+    return; # FIXME PVE 8.x change to die 'implement me' and bump Breaks for older plugins
+}
+
 # Creates a snapshot for the VM/CT.
 sub snapshot_create {
     my ($class, $vmid, $snapname, $save_vmstate, $comment) = @_;
@@ -801,6 +808,8 @@ sub snapshot_create {
     my $drivehash = {};
 
     eval {
+	$class->__snapshot_activate_storages($conf, 0);
+
 	if ($freezefs) {
 	    $class->__snapshot_freeze($vmid, 0);
 	}
@@ -884,6 +893,8 @@ sub snapshot_delete {
 
     die "snapshot '$snapname' does not exist\n" if !defined($snap);
 
+    $class->__snapshot_activate_storages($snap, 1) if !$drivehash;
+
     $snapshot_delete_assert_not_needed_by_replication->($class, $vmid, $conf, $snap, $snapname)
 	if !$drivehash && !$force;
 
@@ -1085,6 +1096,8 @@ sub snapshot_rollback {
 	$snap = $get_snapshot_config->($conf);
 
 	if ($prepare) {
+	    $class->__snapshot_activate_storages($snap, 1);
+
 	    $rollback_remove_replication_snapshots->($class, $vmid, $snap, $snapname);
 
 	    $class->foreach_volume($snap, sub {
-- 
2.30.2

[pve-devel] [PATCH v2 qemu-server 1/1] snapshot: implement __snapshot_activate_storages

[Fabian Ebner]

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

Build-depends on guest-common.

 PVE/QemuConfig.pm                             | 19 +++++++++++
 .../create/qemu-server/303.conf               | 13 +++++++
 .../delete/qemu-server/204.conf               | 33 ++++++++++++++++++
 .../rollback/qemu-server/303.conf             | 34 +++++++++++++++++++
 .../create/qemu-server/303.conf               | 13 +++++++
 .../delete/qemu-server/204.conf               | 33 ++++++++++++++++++
 .../rollback/qemu-server/303.conf             | 34 +++++++++++++++++++
 test/snapshot-test.pm                         | 32 +++++++++++++++++
 8 files changed, 211 insertions(+)
 create mode 100644 test/snapshot-expected/create/qemu-server/303.conf
 create mode 100644 test/snapshot-expected/delete/qemu-server/204.conf
 create mode 100644 test/snapshot-expected/rollback/qemu-server/303.conf
 create mode 100644 test/snapshot-input/create/qemu-server/303.conf
 create mode 100644 test/snapshot-input/delete/qemu-server/204.conf
 create mode 100644 test/snapshot-input/rollback/qemu-server/303.conf

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index b993378..cfef8d3 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -241,6 +241,25 @@ sub __snapshot_save_vmstate {
     return $statefile;
 }
 
+sub __snapshot_activate_storages {
+    my ($class, $conf, $include_vmstate) = @_;
+
+    my $storecfg = PVE::Storage::config();
+    my $opts = $include_vmstate ? { 'extra_keys' => ['vmstate'] } : {};
+    my $storage_hash = {};
+
+    $class->foreach_volume_full($conf, $opts, sub {
+	my ($key, $drive) = @_;
+
+	return if PVE::QemuServer::drive_is_cdrom($drive);
+
+	my ($storeid) = PVE::Storage::parse_volume_id($drive->{file});
+	$storage_hash->{$storeid} = 1;
+    });
+
+    PVE::Storage::activate_storage_list($storecfg, [ sort keys $storage_hash->%* ]);
+}
+
 sub __snapshot_check_running {
     my ($class, $vmid) = @_;
     return PVE::QemuServer::Helpers::vm_running_locally($vmid);
diff --git a/test/snapshot-expected/create/qemu-server/303.conf b/test/snapshot-expected/create/qemu-server/303.conf
new file mode 100644
index 0000000..2731bd1
--- /dev/null
+++ b/test/snapshot-expected/create/qemu-server/303.conf
@@ -0,0 +1,13 @@
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: q35
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
diff --git a/test/snapshot-expected/delete/qemu-server/204.conf b/test/snapshot-expected/delete/qemu-server/204.conf
new file mode 100644
index 0000000..c521154
--- /dev/null
+++ b/test/snapshot-expected/delete/qemu-server/204.conf
@@ -0,0 +1,33 @@
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+parent: test
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
+
+[test]
+#test comment
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: somemachine
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+snaptime: 1234567890
+sockets: 1
+vga: qxl
+vmstate: somestorage:state-volume
diff --git a/test/snapshot-expected/rollback/qemu-server/303.conf b/test/snapshot-expected/rollback/qemu-server/303.conf
new file mode 100644
index 0000000..518c954
--- /dev/null
+++ b/test/snapshot-expected/rollback/qemu-server/303.conf
@@ -0,0 +1,34 @@
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+parent: test
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
+
+[test]
+#test comment
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: q35
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+runningmachine: somemachine
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+snaptime: 1234567890
+sockets: 1
+vga: qxl
+vmstate: somestorage:state-volume
diff --git a/test/snapshot-input/create/qemu-server/303.conf b/test/snapshot-input/create/qemu-server/303.conf
new file mode 100644
index 0000000..2731bd1
--- /dev/null
+++ b/test/snapshot-input/create/qemu-server/303.conf
@@ -0,0 +1,13 @@
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: q35
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
diff --git a/test/snapshot-input/delete/qemu-server/204.conf b/test/snapshot-input/delete/qemu-server/204.conf
new file mode 100644
index 0000000..c521154
--- /dev/null
+++ b/test/snapshot-input/delete/qemu-server/204.conf
@@ -0,0 +1,33 @@
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+parent: test
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
+
+[test]
+#test comment
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: somemachine
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+snaptime: 1234567890
+sockets: 1
+vga: qxl
+vmstate: somestorage:state-volume
diff --git a/test/snapshot-input/rollback/qemu-server/303.conf b/test/snapshot-input/rollback/qemu-server/303.conf
new file mode 100644
index 0000000..518c954
--- /dev/null
+++ b/test/snapshot-input/rollback/qemu-server/303.conf
@@ -0,0 +1,34 @@
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+parent: test
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+sockets: 1
+vga: qxl
+
+[test]
+#test comment
+agent: 1
+bootdisk: ide0
+cores: 4
+ide0: local:snapshotable-disk-1,discard=on,size=32G
+ide2: none,media=cdrom
+machine: q35
+memory: 8192
+name: win
+net0: e1000=12:34:56:78:90:12,bridge=somebr0,firewall=1
+numa: 0
+ostype: win7
+runningmachine: somemachine
+smbios1: uuid=01234567-890a-bcde-f012-34567890abcd
+snaptime: 1234567890
+sockets: 1
+vga: qxl
+vmstate: somestorage:state-volume
diff --git a/test/snapshot-test.pm b/test/snapshot-test.pm
index cc04f01..3f1ac7c 100644
--- a/test/snapshot-test.pm
+++ b/test/snapshot-test.pm
@@ -15,6 +15,7 @@ use PVE::ReplicationConfig;
 use Test::MockModule;
 use Test::More;
 
+my $activate_storage_possible = 1;
 my $nodename;
 my $snapshot_possible;
 my $vol_snapshot_possible = {};
@@ -105,6 +106,15 @@ sub mocked_volume_rollback_is_possible {
     die "volume_rollback_is_possible failed\n";
 }
 
+sub mocked_activate_storage {
+    my ($storecfg, $storeid) = @_;
+    die "Storage config not mocked! aborting\n"
+	if defined($storecfg);
+    die "storage activation failed\n"
+	if !$activate_storage_possible;
+    return;
+}
+
 sub mocked_activate_volumes {
     my ($storecfg, $volumes) = @_;
     die "Storage config not mocked! aborting\n"
@@ -410,6 +420,7 @@ $repl_config_module->mock('check_for_existing_jobs' => sub { return });
 my $storage_module = Test::MockModule->new('PVE::Storage');
 $storage_module->mock('config', sub { return; });
 $storage_module->mock('path', sub { return "/some/store/statefile/path"; });
+$storage_module->mock('activate_storage', \&mocked_activate_storage);
 $storage_module->mock('activate_volumes', \&mocked_activate_volumes);
 $storage_module->mock('deactivate_volumes', \&mocked_deactivate_volumes);
 $storage_module->mock('vdisk_free', \&mocked_vdisk_free);
@@ -555,6 +566,13 @@ $vm_mon->{savevm_start} = 1;
 printf("Successful snapshot_create with no existing snapshots but set machine type\n");
 testcase_create("301", "test", 1, "test comment", "", { "local:snapshotable-disk-1" => "test" });
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_create when storage activation is not possible\n");
+testcase_create("303", "test", 1, "test comment", "storage activation failed\n\n");
+
+$activate_storage_possible = 1;
+
 $nodename = "delete";
 printf("\n");
 printf("Running delete tests\n");
@@ -587,6 +605,13 @@ testcase_delete("202", "test", 0, "volume snapshot delete disabled\n", { "local:
 printf("Expected error for snapshot_delete with locked config\n");
 testcase_delete("203", "test", 0, "VM is locked (backup)\n");
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_delete when storage activation is not possible\n");
+testcase_delete("204", "test", 0, "storage activation failed\n");
+
+$activate_storage_possible = 1;
+
 $nodename = "rollback";
 printf("\n");
 printf("Running rollback tests\n");
@@ -643,6 +668,13 @@ testcase_rollback("301", "test", "", { "local:snapshotable-disk-1" => "test" });
 printf("Successful snapshot_rollback with saved vmstate and machine config and runningmachine \n");
 testcase_rollback("302", "test", "", { "local:snapshotable-disk-1" => "test" });
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_rollback when storage activation is not possible\n");
+testcase_rollback("303", "test", "storage activation failed\n");
+
+$activate_storage_possible = 1;
+
 done_testing();
 
 1;
-- 
2.30.2

[pve-devel] [PATCH v2 container 1/3] config: snapshot_delete_remove_drive: check for parsed value

[Fabian Ebner]

parse_volume is called with noerr=1, so this might be undef instead
of the hash we expect.

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

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 6c2acd6..32d990c 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -192,7 +192,7 @@ sub __snapshot_delete_remove_drive {
 	delete $snap->{$remove_drive};
 
 	$class->add_unused_volume($snap, $mountpoint->{volume})
-	    if ($mountpoint->{type} eq 'volume');
+	    if $mountpoint && ($mountpoint->{type} eq 'volume');
     }
 }
 
-- 
2.30.2

[pve-devel] [PATCH v2 container 2/3] config: parse_volume: don't die when noerr is set

[Fabian Ebner]

AFAICT, the only existing callers using noerr=1 are in
__snapshot_delete_remove_drive, and in AbstractConfig's
foreach_volume_full. The former should not be affected, as unknown
keys should never make their way in there. For the latter, it makes
iterating with
    $opts = { extra_keys => ['vmstate'] }
possible while being agnostic of guest type. Previously, it would die
for LXC configs, but now the unknown key is simply skipped there.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC/Config.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 32d990c..7db023c 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1191,7 +1191,9 @@ sub parse_volume {
 	return $parse_ct_mountpoint_full->($class, $unused_desc, $volume_string, $noerr);
     }
 
-    die "parse_volume - unknown type: $key\n";
+    die "parse_volume - unknown type: $key\n" if !$noerr;
+
+    return;
 }
 
 sub print_volume {
-- 
2.30.2

[pve-devel] [PATCH v2 container 3/3] snapshot: implement __snapshot_activate_storages

[Fabian Ebner]

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

Build depends on guest-common.

 src/PVE/LXC/Config.pm                         | 19 +++++++++++
 .../snapshot-expected/create/lxc/204.conf     | 10 ++++++
 .../snapshot-expected/delete/lxc/204.conf     | 25 +++++++++++++++
 .../snapshot-expected/rollback/lxc/209.conf   | 29 +++++++++++++++++
 src/test/snapshot-input/create/lxc/204.conf   | 10 ++++++
 src/test/snapshot-input/delete/lxc/204.conf   | 25 +++++++++++++++
 src/test/snapshot-input/rollback/lxc/209.conf | 29 +++++++++++++++++
 src/test/snapshot-test.pm                     | 32 +++++++++++++++++++
 8 files changed, 179 insertions(+)
 create mode 100644 src/test/snapshot-expected/create/lxc/204.conf
 create mode 100644 src/test/snapshot-expected/delete/lxc/204.conf
 create mode 100644 src/test/snapshot-expected/rollback/lxc/209.conf
 create mode 100644 src/test/snapshot-input/create/lxc/204.conf
 create mode 100644 src/test/snapshot-input/delete/lxc/204.conf
 create mode 100644 src/test/snapshot-input/rollback/lxc/209.conf

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 7db023c..9429c59 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -101,6 +101,25 @@ sub __snapshot_save_vmstate {
     die "implement me - snapshot_save_vmstate\n";
 }
 
+sub __snapshot_activate_storages {
+    my ($class, $conf, $include_vmstate) = @_;
+
+    my $storecfg = PVE::Storage::config();
+    my $opts = $include_vmstate ? { 'extra_keys' => ['vmstate'] } : {};
+    my $storage_hash = {};
+
+    $class->foreach_volume_full($conf, $opts, sub {
+	my ($vs, $mountpoint) = @_;
+
+	return if $mountpoint->{type} ne 'volume';
+
+	my ($storeid) = PVE::Storage::parse_volume_id($mountpoint->{volume});
+	$storage_hash->{$storeid} = 1;
+    });
+
+    PVE::Storage::activate_storage_list($storecfg, [ sort keys $storage_hash->%* ]);
+}
+
 sub __snapshot_check_running {
     my ($class, $vmid) = @_;
     return PVE::LXC::check_running($vmid);
diff --git a/src/test/snapshot-expected/create/lxc/204.conf b/src/test/snapshot-expected/create/lxc/204.conf
new file mode 100644
index 0000000..4546668
--- /dev/null
+++ b/src/test/snapshot-expected/create/lxc/204.conf
@@ -0,0 +1,10 @@
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+swap: 512
diff --git a/src/test/snapshot-expected/delete/lxc/204.conf b/src/test/snapshot-expected/delete/lxc/204.conf
new file mode 100644
index 0000000..a21c535
--- /dev/null
+++ b/src/test/snapshot-expected/delete/lxc/204.conf
@@ -0,0 +1,25 @@
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+parent: test
+rootfs: local:snapshotable-disk-1
+swap: 512
+
+[test]
+#test comment
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+snaptime: 1234567890
+swap: 512
diff --git a/src/test/snapshot-expected/rollback/lxc/209.conf b/src/test/snapshot-expected/rollback/lxc/209.conf
new file mode 100644
index 0000000..c9a23c9
--- /dev/null
+++ b/src/test/snapshot-expected/rollback/lxc/209.conf
@@ -0,0 +1,29 @@
+# should be preserved
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:snapshotable-disk-2,mp=/invalid/mp0
+mp1: local:unsnapshotable-disk-1,mp=/invalid/mp1
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+parent: test
+rootfs: local:snapshotable-disk-1
+swap: 512
+unused0: preserved:some-disk-1
+
+[test]
+# should be thrown away
+arch: amd64
+cpulimit: 2
+cpuunits: 2048
+hostname: test2
+memory: 4096
+mp0: local:snapshotable-disk-2,mp=/invalid/mp0
+mp1: local:snapshotable-disk-4,mp=/invalid/mp1
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+snaptime: 1234567890
+swap: 1024
diff --git a/src/test/snapshot-input/create/lxc/204.conf b/src/test/snapshot-input/create/lxc/204.conf
new file mode 100644
index 0000000..4546668
--- /dev/null
+++ b/src/test/snapshot-input/create/lxc/204.conf
@@ -0,0 +1,10 @@
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+swap: 512
diff --git a/src/test/snapshot-input/delete/lxc/204.conf b/src/test/snapshot-input/delete/lxc/204.conf
new file mode 100644
index 0000000..a21c535
--- /dev/null
+++ b/src/test/snapshot-input/delete/lxc/204.conf
@@ -0,0 +1,25 @@
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+parent: test
+rootfs: local:snapshotable-disk-1
+swap: 512
+
+[test]
+#test comment
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:unsnapshotable-disk-1,mp=/invalid/mountpoint
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+snaptime: 1234567890
+swap: 512
diff --git a/src/test/snapshot-input/rollback/lxc/209.conf b/src/test/snapshot-input/rollback/lxc/209.conf
new file mode 100644
index 0000000..c9a23c9
--- /dev/null
+++ b/src/test/snapshot-input/rollback/lxc/209.conf
@@ -0,0 +1,29 @@
+# should be preserved
+arch: amd64
+cpulimit: 1
+cpuunits: 1024
+hostname: test
+memory: 2048
+mp0: local:snapshotable-disk-2,mp=/invalid/mp0
+mp1: local:unsnapshotable-disk-1,mp=/invalid/mp1
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+parent: test
+rootfs: local:snapshotable-disk-1
+swap: 512
+unused0: preserved:some-disk-1
+
+[test]
+# should be thrown away
+arch: amd64
+cpulimit: 2
+cpuunits: 2048
+hostname: test2
+memory: 4096
+mp0: local:snapshotable-disk-2,mp=/invalid/mp0
+mp1: local:snapshotable-disk-4,mp=/invalid/mp1
+net0: bridge=vmbr0,hwaddr=12:34:56:78:90:12,ip=dhcp,ip6=dhcp,name=eth0,type=veth
+ostype: redhat
+rootfs: local:snapshotable-disk-1
+snaptime: 1234567890
+swap: 1024
diff --git a/src/test/snapshot-test.pm b/src/test/snapshot-test.pm
index 91a2af9..4fc735b 100644
--- a/src/test/snapshot-test.pm
+++ b/src/test/snapshot-test.pm
@@ -16,6 +16,7 @@ use PVE::ReplicationConfig;
 use Test::MockModule;
 use Test::More;
 
+my $activate_storage_possible = 1;
 my $nodename;
 my $snapshot_possible;
 my $vol_snapshot_possible = {};
@@ -122,6 +123,15 @@ sub mocked_volume_snapshot_needs_fsfreeze {
     return 0;
 }
 
+sub mocked_activate_storage {
+    my ($storecfg, $storeid) = @_;
+    die "Storage config not mocked! aborting\n"
+	if defined($storecfg);
+    die "storage activation failed\n"
+	if !$activate_storage_possible;
+    return;
+}
+
 sub mocked_vm_stop {
     if ($kill_possible) {
 	$running = 0;
@@ -390,6 +400,7 @@ $vol_snapshot_rollback_possible->{"local:snapshotable-disk-4"} = 1;
 printf("\n");
 printf("Setting up Mocking for PVE::Storage\n");
 my $storage_module = new Test::MockModule('PVE::Storage');
+$storage_module->mock('activate_storage', \&mocked_activate_storage);
 $storage_module->mock('config', sub { return undef; });
 $storage_module->mock('volume_snapshot', \&mocked_volume_snapshot);
 $storage_module->mock('volume_snapshot_delete', \&mocked_volume_snapshot_delete);
@@ -429,6 +440,13 @@ $freeze_possible = 1;
 printf("Expected error for snapshot_create when mp volume snapshot is not possible\n");
 testcase_create("203", "test", 0, "test comment", "volume snapshot disabled\n\n", { "local:snapshotable-disk-1" => "test" }, { "local:snapshotable-disk-1" => "test" });
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_create when storage activation is not possible\n");
+testcase_create("204", "test", 0, "test comment", "storage activation failed\n\n");
+
+$activate_storage_possible = 1;
+
 $nodename = "delete";
 printf("\n");
 printf("Running delete tests\n");
@@ -461,6 +479,13 @@ testcase_delete("203", "test", 0, "volume snapshot delete disabled\n", { "local:
 printf("Expected error for snapshot_delete with locked config\n");
 testcase_delete("202", "test", 0, "CT is locked (backup)\n");
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_delete when storage activation is not possible\n");
+testcase_delete("204", "test", 0, "storage activation failed\n");
+
+$activate_storage_possible = 1;
+
 $nodename = "rollback";
 printf("\n");
 printf("Running rollback tests\n");
@@ -511,4 +536,11 @@ testcase_rollback("207", "test", "volume_rollback_is_possible failed\n");
 printf("Expected error for snapshot_rollback with mp rollback failure (results in inconsistent state)\n");
 testcase_rollback("208", "test", "volume snapshot rollback disabled\n", { "local:snapshotable-disk-1" => "test", "local:snapshotable-disk-2" => "test" });
 
+$activate_storage_possible = 0;
+
+printf("Expected error for snapshot_rollback when storage activation is not possible\n");
+testcase_rollback("209", "test", "storage activation failed\n");
+
+$activate_storage_possible = 1;
+
 done_testing();
-- 
2.30.2

[pve-devel] applied: [PATCH v2 guest-common 1/2] config: remove unused variable

[Thomas Lamprecht]

On 13.01.22 12:04, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/AbstractConfig.pm | 2 --
>  1 file changed, 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 guest-common 2/2] config: activate affected storages for snapshot operations

[Thomas Lamprecht]

On 13.01.22 12:04, Fabian Ebner wrote:
> For snapshot creation, the storage for the vmstate file is activated
> via vdisk_alloc when the state file is created.
> 
> Do not activate the volumes themselves, as that has unnecessary side
> effects (e.g. waiting for zvol device link for ZFS, mapping the volume
> for RBD). If a storage can only do snapshot operations on a volume
> that has been activated, it needs to activate the volume itself.
> 
> The actual implementation will be in the plugins to be able to skip
> CD ROM drives and bind-mounts, etc.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/AbstractConfig.pm | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
>

applied, thanks! But I defused the fixme comment, this is internal and doesn't
need to wait until a major release.

[pve-devel] applied-series: [PATCH-SERIES v2 guest-common/qemu-server/container] activate storages for snapshot operations

[Thomas Lamprecht]

On 13.01.22 12:03, Fabian Ebner wrote:
> to make it work when the storage is just not active yet, and have
> early errors when the storage cannot be activated. Also prohibits
> snapshot operations when an involved storage is disabled, but
> otherwise available.
> 
> 
> Both qemu-server and pve-container build-depend upon pve-guest-common
> for the added tests.
> 
> 
> Changes from v1:
>     * Rebase on current master.
> 
> 
> guest-common:
> 
> Fabian Ebner (2):
>   config: remove unused variable
>   config: activate affected storages for snapshot operations

> 
> qemu-server:
> 
> Fabian Ebner (1):
>   snapshot: implement __snapshot_activate_storages
> 

> 
> container:
> 
> Fabian Ebner (3):
>   config: snapshot_delete_remove_drive: check for parsed value
>   config: parse_volume: don't die when noerr is set
>   snapshot: implement __snapshot_activate_storages
> 

applied rest of series a while ago but seems I forgot to reply here, thanks!