From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id CE66A1FF0B3 for ; Wed, 11 Nov 2026 16:41:28 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E278B21682; Wed, 11 Nov 2026 16:41:25 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Wed, 11 Nov 2026 16:41:21 +0100 Message-Id: To: "Fiona Ebner" , Subject: Re: [PATCH-SERIES many v6 00/26] migration: strict config check for intra-cluster migration From: =?utf-8?q?Michael_K=C3=B6ppl?= Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.22.0 References: <20261103124831.139399-1-f.ebner@proxmox.com> In-Reply-To: <20261103124831.139399-1-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794411681107 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.201 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: JLS65U5KJJ5D7DFDDYQXRO44XOQMNDZK X-Message-ID-Hash: JLS65U5KJJ5D7DFDDYQXRO44XOQMNDZK X-MailFrom: m.koeppl@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: Tested this series on a 3-node cluster: - Introduced a new config option for both VMs and CTs on node A (so it's considered valid there), then set the option for VM 100 and attempted to migrate to node B. Checked that the migration was rejected and that the config of VM 100 remained on node A. Checked that this also holds with `--force 1`, since it should only work with `--skip-config-check 1`. Did this for both running and stopped VMs. - Checked that the migration is possible with `--skip-config-check 1`, both online and offline. The option will then be dropped, as before. - Repeated the above for CTs. Used `--restart` here instead of `--online` for the migration and checked that an aborted migration due to the strict config check leaves the container running. - Simulated a lower version on node B and attempted to migrate. Checked that the migration was successful even with the added option due to the config check being skipped. - Ran migrations through the GUI with both 'Config check' on and off. Checked that the request had the fields set correctly and that the `skip-config-check` flag could also be used by non-root users, as opposed to --force. - Ran a migration on a HA-managed VM with `--with-conntrack-state 1` and checked that {"with-conntrack-state":"1"} showed up in the CRM command and checked the task log to ensure that the migration of conntrack state entries was performed. - Checked that `ha-manager help migrate` showed the correct parameters depending on the resource type and that `ha-manager migrate` rejected parameters that aren't valid for that resource type (used `--with-conntrack-state 1` for a CT). - Using `ha-manager migrate`, checked that HA-based migration is also aborted if a property is unknown to the target node. Then used `qm migrate ... --skip-config-check 1` for that HA-managed VM and checked that the migration was performed even with the additional property. - Added a positive affinity rule for VM 100 and CT 101. Then used `ha-manager migrate vm:100 node-b --with-conntrack-state 1` that the CT could also successfully be migrated even with the VM-only option supplied. I haven't noticed anything that hasn't been noticed before (e.g. that for CTs, the option will remain in the config when using `--skip-config-check 1` and will show warnings, whereas for VMs, it will be dropped). Overall, this seems to be in a pretty good state already. I noticed a few minor things, which I noted on the individual patches, but other than that, the series seems very solid to me and I think it's a very good addition. Consider this: Reviewed-by: Michael K=C3=B6ppl Tested-by: Michael K=C3=B6ppl On Tue Nov 3, 2026 at 1:47 PM CET, Fiona Ebner wrote: > Changes in v6: > * Use 'return' instead of 'next' in handle_crm_json_command(). > * Adapt version helpers, where David's version was recently applied > which moved the pvecfg_min_version helper to the Tools module > instead. > * Bump pve-manager version in version checks. > > Changes in v5 (thanks @Dano!): > * Add PVE::Cluster include in patch "resources: add > migrate_json_properties() method". > * Initialize plugin-specific migrate json properties once on module > load. [snip]