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