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

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.