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 ED4041FF0A7 for ; Mon, 17 Aug 2026 10:49:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6DA802396C; Mon, 17 Aug 2026 10:49:24 +0200 (CEST) Message-ID: <31704036-fc55-4ca6-a2b6-37be600d352c@proxmox.com> Date: Mon, 17 Aug 2026 10:49:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 qemu-server] cloud-init: query volume size via vdisk_list() to fix regression with qcow2 on LVM To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260812111316.85049-1-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786956538774 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.239 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 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: CZE6S7TZA66KRHIKU22GFZ6SDPBDR2MN X-Message-ID-Hash: CZE6S7TZA66KRHIKU22GFZ6SDPBDR2MN 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 17.08.26 um 10:27 AM schrieb Dominik Csapak: > On 8/17/26 10:11 AM, Fiona Ebner wrote: >> Am 17.08.26 um 8:59 AM schrieb Dominik Csapak: >>> 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) >> >> Good catch! This is a bit unfortunate. The reason is that the volid >> looks different if it's a linked clone. >> >>> >>> 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. >>> >> >> I'm not fully convinced it should be eval'd, because if listing the >> volume you are interested in fails, that's a good reason to fail. But >> since some plugins don't properly just query the actual volume yet, I'll >> just add it to be sure. >> >>> alternatively we could drop the vollist and just give the $vmid >>> which filters early in most (?) storages. >> >> I do already specify the $vmid as well ;) But why drop vollist? If the >> plugin respects it, it's better to have it. If the plugin doesn't, it >> doesn't hurt. > > yes but e.g. the dir storage explicitly does not use vmid for skipping > early if vollist is given: > > ``` > next if !$vollist && defined($vmid) && ($owner ne $vmid); > ``` > Right. The semantics in our implementations are that if $vollist is specified, then $vmid is completely ignored. I feel like we should have a more tailored volume_exists() or volume_info() at some point, which avoids the complicated semantics and overhead of vdisk_list(). We already had a need in the past: there is rbd_volume_exists(), but unfortunately, no plugin method.