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

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

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)

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