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 AA4901FF0B3 for ; Wed, 28 Oct 2026 10:05:25 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1477A21401; Wed, 28 Oct 2026 10:05:23 +0100 (CET) Message-ID: <4dc611ef-b185-4ebe-bb54-b74b0761014c@proxmox.com> Date: Wed, 28 Oct 2026 10:05:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server 2/4] fix #8209: clone disk: do not ignore failure to query size of target To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20261022093901.52732-1-f.ebner@proxmox.com> <20261022093901.52732-3-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793178318765 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.339 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: QLN2KMZSZS3XMBLFJRPZVWQXUG2VZSTW X-Message-ID-Hash: QLN2KMZSZS3XMBLFJRPZVWQXUG2VZSTW 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: Am 22.10.26 um 3:25 PM schrieb Elias Huhsovitz: > On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote: >> @@ -4997,9 +4998,15 @@ __PACKAGE__->register_method({ >> $conf->{agent}, >> $movelimit, >> ); >> + my $newdrive = $clone_result->{drive}; >> $conf->{$disk} = PVE::QemuServer::print_drive($newdrive); >> >> - if (!$param->{delete}) { >> + if ($do_delete && $clone_result->{'after-copy-error'}) { >> + warn "skipping delete of source because of error after copy\n"; >> + $do_delete = 0; > > Since we now set $clone_result->{'after-copy-error'}, if e.g., the > target NFS share returns an error, we set $do_delete = 0 to prevent the > source disk from being deleted. > >> + } >> + >> + if (!$do_delete) { >> my $unused_key = PVE::QemuConfig->add_unused_volume($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 = 256; > > IMO being unable to add a new disk in a production system, might be a > real possibility. It's not a new issue (my patch only changes that 'delete' is not used in a rare error scenario, delete=0 was already possible before). I'm not aware of any reports about people running into this. It should be quite rare to reach that limit in practice. Still, it's an issue and I'll add a patch to fix it in v2. And the issue is also present in case the template_create() call fails, I'll guard that too. > >> print("adding source volume '$old_volid' as '$unused_key' to config\n"); >> } >> @@ -5030,7 +5037,7 @@ __PACKAGE__->register_method({ >> die "storage migration failed: $err"; >> } >> >> - if ($param->{delete}) { >> + if ($do_delete) { >> print("removing source volume '$old_volid' after successful copy\n"); >> eval { >> PVE::Storage::deactivate_volumes($storecfg, [$old_volid]);