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

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

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