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

[Fiona Ebner <f.ebner@proxmox.com>]

Hi Michal,

thank you for the contribution!

Am 06.10.26 um 10:53 AM schrieb 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.
> 
> Signed-off-by: Michal Fox <me@dualfroz.com>
> ---
> Tested with a new migration test for an ISO image on local storage
> that is only referenced in a snapshot. Without the fix, the error does
> not contain the snapshot name. The whole qemu-server test suite passes.
> 
> The test needed the ISO in the mocked volume list and 'iso' content on
> the mocked 'local' storage.
> 
> The GUI still only shows 'Cannot migrate VM with local CD/DVD' in that
> case, which could be done as a follow-up in pve-manager.

An additional UI patch would be nice.

> 
>  src/PVE/QemuMigrate.pm             | 14 ++++-----
>  src/test/run_qemu_migrate_tests.pl | 47 ++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
> index 8da6f15d..a9b69898 100644
> --- a/src/PVE/QemuMigrate.pm
> +++ b/src/PVE/QemuMigrate.pm
> @@ -412,15 +412,15 @@ 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);
> +                $snapshot_hint = " (referenced in snapshot - $snapnames)";

Nit: Pre-existing, but while we're at it, I'd suggest "snapshot(s)", or
conditionally add the 's' depending on the count.

> +            }
>  
>              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 +475,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 05eed1d9..49248ed9 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",
> @@ -346,6 +347,17 @@ my $source_vdisks = {
>              'volid' => 'local-dir:4567/vm-4567-state-snap2.raw',
>          },
>      ],
> +    'local' => [
> +        {
> +            'ctime' => 1589439681,
> +            'format' => 'iso',
> +            'parent' => undef,
> +            'size' => 663748608,
> +            'used' => 663748608,
> +            'vmid' => '0',

Nit: I'd prefer to keep vmid undef here or even better, not specified at
all.

> +            'volid' => 'local:iso/debian.iso',
> +        },
> +    ],

I'm not sure we should add this to the $source_vdisks, because that is
used for mocking vdisk_list(), which only returns images and not ISOs.

A clean way would probably be to add another file and consider its
contents in the mocked volume_size_info() call in QemuMigrateMock.pm

Or we could rename it to $source_volumes and have non-images carry an
explicit 'vtype' property and have the mocked vdisk_list() filter out
non-images. Images won't need to specify 'vtype' explicitly.

Either way would be nice to have in a separate, preparatory patch.

>      'local-lvm' => [
>          {
>              'ctime' => '1589277334',
> @@ -1236,6 +1248,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)",

Style nit: line too long

> +        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',

Best Regards,
Fiona