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 513DC1FF0A7 for ; Mon, 17 Aug 2026 13:29:25 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A6E0323CA2; Mon, 17 Aug 2026 13:29:24 +0200 (CEST) Message-ID: Date: Mon, 17 Aug 2026 13:29:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 qemu-server] cloud-init: commit: activate volume early enough to fix regression with qcow2 on LVM To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260817103032.73185-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: 1786966140464 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.235 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: 6TKANXWJKTZL3UQCQMHEXRXP2JB5D3YZ X-Message-ID-Hash: 6TKANXWJKTZL3UQCQMHEXRXP2JB5D3YZ 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 1:24 PM schrieb Dominik Csapak: > one small comment inline > > On 8/17/26 12:30 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 activating the volume early enough. >> >> Note that activate_volumes() also activates the storage, which fixes >> another bug, since nothing ensured that the storage was active before. >> For a VM with just the cloud-init disk on the 'nfs' storage: >> >>   # umount /mnt/pve/nfs && qm start 100 >>   failed to stat '/mnt/pve/nfs/images/100/vm-100-cloudinit.raw' >>   disk image '/mnt/pve/nfs/images/100/vm-100-cloudinit.raw' already >> exists >> >> Once there is a proper existence check function in the storage layer, >> that can be used instead. For vdisk_list(), not all plugin >> implementations filter early for $vollist, so there can be overhead >> and some plugins even might fail when there are issues with unrelated >> volumes. Even if using an eval block, if the cloud-init volume already >> exists, but vdisk_list() fails, it would be detected as the volume not >> existing and then allocation would fail. The current approach with >> activation avoids that an issue with a different volume blocks VM >> start. >> >> Signed-off-by: Fiona Ebner >> --- >> >> Changes in v2: >> * Mention that activating the storage early enough is important too. >> * Different approach, because vdisk_list() failing for unrelated >>    volumes should really not block start. >> >>   src/PVE/QemuServer/Cloudinit.pm | 24 +++++++++++++++++++++--- >>   1 file changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/src/PVE/QemuServer/Cloudinit.pm b/src/PVE/QemuServer/ >> Cloudinit.pm >> index c1311da8..c6a83126 100644 >> --- a/src/PVE/QemuServer/Cloudinit.pm >> +++ b/src/PVE/QemuServer/Cloudinit.pm >> @@ -39,16 +39,34 @@ sub commit_cloudinit_disk { >>       my $scfg = PVE::Storage::storage_config($storecfg, $storeid); >>       my $format = checked_volume_format($storecfg, $drive->{file}); >>   +    # Some kinds of volumes, like qcow2 on LVM, need to be active >> to query the size. Note that this >> +    # also activates the storage. Failure is expected if the volume >> does not exist. >> +    # TODO use a proper existence check once there is a storage >> function for it. For vdisk_list(), >> +    # not all plugin implementations filter early for $vollist, so >> there can be overhead and some >> +    # plugins even might fail when there are issues with unrelated >> volumes. >> +    eval { PVE::Storage::activate_volumes($storecfg, [$drive- >> >{file}]); }; >> +    my $activation_error = $@; >> + >>       my $size = eval { PVE::Storage::volume_size_info($storecfg, >> $drive->{file}) }; >>       if (!defined($size) || $size <= 0) { >>           $volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/; >>           my $name = $1; >>           $size = 4 * 1024; >> -        PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, >> $format, $name, $size); >> + >> +        eval { PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, >> $format, $name, $size); }; >> +        if (my $err = $@) { >> +            # Log the activation failure from earlier, since it might >> be relevant. The volume might >> +            # have existed and the failed activation might be the >> cause of not getting a size. >> +            warn $activation_error; > > this should probably be 'warn $activation_error if > defined($activation_error);' > > since the activation might have succeeded, depending on the storage > plugin (or if the file is empty)? > > (not a big issue though, at worst, there is a stray 'something went > wrong' line in the logs) Right, will fix!