Re: [PATCH v2 qemu-server] cloud-init: commit: activate volume early enough to fix regression with qcow2 on LVM

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

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