From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id F20C67050F for ; Fri, 6 May 2022 12:20:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D9339BD75 for ; Fri, 6 May 2022 12:20:37 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 42C6CBD6A for ; Fri, 6 May 2022 12:20:36 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E209942F3E; Fri, 6 May 2022 12:20:35 +0200 (CEST) Message-ID: <574df1ae-04a2-d82c-37aa-826f4546be48@proxmox.com> Date: Fri, 6 May 2022 12:20:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 From: Fabian Ebner To: pve-devel@lists.proxmox.com, aderumier@odiso.com References: <20220427140536.1606918-1-aderumier@odiso.com> <20220427140536.1606918-2-aderumier@odiso.com> Content-Language: en-US In-Reply-To: <20220427140536.1606918-2-aderumier@odiso.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.803 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -3.441 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v4 qemu-server 1/6] cloudinit: add cloudinit section for current generated config. X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 May 2022 10:20:38 -0000 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 , 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 {