[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 at 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 at 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 at 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 at 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 {