Re: [PATCH v2 qemu-server] cloud-init: query volume size via vdisk_list() to fix regression with qcow2 on LVM

[Fiona Ebner <f.ebner@proxmox.com>]

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 <f.ebner@proxmox.com>
>> ---
>>
>> 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.

> 
> 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

I can mention it.

> 
>> +    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;
>