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

["DERUMIER, Alexandre" <Alexandre.DERUMIER@groupe-cyllene.com>]

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