From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id D03B61FF141 for ; Tue, 16 Jun 2026 12:17:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8EBA82FAA; Tue, 16 Jun 2026 12:17:20 +0200 (CEST) Date: Tue, 16 Jun 2026 12:17:13 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH qemu-server v2 06/14] migration: intra-cluster: check config can be parsed on target node To: Fiona Ebner , pve-devel@lists.proxmox.com References: <20260225151931.176335-1-f.ebner@proxmox.com> <20260225151931.176335-7-f.ebner@proxmox.com> In-Reply-To: <20260225151931.176335-7-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1781604209.xzojnasr46.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1781604981240 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: EENTZM3DG5QV4ZTRKE3YU7PK66V2E7SS X-Message-ID-Hash: EENTZM3DG5QV4ZTRKE3YU7PK66V2E7SS X-MailFrom: f.gruenbichler@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 February 25, 2026 4:18 pm, Fiona Ebner wrote: > For remote migration, we already check that the config can be parsed > on the target. Do the same for intra-cluster migration, to avoid > issues like [0] for future new settings, with lines being unexpectedly > and relatively silently dropped (there are warnings in the target's > system logs). >=20 > Unfortunately, before commit "qm: mtunnel: reply when a command is > unknown", which is part of the same patch series, when a command is > unknown, mtunnel did not reply at all. Therefore, this delays > backwards migrations to qemu-server versions less than the next bumped > version (at the time of this writing expected to be 9.1.5) by 3 > seconds. >=20 > [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=3D7341 >=20 > Signed-off-by: Fiona Ebner > --- >=20 > Changes in v2: > * log when skipping config check >=20 > For easier testing: >=20 > # commented out so 'git am' does not apply it O;) > # diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > # index 545758dc..b914314e 100644 > # --- a/src/PVE/QemuServer.pm > # +++ b/src/PVE/QemuServer.pm > # @@ -236,6 +236,13 @@ my $spice_enhancements_fmt =3D { > # }; > #=20 > # my $confdesc =3D { > #+ 'shiny-new' =3D> { > #+ type =3D> 'string', > #+ enum =3D> ['shiny', 'new'], > #+ default =3D> 'shiny', > #+ optional =3D> 1, > #+ description =3D> "you know you want it", > #+ }, > # onboot =3D> { > # optional =3D> 1, > # type =3D> 'boolean', >=20 > src/PVE/API2/Qemu.pm | 4 +++- > src/PVE/QemuMigrate.pm | 27 +++++++++++++++++++++++ > src/test/MigrationTest/QemuMigrateMock.pm | 11 ++++++++- > 3 files changed, 40 insertions(+), 2 deletions(-) >=20 > diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm > index 1f0864f5..47466513 100644 > --- a/src/PVE/API2/Qemu.pm > +++ b/src/PVE/API2/Qemu.pm > @@ -5399,7 +5399,9 @@ __PACKAGE__->register_method({ > force =3D> { > type =3D> 'boolean', > description =3D> > - "Allow to migrate VMs which use local devices. Only = root may use this option.", > + "Allow to migrate VMs which use local devices and fo= r intra-cluster migration," > + . " configuration options not understood by the targ= et. Only root may use this" > + . " option.", > optional =3D> 1, > }, > migration_type =3D> { > diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm > index f7ec3227..901fe96d 100644 > --- a/src/PVE/QemuMigrate.pm > +++ b/src/PVE/QemuMigrate.pm > @@ -355,6 +355,33 @@ sub prepare { > my $cmd =3D [@{ $self->{rem_ssh} }, '/bin/true']; > eval { $self->cmd_quiet($cmd); }; > die "Can't connect to destination address using public key\n" if= $@; > + > + if (!$self->{opts}->{force}) { > + # Fork a short-lived tunnel for checking the config. Later, = the proper tunnel with SSH > + # forwaring info is forked. typo: forwarding > + my $tunnel =3D $self->fork_tunnel(); > + # Compared to remote migration, which also does volume activ= ation, this only strictly > + # parses the config, so no large timeout is needed. Unfortun= ately, mtunnel did not > + # indicate that a command is unknown, but not reply at all, = so the timeout must be very > + # low right now. > + # FIXME PVE 10 - bump timeout, the trade-off between delayin= g backwards migration and > + # giving config check more time should now be in favor of co= nfig checking > + eval { > + my $nodename =3D PVE::INotify::nodename(); > + PVE::Tunnel::write_tunnel($tunnel, 3, "config $vmid $nod= ename"); > + }; > + if (my $err =3D $@) { > + chomp($err); > + # if there is no reply, assume target did not know the c= ommand yet > + if ($err =3D~ m/^no reply to command/) { > + $self->log('info', "skipping strict configuration ch= eck (target too old?)"); > + } else { > + die "$err - use --force to migrate regardless\n"; if we ever have a bug in our config handling that causes the writer to emit something that the parser would skip, affected VMs are effectively only migratable as root.. which in turn means that a lot of new->old migrations would become root only? maybe it would make sense to split this from the existing force logic to make it available for regular users? this also applies to the matching pve-container patch. > + } > + } > + eval { PVE::Tunnel::finish_tunnel($tunnel); }; > + $self->log('warn', "failed to finish tunnel in prepare() - $= @") if $@; > + } > } > =20 > return $running; > diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/Migrati= onTest/QemuMigrateMock.pm > index df8b575a..170634de 100644 > --- a/src/test/MigrationTest/QemuMigrateMock.pm > +++ b/src/test/MigrationTest/QemuMigrateMock.pm > @@ -65,6 +65,10 @@ $tunnel_module->mock( > my $vmid =3D $1; > die "resuming wrong VM '$vmid'\n" if $vmid ne $test_vmid; > return; > + } elsif ($command =3D~ m/^config (\d+) (\S+)$/) { > + my ($vmid, $node) =3D ($1, $2); > + die "check config for wrong VM '$vmid'\n" if $vmid ne $test_= vmid; > + return; > } > die "write_tunnel (mocked) - implement me: $command\n"; > }, > @@ -73,7 +77,12 @@ $tunnel_module->mock( > my $qemu_migrate_module =3D Test::MockModule->new("PVE::QemuMigrate"); > $qemu_migrate_module->mock( > fork_tunnel =3D> sub { > - die "fork_tunnel (mocked) - implement me\n"; # currently no call= should lead here > + return { > + writer =3D> "mocked", > + reader =3D> "mocked", > + pid =3D> 123456, > + version =3D> 1, > + }; > }, > start_remote_tunnel =3D> sub { > my ($self, $raddr, $rport, $ruri, $unix_socket_info) =3D @_; > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20