[PATCH qemu-server v2 0/2] fix #4012: migrate: mention snapshots referencing a local ISO image

[PATCH qemu-server v2 0/2] fix #4012: migrate: mention snapshots referencing a local ISO image

[Michal Fox]

From: DualFroz <me@dualfroz.com>

v1: https://lore.proxmox.com/pve-devel/20261006085253.7-1-me@dualfroz.com/

Patch 1/2 adds a separate list of non-image volumes to the migration
test mocks, as suggested by Fiona, so the ISO image in the new test no
longer has to go into $source_vdisks, which mocks vdisk_list().

Patch 2/2 is the fix itself, with the review comments addressed, see
the changelog there.

Michal Fox (2):
  test: migration: support volumes other than images
  fix #4012: migrate: mention snapshots referencing a local ISO image

 src/PVE/QemuMigrate.pm                    | 15 ++++---
 src/test/MigrationTest/QemuMigrateMock.pm |  5 ++-
 src/test/run_qemu_migrate_tests.pl        | 52 +++++++++++++++++++++++
 3 files changed, 63 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH qemu-server v2 1/2] test: migration: support volumes other than images

[Michal Fox]

The mocked vdisk_list() is based on $source_vdisks, but the real one
only returns images. Add a separate list for other volumes on the
source, e.g. ISO images, which is only considered by the mocked
volume_size_info(). This is in preparation for a test with an ISO
image that is only referenced by a snapshot.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Michal Fox <me@dualfroz.com>
---
 src/test/MigrationTest/QemuMigrateMock.pm | 5 +++--
 src/test/run_qemu_migrate_tests.pl        | 5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/QemuMigrateMock.pm
index 3a483817..c715a60e 100644
--- a/src/test/MigrationTest/QemuMigrateMock.pm
+++ b/src/test/MigrationTest/QemuMigrateMock.pm
@@ -21,6 +21,7 @@ my $QM_LIB_PATH = $ENV{QM_LIB_PATH} or die "no QM_LIB_PATH set\n";
 
 my $source_volids = decode_json(file_get_contents("${RUN_DIR_PATH}/source_volids"));
 my $source_vdisks = decode_json(file_get_contents("${RUN_DIR_PATH}/source_vdisks"));
+my $source_other_volumes = decode_json(file_get_contents("${RUN_DIR_PATH}/source_other_volumes"));
 my $vm_status = decode_json(file_get_contents("${RUN_DIR_PATH}/vm_status"));
 my $expected_calls = decode_json(file_get_contents("${RUN_DIR_PATH}/expected_calls"));
 my $fail_config = decode_json(file_get_contents("${RUN_DIR_PATH}/fail_config"));
@@ -304,11 +305,11 @@ $MigrationTest::Shared::storage_module->mock(
         my ($scfg, $volid) = @_;
         my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
 
-        for my $v ($source_vdisks->{$storeid}->@*) {
+        for my $v ($source_vdisks->{$storeid}->@*, $source_other_volumes->{$storeid}->@*) {
             return wantarray ? ($v->{size}, $v->{format}, $v->{used}, $v->{parent}) : $v->{size}
                 if $v->{volid} eq $volid;
         }
-        die "could not find '$volid' in mock 'source_vdisks'\n";
+        die "could not find '$volid' in mock 'source_vdisks' or 'source_other_volumes'\n";
     },
 );
 
diff --git a/src/test/run_qemu_migrate_tests.pl b/src/test/run_qemu_migrate_tests.pl
index 05eed1d9..13962f72 100755
--- a/src/test/run_qemu_migrate_tests.pl
+++ b/src/test/run_qemu_migrate_tests.pl
@@ -436,6 +436,10 @@ my $source_vdisks = {
     ],
 };
 
+# non-image volumes on the source, e.g. ISO images, in the same format as $source_vdisks. They
+# are only used for the mocked volume_size_info(), because vdisk_list() only returns images.
+my $source_other_volumes = {};
+
 my $default_expected_calls_online = {
     move_config_to_node => 1,
     ssh_qm_start => 1,
@@ -1732,6 +1736,7 @@ foreach my $test (@{$tests}) {
     file_set_contents("${run_dir}/replication_config", to_json($replication_config));
     file_set_contents("${run_dir}/storage_config", to_json($storage_config));
     file_set_contents("${run_dir}/source_vdisks", to_json($source_vdisks));
+    file_set_contents("${run_dir}/source_other_volumes", to_json($source_other_volumes));
 
     my $expect_die = $test->{expect_die};
     my $expected = $test->{expected};
-- 
2.43.0

[PATCH qemu-server v2 2/2] fix #4012: migrate: mention snapshots referencing a local ISO image

[Michal Fox]

When a VM has no local ISO image attached anymore, but one of its
snapshots still references it, migration fails with:

  can't migrate local disk 'local:iso/debian.iso': local cdrom image

which does not tell where the image is coming from, since it is not
visible in the current configuration of the VM.

Add the names of the snapshots referencing the image to the error in
that case, like it is already done for a physical CD/DVD drive. The
check for the physical drive now reuses the same hint, which also
uses the plural form if more than one snapshot references it.

Add a migration test with an ISO image on the 'local' storage that is
only referenced in a snapshot. This requires the 'iso' content type
for the 'local' storage in the test.

Signed-off-by: Michal Fox <me@dualfroz.com>
---
Changes in v2:
* add the ISO image to the new $source_other_volumes in the test
  instead of $source_vdisks (preparatory patch 1/2)
* drop 'vmid' from the mocked ISO volume
* say "snapshots" in the hint if more than one snapshot references
  the volume
* split overlong line in the test

 src/PVE/QemuMigrate.pm             | 15 ++++-----
 src/test/run_qemu_migrate_tests.pl | 49 +++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..6605f5d8 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -412,15 +412,16 @@ sub scan_local_volumes {
             }
 
             my $snaprefs = $attr->{referenced_in_snapshot};
+            my $snapshot_hint = '';
+            if (defined($snaprefs) && !$attr->{is_attached}) {
+                my $snapnames = join(', ', sort keys %$snaprefs);
+                my $plural = scalar(keys %$snaprefs) > 1 ? 's' : '';
+                $snapshot_hint = " (referenced in snapshot$plural - $snapnames)";
+            }
 
             if ($attr->{cdrom}) {
                 if ($volid eq 'cdrom') {
-                    my $msg = "can't migrate local cdrom drive";
-                    if (defined($snaprefs) && !$attr->{is_attached}) {
-                        my $snapnames = join(', ', sort keys %$snaprefs);
-                        $msg .= " (referenced in snapshot - $snapnames)";
-                    }
-                    &$log_error("$msg\n");
+                    &$log_error("can't migrate local cdrom drive$snapshot_hint\n");
                     return;
                 }
                 return if $volid eq 'none';
@@ -475,7 +476,7 @@ sub scan_local_volumes {
                     $local_volumes->{$volid}->{ref} = 'generated';
                     return;
                 }
-                die "local cdrom image\n";
+                die "local cdrom image$snapshot_hint\n";
             }
 
             my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
diff --git a/src/test/run_qemu_migrate_tests.pl b/src/test/run_qemu_migrate_tests.pl
index 13962f72..37d52694 100755
--- a/src/test/run_qemu_migrate_tests.pl
+++ b/src/test/run_qemu_migrate_tests.pl
@@ -37,6 +37,7 @@ my $storage_config = {
         local => {
             content => {
                 images => 1,
+                iso => 1,
             },
             path => "/var/lib/vz",
             type => "dir",
@@ -438,7 +439,18 @@ my $source_vdisks = {
 
 # non-image volumes on the source, e.g. ISO images, in the same format as $source_vdisks. They
 # are only used for the mocked volume_size_info(), because vdisk_list() only returns images.
-my $source_other_volumes = {};
+my $source_other_volumes = {
+    'local' => [
+        {
+            'ctime' => 1589439681,
+            'format' => 'iso',
+            'parent' => undef,
+            'size' => 663748608,
+            'used' => 663748608,
+            'volid' => 'local:iso/debian.iso',
+        },
+    ],
+};
 
 my $default_expected_calls_online = {
     move_config_to_node => 1,
@@ -1240,6 +1252,41 @@ my $tests = [
             },
         },
     },
+    {
+        name => '105_local_iso_in_snapshot',
+        target => 'pve1',
+        vmid => 105,
+        vm_status => {
+            running => 0,
+        },
+        config_patch => {
+            snapshots => {
+                ohsnap => {
+                    ide2 => 'local:iso/debian.iso,media=cdrom',
+                },
+            },
+        },
+        expected_calls => {},
+        expect_die => "can't migrate local disk 'local:iso/debian.iso': local cdrom image"
+            . " (referenced in snapshot - ohsnap)",
+        expected => {
+            source_volids => local_volids_for_vm(105),
+            target_volids => {},
+            vm_config => get_patched_config(
+                105,
+                {
+                    snapshots => {
+                        ohsnap => {
+                            ide2 => 'local:iso/debian.iso,media=cdrom',
+                        },
+                    },
+                },
+            ),
+            vm_status => {
+                running => 0,
+            },
+        },
+    },
     {
         name => '105_cdrom',
         target => 'pve1',
-- 
2.43.0