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

[Dominik Csapak <d.csapak@proxmox.com>]

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

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);
```

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