[pve-devel] [PATCH qemu-server 0/6] Manage [special:cloudinit] in hotplug code

[pve-devel] [PATCH qemu-server 0/6] Manage [special:cloudinit] in hotplug code

[Wolfgang Bumiller]

The inital series by Alexandre always dumped this section into the
config even if no cloud-init was being used and the api to get the
pending changes performed filtering which should instead be part of the
hotplug code.

See the individual commit messages.

This partially reverts some of Thomas' cleanups as - unless I missed
something - image generation and writing of the config only happens in
paths that are already allowed to write it, plus, the hotplug code will
now fully manage the [special:cloudinit] section, including removing
values which have been reverted to the state that was used for the last
cloud-init image generation.

Wolfgang Bumiller (6):
  Revert "cloudinit: avoid unsafe write of VM config"
  Partially-revert "cloudinit: add cloudinit section for current
    generated config"
  delay cloudinit generation in hotplug
  record cloud-init changes in the cloudinit section
  don't call 'cleanup_config' the cloudinit section
  drop get_pending_changes and simplify cloudinit_pending api call

 PVE/API2/Qemu.pm            |  44 ++++++------
 PVE/QemuServer.pm           | 132 +++++++++++++++++++++++++++++-------
 PVE/QemuServer/Cloudinit.pm | 122 ++++++---------------------------
 3 files changed, 153 insertions(+), 145 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/6] Revert "cloudinit: avoid unsafe write of VM config"

[Wolfgang Bumiller]

This reverts commit b137c30c3a5e4f5394e961a2048724fa18f86b2c.

In preparation of turning the special:cloudinig section from
a "current-state" into a "pending-changes" section.
---
 PVE/QemuServer.pm           | 20 ++++++++------------
 PVE/QemuServer/Cloudinit.pm | 19 +++++++++++--------
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 3a8bc26..a585680 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5020,7 +5020,7 @@ sub vmconfig_hotplug_pending {
 		# some changes can be done without hotplug
 		my $drive = parse_drive($opt, $value);
 		if (drive_is_cloudinit($drive)) {
-		    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+		    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
 		}
 		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
 				     $vmid, $opt, $value, $arch, $machine_type);
@@ -5062,7 +5062,7 @@ sub vmconfig_hotplug_pending {
 
     PVE::QemuConfig->write_config($vmid, $conf);
 
-    if ($hotplug_features->{cloudinit}) {
+    if($hotplug_features->{cloudinit}) {
 	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
 	my $regenerate = undef;
 	for my $item (@$pending) {
@@ -5169,11 +5169,9 @@ sub vmconfig_apply_pending {
 	}
     }
 
-    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid)
-	if $generate_cloudnit;
-
     # 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;
 }
 
 sub vmconfig_update_net {
@@ -5377,8 +5375,7 @@ sub vmconfig_update_cloudinit_drive {
 
     return if !$cloudinit_drive;
 
-    $conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
-    # FIXME: write out changed config here? needs to be sure that config is locked though!
+    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
     my $running = PVE::QemuServer::check_running($vmid);
 
     if ($running) {
@@ -5560,13 +5557,12 @@ sub vm_start_nolock {
     if (!$statefile && scalar(keys %{$conf->{pending}})) {
 	vmconfig_apply_pending($vmid, $conf, $storecfg);
 	$conf = PVE::QemuConfig->load_config($vmid); # update/reload
-    } elsif (!$migratedfrom) {
-	# 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
-	$conf->{cloudinit} = PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
-	# FIXME: write out changed config here? if any changes
     }
 
+    # 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;
+
     # override offline migrated volumes, conf is out of date still
     if (my $offline_volumes = $migrate_opts->{offline_volumes}) {
 	for my $key (sort keys $offline_volumes->%*) {
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index d4a34c4..b616c7b 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -566,6 +566,7 @@ sub generate_cloudinitconfig {
 
     PVE::QemuConfig->foreach_volume($conf, sub {
         my ($ds, $drive) = @_;
+
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file}, 1);
 
 	return if !$volname || $volname !~ m/vm-$vmid-cloudinit/;
@@ -576,34 +577,36 @@ sub generate_cloudinitconfig {
 	$generator->($conf, $vmid, $drive, $volname, $storeid);
     });
 
-    my $cloudinit_conf = {};
+    my $cloudinitconf = delete $conf->{cloudinit};
+    $cloudinitconf = {};
 
     my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
     push @cloudinit_opts, 'name';
 
     for my $opt (@cloudinit_opts) {
+
 	if ($opt =~ m/^ipconfig(\d+)/) {
 	    my $netid = "net$1";
 	    next if !defined($conf->{$netid});
-	    $cloudinit_conf->{$netid} = $conf->{$netid};
+	    $conf->{cloudinit}->{$netid} = $conf->{$netid};
 	}
 
-	$cloudinit_conf->{$opt} = $conf->{$opt} if $conf->{$opt};
+	$conf->{cloudinit}->{$opt} = $conf->{$opt} if $conf->{$opt};
     }
 
-    my $has_cloudinit_drive = 0;
+    $conf->{cloudinit}->{name} = "VM$vmid" if !$conf->{cloudinit}->{name};
+
     for my $opt (keys %{$conf}) {
 	if (PVE::QemuServer::is_valid_drivename($opt)) {
 	    my $drive = PVE::QemuServer::parse_drive($opt, $conf->{$opt});
 	    if (PVE::QemuServer::drive_is_cloudinit($drive)) {
-		$has_cloudinit_drive = 1;
-		$cloudinit_conf->{$opt} = $conf->{$opt};
+		$conf->{cloudinit}->{$opt} = $conf->{$opt};
 	    }
 	}
     }
-    $cloudinit_conf->{name} //= "VM$vmid" if $has_cloudinit_drive;
 
-    return $cloudinit_conf;
+    PVE::QemuConfig->write_config($vmid, $conf);
+
 }
 
 sub dump_cloudinit_config {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/6] Partially-revert "cloudinit: add cloudinit section for current generated config"

[Wolfgang Bumiller]

This partially reverts commit 95a5135dad974c7eae249cf92b62b06fe911af33.
Particularly the unprotected write to the config when
generating the cloudinit file. We leave the rest as is for
now and update the callers to deal with the config later.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/QemuServer/Cloudinit.pm | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index b616c7b..f9bcbbc 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -576,37 +576,6 @@ sub generate_cloudinitconfig {
 
 	$generator->($conf, $vmid, $drive, $volname, $storeid);
     });
-
-    my $cloudinitconf = delete $conf->{cloudinit};
-    $cloudinitconf = {};
-
-    my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
-    push @cloudinit_opts, 'name';
-
-    for my $opt (@cloudinit_opts) {
-
-	if ($opt =~ m/^ipconfig(\d+)/) {
-	    my $netid = "net$1";
-	    next if !defined($conf->{$netid});
-	    $conf->{cloudinit}->{$netid} = $conf->{$netid};
-	}
-
-	$conf->{cloudinit}->{$opt} = $conf->{$opt} if $conf->{$opt};
-    }
-
-    $conf->{cloudinit}->{name} = "VM$vmid" if !$conf->{cloudinit}->{name};
-
-    for my $opt (keys %{$conf}) {
-	if (PVE::QemuServer::is_valid_drivename($opt)) {
-	    my $drive = PVE::QemuServer::parse_drive($opt, $conf->{$opt});
-	    if (PVE::QemuServer::drive_is_cloudinit($drive)) {
-		$conf->{cloudinit}->{$opt} = $conf->{$opt};
-	    }
-	}
-    }
-
-    PVE::QemuConfig->write_config($vmid, $conf);
-
 }
 
 sub dump_cloudinit_config {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 3/6] delay cloudinit generation in hotplug

[Wolfgang Bumiller]

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 a585680..d8bcfff 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 f9bcbbc..24725e7 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 {
-- 
2.30.2

[pve-devel] [PATCH qemu-server 4/6] record cloud-init changes in the cloudinit section

[Wolfgang Bumiller]

introducing an 'added' value in the cloudinit section for
values which have not been present when the cloudinit image
has been generated

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/QemuServer.pm | 67 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d8bcfff..6053d6c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2290,6 +2290,15 @@ sub cloudinit_config_properties {
     return dclone($confdesc_cloudinit);
 }
 
+sub cloudinit_pending_properties {
+    my $p = {
+	map { $_ => 1 } keys $confdesc_cloudinit->%*,
+	name => 1,
+    };
+    $p->{"net$_"} = 1 for 0..($MAX_NETS-1);
+    return $p;
+}
+
 sub check_type {
     my ($key, $value) = @_;
 
@@ -4892,11 +4901,61 @@ sub vmconfig_hotplug_pending {
 	$errors->{$opt} = "hotplug problem - $msg";
     };
 
+    my $cloudinit_pending_properties = PVE::QemuServer::cloudinit_pending_properties();
+
+    my $cloudinit_record_changed = sub {
+	my ($conf, $opt, $old, $new) = @_;
+	return if !$cloudinit_pending_properties->{$opt};
+
+	my $ci = ($conf->{cloudinit} //= {});
+
+	my $recorded = $ci->{$opt};
+
+	my $remove_added = sub {
+	    my @added = grep { $_ ne $opt } PVE::Tools::split_list(delete($ci->{added}) // '');
+	    my $added = join(',', @added);
+	    $ci->{added} = $added if length($added);
+	};
+
+	my $record_added = sub {
+	    my %added = map { $_ => 1 } PVE::Tools::split_list(delete($ci->{added}) // '');
+	    $added{$opt} = 1;
+	    $ci->{added} = join(',', keys %added);
+	};
+
+	if (defined($recorded)) {
+	    # cloud-init already had a version of this key
+	    if (!defined($new)) {
+		# the option existed temporarily and was now removed:
+		$remove_added->();
+	    } elsif ($new eq $recorded) {
+		# it was reverted to the state in cloud-init
+		delete $ci->{$opt};
+	    } else {
+		# $old was newer than $recorded, do nothing
+	    }
+	} else {
+	    # the key was not present the last time we generated the cloud-init image:
+	    if (!defined($new)) {
+		# it is being deleted, which means we're back to the cloud-init sate:
+		delete $ci->{$opt};
+		$remove_added->();
+	    } elsif (defined($old)) {
+		# the key was modified
+		$ci->{$opt} = $old;
+	    } else {
+		# the key was added, no old value exists
+		$record_added->();
+	    }
+	}
+    };
+
     my $changes = 0;
     foreach my $opt (keys %{$conf->{pending}}) { # add/change
 	if ($fast_plug_option->{$opt}) {
-	    $conf->{$opt} = $conf->{pending}->{$opt};
-	    delete $conf->{pending}->{$opt};
+	    my $new = delete $conf->{pending}->{$opt};
+	    $cloudinit_record_changed->($conf, $opt, $conf->{$opt}, $new);
+	    $conf->{$opt} = $new;
 	    $changes = 1;
 	}
     }
@@ -4914,6 +4973,7 @@ sub vmconfig_hotplug_pending {
 
     my $cgroup = PVE::QemuServer::CGroup->new($vmid);
     my $pending_delete_hash = PVE::QemuConfig->parse_pending_delete($conf->{pending}->{delete});
+
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	my $force = $pending_delete_hash->{$opt}->{force};
@@ -4967,7 +5027,8 @@ sub vmconfig_hotplug_pending {
 	if (my $err = $@) {
 	    &$add_error($opt, $err) if $err ne "skip\n";
 	} else {
-	    delete $conf->{$opt};
+	    my $old = delete $conf->{$opt};
+	    $cloudinit_record_changed->($conf, $opt, $old, undef);
 	    PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
 	}
     }
-- 
2.30.2

[pve-devel] [PATCH qemu-server 5/6] don't call 'cleanup_config' the cloudinit section

[Wolfgang Bumiller]

It performs schema valdiation (and normalization).

We only ever write values into it which came from an
already validated config, and we also add an additional
"added" key which is not covered by the schema, so this
would fail.

Simply skip it.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/QemuServer.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6053d6c..90cedea 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2573,8 +2573,6 @@ sub write_vm_config {
 
     &$cleanup_config($conf->{pending}, 1);
 
-    &$cleanup_config($conf->{cloudinit});
-
     foreach my $snapname (keys %{$conf->{snapshots}}) {
 	die "internal error: snapshot name '$snapname' is forbidden" if lc($snapname) eq 'pending';
 	&$cleanup_config($conf->{snapshots}->{$snapname}, undef, $snapname);
-- 
2.30.2

[pve-devel] [PATCH qemu-server 6/6] drop get_pending_changes and simplify cloudinit_pending api call

[Wolfgang Bumiller]

- The forced-remove flag wasn't really used AFAICT and makes
  no sense IMO.
- Whether or not we care about non-MAC changes does not
  belong here, but should instead taken into account in the
  actual hotplug path recording the cloud-init state (iow.
  into $cloudinit_record_changed().)
  (This is not done here atm.)
- It seems much simpler to just have:
  * 'old' = the old value if it's not a new value
  * 'new' = the new value unless it's being deleted
  * If only one of them is set it's an addition or removal.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/API2/Qemu.pm            | 44 +++++++++++----------
 PVE/QemuServer/Cloudinit.pm | 78 -------------------------------------
 2 files changed, 24 insertions(+), 98 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 30348e6..edb495b 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1338,24 +1338,16 @@ __PACKAGE__->register_method({
 		    description => "Configuration option name.",
 		    type => 'string',
 		},
-		value => {
-		    description => "Current value.",
+		old => {
+		    description => "Value as it was used to generate the current cloudinit image.",
 		    type => 'string',
 		    optional => 1,
 		},
-		pending => {
-		    description => "Pending value.",
+		new => {
+		    description => "The new pending value.",
 		    type => 'string',
 		    optional => 1,
 		},
-		delete => {
-		    description => "Indicates a pending delete request if present and not 0. " .
-		                   "The value 2 indicates a force-delete request.",
-		    type => 'integer',
-		    minimum => 0,
-		    maximum => 2,
-		    optional => 1,
-		},
 	    },
 	},
     },
@@ -1365,17 +1357,29 @@ __PACKAGE__->register_method({
 	my $vmid = $param->{vmid};
 	my $conf = PVE::QemuConfig->load_config($vmid);
 
-	if (defined($conf->{cipassword}) &&
-	    defined($conf->{cloudinit}->{cipassword}) &&
-	    $conf->{cipassword} ne $conf->{cloudinit}->{cipassword}) {
-	    $conf->{cipassword} = '********** ';
-	} elsif (defined($conf->{cipassword})) {
-	    $conf->{cipassword} = '**********';
+	my $ci = $conf->{cloudinit};
+
+	my $res = {};
+	my $added = delete($ci->{added}) // '';
+	for my $key (PVE::Tools::split_list($added)) {
+	    $res->{$key} = { new => $conf->{$key} };
 	}
 
-	$conf->{cloudinit}->{cipassword} = '**********' if defined($conf->{cloudinit}->{cipassword});
+	for my $key (keys %$ci) {
+	    if (!exists($conf->{$key})) {
+		$res->{$key} = { old => $ci->{$key} };
+	    } else {
+		$res->{$key} = {
+		    old => $ci->{$key},
+		    new => $conf->{$key},
+		};
+	    }
+	}
 
-	my $res = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
+	if (defined(my $pw = $res->{cipassword})) {
+	    $pw->{old} = '**********' if exists $pw->{old};
+	    $pw->{new} = '**********' if exists $pw->{new};
+	}
 
 	return $res;
    }});
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 24725e7..a0c3d60 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -627,82 +627,4 @@ sub dump_cloudinit_config {
     }
 }
 
-sub get_pending_config {
-    my ($conf, $vmid) = @_;
-
-    my $newconf = dclone($conf);
-
-    my $cloudinit_current = $newconf->{cloudinit};
-    my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
-    push @cloudinit_opts, 'name';
-
-    #add cloud-init drive
-    my $drives = {};
-    PVE::QemuConfig->foreach_volume($newconf, sub {
-	my ($ds, $drive) = @_;
-	$drives->{$ds} = 1 if PVE::QemuServer::drive_is_cloudinit($drive);
-    });
-
-    PVE::QemuConfig->foreach_volume($cloudinit_current, sub {
-	my ($ds, $drive) = @_;
-	$drives->{$ds} = 1 if PVE::QemuServer::drive_is_cloudinit($drive);
-    });
-    for my $ds (keys %{$drives}) {
-	push @cloudinit_opts, $ds;
-    }
-
-    $newconf->{name} = "VM$vmid" if !$newconf->{name};
-    $cloudinit_current->{name} = "VM$vmid" if !$cloudinit_current->{name};
-
-    #only mac-address is used in cloud-init config.
-    #We don't want to display other pending net changes.
-    my $print_cloudinit_net = sub {
-	my ($conf, $opt) = @_;
-
-	if (defined($conf->{$opt})) {
-	    my $net = PVE::QemuServer::parse_net($conf->{$opt});
-	    $conf->{$opt} = "macaddr=".$net->{macaddr} if $net->{macaddr};
-	}
-    };
-
-    my $cloudinit_options = {};
-    for my $opt (@cloudinit_opts) {
-	if ($opt =~ m/^ipconfig(\d+)/) {
-	    my $netid = "net$1";
-
-	    next if !defined($newconf->{$netid}) && !defined($cloudinit_current->{$netid}) &&
-		    !defined($newconf->{$opt}) && !defined($cloudinit_current->{$opt});
-
-	    &$print_cloudinit_net($newconf, $netid);
-	    &$print_cloudinit_net($cloudinit_current, $netid);
-	    $cloudinit_options->{$netid} = 1;
-	}
-	$cloudinit_options->{$opt} = 1;
-    }
-
-    my $res = [];
-
-    for my $opt (keys %{$cloudinit_options}) {
-
-	my $item = {
-	    key => $opt,
-	};
-	if ($cloudinit_current->{$opt}) {
-	    $item->{value} = $cloudinit_current->{$opt};
-	    if (defined($newconf->{$opt})) {
-		$item->{pending} = $newconf->{$opt} 
-		    if $newconf->{$opt} ne $cloudinit_current->{$opt};
-	    } else {
-		$item->{delete} = 1;
-	    }
-	} else {
-	    $item->{pending} = $newconf->{$opt} if $newconf->{$opt}
-	}
-
-	push @$res, $item;
-   }
-
-   return $res;
-}
-
 1;
-- 
2.30.2

[pve-devel] applied-series: [PATCH qemu-server 0/6] Manage [special:cloudinit] in hotplug code

[Thomas Lamprecht]

Am 16/11/2022 um 18:14 schrieb Wolfgang Bumiller:
> The inital series by Alexandre always dumped this section into the
> config even if no cloud-init was being used and the api to get the
> pending changes performed filtering which should instead be part of the
> hotplug code.
> 
> See the individual commit messages.
> 
> This partially reverts some of Thomas' cleanups as - unless I missed
> something - image generation and writing of the config only happens in
> paths that are already allowed to write it, plus, the hotplug code will
> now fully manage the [special:cloudinit] section, including removing
> values which have been reverted to the state that was used for the last
> cloud-init image generation.
> 
> Wolfgang Bumiller (6):
>   Revert "cloudinit: avoid unsafe write of VM config"
>   Partially-revert "cloudinit: add cloudinit section for current
>     generated config"
>   delay cloudinit generation in hotplug
>   record cloud-init changes in the cloudinit section
>   don't call 'cleanup_config' the cloudinit section
>   drop get_pending_changes and simplify cloudinit_pending api call
> 
>  PVE/API2/Qemu.pm            |  44 ++++++------
>  PVE/QemuServer.pm           | 132 +++++++++++++++++++++++++++++-------
>  PVE/QemuServer/Cloudinit.pm | 122 ++++++---------------------------
>  3 files changed, 153 insertions(+), 145 deletions(-)
> 


applied series, much thanks!