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 DE7C81FF0A7 for ; Mon, 17 Aug 2026 13:25:16 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 5ED1A23C76; Mon, 17 Aug 2026 13:25:16 +0200 (CEST) Message-ID: Date: Mon, 17 Aug 2026 13:25:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH v2 qemu-server] cloud-init: commit: activate volume early enough to fix regression with qcow2 on LVM To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20260817103032.73185-1-f.ebner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260817103032.73185-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: 1786965891663 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.330 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: JAPQUKMJO5P2CZ3JVZFWDTNQANOOEMQP X-Message-ID-Hash: JAPQUKMJO5P2CZ3JVZFWDTNQANOOEMQP 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: 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) > + die $err; > + } > + PVE::Storage::activate_volumes($storecfg, [$drive->{file}]); > + > $size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte > + } elsif ($activation_error) { > + # The volume does exist, so abort if activation failed. > + die $activation_error; > } > - my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); > - $plugin->activate_volume($storeid, $scfg, $volname); > > print "generating cloud-init ISO\n"; > eval {