[PATCH e2e-tests 1/1] tests: storage: report a non-qcow2 volume-chain storage as a setup gap

["Michael Köppl" <m.koeppl@proxmox.com>]

For storage that can only snapshot through volume chains, the snapshot
feature is only enabled for qcow2 volumes. LVM switches its default
format to qcow2 once `snapshot-as-volume-chain` is enabled. On other
storages, disks keep being allocated as raw, so every snapshot attempt
fails with "snapshot feature not available" and the suite reported the
snapshot capabilities as unsupported, even though the plugin implements
them.

The plugin is not strictly missing the capability. However, it is a
setup gap, so report is accordingly. The volume-chain snapshot mechanism
itself stays covered by the volume-chain snapshot test, which allocates
qcow2 because that is what it verifies.

The messages now also name the format that was tried, which makes it
easier to tell the two "unsupported" cases apart.

Reported-by: Hannes Dürr <h.duerr@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
The initial idea here was to automatically detect whether qcow2 should
be used based on whether the storage config had
`snapshot-as-volume-chain 1`. However, making that assumption seems
wrong. For LVM, PVE automatically changes the disk format to qcow2 if
snapshot-as-volume-chain is enabled. If a custom storage requires the
features to be enabled in order to support snapshots, the configuration
should also be adapted accordingly instead of the test suite picking the
disk format for the storage plugin automatically.

Should this be applied, the section from the README should also be added
to the wiki page.

 Proxmox/Test/Util.pm                          | 43 ++++++++++++++++++-
 storage-plugin-tests/README                   | 18 ++++++++
 storage-plugin-tests/tests/disk_snapshot.pl   | 12 +++---
 .../tests/snapshot_rollback_data.pl           |  6 ++-
 .../tests/vm_destroy_with_snapshots.pl        | 12 ++++--
 5 files changed, 76 insertions(+), 15 deletions(-)

diff --git a/Proxmox/Test/Util.pm b/Proxmox/Test/Util.pm
index 9b38924..0701747 100644
--- a/Proxmox/Test/Util.pm
+++ b/Proxmox/Test/Util.pm
@@ -3,8 +3,17 @@ use strict;
 use warnings FATAL => 'all';
 
 use parent 'Exporter';
-our @EXPORT_OK =
-    qw(poll_until retry skip_unsupported skip_prerequisite guest_name node_from_upid backup_target);
+our @EXPORT_OK = qw(
+    poll_until
+    retry
+    skip_unsupported
+    skip_prerequisite
+    skip_failed_snapshot
+    guest_name
+    node_from_upid
+    backup_target
+    guest_disk_format
+);
 
 use Time::HiRes qw(time sleep);
 use Test::More ();
@@ -99,6 +108,22 @@ sub skip_prerequisite {
     Test::More::plan(skip_all => "PREREQUISITE: $reason");
 }
 
+# Skip test if snapshot could not be taken. A storage that snapshots through volume chains offers
+# the snapshot feature for qcow2 volumes only, so a volume in another format means the storage is
+# configured such that its own snapshot mechanism cannot be used at all. Suggest a re-run with an
+# updated configuration instead of assuming a plugin gap.
+sub skip_failed_snapshot {
+    my ($storage_cfg, $storage_id, $vol_format, $error) = @_;
+
+    skip_prerequisite("storage '$storage_id' requires qcow2 volumes for snapshots through volume"
+            . " chains, but the disk was allocated as '$vol_format'. Change the storage's default"
+            . " format or set 'format qcow2' on the storage and re-run")
+        if $storage_cfg->{'snapshot-as-volume-chain'} && $vol_format ne 'qcow2';
+
+    skip_unsupported("the plugin does not support snapshots of '$vol_format' volumes"
+        . (length($error // '') ? ": $error" : ''));
+}
+
 # Where a backup test should dump to: the storage under test if it can hold backups, else the
 # validated --backup-fallback-storage. $content is the storage's content-type hashref. No
 # auto-picking: that could grab a disabled storage and fail the run for an unrelated reason. Skips
@@ -126,4 +151,18 @@ sub backup_target {
     return $fallback;
 }
 
+# The format the storage reports for a guest's disk. Returns 'unknown' when the volume cannot be
+# queried.
+sub guest_disk_format {
+    my ($pve, $storage_id, $vmid, $drive) = @_;
+
+    my $node = $pve->get_nodename();
+    my $cfg = eval { $pve->client()->get("/nodes/$node/qemu/$vmid/config", {}) } // {};
+    my ($volid) = ($cfg->{$drive} // '') =~ m/^([^,]+)/;
+    return 'unknown' if !$volid;
+
+    my $vol = eval { $pve->client()->get("/nodes/$node/storage/$storage_id/content/$volid", {}) };
+    return ($vol // {})->{format} // 'unknown';
+}
+
 1;
diff --git a/storage-plugin-tests/README b/storage-plugin-tests/README
index 5f486a4..165888d 100644
--- a/storage-plugin-tests/README
+++ b/storage-plugin-tests/README
@@ -50,6 +50,24 @@ They need either internet access plus `libguestfs-tools` (so a Debian cloud
 image can be downloaded and prepared once), or `--qga-image` pointing at your
 own agent-equipped image.
 
+Snapshots and the disk format
+-----------------------------
+
+PVE offers the snapshot feature for storages that snapshot through volume chains
+(`snapshot-as-volume-chain`) only for qcow2 volumes. An LVM storage switches its
+default format to qcow2 as soon as that option is enabled. Other storages keep
+allocating `raw`, so no disk created on them the usual way can be snapshotted at
+all.
+
+The suite tests the storage as it is configured and does not pick a format of
+its own, so on such a storage the snapshot capabilities come out as not tested
+until the storage allocates qcow2 by default:
+
+    pvesm set mystorage --format qcow2
+
+The one exception is `volume-chain snapshots`, which allocates a qcow2 disk
+itself, because a qcow2 backing chain is the mechanism it verifies.
+
 Reading the result
 ------------------
 
diff --git a/storage-plugin-tests/tests/disk_snapshot.pl b/storage-plugin-tests/tests/disk_snapshot.pl
index 8191069..2a20a51 100755
--- a/storage-plugin-tests/tests/disk_snapshot.pl
+++ b/storage-plugin-tests/tests/disk_snapshot.pl
@@ -6,7 +6,7 @@ use Test::More;
 
 use lib '..';
 use Proxmox::Test::PVEInstance;
-use Proxmox::Test::Util qw(skip_unsupported skip_prerequisite guest_name);
+use Proxmox::Test::Util qw(skip_failed_snapshot skip_prerequisite guest_name guest_disk_format);
 
 my $storage_id = $ENV{PLUGIN_STORAGE_ID};
 if (!$storage_id) {
@@ -93,9 +93,7 @@ $pve->task_ok($upid, "VM started successfully") or do {
 };
 
 # A direct write at offset 0 of a non-raw volume (qcow2) would corrupt the image header.
-my $vol_format =
-    eval { $pve->client()->get("/nodes/$node/storage/$storage_id/content/$volid", {})->{format}; }
-    // 'unknown';
+my $vol_format = guest_disk_format($pve, $storage_id, $vmid, 'scsi0');
 my $can_verify_data = 0;
 if ($vol_format eq 'raw') {
     $can_verify_data = $write_marker->('A');
@@ -103,8 +101,8 @@ if ($vol_format eq 'raw') {
     diag("volume format '$vol_format' cannot be verified by direct writes, skipping data checks");
 }
 
-# A plugin without snapshot support errors on the API call or fails the task; report that as
-# unsupported instead of failing.
+# A plugin without snapshot support for this volume format errors here. Report that as unsupported
+# or as a storage setup gap instead of failing.
 my $snapshot = sub {
     my ($name) = @_;
     return $pve->client()->post("/nodes/$node/qemu/$vmid/snapshot", { snapname => $name });
@@ -121,7 +119,7 @@ if (!$pve->task_successful($snap1_result)) {
         . ($snap1_result // 'n/a')
         . ", err=$@). Plugin likely doesn't support snapshots.");
     $pve->destroy_guest(qemu => $vmid);
-    skip_unsupported("the plugin does not support snapshots");
+    skip_failed_snapshot($storage_cfg, $storage_id, $vol_format);
 }
 
 pass("snap1 created");
diff --git a/storage-plugin-tests/tests/snapshot_rollback_data.pl b/storage-plugin-tests/tests/snapshot_rollback_data.pl
index 2ab5c5d..2dd8773 100755
--- a/storage-plugin-tests/tests/snapshot_rollback_data.pl
+++ b/storage-plugin-tests/tests/snapshot_rollback_data.pl
@@ -6,7 +6,7 @@ use Test::More;
 
 use lib '..';
 use Proxmox::Test::PVEInstance;
-use Proxmox::Test::Util qw(skip_unsupported skip_prerequisite guest_name);
+use Proxmox::Test::Util qw(skip_failed_snapshot skip_prerequisite guest_name guest_disk_format);
 use Proxmox::Test::StorageGuest qw(boot_storage_guest guest_write_marker guest_read_marker);
 
 # Verifies snapshot rollback restores on-disk data, for any format: a guest writes a marker, the
@@ -47,8 +47,10 @@ eval {
         );
     };
     if ($@ || !$pve->task_successful($snap)) {
+        my $err = $@ || $snap;
+        my $vol_format = guest_disk_format($pve, $storage_id, $vmid, 'scsi0');
         $pve->destroy_guest(qemu => $vmid);
-        skip_unsupported("the plugin does not support snapshots: " . ($@ || $snap));
+        skip_failed_snapshot($scfg, $storage_id, $vol_format, $err);
     }
     pass('took a disk snapshot');
 
diff --git a/storage-plugin-tests/tests/vm_destroy_with_snapshots.pl b/storage-plugin-tests/tests/vm_destroy_with_snapshots.pl
index 0e18bb7..35639b6 100755
--- a/storage-plugin-tests/tests/vm_destroy_with_snapshots.pl
+++ b/storage-plugin-tests/tests/vm_destroy_with_snapshots.pl
@@ -6,7 +6,8 @@ use Test::More;
 
 use lib '..';
 use Proxmox::Test::PVEInstance;
-use Proxmox::Test::Util qw(skip_unsupported skip_prerequisite guest_name poll_until);
+use Proxmox::Test::Util
+    qw(skip_failed_snapshot skip_prerequisite guest_name poll_until guest_disk_format);
 
 # Verifies that a VM can be destroyed while snapshots still exist, without deleting them one by one
 # first.
@@ -73,8 +74,8 @@ my $snap1_upid = eval { $snapshot->('snap1') };
 my $snap1_result;
 $snap1_result = eval { $pve->wait_for_task($snap1_upid, 1) } if $snap1_upid;
 
-# A plugin without snapshot support errors on the API call or fails the task. Report that as
-# unsupported instead of failing.
+# A plugin without snapshot support for this volume format errors here. Report that as unsupported
+# or as a storage setup gap instead of failing.
 if (!$pve->task_successful($snap1_result)) {
     diag("First snapshot did not succeed (UPID="
         . ($snap1_upid // 'undef')
@@ -82,9 +83,12 @@ if (!$pve->task_successful($snap1_result)) {
         . ($snap1_result // 'n/a')
         . ", err=$@). Plugin likely doesn't support snapshots.");
 
+    # Query the format while the volume is still around for printing in the skip message.
+    my $vol_format = guest_disk_format($pve, $storage_id, $vmid, 'scsi0');
+
     $pve->destroy_guest(qemu => $vmid);
 
-    skip_unsupported("the plugin does not support snapshots");
+    skip_failed_snapshot($storage_cfg, $storage_id, $vol_format);
 }
 pass("snap1 created");
 
-- 
2.47.3