From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 9736B1FF0B3 for ; Wed, 14 Oct 2026 09:59:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 207E12147C; Wed, 14 Oct 2026 09:59:41 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 14 Oct 2026 09:59:32 +0200 Message-Id: From: "Daniel Kral" To: "Fiona Ebner" , Subject: Re: [PATCH ha-manager v3 10/21] api: resources: migration: support additional migration options X-Mailer: aerc 0.22.0-10-g6373ac9d2179-dirty References: <20260918160841.128088-1-f.ebner@proxmox.com> <20260918160841.128088-11-f.ebner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791964772821 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.811 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: PZ3KFD7Q54N73WAUWLOIVB7PRSYWU4JX X-Message-ID-Hash: PZ3KFD7Q54N73WAUWLOIVB7PRSYWU4JX X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue Oct 13, 2026 at 4:34 PM CEST, Fiona Ebner wrote: > Am 12.10.26 um 11:59 AM schrieb Daniel Kral: >> On Fri Sep 18, 2026 at 6:08 PM CEST, Fiona Ebner wrote: >>> + my $version_info =3D PVE::Cluster::get_node_kv('version-info'); >>> + for my $node (sort keys $version_info->%*) { >>> + my $node_info =3D eval { JSON::decode_json($version_info->{$no= de}); }; >>> + if (my $err =3D $@) { # warn, but continue >>> + syslog('warn', "cannot parse version info for $node as JSO= N - $err"); >>=20 >> Shouldn't we fail if it's not possible to determine a node's version? > > No, this is deliberate. Updated cluster is the usual situation. An error > here should not affect the usual situation. I'm choosing to affect the > unusual situation, where the master happens to be too old and unable to > report its version properly, which should be rare. The failure is also > not critical: it will be an ignored CRM command with warnings/errors > logged until the version can be determined again. Yeah, since 'version-info' is broadcasted in the same format since Proxmox VE 7.1 so that shouldn't be a problem. > > But I will switch the order below and keep submitting a non-JSON CRM > command if the new style is not actually required. This gives an escape > hatch. This depends if we want to switch to only the JSON CRM command format in the future, then it would be preferable to always use the new format so we can switch to it in some future major version, at least for the migrate command? > >>=20 >> Any way, the syslog should probably have some context why parsing the >> version info is needed here and should also be shown to stdout/stderr >> for callers of ha-manager. > > Ack. > >>=20 >>> + } elsif (!PVE::Cluster::pvecfg_min_version($node_info->{versio= n}, 9, 2, 21)) { >>> + return 0; >>> + } >>> + } >>> + >>> + return 1; >>> +} >>=20 >> IMO a generic version of this helper would be nice to have in >> PVE::Cluster as well, as there already are users that need that >> functionality (David's vNet pool series [0] and Lukas' notification >> matcher series [1]), but no blocker for this. >>=20 >> [0] https://lore.proxmox.com/pve-devel/20260724142528.109453-4-d.riley@p= roxmox.com/ >> [1] https://lore.proxmox.com/pve-devel/20261009125646.284673-39-l.wagner= @proxmox.com/ >>=20 > > I'm not sure, because not every caller might have the same kind of > failure consideration I described above. Other callers might want to be > stricter if the failure scenario (i.e. in case of an old node that can't > be detected) is worse or more common. There also is the consideration > with offline nodes, which is another angle to the check. So I'm somewhat > inclined to keep it local (for now). Right, good point, haven't thought about this! Then let's keep it as it is for now.