From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 4D3361FF0C2 for ; Thu, 22 Oct 2026 15:25:58 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3C75A2177B; Thu, 22 Oct 2026 15:25:56 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 22 Oct 2026 15:25:51 +0200 Message-Id: To: "Fiona Ebner" , Subject: Re: [PATCH qemu-server 2/4] fix #8209: clone disk: do not ignore failure to query size of target From: "Elias Huhsovitz" X-Mailer: aerc 0.20.0 References: <20261022093901.52732-1-f.ebner@proxmox.com> <20261022093901.52732-3-f.ebner@proxmox.com> In-Reply-To: <20261022093901.52732-3-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792675551879 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.508 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: O3FPLY7WQXYAYBOBW2TNPF3PH5M2EZ4S X-Message-ID-Hash: O3FPLY7WQXYAYBOBW2TNPF3PH5M2EZ4S X-MailFrom: e.huhsovitz@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: Comments inline (This is not a full review yet) On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote: > Since commit 3bae384f ("clone disk: avoid errors after disk was moved > by QEMU"), failure to query the target volume's size after clone is > ignored. It's sensible not to fail hard at this stage, because in case > of live mirror for the same VM with 'complete' completion mode, the > drive was already switched. Also, the error might be a harmless > timeout. Still, as reported in bug #8209, there could be an actual > issue, so log a warning and communicate a failure to the caller, so it > can avoid destructive action. > > Some storage plugin implementations of the volume_size_info() method > do not fail with a hard error. In particular, the native file-based > plugins use file_size_info() which returns undef upon failure. Thus, > explicitly check the size. > > Signed-off-by: Fiona Ebner > --- > src/PVE/API2/Qemu.pm | 21 ++++++++++++++------- > src/PVE/QemuServer.pm | 14 +++++++++++++- > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm > index f4dbd2a1..8e7ece52 100644 > --- a/src/PVE/API2/Qemu.pm > +++ b/src/PVE/API2/Qemu.pm > @@ -432,7 +432,7 @@ my $import_from_volid =3D sub { > $cloned =3D $clonefn->(); > } > =20 > - return $cloned->@{qw(file size)}; > + return $cloned->{drive}->@{qw(file size)}; > }; > =20 > my sub prohibit_tpm_version_change { > @@ -4703,7 +4703,7 @@ __PACKAGE__->register_method({ > $dest_info->{efisize} =3D PVE::QemuServer::get_efiva= rs_size($oldconf) > if $opt eq 'efidisk0'; > =20 > - my $newdrive =3D PVE::QemuServer::clone_disk( > + my $clone_result =3D PVE::QemuServer::clone_disk( > $storecfg, > $source_info, > $dest_info, > @@ -4715,7 +4715,7 @@ __PACKAGE__->register_method({ > $clonelimit, > ); > =20 > - $newconf->{$opt} =3D PVE::QemuServer::print_drive($n= ewdrive); > + $newconf->{$opt} =3D PVE::QemuServer::print_drive($c= lone_result->{drive}); > =20 > PVE::QemuConfig->write_config($newid, $newconf); > $i++; > @@ -4900,6 +4900,7 @@ __PACKAGE__->register_method({ > my $target_disk =3D extract_param($param, 'target-disk') // $dis= k; > my $storeid =3D extract_param($param, 'storage'); > my $format =3D extract_param($param, 'format'); > + my $do_delete =3D extract_param($param, 'delete'); > =20 > my $storecfg =3D PVE::Storage::config(); > =20 > @@ -4932,7 +4933,7 @@ __PACKAGE__->register_method({ > $storecfg, $conf, $disk, $old_volid, > ); > die "you can't move a disk with snapshots and delete the sou= rce\n" > - if $snapshotted && $param->{delete}; > + if $snapshotted && $do_delete; > =20 > return ($conf, $drive, $oldstoreid, $snapshotted); > }; > @@ -4986,7 +4987,7 @@ __PACKAGE__->register_method({ > $dest_info->{efisize} =3D PVE::QemuServer::get_efivars_s= ize($conf) > if $disk eq 'efidisk0'; > =20 > - my $newdrive =3D PVE::QemuServer::clone_disk( > + my $clone_result =3D PVE::QemuServer::clone_disk( > $storecfg, > $source_info, > $dest_info, > @@ -4997,9 +4998,15 @@ __PACKAGE__->register_method({ > $conf->{agent}, > $movelimit, > ); > + my $newdrive =3D $clone_result->{drive}; > $conf->{$disk} =3D PVE::QemuServer::print_drive($newdriv= e); > =20 > - if (!$param->{delete}) { > + if ($do_delete && $clone_result->{'after-copy-error'}) { > + warn "skipping delete of source because of error aft= er copy\n"; > + $do_delete =3D 0; Since we now set $clone_result->{'after-copy-error'}, if e.g., the target NFS share returns an error, we set $do_delete =3D 0 to prevent the source disk from being deleted. > + } > + > + if (!$do_delete) { > my $unused_key =3D PVE::QemuConfig->add_unused_volum= e($conf, $old_volid); So now in the case that $clone_result->{'after-copy-error'} is set, we always call add_unused_volume(). But add_unused_volume() can fail with "Too many unused volumes - please delete them first.\n" according to AbstractConfig::add_unused_volume() Couldn't this kill the disk during a live migration, if we are unable to add the unused volume? In Drive.pm the maximum number for unused disks is set to: our $MAX_UNUSED_DISKS =3D 256; IMO being unable to add a new disk in a production system, might be a real possibility. > print("adding source volume '$old_volid' as '$unused= _key' to config\n"); > } > @@ -5030,7 +5037,7 @@ __PACKAGE__->register_method({ > die "storage migration failed: $err"; > } > =20 > - if ($param->{delete}) { > + if ($do_delete) { > print("removing source volume '$old_volid' after success= ful copy\n"); > eval { > PVE::Storage::deactivate_volumes($storecfg, [$old_vo= lid]); > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 1efeeb66..8f062466 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -7929,14 +7929,26 @@ sub clone_disk { > } > } > =20 > + my $result =3D {}; > my $size =3D eval { PVE::Storage::volume_size_info($storecfg, $newvo= lid, 10) }; > + my $err =3D $@; > + $err =3D "no size in result" if !$err && !$size; > + if ($err) { > + # The failure might be a harmless timeout. In case of live mirro= r for the same VM with > + # 'complete' completion mode, the drive was already switched. Th= us, avoid a hard error at > + # this stage. Still, there could be an actual issue, so the call= er can avoid destructive > + # action based on the presence of 'after-copy-error'. > + log_warn("unable to query size for '$newvolid' after copy - $err= "); > + $result->{'after-copy-error'} =3D 1; > + } > =20 > my $disk =3D dclone($drive); > delete $disk->{format}; > $disk->{file} =3D $newvolid; > $disk->{size} =3D $size if defined($size) && !$unused; > =20 > - return $disk; > + $result->{drive} =3D $disk; > + return $result; > } > =20 > sub qemu_use_old_bios_files {