[pve-devel] [PATCH v4 qemu-server 0/6] cloudinit pending behaviour change

[pve-devel] [PATCH v4 qemu-server 0/6] cloudinit pending behaviour change

[Alexandre Derumier]

Hi,

This is an attempt to cleanup current behaviour of cloudinit online changes.

Currently, we setup cloudinit options as pending, until we generate the config drive.

This is not 100% true, because some option like vm name, nic mac address can be changed,
without going to pending, so user can't known if it need to regenerated it.

Also, some can are very difficult to handle, if you hotplug a nic but it's failing,so pending,
then you defined an ipconfig, and then you revert hotplug.
or if you delete a nic, the ipconfig is no more displayed in the gui.


So, instead of setting cloudinit values in pending,
this patch serie copy the current cloudinit config in a new section [special:cloudinit],
when the config drive is generated.
This is only an hint, to allow to display diff between the generated cloudinit
drive, and the current vm config.

A new specific cloudinit config api is added too displaying the diff between current and generated config.

Changelog V1:

- use [special:cloudinit] instead [CLOUDINIT] for section
- delete config section on drive removal
- config api: move code to new PVE::QemuServer::Cloudinit::get_pending_config
- config api: add "qm cloudinit pending" cli
- add update api to regenerate drive with 1 api call
- add cloudinit hotplug option

Changelog v2:

- fix trailing whitespace in first patch
- revert previous "cloudinit" check in snapshot name (":" character is already forbidden)

Changelog v3:

- extract the current conf from cloudinit drive instead write the special cloudinit section

Changelog v4:

- rebase on v2, keep current cloudinit config in vm configuration
- pending api: display mac address change on netX
- cleanup && fix from Fabian comments


Alexandre Derumier (6):
  cloudinit: add cloudinit section for current generated config.
  generate cloudinit drive on offline plug
  cloudinit: make cloudnit options fastplug
  api2: add cloudinit config api
  api2: add cloudinit_update
  add cloudinit hotplug

 PVE/API2/Qemu.pm            | 111 ++++++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm               |   2 +
 PVE/QemuServer.pm           | 102 +++++++++++++++++++++++----------
 PVE/QemuServer/Cloudinit.pm | 109 +++++++++++++++++++++++++++++++++++
 4 files changed, 293 insertions(+), 31 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config.

[Alexandre Derumier]

Instead using vm pending options for pending cloudinit generated config,

write current generated cloudinit config in a new [special:cloudinit] SECTION.

Currently, some options like vm name, nic mac address can be hotplugged,
so they are not way to know if the cloud-init disk is already updated.
---
 PVE/QemuServer.pm           | 20 +++++++++++++++++---
 PVE/QemuServer/Cloudinit.pm | 31 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0be6be9..8aa550b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1993,6 +1993,7 @@ sub vmconfig_register_unused_drive {
     if (drive_is_cloudinit($drive)) {
 	eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
 	warn $@ if $@;
+	delete $conf->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
 	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
@@ -2363,6 +2364,7 @@ sub parse_vm_config {
 	digest => Digest::SHA::sha1_hex($raw),
 	snapshots => {},
 	pending => {},
+	cloudinit => {},
     };
 
     my $handle_error = sub {
@@ -2397,6 +2399,11 @@ sub parse_vm_config {
 	    $descr = undef;
 	    $conf = $res->{$section} = {};
 	    next;
+	} elsif ($line =~ m/^\[special:cloudinit\]\s*$/i) {
+	    $section = 'cloudinit';
+	    $descr = undef;
+	    $conf = $res->{$section} = {};
+	    next;
 
 	} elsif ($line =~ m/^\[([a-z][a-z0-9_\-]+)\]\s*$/i) {
 	    $section = $1;
@@ -2494,7 +2501,7 @@ sub write_vm_config {
 
 	foreach my $key (keys %$cref) {
 	    next if $key eq 'digest' || $key eq 'description' || $key eq 'snapshots' ||
-		$key eq 'snapstate' || $key eq 'pending';
+		$key eq 'snapstate' || $key eq 'pending' || $key eq 'cloudinit';
 	    my $value = $cref->{$key};
 	    if ($key eq 'delete') {
 		die "propertry 'delete' is only allowed in [PENDING]\n"
@@ -2518,6 +2525,8 @@ sub write_vm_config {
 
     &$cleanup_config($conf->{pending}, 1);
 
+    &$cleanup_config($conf->{cloudinit}, 1);
+
     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);
@@ -2548,7 +2557,7 @@ sub write_vm_config {
 	}
 
 	foreach my $key (sort keys %$conf) {
-	    next if $key =~ /^(digest|description|pending|snapshots)$/;
+	    next if $key =~ /^(digest|description|pending|cloudinit|snapshots)$/;
 	    $raw .= "$key: $conf->{$key}\n";
 	}
 	return $raw;
@@ -2561,6 +2570,11 @@ sub write_vm_config {
 	$raw .= &$generate_raw_config($conf->{pending}, 1);
     }
 
+    if (scalar(keys %{$conf->{cloudinit}})){
+	$raw .= "\n[special:cloudinit]\n";
+	$raw .= &$generate_raw_config($conf->{cloudinit}, 1);
+    }
+
     foreach my $snapname (sort keys %{$conf->{snapshots}}) {
 	$raw .= "\n[$snapname]\n";
 	$raw .= &$generate_raw_config($conf->{snapshots}->{$snapname});
@@ -5087,9 +5101,9 @@ sub vmconfig_apply_pending {
 	    $conf->{$opt} = delete $conf->{pending}->{$opt};
 	}
     }
-
     # write all changes at once to avoid unnecessary i/o
     PVE::QemuConfig->write_config($vmid, $conf);
+
 }
 
 sub vmconfig_update_net {
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index b7daa2a..cdaf4e5 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -574,6 +574,37 @@ 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 v4 qemu-server 2/6] generate cloudinit drive on offline plug

[Alexandre Derumier]

Currently when only generate it at vm start
---
 PVE/QemuServer.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8aa550b..53be830 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5088,6 +5088,8 @@ sub vmconfig_apply_pending {
 
     PVE::QemuConfig->cleanup_pending($conf);
 
+    my $generate_cloudnit = undef;
+
     foreach my $opt (keys %{$conf->{pending}}) { # add/change
 	next if $opt eq 'delete'; # just to be sure
 	eval {
@@ -5095,15 +5097,24 @@ sub vmconfig_apply_pending {
 		vmconfig_register_unused_drive($storecfg, $vmid, $conf, parse_drive($opt, $conf->{$opt}))
 	    }
 	};
+
 	if (my $err = $@) {
 	    $add_apply_error->($opt, $err);
 	} else {
+
+	    if (is_valid_drivename($opt)) {
+		my $drive = parse_drive($opt, $conf->{pending}->{$opt});
+		$generate_cloudnit = 1 if drive_is_cloudinit($drive);
+	    }
+
 	    $conf->{$opt} = delete $conf->{pending}->{$opt};
 	}
     }
+
     # 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 {
-- 
2.30.2

[pve-devel] [PATCH v4 qemu-server 3/6] cloudinit: make cloudnit options fastplug

[Alexandre Derumier]

---
 PVE/QemuServer.pm | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 53be830..998f7c8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4837,6 +4837,10 @@ sub vmconfig_hotplug_pending {
 	$errors->{$opt} = "hotplug problem - $msg";
     };
 
+    for my $opt (keys %$confdesc_cloudinit) {
+	$fast_plug_option->{$opt} = 1;
+    }
+
     my $changes = 0;
     foreach my $opt (keys %{$conf->{pending}}) { # add/change
 	if ($fast_plug_option->{$opt}) {
@@ -4912,31 +4916,6 @@ sub vmconfig_hotplug_pending {
 	}
     }
 
-    my ($apply_pending_cloudinit, $apply_pending_cloudinit_done);
-    $apply_pending_cloudinit = sub {
-	return if $apply_pending_cloudinit_done; # once is enough
-	$apply_pending_cloudinit_done = 1; # once is enough
-
-	my ($key, $value) = @_;
-
-	my @cloudinit_opts = keys %$confdesc_cloudinit;
-	foreach my $opt (keys %{$conf->{pending}}) {
-	    next if !grep { $_ eq $opt } @cloudinit_opts;
-	    $conf->{$opt} = delete $conf->{pending}->{$opt};
-	}
-
-	my $pending_delete_hash = PVE::QemuConfig->parse_pending_delete($conf->{pending}->{delete});
-	foreach my $opt (sort keys %$pending_delete_hash) {
-	    next if !grep { $_ eq $opt } @cloudinit_opts;
-	    PVE::QemuConfig->remove_from_pending_delete($conf, $opt);
-	    delete $conf->{$opt};
-	}
-
-	my $new_conf = { %$conf };
-	$new_conf->{$key} = $value;
-	PVE::QemuServer::Cloudinit::generate_cloudinitconfig($new_conf, $vmid);
-    };
-
     foreach my $opt (keys %{$conf->{pending}}) {
 	next if $selection && !$selection->{$opt};
 	my $value = $conf->{pending}->{$opt};
@@ -4983,7 +4962,7 @@ sub vmconfig_hotplug_pending {
 		# some changes can be done without hotplug
 		my $drive = parse_drive($opt, $value);
 		if (drive_is_cloudinit($drive)) {
-		    &$apply_pending_cloudinit($opt, $value);
+		    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
 		}
 		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
 				     $vmid, $opt, $value, $arch, $machine_type);
-- 
2.30.2

[pve-devel] [PATCH v4 qemu-server 4/6] api2: add cloudinit config api

[Alexandre Derumier]

---
 PVE/API2/Qemu.pm            | 68 ++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm               |  1 +
 PVE/QemuServer/Cloudinit.pm | 78 +++++++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3af2132..5608ebb 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -21,6 +21,7 @@ use PVE::ReplicationConfig;
 use PVE::GuestHelpers;
 use PVE::QemuConfig;
 use PVE::QemuServer;
+use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
 use PVE::QemuServer::ImportDisk;
@@ -1287,6 +1288,73 @@ __PACKAGE__->register_method({
 	return PVE::GuestHelpers::config_with_pending_array($conf, $pending_delete_hash);
    }});
 
+__PACKAGE__->register_method({
+    name => 'cloudinit_pending',
+    path => '{vmid}/cloudinit',
+    method => 'GET',
+    proxyto => 'node',
+    description => "Get the cloudinit configuration with both current and pending values.",
+    permissions => {
+	check => ['perm', '/vms/{vmid}', [ 'VM.Audit' ]],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+	},
+    },
+    returns => {
+	type => "array",
+	items => {
+	    type => "object",
+	    properties => {
+		key => {
+		    description => "Configuration option name.",
+		    type => 'string',
+		},
+		value => {
+		    description => "Current value.",
+		    type => 'string',
+		    optional => 1,
+		},
+		pending => {
+		    description => "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,
+		},
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	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} = '**********';
+	}
+
+	$conf->{cloudinit}->{cipassword} = '**********' if defined($conf->{cloudinit}->{cipassword});
+
+	my $res = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
+
+	return $res;
+   }});
+
 # POST/PUT {vmid}/config implementation
 #
 # The original API used PUT (idempotent) an we assumed that all operations
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index cf0d6f3..ba20dd5 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -986,6 +986,7 @@ our $cmddef = {
 		my $data = shift;
 		print "$data\n";
 	    }],
+	pending => [ "PVE::API2::Qemu", 'cloudinit_pending', ['vmid'], { node => $nodename }, \&PVE::GuestHelpers::format_pending ]
     },
 
 };
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index cdaf4e5..2355953 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -7,6 +7,7 @@ use File::Path;
 use Digest::SHA;
 use URI::Escape;
 use MIME::Base64 qw(encode_base64);
+use Storable qw(dclone);
 
 use PVE::Tools qw(run_command file_set_contents);
 use PVE::Storage;
@@ -632,4 +633,81 @@ 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);
+    });
+    foreach 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] [PATCH v4 qemu-server 5/6] api2: add cloudinit_update

[Alexandre Derumier]

This allow to regenerate the config drive with 1 api call.

This also avoid to delete drive first, and recreate it again.

As it's a readonly drive, we can simply live update it,
and eject/replace it with qemu monitor
---
 PVE/API2/Qemu.pm  | 43 +++++++++++++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm     |  3 ++-
 PVE/QemuServer.pm | 28 ++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 5608ebb..089d9b9 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1355,6 +1355,49 @@ __PACKAGE__->register_method({
 	return $res;
    }});
 
+__PACKAGE__->register_method({
+    name => 'cloudinit_update',
+    path => '{vmid}/cloudinit',
+    method => 'PUT',
+    protected => 1,
+    proxyto => 'node',
+    description => "Regenerate and change cloudinit config drive.",
+    permissions => {
+	check => ['perm', '/vms/{vmid}', 'VM.Config.Cloudinit'],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    vmid => get_standard_option('pve-vmid'),
+	},
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+
+	my $authuser = $rpcenv->get_user();
+
+	my $vmid = extract_param($param, 'vmid');
+
+	my $updatefn =  sub {
+
+	    my $conf = PVE::QemuConfig->load_config($vmid);
+
+	    PVE::QemuConfig->check_lock($conf);
+
+	    my $storecfg = PVE::Storage::config();
+
+	    PVE::QemuServer::vmconfig_update_cloudinit_drive($storecfg, $conf, $vmid);
+	};
+
+	PVE::QemuConfig->lock_config($vmid, $updatefn);
+
+	return;
+    }});
+
 # POST/PUT {vmid}/config implementation
 #
 # The original API used PUT (idempotent) an we assumed that all operations
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index ba20dd5..ef66ccf 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -986,7 +986,8 @@ our $cmddef = {
 		my $data = shift;
 		print "$data\n";
 	    }],
-	pending => [ "PVE::API2::Qemu", 'cloudinit_pending', ['vmid'], { node => $nodename }, \&PVE::GuestHelpers::format_pending ]
+	pending => [ "PVE::API2::Qemu", 'cloudinit_pending', ['vmid'], { node => $nodename }, \&PVE::GuestHelpers::format_pending ],
+	update => [ "PVE::API2::Qemu", 'cloudinit_update', ['vmid'], { node => $nodename }],
     },
 
 };
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 998f7c8..2710f53 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5280,6 +5280,34 @@ sub vmconfig_update_disk {
     vm_deviceplug($storecfg, $conf, $vmid, $opt, $drive, $arch, $machine_type);
 }
 
+sub vmconfig_update_cloudinit_drive {
+    my ($storecfg, $conf, $vmid) = @_;
+
+    my $cloudinit_ds = undef;
+    my $cloudinit_drive = undef;
+
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($ds, $drive) = @_;
+	if (PVE::QemuServer::drive_is_cloudinit($drive)) {
+	    $cloudinit_ds = $ds;
+	    $cloudinit_drive = $drive;
+	}
+    });
+
+    return if !$cloudinit_drive;
+
+    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+    my $running = PVE::QemuServer::check_running($vmid);
+
+    if ($running) {
+	my $path = PVE::Storage::path($storecfg, $cloudinit_drive->{file});
+	if ($path) {
+	    mon_cmd($vmid, "eject", force => JSON::true, id => "$cloudinit_ds");
+	    mon_cmd($vmid, "blockdev-change-medium", id => "$cloudinit_ds", filename => "$path");
+	}
+    }
+}
+
 # called in locked context by incoming migration
 sub vm_migrate_get_nbd_disks {
     my ($storecfg, $conf, $replicated_volumes) = @_;
-- 
2.30.2

[pve-devel] [PATCH v4 qemu-server 6/6] add cloudinit hotplug

[Alexandre Derumier]

This allow to regenerate config drive if pending values exist
when we change vm options.
---
 PVE/QemuServer.pm | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2710f53..56d77f4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -303,7 +303,7 @@ my $confdesc = {
 	optional => 1,
 	type => 'string', format => 'pve-hotplug-features',
 	description => "Selectively enable hotplug features. This is a comma separated list of"
-	    ." hotplug features: 'network', 'disk', 'cpu', 'memory' and 'usb'. Use '0' to disable"
+	    ." hotplug features: 'network', 'disk', 'cpu', 'memory', 'usb' and 'cloudinit'. Use '0' to disable"
 	    ." hotplug completely. Using '1' as value is an alias for the default `network,disk,usb`.",
         default => 'network,disk,usb',
     },
@@ -1356,7 +1356,7 @@ sub parse_hotplug_features {
     $data = $confdesc->{hotplug}->{default} if $data eq '1';
 
     foreach my $feature (PVE::Tools::split_list($data)) {
-	if ($feature =~ m/^(network|disk|cpu|memory|usb)$/) {
+	if ($feature =~ m/^(network|disk|cpu|memory|usb|cloudinit)$/) {
 	    $res->{$1} = 1;
 	} else {
 	    die "invalid hotplug feature '$feature'\n";
@@ -4988,8 +4988,16 @@ sub vmconfig_hotplug_pending {
 	    delete $conf->{pending}->{$opt};
 	}
     }
-
     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;
+    }
 }
 
 sub try_deallocate_drive {
-- 
2.30.2

Re: [pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config.

[Fabian Ebner]

Am 27.04.22 um 16:05 schrieb Alexandre Derumier:
> Instead using vm pending options for pending cloudinit generated config,
> 
> write current generated cloudinit config in a new [special:cloudinit] SECTION.
> 
> Currently, some options like vm name, nic mac address can be hotplugged,
> so they are not way to know if the cloud-init disk is already updated.

Series looks pretty good to me, but there are some issues, all related
to this patch (number 4 is the big one):

1. assemble() in PVE/VZDump/QemuServer.pm requires changes or the message
INFO: snapshots found (not included into backup)
will be printed during backup when there is a cloudinit section (even if
there are no snapshots).

2. With qm config <ID>,
cloudinit: HASH(0x55ceb9a39298)
shows up in the output.

3. The API/series assumes that there's only one cloudinit drive, but
there currently is no checks against adding multiple cloudinit drives. I
sent a patch for discussion:
https://lists.proxmox.com/pipermail/pve-devel/2022-May/052939.html

4. Migration new -> old is subtly broken now, because the old config
parser will skip [special:cloudinit], but continue parsing the rest,
meaning that settings from [special:cloudinit] will override the
settings from the actual current config. It's true that migration new ->
old doesn't /have/ to keep working, but in this case it doesn't
completely fail, but quietly messes up the config, which is worse than
failing.

A way to fix it would be to prepare the parser for such special sections
now (skipping the whole section if it's not known), and only introduce
the special section in the next major release, because only then can we
be sure that every migration target is prepared.

But maybe somebody has a better idea?

Example (with pve702 running unpatched qemu-server):

root@pve701 ~ # qm config 118
boot: order=scsi0;ide2;net0
cloudinit: HASH(0x55ded04408c0)
cores: 1
ide0: rbdkvm:vm-118-cloudinit,media=cdrom
ide2: none,media=cdrom
memory: 2048
meta: creation-qemu=6.2.0,ctime=1651053058
name: BBBB
net0: virtio=12:12:34:34:56:56,bridge=vmbr0,firewall=1
numa: 0
ostype: l26
scsi0: rbdkvm:vm-118-disk-0,size=1G
scsihw: virtio-scsi-pci
smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
sockets: 1
vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13

root@pve701 ~ # qm cloudinit pending 118
cur ide0: rbdkvm:vm-118-cloudinit,media=cdrom
cur name: AAAA
new name: BBBB
cur net0: macaddr=4A:89:E8:C9:04:98
new net0: macaddr=12:12:34:34:56:56

root@pve701 ~ # qm migrate 118 pve702
2022-05-06 09:36:15 use dedicated network address for sending migration
traffic (10.10.50.12)
2022-05-06 09:36:15 starting migration of VM 118 to node 'pve702'
(10.10.50.12)
2022-05-06 09:36:16 migration finished successfully (duration 00:00:01)

root@pve701 ~ # ssh 10.10.50.12 qm config 118
boot: order=scsi0;ide2;net0
cores: 1
ide0: rbdkvm:vm-118-cloudinit,media=cdrom
ide2: none,media=cdrom
memory: 2048
meta: creation-qemu=6.2.0,ctime=1651053058
name: AAAA
net0: virtio=4A:89:E8:C9:04:98,bridge=vmbr0,firewall=1
numa: 0
ostype: l26
scsi0: rbdkvm:vm-118-disk-0,size=1G
scsihw: virtio-scsi-pci
smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
sockets: 1
vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13


> ---
>  PVE/QemuServer.pm           | 20 +++++++++++++++++---
>  PVE/QemuServer/Cloudinit.pm | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 0be6be9..8aa550b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1993,6 +1993,7 @@ sub vmconfig_register_unused_drive {
>      if (drive_is_cloudinit($drive)) {
>  	eval { PVE::Storage::vdisk_free($storecfg, $drive->{file}) };
>  	warn $@ if $@;
> +	delete $conf->{cloudinit};

Currently, it's not prohibited to add more than one cloud-init drive,
but this series implicitly assumes that.

>      } elsif (!drive_is_cdrom($drive)) {
>  	my $volid = $drive->{file};
>  	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> @@ -2363,6 +2364,7 @@ sub parse_vm_config {
>  	digest => Digest::SHA::sha1_hex($raw),
>  	snapshots => {},
>  	pending => {},
> +	cloudinit => {},
>      };
>  
>      my $handle_error = sub {
> @@ -2397,6 +2399,11 @@ sub parse_vm_config {
>  	    $descr = undef;
>  	    $conf = $res->{$section} = {};
>  	    next;
> +	} elsif ($line =~ m/^\[special:cloudinit\]\s*$/i) {
> +	    $section = 'cloudinit';
> +	    $descr = undef;
> +	    $conf = $res->{$section} = {};
> +	    next;
>  

Style nit and nothing new, but you could remove this trailing blank line
while you're at it.

>  	} elsif ($line =~ m/^\[([a-z][a-z0-9_\-]+)\]\s*$/i) {
>  	    $section = $1;
> @@ -2494,7 +2501,7 @@ sub write_vm_config {
>  
>  	foreach my $key (keys %$cref) {
>  	    next if $key eq 'digest' || $key eq 'description' || $key eq 'snapshots' ||
> -		$key eq 'snapstate' || $key eq 'pending';
> +		$key eq 'snapstate' || $key eq 'pending' || $key eq 'cloudinit';
>  	    my $value = $cref->{$key};
>  	    if ($key eq 'delete') {
>  		die "propertry 'delete' is only allowed in [PENDING]\n"
> @@ -2518,6 +2525,8 @@ sub write_vm_config {
>  
>      &$cleanup_config($conf->{pending}, 1);
>  
> +    &$cleanup_config($conf->{cloudinit}, 1);

The second parameter should not be 1 here (it's called $pending and used
to check if the key 'delete' is allowed).

> +
>      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);
> @@ -2548,7 +2557,7 @@ sub write_vm_config {
>  	}
>  
>  	foreach my $key (sort keys %$conf) {
> -	    next if $key =~ /^(digest|description|pending|snapshots)$/;
> +	    next if $key =~ /^(digest|description|pending|cloudinit|snapshots)$/;
>  	    $raw .= "$key: $conf->{$key}\n";
>  	}
>  	return $raw;
> @@ -2561,6 +2570,11 @@ sub write_vm_config {
>  	$raw .= &$generate_raw_config($conf->{pending}, 1);
>      }
>  
> +    if (scalar(keys %{$conf->{cloudinit}})){
> +	$raw .= "\n[special:cloudinit]\n";
> +	$raw .= &$generate_raw_config($conf->{cloudinit}, 1);

Similar here, setting the second parameter is specific to pending.

> +    }
> +
>      foreach my $snapname (sort keys %{$conf->{snapshots}}) {
>  	$raw .= "\n[$snapname]\n";
>  	$raw .= &$generate_raw_config($conf->{snapshots}->{$snapname});
> @@ -5087,9 +5101,9 @@ sub vmconfig_apply_pending {
>  	    $conf->{$opt} = delete $conf->{pending}->{$opt};
>  	}
>      }
> -
>      # write all changes at once to avoid unnecessary i/o
>      PVE::QemuConfig->write_config($vmid, $conf);
> +

Style nit: unrelated and doesn't make it better IMHO.

>  }
>  
>  sub vmconfig_update_net {

Re: [pve-devel] [PATCH v4 qemu-server 3/6] cloudinit: make cloudnit options fastplug

[Fabian Ebner]

Am 27.04.22 um 16:05 schrieb Alexandre Derumier:
> ---
>  PVE/QemuServer.pm | 31 +++++--------------------------
>  1 file changed, 5 insertions(+), 26 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 53be830..998f7c8 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4837,6 +4837,10 @@ sub vmconfig_hotplug_pending {
>  	$errors->{$opt} = "hotplug problem - $msg";
>      };
>  
> +    for my $opt (keys %$confdesc_cloudinit) {
> +	$fast_plug_option->{$opt} = 1;
> +    }
> +

This should be done outside the function, so that it happens only once
during module load.

Re: [pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config.

[DERUMIER, Alexandre]

Thanks for the review fabian,

I'll read your comments and work on it tomorrow.

Le vendredi 06 mai 2022 à 12:20 +0200, Fabian Ebner a écrit :
> Am 27.04.22 um 16:05 schrieb Alexandre Derumier:
> > Instead using vm pending options for pending cloudinit generated
> > config,
> > 
> > write current generated cloudinit config in a new
> > [special:cloudinit] SECTION.
> > 
> > Currently, some options like vm name, nic mac address can be
> > hotplugged,
> > so they are not way to know if the cloud-init disk is already
> > updated.
> 
> Series looks pretty good to me, but there are some issues, all
> related
> to this patch (number 4 is the big one):
> 
> 1. assemble() in PVE/VZDump/QemuServer.pm requires changes or the
> message
> INFO: snapshots found (not included into backup)
> will be printed during backup when there is a cloudinit section (even
> if
> there are no snapshots).
> 
> 2. With qm config <ID>,
> cloudinit: HASH(0x55ceb9a39298)
> shows up in the output.
> 
> 3. The API/series assumes that there's only one cloudinit drive, but
> there currently is no checks against adding multiple cloudinit
> drives. I
> sent a patch for discussion:
> https://antiphishing.cetsi.fr/proxy/v3?i=SXVFem5DOGVpUU1rNjdmQuxbAYzjRE578NJDXO0bRW0&r=bWt1djZ5QzcyUms5R1Nzatwfz4p60Sh_bGp_TdGIYHovbc8XVtFiCyXKb5Z3syuM&f=Q3ZQNmU2SnpsRFlRbUF3dn6kRnWHbwuHAEe0xjejDEW24V9IFvZWk68ZDZuPzrkP&u=https%3A//lists.proxmox.com/pipermail/pve-devel/2022-May/052939.html&k=syJL
> 
> 4. Migration new -> old is subtly broken now, because the old config
> parser will skip [special:cloudinit], but continue parsing the rest,
> meaning that settings from [special:cloudinit] will override the
> settings from the actual current config. It's true that migration new
> ->
> old doesn't /have/ to keep working, but in this case it doesn't
> completely fail, but quietly messes up the config, which is worse
> than
> failing.
> 
> A way to fix it would be to prepare the parser for such special
> sections
> now (skipping the whole section if it's not known), and only
> introduce
> the special section in the next major release, because only then can
> we
> be sure that every migration target is prepared.
> 
> But maybe somebody has a better idea?
> 
> Example (with pve702 running unpatched qemu-server):
> 
> root@pve701 ~ # qm config 118
> boot: order=scsi0;ide2;net0
> cloudinit: HASH(0x55ded04408c0)
> cores: 1
> ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> ide2: none,media=cdrom
> memory: 2048
> meta: creation-qemu=6.2.0,ctime=1651053058
> name: BBBB
> net0: virtio=12:12:34:34:56:56,bridge=vmbr0,firewall=1
> numa: 0
> ostype: l26
> scsi0: rbdkvm:vm-118-disk-0,size=1G
> scsihw: virtio-scsi-pci
> smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> sockets: 1
> vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> 
> root@pve701 ~ # qm cloudinit pending 118
> cur ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> cur name: AAAA
> new name: BBBB
> cur net0: macaddr=4A:89:E8:C9:04:98
> new net0: macaddr=12:12:34:34:56:56
> 
> root@pve701 ~ # qm migrate 118 pve702
> 2022-05-06 09:36:15 use dedicated network address for sending
> migration
> traffic (10.10.50.12)
> 2022-05-06 09:36:15 starting migration of VM 118 to node 'pve702'
> (10.10.50.12)
> 2022-05-06 09:36:16 migration finished successfully (duration
> 00:00:01)
> 
> root@pve701 ~ # ssh 10.10.50.12 qm config 118
> boot: order=scsi0;ide2;net0
> cores: 1
> ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> ide2: none,media=cdrom
> memory: 2048
> meta: creation-qemu=6.2.0,ctime=1651053058
> name: AAAA
> net0: virtio=4A:89:E8:C9:04:98,bridge=vmbr0,firewall=1
> numa: 0
> ostype: l26
> scsi0: rbdkvm:vm-118-disk-0,size=1G
> scsihw: virtio-scsi-pci
> smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> sockets: 1
> vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> 
> 
> > ---
> >  PVE/QemuServer.pm           | 20 +++++++++++++++++---
> >  PVE/QemuServer/Cloudinit.pm | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+), 3 deletions(-)
> > 
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index 0be6be9..8aa550b 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -1993,6 +1993,7 @@ sub vmconfig_register_unused_drive {
> >      if (drive_is_cloudinit($drive)) {
> >         eval { PVE::Storage::vdisk_free($storecfg, $drive->{file})
> > };
> >         warn $@ if $@;
> > +       delete $conf->{cloudinit};
> 
> Currently, it's not prohibited to add more than one cloud-init drive,
> but this series implicitly assumes that.
> 
> >      } elsif (!drive_is_cdrom($drive)) {
> >         my $volid = $drive->{file};
> >         if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> > @@ -2363,6 +2364,7 @@ sub parse_vm_config {
> >         digest => Digest::SHA::sha1_hex($raw),
> >         snapshots => {},
> >         pending => {},
> > +       cloudinit => {},
> >      };
> >  
> >      my $handle_error = sub {
> > @@ -2397,6 +2399,11 @@ sub parse_vm_config {
> >             $descr = undef;
> >             $conf = $res->{$section} = {};
> >             next;
> > +       } elsif ($line =~ m/^\[special:cloudinit\]\s*$/i) {
> > +           $section = 'cloudinit';
> > +           $descr = undef;
> > +           $conf = $res->{$section} = {};
> > +           next;
> >  
> 
> Style nit and nothing new, but you could remove this trailing blank
> line
> while you're at it.
> 
> >         } elsif ($line =~ m/^\[([a-z][a-z0-9_\-]+)\]\s*$/i) {
> >             $section = $1;
> > @@ -2494,7 +2501,7 @@ sub write_vm_config {
> >  
> >         foreach my $key (keys %$cref) {
> >             next if $key eq 'digest' || $key eq 'description' ||
> > $key eq 'snapshots' ||
> > -               $key eq 'snapstate' || $key eq 'pending';
> > +               $key eq 'snapstate' || $key eq 'pending' || $key eq
> > 'cloudinit';
> >             my $value = $cref->{$key};
> >             if ($key eq 'delete') {
> >                 die "propertry 'delete' is only allowed in
> > [PENDING]\n"
> > @@ -2518,6 +2525,8 @@ sub write_vm_config {
> >  
> >      &$cleanup_config($conf->{pending}, 1);
> >  
> > +    &$cleanup_config($conf->{cloudinit}, 1);
> 
> The second parameter should not be 1 here (it's called $pending and
> used
> to check if the key 'delete' is allowed).
> 
> > +
> >      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);
> > @@ -2548,7 +2557,7 @@ sub write_vm_config {
> >         }
> >  
> >         foreach my $key (sort keys %$conf) {
> > -           next if $key =~
> > /^(digest|description|pending|snapshots)$/;
> > +           next if $key =~
> > /^(digest|description|pending|cloudinit|snapshots)$/;
> >             $raw .= "$key: $conf->{$key}\n";
> >         }
> >         return $raw;
> > @@ -2561,6 +2570,11 @@ sub write_vm_config {
> >         $raw .= &$generate_raw_config($conf->{pending}, 1);
> >      }
> >  
> > +    if (scalar(keys %{$conf->{cloudinit}})){
> > +       $raw .= "\n[special:cloudinit]\n";
> > +       $raw .= &$generate_raw_config($conf->{cloudinit}, 1);
> 
> Similar here, setting the second parameter is specific to pending.
> 
> > +    }
> > +
> >      foreach my $snapname (sort keys %{$conf->{snapshots}}) {
> >         $raw .= "\n[$snapname]\n";
> >         $raw .= &$generate_raw_config($conf->{snapshots}-
> > >{$snapname});
> > @@ -5087,9 +5101,9 @@ sub vmconfig_apply_pending {
> >             $conf->{$opt} = delete $conf->{pending}->{$opt};
> >         }
> >      }
> > -
> >      # write all changes at once to avoid unnecessary i/o
> >      PVE::QemuConfig->write_config($vmid, $conf);
> > +
> 
> Style nit: unrelated and doesn't make it better IMHO.
> 
> >  }
> >  
> >  sub vmconfig_update_net {
> 
> 
> 

Re: [pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config.

[DERUMIER, Alexandre]

Hi Fabian, sorry to be late, I was very busy last week

> Series looks pretty good to me, but there are some issues, all
> related
> to this patch (number 4 is the big one):
> 
> 1. assemble() in PVE/VZDump/QemuServer.pm requires changes or the
> message
> INFO: snapshots found (not included into backup)
> will be printed during backup when there is a cloudinit section (even
> if
> there are no snapshots).
> 
> 
ok, I'll fix this
> 2. With qm config <ID>,
> cloudinit: HASH(0x55ceb9a39298)
> shows up in the output.
> 
ok, I'll fix this
> 3. The API/series assumes that there's only one cloudinit drive, but
> there currently is no checks against adding multiple cloudinit
> drives. I
> sent a patch for discussion:
> 
> 4. Migration new -> old is subtly broken now, because the old config
> parser will skip [special:cloudinit], but continue parsing the rest,
> meaning that settings from [special:cloudinit] will override the
> settings from the actual current config. It's true that migration new
> ->
> old doesn't /have/ to keep working, but in this case it doesn't
> completely fail, but quietly messes up the config, which is worse
> than
> failing.
> 
> 
Are you sure it's a problem ?
I mean, [special:cloudinit], are the current running values in the
cloudinit drive.

if they override values after the migration to the old server,
that's ok, because old behaviour was to display "current" values too.

we simply loose pending values.

or did I miss something ?

> A way to fix it would be to prepare the parser for such special
> sections
> now (skipping the whole section if it's not known), and only
> introduce
> the special section in the next major release, because only then can
> we
> be sure that every migration target is prepared.
> 
> But maybe somebody has a better idea?
> 
> Example (with pve702 running unpatched qemu-server):
> 
> root@pve701 ~ # qm config 118
> boot: order=scsi0;ide2;net0
> cloudinit: HASH(0x55ded04408c0)
> cores: 1
> ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> ide2: none,media=cdrom
> memory: 2048
> meta: creation-qemu=6.2.0,ctime=1651053058
> name: BBBB
> net0: virtio=12:12:34:34:56:56,bridge=vmbr0,firewall=1
> numa: 0
> ostype: l26
> scsi0: rbdkvm:vm-118-disk-0,size=1G
> scsihw: virtio-scsi-pci
> smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> sockets: 1
> vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> 
> root@pve701 ~ # qm cloudinit pending 118
> cur ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> cur name: AAAA
> new name: BBBB
> cur net0: macaddr=4A:89:E8:C9:04:98
> new net0: macaddr=12:12:34:34:56:56
> 
> root@pve701 ~ # qm migrate 118 pve702
> 2022-05-06 09:36:15 use dedicated network address for sending
> migration
> traffic (10.10.50.12)
> 2022-05-06 09:36:15 starting migration of VM 118 to node 'pve702'
> (10.10.50.12)
> 2022-05-06 09:36:16 migration finished successfully (duration
> 00:00:01)
> 
> root@pve701 ~ # ssh 10.10.50.12 qm config 118
> boot: order=scsi0;ide2;net0
> cores: 1
> ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> ide2: none,media=cdrom
> memory: 2048
> meta: creation-qemu=6.2.0,ctime=1651053058
> name: AAAA
> net0: virtio=4A:89:E8:C9:04:98,bridge=vmbr0,firewall=1
> numa: 0
> ostype: l26
> scsi0: rbdkvm:vm-118-disk-0,size=1G
> scsihw: virtio-scsi-pci
> smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> sockets: 1
> vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> 
> 
> > ---
> >  PVE/QemuServer.pm           | 20 +++++++++++++++++---
> >  PVE/QemuServer/Cloudinit.pm | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+), 3 deletions(-)
> > 
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index 0be6be9..8aa550b 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -1993,6 +1993,7 @@ sub vmconfig_register_unused_drive {
> >      if (drive_is_cloudinit($drive)) {
> >         eval { PVE::Storage::vdisk_free($storecfg, $drive->{file})
> > };
> >         warn $@ if $@;
> > +       delete $conf->{cloudinit};
> 
> Currently, it's not prohibited to add more than one cloud-init drive,
> but this series implicitly assumes that.
> 
> >      } elsif (!drive_is_cdrom($drive)) {
> >         my $volid = $drive->{file};
> >         if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> > @@ -2363,6 +2364,7 @@ sub parse_vm_config {
> >         digest => Digest::SHA::sha1_hex($raw),
> >         snapshots => {},
> >         pending => {},
> > +       cloudinit => {},
> >      };
> >  
> >      my $handle_error = sub {
> > @@ -2397,6 +2399,11 @@ sub parse_vm_config {
> >             $descr = undef;
> >             $conf = $res->{$section} = {};
> >             next;
> > +       } elsif ($line =~ m/^\[special:cloudinit\]\s*$/i) {
> > +           $section = 'cloudinit';
> > +           $descr = undef;
> > +           $conf = $res->{$section} = {};
> > +           next;
> >  
> 
> Style nit and nothing new, but you could remove this trailing blank
> line
> while you're at it.
> 
> >         } elsif ($line =~ m/^\[([a-z][a-z0-9_\-]+)\]\s*$/i) {
> >             $section = $1;
> > @@ -2494,7 +2501,7 @@ sub write_vm_config {
> >  
> >         foreach my $key (keys %$cref) {
> >             next if $key eq 'digest' || $key eq 'description' ||
> > $key eq 'snapshots' ||
> > -               $key eq 'snapstate' || $key eq 'pending';
> > +               $key eq 'snapstate' || $key eq 'pending' || $key eq
> > 'cloudinit';
> >             my $value = $cref->{$key};
> >             if ($key eq 'delete') {
> >                 die "propertry 'delete' is only allowed in
> > [PENDING]\n"
> > @@ -2518,6 +2525,8 @@ sub write_vm_config {
> >  
> >      &$cleanup_config($conf->{pending}, 1);
> >  
> > +    &$cleanup_config($conf->{cloudinit}, 1);
> 
> The second parameter should not be 1 here (it's called $pending and
> used
> to check if the key 'delete' is allowed).
> 
> > +
> >      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);
> > @@ -2548,7 +2557,7 @@ sub write_vm_config {
> >         }
> >  
> >         foreach my $key (sort keys %$conf) {
> > -           next if $key =~
> > /^(digest|description|pending|snapshots)$/;
> > +           next if $key =~
> > /^(digest|description|pending|cloudinit|snapshots)$/;
> >             $raw .= "$key: $conf->{$key}\n";
> >         }
> >         return $raw;
> > @@ -2561,6 +2570,11 @@ sub write_vm_config {
> >         $raw .= &$generate_raw_config($conf->{pending}, 1);
> >      }
> >  
> > +    if (scalar(keys %{$conf->{cloudinit}})){
> > +       $raw .= "\n[special:cloudinit]\n";
> > +       $raw .= &$generate_raw_config($conf->{cloudinit}, 1);
> 
> Similar here, setting the second parameter is specific to pending.
> 
> > +    }
> > +
> >      foreach my $snapname (sort keys %{$conf->{snapshots}}) {
> >         $raw .= "\n[$snapname]\n";
> >         $raw .= &$generate_raw_config($conf->{snapshots}-
> > >{$snapname});
> > @@ -5087,9 +5101,9 @@ sub vmconfig_apply_pending {
> >             $conf->{$opt} = delete $conf->{pending}->{$opt};
> >         }
> >      }
> > -
> >      # write all changes at once to avoid unnecessary i/o
> >      PVE::QemuConfig->write_config($vmid, $conf);
> > +
> 
> Style nit: unrelated and doesn't make it better IMHO.
> 
> >  }
> >  
> >  sub vmconfig_update_net {
> 
> 
> 

Re: [pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config.

[DERUMIER, Alexandre]

> > old doesn't /have/ to keep working, but in this case it doesn't
> > completely fail, but quietly messes up the config, which is worse
> > than
> > failing.
> > 
> > 
> Are you sure it's a problem ?
> I mean, [special:cloudinit], are the current running values in the
> cloudinit drive.
> 
> if they override values after the migration to the old server,
> that's ok, because old behaviour was to display "current" values too.
> 
> we simply loose pending values.
> 
> or did I miss something ?
> 

oh, ok, sorry.  It's a problem indeed for values like netX.
(Migration will work fine, but config will be replace after migration,
and if we have changes in mac,bridge,... that's really bad.


Maybe some solutions ideas:

- add an new check in migration to see if new cloudinit api is
supported, and forbib migration if cloudinit pending changes exists ?
(or maybe is it already possible to check the version of running qemu-
server ? or some kind of migrate api version ? )

- unconditionally forbid live migration if cloudinit pending change
exists

- force regeneration of pending changes before a live migration.

?



Le lundi 16 mai 2022 à 12:00 +0000, DERUMIER, Alexandre a écrit :
> Hi Fabian, sorry to be late, I was very busy last week
> 
> > Series looks pretty good to me, but there are some issues, all
> > related
> > to this patch (number 4 is the big one):
> > 
> > 1. assemble() in PVE/VZDump/QemuServer.pm requires changes or the
> > message
> > INFO: snapshots found (not included into backup)
> > will be printed during backup when there is a cloudinit section
> > (even
> > if
> > there are no snapshots).
> > 
> > 
> ok, I'll fix this
> > 2. With qm config <ID>,
> > cloudinit: HASH(0x55ceb9a39298)
> > shows up in the output.
> > 
> ok, I'll fix this
> > 3. The API/series assumes that there's only one cloudinit drive,
> > but
> > there currently is no checks against adding multiple cloudinit
> > drives. I
> > sent a patch for discussion:
> > 
> > 4. Migration new -> old is subtly broken now, because the old
> > config
> > parser will skip [special:cloudinit], but continue parsing the
> > rest,
> > meaning that settings from [special:cloudinit] will override the
> > settings from the actual current config. It's true that migration
> > new
> > ->
> > old doesn't /have/ to keep working, but in this case it doesn't
> > completely fail, but quietly messes up the config, which is worse
> > than
> > failing.
> > 
> > 
> Are you sure it's a problem ?
> I mean, [special:cloudinit], are the current running values in the
> cloudinit drive.
> 
> if they override values after the migration to the old server,
> that's ok, because old behaviour was to display "current" values too.
> 
> we simply loose pending values.
> 
> or did I miss something ?
> 
> > A way to fix it would be to prepare the parser for such special
> > sections
> > now (skipping the whole section if it's not known), and only
> > introduce
> > the special section in the next major release, because only then
> > can
> > we
> > be sure that every migration target is prepared.
> > 
> > But maybe somebody has a better idea?
> > 
> > Example (with pve702 running unpatched qemu-server):
> > 
> > root@pve701 ~ # qm config 118
> > boot: order=scsi0;ide2;net0
> > cloudinit: HASH(0x55ded04408c0)
> > cores: 1
> > ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> > ide2: none,media=cdrom
> > memory: 2048
> > meta: creation-qemu=6.2.0,ctime=1651053058
> > name: BBBB
> > net0: virtio=12:12:34:34:56:56,bridge=vmbr0,firewall=1
> > numa: 0
> > ostype: l26
> > scsi0: rbdkvm:vm-118-disk-0,size=1G
> > scsihw: virtio-scsi-pci
> > smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> > sockets: 1
> > vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> > 
> > root@pve701 ~ # qm cloudinit pending 118
> > cur ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> > cur name: AAAA
> > new name: BBBB
> > cur net0: macaddr=4A:89:E8:C9:04:98
> > new net0: macaddr=12:12:34:34:56:56
> > 
> > root@pve701 ~ # qm migrate 118 pve702
> > 2022-05-06 09:36:15 use dedicated network address for sending
> > migration
> > traffic (10.10.50.12)
> > 2022-05-06 09:36:15 starting migration of VM 118 to node 'pve702'
> > (10.10.50.12)
> > 2022-05-06 09:36:16 migration finished successfully (duration
> > 00:00:01)
> > 
> > root@pve701 ~ # ssh 10.10.50.12 qm config 118
> > boot: order=scsi0;ide2;net0
> > cores: 1
> > ide0: rbdkvm:vm-118-cloudinit,media=cdrom
> > ide2: none,media=cdrom
> > memory: 2048
> > meta: creation-qemu=6.2.0,ctime=1651053058
> > name: AAAA
> > net0: virtio=4A:89:E8:C9:04:98,bridge=vmbr0,firewall=1
> > numa: 0
> > ostype: l26
> > scsi0: rbdkvm:vm-118-disk-0,size=1G
> > scsihw: virtio-scsi-pci
> > smbios1: uuid=5b5424be-b2b9-403c-91c1-e2f0d31e6ae6
> > sockets: 1
> > vmgenid: 1bf04ec4-d6f8-477e-9703-1bb403888e13
> > 
> > 
> > > ---
> > >  PVE/QemuServer.pm           | 20 +++++++++++++++++---
> > >  PVE/QemuServer/Cloudinit.pm | 31 +++++++++++++++++++++++++++++++
> > >  2 files changed, 48 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > > index 0be6be9..8aa550b 100644
> > > --- a/PVE/QemuServer.pm
> > > +++ b/PVE/QemuServer.pm
> > > @@ -1993,6 +1993,7 @@ sub vmconfig_register_unused_drive {
> > >      if (drive_is_cloudinit($drive)) {
> > >         eval { PVE::Storage::vdisk_free($storecfg, $drive-
> > > >{file})
> > > };
> > >         warn $@ if $@;
> > > +       delete $conf->{cloudinit};
> > 
> > Currently, it's not prohibited to add more than one cloud-init
> > drive,
> > but this series implicitly assumes that.
> > 
> > >      } elsif (!drive_is_cdrom($drive)) {
> > >         my $volid = $drive->{file};
> > >         if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> > > @@ -2363,6 +2364,7 @@ sub parse_vm_config {
> > >         digest => Digest::SHA::sha1_hex($raw),
> > >         snapshots => {},
> > >         pending => {},
> > > +       cloudinit => {},
> > >      };
> > >  
> > >      my $handle_error = sub {
> > > @@ -2397,6 +2399,11 @@ sub parse_vm_config {
> > >             $descr = undef;
> > >             $conf = $res->{$section} = {};
> > >             next;
> > > +       } elsif ($line =~ m/^\[special:cloudinit\]\s*$/i) {
> > > +           $section = 'cloudinit';
> > > +           $descr = undef;
> > > +           $conf = $res->{$section} = {};
> > > +           next;
> > >  
> > 
> > Style nit and nothing new, but you could remove this trailing blank
> > line
> > while you're at it.
> > 
> > >         } elsif ($line =~ m/^\[([a-z][a-z0-9_\-]+)\]\s*$/i) {
> > >             $section = $1;
> > > @@ -2494,7 +2501,7 @@ sub write_vm_config {
> > >  
> > >         foreach my $key (keys %$cref) {
> > >             next if $key eq 'digest' || $key eq 'description' ||
> > > $key eq 'snapshots' ||
> > > -               $key eq 'snapstate' || $key eq 'pending';
> > > +               $key eq 'snapstate' || $key eq 'pending' || $key
> > > eq
> > > 'cloudinit';
> > >             my $value = $cref->{$key};
> > >             if ($key eq 'delete') {
> > >                 die "propertry 'delete' is only allowed in
> > > [PENDING]\n"
> > > @@ -2518,6 +2525,8 @@ sub write_vm_config {
> > >  
> > >      &$cleanup_config($conf->{pending}, 1);
> > >  
> > > +    &$cleanup_config($conf->{cloudinit}, 1);
> > 
> > The second parameter should not be 1 here (it's called $pending and
> > used
> > to check if the key 'delete' is allowed).
> > 
> > > +
> > >      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);
> > > @@ -2548,7 +2557,7 @@ sub write_vm_config {
> > >         }
> > >  
> > >         foreach my $key (sort keys %$conf) {
> > > -           next if $key =~
> > > /^(digest|description|pending|snapshots)$/;
> > > +           next if $key =~
> > > /^(digest|description|pending|cloudinit|snapshots)$/;
> > >             $raw .= "$key: $conf->{$key}\n";
> > >         }
> > >         return $raw;
> > > @@ -2561,6 +2570,11 @@ sub write_vm_config {
> > >         $raw .= &$generate_raw_config($conf->{pending}, 1);
> > >      }
> > >  
> > > +    if (scalar(keys %{$conf->{cloudinit}})){
> > > +       $raw .= "\n[special:cloudinit]\n";
> > > +       $raw .= &$generate_raw_config($conf->{cloudinit}, 1);
> > 
> > Similar here, setting the second parameter is specific to pending.
> > 
> > > +    }
> > > +
> > >      foreach my $snapname (sort keys %{$conf->{snapshots}}) {
> > >         $raw .= "\n[$snapname]\n";
> > >         $raw .= &$generate_raw_config($conf->{snapshots}-
> > > > {$snapname});
> > > @@ -5087,9 +5101,9 @@ sub vmconfig_apply_pending {
> > >             $conf->{$opt} = delete $conf->{pending}->{$opt};
> > >         }
> > >      }
> > > -
> > >      # write all changes at once to avoid unnecessary i/o
> > >      PVE::QemuConfig->write_config($vmid, $conf);
> > > +
> > 
> > Style nit: unrelated and doesn't make it better IMHO.
> > 
> > >  }
> > >  
> > >  sub vmconfig_update_net {
> > 
> > 
> > 
>