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

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

[Fiona Ebner]

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

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

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

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

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

[Fiona Ebner]

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!

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

[Fiona Ebner]

Am 17.08.26 um 12:30 PM schrieb Fiona Ebner:
> 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>

Applied, with Dominik's feedback incorporated, thank you very much for
the review!

[1/1] cloud-init: commit: activate volume early enough to fix regression
      with qcow2 on LVM
      commit f0c2cc3bee10daadd153cbfae8e92d5678032a6a