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 { _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
