[PATCH qemu-server v2 06/14] migration: intra-cluster: check config can be parsed on target node

[Fiona Ebner <f.ebner@proxmox.com>]

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).

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.

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=7341

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* log when skipping config check

For easier testing:

# 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 = {
# };
# 
# my $confdesc = {
#+    'shiny-new' => {
#+        type => 'string',
#+        enum => ['shiny', 'new'],
#+        default => 'shiny',
#+        optional => 1,
#+        description => "you know you want it",
#+    },
#     onboot => {
#         optional => 1,
#         type => 'boolean',

 src/PVE/API2/Qemu.pm                      |  4 +++-
 src/PVE/QemuMigrate.pm                    | 27 +++++++++++++++++++++++
 src/test/MigrationTest/QemuMigrateMock.pm | 11 ++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

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 => {
                 type => 'boolean',
                 description =>
-                    "Allow to migrate VMs which use local devices. Only root may use this option.",
+                    "Allow to migrate VMs which use local devices and for intra-cluster migration,"
+                    . " configuration options not understood by the target. Only root may use this"
+                    . " option.",
                 optional => 1,
             },
             migration_type => {
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 = [@{ $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.
+            my $tunnel = $self->fork_tunnel();
+            # Compared to remote migration, which also does volume activation, this only strictly
+            # parses the config, so no large timeout is needed. Unfortunately, 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 delaying backwards migration and
+            # giving config check more time should now be in favor of config checking
+            eval {
+                my $nodename = PVE::INotify::nodename();
+                PVE::Tunnel::write_tunnel($tunnel, 3, "config $vmid $nodename");
+            };
+            if (my $err = $@) {
+                chomp($err);
+                # if there is no reply, assume target did not know the command yet
+                if ($err =~ m/^no reply to command/) {
+                    $self->log('info', "skipping strict configuration check (target too old?)");
+                } else {
+                    die "$err - use --force to migrate regardless\n";
+                }
+            }
+            eval { PVE::Tunnel::finish_tunnel($tunnel); };
+            $self->log('warn', "failed to finish tunnel in prepare() - $@") if $@;
+        }
     }
 
     return $running;
diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/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 = $1;
             die "resuming wrong VM '$vmid'\n" if $vmid ne $test_vmid;
             return;
+        } elsif ($command =~ m/^config (\d+) (\S+)$/) {
+            my ($vmid, $node) = ($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 = Test::MockModule->new("PVE::QemuMigrate");
 $qemu_migrate_module->mock(
     fork_tunnel => sub {
-        die "fork_tunnel (mocked) - implement me\n"; # currently no call should lead here
+        return {
+            writer => "mocked",
+            reader => "mocked",
+            pid => 123456,
+            version => 1,
+        };
     },
     start_remote_tunnel => sub {
         my ($self, $raddr, $rport, $ruri, $unix_socket_info) = @_;
-- 
2.47.3