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 8FD951FF0A7 for ; Mon, 17 Aug 2026 08:59:45 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3015A23764; Mon, 17 Aug 2026 08:59:42 +0200 (CEST) Message-ID: Date: Mon, 17 Aug 2026 08:59:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH v2 qemu-server] cloud-init: query volume size via vdisk_list() to fix regression with qcow2 on LVM To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20260812111316.85049-1-f.ebner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260812111316.85049-1-f.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786949954950 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.824 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) RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS 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: UMBBUMYNRYB5UMWFTGQW4OLZC7Q7QNH6 X-Message-ID-Hash: UMBBUMYNRYB5UMWFTGQW4OLZC7Q7QNH6 X-MailFrom: d.csapak@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: generally works for the issue that is described, but see my comment inline On 8/12/26 1:13 PM, Fiona Ebner wrote: > Since pve-storage commit 05032c5 ("fix #7811: storage: lvm: reject > allocation on format and volume name mismatch") and its follow-ups, > cloud-init disks are named with a '.qcow2' extension on LVM storages > with snapshot-as-volume-chain enabled. > > This causes a regression, because volume_size_info() fails and returns > undef when the qcow2 volume is not active. When the size cannot be > determined, commit_cloudinit_disk() function assumes that the disk > does not yet exist and tries to allocate new disk with the same name, > which fails. > > # qm start 100 > failed to stat '/dev/lvm/vm-100-cloudinit.qcow2' > Rounding up size to full physical extent 8.00 MiB > lvcreate 'lvm/vm-100-cloudinit.qcow2' error: Logical Volume > "vm-100-cloudinit.qcow2" already exists in volume group "lvm" > > Fix the issue by checking the existence/size with vdisk_list(), which > also works for deactivated volumes. > > Activating the volume earlier is an alternative, but would need to be > inside an eval block that silently ignores failure, since the volume > might not exist yet, which should not be logged. And in case where the > qcow2 on LVM volume does exist, but activation fails, there would be > an attempt to allocate a new volume with the same name, which also > seems less than ideal. So that approach is a bit hacky. > > Signed-off-by: Fiona Ebner > --- > > Changes in v2: > * don't include full shell prompt in commit message > * fix commit title > > src/PVE/QemuServer/Cloudinit.pm | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/PVE/QemuServer/Cloudinit.pm b/src/PVE/QemuServer/Cloudinit.pm > index c1311da8..5af6b608 100644 > --- a/src/PVE/QemuServer/Cloudinit.pm > +++ b/src/PVE/QemuServer/Cloudinit.pm > @@ -39,7 +39,18 @@ sub commit_cloudinit_disk { > my $scfg = PVE::Storage::storage_config($storecfg, $storeid); > my $format = checked_volume_format($storecfg, $drive->{file}); > > - my $size = eval { PVE::Storage::volume_size_info($storecfg, $drive->{file}) }; > + my $volumes = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid, [$drive->{file}], 'images'); for lvm this looks alright, but for other storages this is now a relatively large performance impact? vdisk_list calls list_images of the plugin, and for e.g. nfs/dir/etc. this calls 'file_size_info' for *all* found volumes (since that does not filter early for vollist) on nfs this could maybe be problematic with many qcow2 files? also, this is not eval'd, so any plugin that dies during list_images (e.g. because of permissions, or some other error condition) blocks the vm start now.. IMHO this should be in an eval and the storage plugins should filter early on vollist to avoid unnecessary work. alternatively we could drop the vollist and just give the $vmid which filters early in most (?) storages. a third thing i noticed is that vdisk_list actives the storage, which volume_size_info does not (AFAICS). This is probably correct anyway but worth noting in the commit message IMO > + if (scalar($volumes->{$storeid}->@*) > 1) { > + # just to be sure, since this is the first caller with $vollist outside of the storage tests > + print "bug: storage plugin for '$storeid' does not honor \$vollist for list_images()\n"; > + } > + my $size; > + for my $volume_info ($volumes->{$storeid}->@*) { > + next if $volume_info->{volid} ne $drive->{file}; > + $size = $volume_info->{size} // $volume_info->{'approximate-size'}; > + last; > + } > + > if (!defined($size) || $size <= 0) { > $volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/; > my $name = $1;