From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 3A5031FF0AA for ; Tue, 06 Oct 2026 12:00:16 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 08026214DA; Tue, 06 Oct 2026 12:00:12 +0200 (CEST) Message-ID: Date: Tue, 6 Oct 2026 12:00:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server] fix #4012: migrate: mention snapshots referencing a local ISO image To: Michal Fox , pve-devel@lists.proxmox.com References: <20261006085253.7-1-me@dualfroz.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261006085253.7-1-me@dualfroz.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791280807594 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.460 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 4L3ZRXPIEZAU7CFTGDJYEDI7TF7M72FA X-Message-ID-Hash: 4L3ZRXPIEZAU7CFTGDJYEDI7TF7M72FA X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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