[pve-devel] [PATCH v3 qemu-server 0/7] cloudinit pending behaviour change

[pve-devel] [PATCH v3 qemu-server 0/7] 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 custom config can be done with snippets file, without any pending state.

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.

(This will be really usefull with ipam implementation, where ipconfig pending state is really
 needed, as we need to follow the pending state of the netX interface)

So, instead of setting cloudinit values in pending,
this patch serie extract the current config from the cloudinit drive and compare it to vm config (pending config).

(Currently the vm config is simply copied inside the iso at generation, but we could implemented
 configdrive format parsers) 

A new specific cloudinit config api is added too, merging ipaddrX && netX mac
in same field, and displaying the diff between current and generated config.
(we could implemented read config from custom snippet too later)


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


Alexandre Derumier (7):
  cloudinit: add vm config to cloudinit drive
  cloudinit: generate cloudinit drive on offline plug
  cloudinit: make cloudnit options fastplug
  api2: add cloudinit config api
  cloudinit : add extract_cloudinit_config
  api2: add cloudinit_update
  add cloudinit hotplug

 PVE/API2/Qemu.pm            | 118 ++++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm               |   2 +
 PVE/QemuServer.pm           |  81 ++++++++++++++---------
 PVE/QemuServer/Cloudinit.pm | 125 ++++++++++++++++++++++++++++++++++++
 4 files changed, 296 insertions(+), 30 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v3 qemu-server 1/7] cloudinit: add vm config to cloudinit drive

[Alexandre Derumier]

To have current running cloudinit config.

An improvement could be to implement parsing of config drive format,
and also compare cicustom snippets content

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer/Cloudinit.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index a5474d3..abc62b7 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -19,6 +19,7 @@ sub commit_cloudinit_disk {
 
     my $path = "/run/pve/cloudinit/$vmid/";
     mkpath $path;
+
     foreach my $filepath (keys %$files) {
 	if ($filepath !~ m@^(.*)\/[^/]+$@) {
 	    die "internal error: bad file name in cloud-init image: $filepath\n";
@@ -30,6 +31,15 @@ sub commit_cloudinit_disk {
 	file_set_contents("$path/$filepath", $contents);
     }
 
+    mkpath "$path/proxmox";
+    my $confcontent = "";
+    foreach my $opt (keys %$conf) {
+	next if $opt =~ m/^(pending|snapshots|digest)$/;
+	$confcontent .= "$opt: $conf->{$opt}\n";
+    }
+
+    file_set_contents("$path/proxmox/vmconf", $confcontent);
+
     my $storecfg = PVE::Storage::config();
     my $iso_path = PVE::Storage::path($storecfg, $drive->{file});
     my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-- 
2.20.1

Re: [pve-devel] [PATCH v3 qemu-server 1/7] cloudinit: add vm config to cloudinit drive

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> To have current running cloudinit config.
> 
> An improvement could be to implement parsing of config drive format,
> and also compare cicustom snippets content
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer/Cloudinit.pm | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
> index a5474d3..abc62b7 100644
> --- a/PVE/QemuServer/Cloudinit.pm
> +++ b/PVE/QemuServer/Cloudinit.pm
> @@ -19,6 +19,7 @@ sub commit_cloudinit_disk {
>  
>      my $path = "/run/pve/cloudinit/$vmid/";
>      mkpath $path;
> +
>      foreach my $filepath (keys %$files) {
>  	if ($filepath !~ m@^(.*)\/[^/]+$@) {
>  	    die "internal error: bad file name in cloud-init image: $filepath\n";
> @@ -30,6 +31,15 @@ sub commit_cloudinit_disk {
>  	file_set_contents("$path/$filepath", $contents);
>      }
>  
> +    mkpath "$path/proxmox";
> +    my $confcontent = "";
> +    foreach my $opt (keys %$conf) {
> +	next if $opt =~ m/^(pending|snapshots|digest)$/;
> +	$confcontent .= "$opt: $conf->{$opt}\n";
> +    }

For consistency, I'd use write_vm_config() (after dclone() and removing
the snapshots, pending) to generate the contents of the file.

> +
> +    file_set_contents("$path/proxmox/vmconf", $confcontent);
> +
>      my $storecfg = PVE::Storage::config();
>      my $iso_path = PVE::Storage::path($storecfg, $drive->{file});
>      my $scfg = PVE::Storage::storage_config($storecfg, $storeid);

[pve-devel] [PATCH v3 qemu-server 2/7] cloudinit: generate cloudinit drive on offline plug

[Alexandre Derumier]

Currently when only generate it at vm start

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 25ac052..6ddac72 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4776,6 +4776,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 {
@@ -4783,6 +4785,12 @@ sub vmconfig_apply_pending {
 		vmconfig_register_unused_drive($storecfg, $vmid, $conf, parse_drive($opt, $conf->{$opt}))
 	    }
 	};
+
+	if (is_valid_drivename($opt)) {
+	    my $drive = parse_drive($opt, $conf->{pending}->{$opt});
+	    $generate_cloudnit = 1 if drive_is_cloudinit($drive);
+	}
+
 	if (my $err = $@) {
 	    $add_apply_error->($opt, $err);
 	} else {
@@ -4792,6 +4800,8 @@ 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;
 }
 
 sub vmconfig_update_net {
-- 
2.20.1

Re: [pve-devel] [PATCH v3 qemu-server 2/7] cloudinit: generate cloudinit drive on offline plug

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> Currently when only generate it at vm start
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer.pm | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 25ac052..6ddac72 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4776,6 +4776,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 {
> @@ -4783,6 +4785,12 @@ sub vmconfig_apply_pending {
>  		vmconfig_register_unused_drive($storecfg, $vmid, $conf, parse_drive($opt, $conf->{$opt}))
>  	    }
>  	};
> +
> +	if (is_valid_drivename($opt)) {
> +	    my $drive = parse_drive($opt, $conf->{pending}->{$opt});
> +	    $generate_cloudnit = 1 if drive_is_cloudinit($drive);
> +	}

This is bad, because it introduces code (and in particular function
calls, which could contain another eval) between an eval and its error
handling.

> +
>  	if (my $err = $@) {
>  	    $add_apply_error->($opt, $err);
>  	} else {
> @@ -4792,6 +4800,8 @@ 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;
>  }
>  
>  sub vmconfig_update_net {

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

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6ddac72..0be4c45 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4530,9 +4530,10 @@ sub vmconfig_hotplug_pending {
 	$errors->{$opt} = "hotplug problem - $msg";
     };
 
+    my @cloudinit_opts = keys %$confdesc_cloudinit;
     my $changes = 0;
     foreach my $opt (keys %{$conf->{pending}}) { # add/change
-	if ($fast_plug_option->{$opt}) {
+	if ($fast_plug_option->{$opt} || grep { $_ eq $opt } @cloudinit_opts) {
 	    $conf->{$opt} = $conf->{pending}->{$opt};
 	    delete $conf->{pending}->{$opt};
 	    $changes = 1;
@@ -4605,31 +4606,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};
@@ -4676,7 +4652,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.20.1

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

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer.pm | 30 +++---------------------------
>  1 file changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 6ddac72..0be4c45 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4530,9 +4530,10 @@ sub vmconfig_hotplug_pending {
>  	$errors->{$opt} = "hotplug problem - $msg";
>      };
>  
> +    my @cloudinit_opts = keys %$confdesc_cloudinit;
>      my $changes = 0;
>      foreach my $opt (keys %{$conf->{pending}}) { # add/change
> -	if ($fast_plug_option->{$opt}) {
> +	if ($fast_plug_option->{$opt} || grep { $_ eq $opt } @cloudinit_opts) {

Maybe add them to the fast_plug_option hash directly? Then deleting a
cloudinit option would also be fastplugged (in the pending_delete_hash
handling below, it currently only checks for $fast_plug_option).

>  	    $conf->{$opt} = $conf->{pending}->{$opt};
>  	    delete $conf->{pending}->{$opt};
>  	    $changes = 1;

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

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Qemu.pm            | 73 +++++++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm               |  1 +
 PVE/QemuServer/Cloudinit.pm | 70 +++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 24dba86..cc58744 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::Drive;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Monitor qw(mon_cmd);
@@ -1057,6 +1058,78 @@ __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 = [];
+	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
+
+	foreach my $opt (keys %{$pending}) {
+	    push @$res, $pending->{$opt};
+	}
+
+	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 1c199b6..d16bf2c 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -994,6 +994,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 abc62b7..156e073 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -607,4 +607,74 @@ sub dump_cloudinit_config {
     }
 }
 
+sub get_pending_config {
+    my ($conf, $vmid) = @_;
+
+    my $newconf = { %{$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};
+
+    my $print_net_addr = sub {
+	my ($conf, $opt, $netid) = @_;
+
+	if (defined($conf->{$netid})) {
+
+	    my $net = PVE::QemuServer::parse_net($conf->{$netid});
+	    if (defined($conf->{$opt})) {
+		$conf->{$opt} .= ",macaddr=".$net->{macaddr} if $net->{macaddr};
+	    } else {
+		$conf->{$opt} = "";
+	    }
+	}
+    };
+
+    my $res = {};
+    foreach my $opt (@cloudinit_opts) {
+
+	#add macaddr to ipconfig
+	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_net_addr($newconf, $opt, $netid);
+	    &$print_net_addr($cloudinit_current, $opt, $netid);
+	}
+
+	my $item = {
+	    key => $opt,
+	};
+	if ($cloudinit_current->{$opt}) {
+	    $item->{value} = $cloudinit_current->{$opt};
+	    if ($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}
+	}
+
+	$res->{$opt} = $item;
+   }
+   return $res;
+}
+
 1;
-- 
2.20.1

Re: [pve-devel] [PATCH v3 qemu-server 4/7] api2: add cloudinit config api

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> +    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}) {

Style nit: trailing spaces and should be "if (defined..." at the beginning.

> +	    $conf->{cipassword} = '********** ';
> +	} elsif (defined($conf->{cipassword})) {
> +	    $conf->{cipassword} = '**********';
> +	}
> +
> +	$conf->{cloudinit}->{cipassword} = '**********' if defined($conf->{cloudinit}->{cipassword});
> +

The handling above is still from v1/v2 of the series? IIUC, there is no
cloudinit section anymore now.

> +	my $res = [];
> +	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
> +
> +	foreach my $opt (keys %{$pending}) {

Style nit: please use "for" instead of "foreach" for new code

> +	    push @$res, $pending->{$opt};
> +	}
> +
> +	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 1c199b6..d16bf2c 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -994,6 +994,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 abc62b7..156e073 100644
> --- a/PVE/QemuServer/Cloudinit.pm
> +++ b/PVE/QemuServer/Cloudinit.pm
> @@ -607,4 +607,74 @@ sub dump_cloudinit_config {
>      }
>  }
>  
> +sub get_pending_config {
> +    my ($conf, $vmid) = @_;
> +
> +    my $newconf = { %{$conf} };

Might not matter right now, but using dclone() is more future-proof.

> +    my $cloudinit_current = $newconf->{cloudinit};

The next patch should be ordered before this one and this one, so this
one can use extract_cloudinit_config right away.

> +    my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
> +    push @cloudinit_opts, 'name';
> +
> +    #add cloud-init drive 

Is there a reason to care about pending changes on the drive itself here?

> +    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};

Needs to also happen for the old config, or not having 'name' in both
configs will wrongly be detected as a change below.

> +
> +    my $print_net_addr = sub {
> +	my ($conf, $opt, $netid) = @_;
> +
> +	if (defined($conf->{$netid})) {
> +
> +	    my $net = PVE::QemuServer::parse_net($conf->{$netid});
> +	    if (defined($conf->{$opt})) {
> +		$conf->{$opt} .= ",macaddr=".$net->{macaddr} if $net->{macaddr};
> +	    } else {
> +		$conf->{$opt} = "";
> +	    }
> +	}
> +    };
> +
> +    my $res = {};

Could also construct the array already here instead of a hash.

> +    foreach my $opt (@cloudinit_opts) {
> +
> +	#add macaddr to ipconfig

Should we instead consider 'net\d+' to be cloudinit options in this
context (similar to 'name' above) and show the changes to those
directly? That would avoid adding macaddr to ipconfig, which after all
isn't part of its schema.

> +	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} );

Style nit: line too long

> +
> +	    &$print_net_addr($newconf, $opt, $netid);
> +	    &$print_net_addr($cloudinit_current, $opt, $netid);
> +	}
> +
> +	my $item = {
> +	    key => $opt,
> +	};
> +	if ($cloudinit_current->{$opt}) {
> +	    $item->{value} = $cloudinit_current->{$opt};
> +	    if ($newconf->{$opt}) {

Needs defined() or everything falsy will be detected as delete

> +		$item->{pending} = $newconf->{$opt} if $newconf->{$opt} ne $cloudinit_current->{$opt};
> +	    } else {
> +		$item->{delete} = 1;
> +	    }
> +	} else {
> +	    $item->{pending} = $newconf->{$opt} if $newconf->{$opt}
> +	}
> +
> +	$res->{$opt} = $item;
> +   }
> +   return $res;
> +}
> +
>  1;

Re: [pve-devel] [PATCH v3 qemu-server 4/7] api2: add cloudinit config api

[DERUMIER, Alexandre]

Hi,
I have send a v4 with fixes for all your comments

Le jeudi 31 mars 2022 à 15:01 +0200, Fabian Ebner a écrit :
> > +    #add cloud-init drive 
> 
> Is there a reason to care about pending changes on the drive itself
> here?

About this, I'm currently return the config drive in get_pending_config


- for the gui,
https://www.mail-archive.com/pve-devel@lists.proxmox.com/msg04228.html

to display or not the cloudinit options if no drive exist, without need
to do an extra api call.


- Also when cloudinit hotplug is enabled, to auto regenerate the drive
if user change the storage.

[pve-devel] [PATCH v3 qemu-server 5/7] cloudinit : add extract_cloudinit_config

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Qemu.pm            |  1 +
 PVE/QemuServer/Cloudinit.pm | 47 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index cc58744..8ac3ae3 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1063,6 +1063,7 @@ __PACKAGE__->register_method({
     path => '{vmid}/cloudinit',
     method => 'GET',
     proxyto => 'node',
+    protected => '1',
     description => "Get the cloudinit configuration with both current and pending values.",
     permissions => {
 	check => ['perm', '/vms/{vmid}', [ 'VM.Audit' ]],
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 156e073..917511e 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -607,11 +607,56 @@ sub dump_cloudinit_config {
     }
 }
 
+sub extract_cloudinit_config {
+    my ($conf, $vmid) = @_;
+
+    my $current_drive = undef;
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($ds, $drive) = @_;
+	$current_drive = $drive if PVE::QemuServer::drive_is_cloudinit($drive);
+
+    });
+ 
+    my $running = PVE::QemuServer::check_running($vmid);
+    my $storecfg = PVE::Storage::config();
+    my $iso_path = PVE::Storage::path($storecfg, $current_drive->{file});
+    my ($storeid, $volname) = PVE::Storage::parse_volume_id($current_drive->{file}, 1);
+    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+    my $format = PVE::QemuServer::qemu_img_format($scfg, $volname);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+    $plugin->activate_volume($storeid, $scfg, $volname) if !$running;
+
+    my $raw_cloudinit_config = undef;
+
+    my $path = "/run/pve/cloudinit_current/$vmid";
+    mkpath $path;
+    my $parser = sub {
+	my $line = shift;
+	$raw_cloudinit_config .= "$line\n";
+    };
+    eval {
+	#qemu-img dd is really slower (5s) than convert (0.2s)
+        run_command([
+            ['qemu-img', 'convert', '-f', 'raw', '-O', 'raw', "$iso_path", "$path/iso"]
+        ]);
+
+        run_command([
+            ['isoinfo', '-i', "$path/iso", '-x', "/PROXMOX/VMCONF.\;1"]
+        ], outfunc => $parser);
+    };
+    rmtree($path);
+    $plugin->deactivate_volume($storeid, $scfg, $volname) if !$running;
+
+    my $cloudinit_config = PVE::QemuServer::parse_vm_config("/qemu-server/$vmid.conf", $raw_cloudinit_config);
+    return $cloudinit_config;
+}
+
 sub get_pending_config {
     my ($conf, $vmid) = @_;
 
+
     my $newconf = { %{$conf} };
-    my $cloudinit_current = $newconf->{cloudinit};
+    my $cloudinit_current = extract_cloudinit_config($conf, $vmid);
     my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
     push @cloudinit_opts, 'name';
 
-- 
2.20.1

Re: [pve-devel] [PATCH v3 qemu-server 5/7] cloudinit : add extract_cloudinit_config

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> --- a/PVE/QemuServer/Cloudinit.pm
> +++ b/PVE/QemuServer/Cloudinit.pm
> @@ -607,11 +607,56 @@ sub dump_cloudinit_config {
>      }
>  }
>  
> +sub extract_cloudinit_config {
> +    my ($conf, $vmid) = @_;
> +
> +    my $current_drive = undef;
> +    PVE::QemuConfig->foreach_volume($conf, sub {
> +	my ($ds, $drive) = @_;
> +	$current_drive = $drive if PVE::QemuServer::drive_is_cloudinit($drive);
> +

Style nit: blank line shouldn't be there

> +    });
> + 

Style nit: above line starts with a space

> +    my $running = PVE::QemuServer::check_running($vmid);
> +    my $storecfg = PVE::Storage::config();
> +    my $iso_path = PVE::Storage::path($storecfg, $current_drive->{file});
> +    my ($storeid, $volname) = PVE::Storage::parse_volume_id($current_drive->{file}, 1);
> +    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +    my $format = PVE::QemuServer::qemu_img_format($scfg, $volname);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +    $plugin->activate_volume($storeid, $scfg, $volname) if !$running;

Please use the abstraction the storage module provides, i.e.
PVE::Storage::activate_volumes().

> +
> +    my $raw_cloudinit_config = undef;
> +
> +    my $path = "/run/pve/cloudinit_current/$vmid";
> +    mkpath $path;
> +    my $parser = sub {
> +	my $line = shift;
> +	$raw_cloudinit_config .= "$line\n";
> +    };
> +    eval {
> +	#qemu-img dd is really slower (5s) than convert (0.2s)
> +        run_command([
> +            ['qemu-img', 'convert', '-f', 'raw', '-O', 'raw', "$iso_path", "$path/iso"]

Should use $format (or it won't work for qcow2).
No need for the second surrounding [].

> +        ]);
> +
> +        run_command([

Also shouldn't need the second [] I think.

> +            ['isoinfo', '-i', "$path/iso", '-x', "/PROXMOX/VMCONF.\;1"]
> +        ], outfunc => $parser);
> +    };
> +    rmtree($path);
> +    $plugin->deactivate_volume($storeid, $scfg, $volname) if !$running;

PVE::Storage::deactivate_volumes().

> +
> +    my $cloudinit_config = PVE::QemuServer::parse_vm_config("/qemu-server/$vmid.conf", $raw_cloudinit_config);
> +    return $cloudinit_config;

Style nit: no need to introduce that variable

> +}
> +

[pve-devel] [PATCH v3 qemu-server 6/7] api2: add cloudinit_update

[Alexandre Derumier]

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

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

we can simply:
- eject
- regenerated the volume
- replace it with qemu monitor

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Qemu.pm  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 PVE/CLI/qm.pm     |  3 ++-
 PVE/QemuServer.pm | 26 ++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8ac3ae3..7843dcb 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1131,6 +1131,50 @@ __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', any => 1],
+    },
+    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 d16bf2c..f7c97ed 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -994,7 +994,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 0be4c45..ff5d473 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4941,6 +4941,32 @@ 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;
+
+    my $running = PVE::QemuServer::check_running($vmid);
+    my $path = PVE::Storage::path($storecfg, $cloudinit_drive->{file});
+
+    mon_cmd($vmid, "eject", force => JSON::true, id => "$cloudinit_ds") if $running;
+
+    PVE::QemuServer::Cloudinit::generate_cloudinitconfig($conf, $vmid);
+
+    mon_cmd($vmid, "blockdev-change-medium", id => "$cloudinit_ds", filename => "$path") if $running;
+}
+
 # called in locked context by incoming migration
 sub vm_migrate_get_nbd_disks {
     my ($storecfg, $conf, $replicated_volumes) = @_;
-- 
2.20.1

Re: [pve-devel] [PATCH v3 qemu-server 6/7] api2: add cloudinit_update

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> This allow to regenerate the config drive with 1 api call.
> 
> This also avoid to delete drive volume first, and recreate it again.
> 
> we can simply:
> - eject
> - regenerated the volume
> - replace it with qemu monitor
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  PVE/CLI/qm.pm     |  3 ++-
>  PVE/QemuServer.pm | 26 ++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8ac3ae3..7843dcb 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1131,6 +1131,50 @@ __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', any => 1],

Nit: no need for "any => 1"

> +    },
> +    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);

Should be renamed if we go for the "config inside ISO"-approach, because
it doesn't change the config itself now.

> +

Style nit: too many blanks

> +	};
> +
> +	PVE::QemuConfig->lock_config($vmid, $updatefn);
> +
> +	return;
> +    }});
> +
>  # POST/PUT {vmid}/config implementation
>  #
>  # The original API used PUT (idempotent) an we assumed that all operations

[pve-devel] [PATCH v3 qemu-server 7/7] add cloudinit hotplug

[Alexandre Derumier]

This allow to regenerate config drive if pending values exist
when we change vm options.

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ff5d473..80b81c7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -288,7 +288,7 @@ my $confdesc = {
     hotplug => {
         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 completely. Value '1' is an alias for the default 'network,disk,usb'.",
+        description => "Selectively enable hotplug features. This is a comma separated list of hotplug features: 'network', 'disk', 'cpu', 'memory', 'usb' and 'cloudinit'. Use '0' to disable hotplug completely. Value '1' is an alias for the default 'network,disk,usb'.",
         default => 'network,disk,usb',
     },
     reboot => {
@@ -1300,7 +1300,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";
@@ -4675,8 +4675,17 @@ 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;
+	foreach my $opt (keys %{$pending}) {
+	    my $item = $pending->{$opt};
+	    $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.20.1

Re: [pve-devel] [PATCH v3 qemu-server 0/7] cloudinit pending behaviour change

[Fabian Ebner]

Am 09.06.21 um 13:54 schrieb 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 custom config can be done with snippets file, without any pending state.
> 
> 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.
> 
> (This will be really usefull with ipam implementation, where ipconfig pending state is really
>  needed, as we need to follow the pending state of the netX interface)
> 
> So, instead of setting cloudinit values in pending,
> this patch serie extract the current config from the cloudinit drive and compare it to vm config (pending config).
> 
> (Currently the vm config is simply copied inside the iso at generation, but we could implemented
>  configdrive format parsers) 
> 
> A new specific cloudinit config api is added too, merging ipaddrX && netX mac
> in same field, and displaying the diff between current and generated config.
> (we could implemented read config from custom snippet too later)
> 
> 
First off all, sorry for the very late review.

The biggest question still is which approach should be used.

Two downsides of this approach:
* The VM config is made available inside the guest via the ISO, but the
guest doesn't really have any business knowing it.
* The extraction is a bit involved/costly. And technically, we'd need to
lock the config during the extraction (so the drive can't be removed
under our noses, and to prohibit two extractions at the same time). And
it's difficult to tell if extraction failed because it's an old image
that doesn't include the config yet, or if it failed for real.

So IMHO the other approach is a bit better. Much of the review should
also apply to v2 of the series.

A small problem with both approaches is how to handle already existing
configs, because everything will show up as changed. Not really sure
what could be done about that though. Ignoring it and having it
auto-fixed the next time the cloud-init is generated doesn't seem too bad.

Re: [pve-devel] [PATCH v3 qemu-server 0/7] cloudinit pending behaviour change

[DERUMIER, Alexandre]

Thanks for the review Fabian.

I need to look it again between other approach. I don't remember
exactly as I send it last year ;)

I'm going on holiday for 2 weeks tomorrow, so I'll look it when I'll be
back.

thanks again

Alexandre

Le jeudi 31 mars 2022 à 15:01 +0200, Fabian Ebner a écrit :
> Am 09.06.21 um 13:54 schrieb 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 custom config can be done with snippets file, without any
> > pending state.
> > 
> > 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.
> > 
> > (This will be really usefull with ipam implementation, where
> > ipconfig pending state is really
> >  needed, as we need to follow the pending state of the netX
> > interface)
> > 
> > So, instead of setting cloudinit values in pending,
> > this patch serie extract the current config from the cloudinit
> > drive and compare it to vm config (pending config).
> > 
> > (Currently the vm config is simply copied inside the iso at
> > generation, but we could implemented
> >  configdrive format parsers) 
> > 
> > A new specific cloudinit config api is added too, merging ipaddrX
> > && netX mac
> > in same field, and displaying the diff between current and
> > generated config.
> > (we could implemented read config from custom snippet too later)
> > 
> > 
> First off all, sorry for the very late review.
> 
> The biggest question still is which approach should be used.
> 
> Two downsides of this approach:
> * The VM config is made available inside the guest via the ISO, but
> the
> guest doesn't really have any business knowing it.
> * The extraction is a bit involved/costly. And technically, we'd need
> to
> lock the config during the extraction (so the drive can't be removed
> under our noses, and to prohibit two extractions at the same time).
> And
> it's difficult to tell if extraction failed because it's an old image
> that doesn't include the config yet, or if it failed for real.
> 
> So IMHO the other approach is a bit better. Much of the review should
> also apply to v2 of the series.
> 
> A small problem with both approaches is how to handle already
> existing
> configs, because everything will show up as changed. Not really sure
> what could be done about that though. Ignoring it and having it
> auto-fixed the next time the cloud-init is generated doesn't seem too
> bad.
> 

Re: [pve-devel] [PATCH v3 qemu-server 0/7] cloudinit pending behaviour change

[DERUMIER, Alexandre]

Hi Fabian,

I'm back from holiday.

I'll rework the v2 patch series with all your comments.


About:

>>A small problem with both approaches is how to handle already
> existing
> configs, because everything will show up as changed.
> Not really sure
> what could be done about that though. Ignoring it and having it
> auto-fixed the next time the cloud-init is generated doesn't seem too
> bad.

Yes, I was thinking about simply wait to next boot for regenerate the
new section.
But maybe I could be possible to not display all changes if the new
[cloudinit] section don't exist yet.



I'll try to send a v4 next week.


Alexandre





Le jeudi 31 mars 2022 à 15:01 +0200, Fabian Ebner a écrit :
> Am 09.06.21 um 13:54 schrieb 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 custom config can be done with snippets file, without any
> > pending state.
> > 
> > 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.
> > 
> > (This will be really usefull with ipam implementation, where
> > ipconfig pending state is really
> >  needed, as we need to follow the pending state of the netX
> > interface)
> > 
> > So, instead of setting cloudinit values in pending,
> > this patch serie extract the current config from the cloudinit
> > drive and compare it to vm config (pending config).
> > 
> > (Currently the vm config is simply copied inside the iso at
> > generation, but we could implemented
> >  configdrive format parsers) 
> > 
> > A new specific cloudinit config api is added too, merging ipaddrX
> > && netX mac
> > in same field, and displaying the diff between current and
> > generated config.
> > (we could implemented read config from custom snippet too later)
> > 
> > 
> First off all, sorry for the very late review.
> 
> The biggest question still is which approach should be used.
> 
> Two downsides of this approach:
> * The VM config is made available inside the guest via the ISO, but
> the
> guest doesn't really have any business knowing it.
> * The extraction is a bit involved/costly. And technically, we'd need
> to
> lock the config during the extraction (so the drive can't be removed
> under our noses, and to prohibit two extractions at the same time).
> And
> it's difficult to tell if extraction failed because it's an old image
> that doesn't include the config yet, or if it failed for real.
> 
> So IMHO the other approach is a bit better. Much of the review should
> also apply to v2 of the series.
> 
> A small problem with both approaches is how to handle already
> existing
> configs, because everything will show up as changed. Not really sure
> what could be done about that though. Ignoring it and having it
> auto-fixed the next time the cloud-init is generated doesn't seem too
> bad.
>