From 4b785da1a92c923460d6db1baa5cab1abf9f6793 Mon Sep 17 00:00:00 2001
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Wed, 16 Nov 2022 18:14:05 +0100
Subject: [PATCH] delay cloudinit generation in hotplug

Hotpluggieg generated a cloudinit image based on old values
in order to attach the device and later update it again, but
the update was only done if cloudinit hotplug was enabled.
This is weird, let's not.

Also introduce 'apply_cloudinit_config' which also write the
config, which, as it turns out, is the only thing we
actually need anyway, currently.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/QemuServer.pm           | 53 +++++++++++++++++++++++++++++--------
 PVE/QemuServer/Cloudinit.pm | 26 +++++++++++++++++-
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a5856802..d8bcfffa 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4972,6 +4972,7 @@ sub vmconfig_hotplug_pending {
 	}
     }
 
+    my $cloudinit_opt;
     foreach my $opt (keys %{$conf->{pending}}) {
 	next if $selection && !$selection->{$opt};
 	my $value = $conf->{pending}->{$opt};
@@ -5020,7 +5021,9 @@ sub vmconfig_hotplug_pending {
 		# some changes can be done without hotplug
 		my $drive = parse_drive($opt, $value);
 		if (drive_is_cloudinit($drive)) {
-		    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+		    $cloudinit_opt = [$opt, $drive];
+		    # apply all the other changes first, then generate the cloudinit disk
+		    die "skip\n";
 		}
 		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
 				     $vmid, $opt, $value, $arch, $machine_type);
@@ -5039,6 +5042,23 @@ sub vmconfig_hotplug_pending {
 		die "skip\n";  # skip non-hot-pluggable options
 	    }
 	};
+	if (my $err = $@) {
+	    &$add_error($opt, $err) if $err ne "skip\n";
+	} else {
+	    $cloudinit_record_changed->($conf, $opt, $conf->{$opt}, $value);
+	    $conf->{$opt} = $value;
+	    delete $conf->{pending}->{$opt};
+	}
+    }
+
+    if (defined($cloudinit_opt)) {
+	my ($opt, $drive) = @$cloudinit_opt;
+	my $value = $conf->{pending}->{$opt};
+	eval {
+	    PVE::QemuServer::Cloudinit::apply_cloudinit_config($conf, $vmid);
+	    vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
+				 $vmid, $opt, $value, $arch, $machine_type);
+	};
 	if (my $err = $@) {
 	    &$add_error($opt, $err) if $err ne "skip\n";
 	} else {
@@ -5062,13 +5082,8 @@ sub vmconfig_hotplug_pending {
 
     PVE::QemuConfig->write_config($vmid, $conf);
 
-    if($hotplug_features->{cloudinit}) {
-	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
-	my $regenerate = undef;
-	for my $item (@$pending) {
-	    $regenerate = 1 if defined($item->{delete}) or defined($item->{pending});
-	}
-	PVE::QemuServer::vmconfig_update_cloudinit_drive($storecfg, $conf, $vmid) if $regenerate;
+    if ($hotplug_features->{cloudinit} && PVE::QemuServer::Cloudinit::has_changes($conf)) {
+	PVE::QemuServer::vmconfig_update_cloudinit_drive($storecfg, $conf, $vmid);
     }
 }
 
@@ -5171,7 +5186,13 @@ sub vmconfig_apply_pending {
 
     # write all changes at once to avoid unnecessary i/o
     PVE::QemuConfig->write_config($vmid, $conf);
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid) if $generate_cloudnit;
+    if ($generate_cloudnit) {
+	if (PVE::QemuServer::Cloudinit::apply_cloudinit_config($conf, $vmid)) {
+	    # After successful generation and if there were changes to be applied, update the
+	    # config to drop the {cloudinit} entry.
+	    PVE::QemuConfig->write_config($vmid, $conf);
+	}
+    }
 }
 
 sub vmconfig_update_net {
@@ -5375,7 +5396,10 @@ sub vmconfig_update_cloudinit_drive {
 
     return if !$cloudinit_drive;
 
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+    if (PVE::QemuServer::Cloudinit::apply_cloudinit_config($conf, $vmid)) {
+	PVE::QemuConfig->write_config($vmid, $conf);
+    }
+
     my $running = PVE::QemuServer::check_running($vmid);
 
     if ($running) {
@@ -5561,7 +5585,14 @@ sub vm_start_nolock {
 
     # don't regenerate the ISO if the VM is started as part of a live migration
     # this way we can reuse the old ISO with the correct config
-    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid) if !$migratedfrom;
+    if (!$migratedfrom) {
+	if (PVE::QemuServer::Cloudinit::apply_cloudinit_config($conf, $vmid)) {
+	    # FIXME: apply_cloudinit_config updates $conf in this case, and it would only drop
+	    # $conf->{cloudinit}, so we could just not do this?
+	    # But we do it above, so for now let's be consistent.
+	    $conf = PVE::QemuConfig->load_config($vmid); # update/reload
+	}
+    }
 
     # override offline migrated volumes, conf is out of date still
     if (my $offline_volumes = $migrate_opts->{offline_volumes}) {
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index f9bcbbcf..24725e75 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -559,11 +559,19 @@ my $cloudinit_methods = {
     opennebula => \&generate_opennebula,
 };
 
-sub generate_cloudinitconfig {
+sub has_changes {
+    my ($conf) = @_;
+
+    return !!$conf->{cloudinit}->%*;
+}
+
+sub generate_cloudinit_config {
     my ($conf, $vmid) = @_;
 
     my $format = get_cloudinit_format($conf);
 
+    my $has_changes = has_changes($conf);
+
     PVE::QemuConfig->foreach_volume($conf, sub {
         my ($ds, $drive) = @_;
 
@@ -576,6 +584,22 @@ sub generate_cloudinitconfig {
 
 	$generator->($conf, $vmid, $drive, $volname, $storeid);
     });
+
+    return $has_changes;
+}
+
+sub apply_cloudinit_config {
+    my ($conf, $vmid) = @_;
+
+    my $has_changes = generate_cloudinit_config($conf, $vmid);
+
+    if ($has_changes) {
+	delete $conf->{cloudinit};
+	PVE::QemuConfig->write_config($vmid, $conf);
+	return 1;
+    }
+
+    return $has_changes;
 }
 
 sub dump_cloudinit_config {