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

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

Hi Fabian, sorry to be late, I was very busy last week

> 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).
> 
> 
ok, I'll fix this
> 2. With qm config <ID>,
> cloudinit: HASH(0x55ceb9a39298)
> shows up in the output.
> 
ok, I'll fix this
> 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:
> 
> 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.
> 
> 
Are you sure it's a problem ?
I mean, [special:cloudinit], are the current running values in the
cloudinit drive.

if they override values after the migration to the old server,
that's ok, because old behaviour was to display "current" values too.

we simply loose pending values.

or did I miss something ?

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