[PATCH-SERIES qemu-server 0/6] migration: strict config check for intra-cluster migration

[PATCH-SERIES qemu-server 0/6] migration: strict config check for intra-cluster migration

[Fiona Ebner]

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

The first few patches are cleanups/tiny improvements that would be
nice to have in any case.

Unfortunately, before patch "qm: mtunnel: reply when a command is
unknown", 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.

I opted for 3 seconds, since config parsing should be very quick and
5 seconds would still be very noticeable for a bulk migration of VMs
on a shared storage with a fast network. Right now, the option won't
help anyways, only once we add a new config option (at which point
we could bump it to 5 seconds).

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

qemu-server:

Fiona Ebner (6):
  d/control: bump versioned build dependency for libpve-common-perl to
    9.0.12
  tests: migration: get rid of mocking for removed
    PVE::QemuMigrate::read_tunnel()
  qm: mtunnel: avoid using deprecated check_running() helper
  mtunnel: add 'conf' command to do strict configuration parsing
  qm: mtunnel: reply when a command is unknown
  migration: intra-cluster: check config can be parsed on target node

 debian/control                            |  2 +-
 src/PVE/API2/Qemu.pm                      |  4 +++-
 src/PVE/CLI/qm.pm                         | 21 ++++++++++++++++++---
 src/PVE/QemuMigrate.pm                    | 23 +++++++++++++++++++++++
 src/test/MigrationTest/QemuMigrateMock.pm | 14 ++++++++++----
 5 files changed, 55 insertions(+), 9 deletions(-)


Summary over all repositories:
  5 files changed, 55 insertions(+), 9 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server 1/6] d/control: bump versioned build dependency for libpve-common-perl to 9.0.12

[Fiona Ebner]

The config2command test script uses PVE::File which was introduced
with libpve-common-perl=9.0.12.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 debian/control | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index 7f87060a..0c8ea812 100644
--- a/debian/control
+++ b/debian/control
@@ -10,7 +10,7 @@ Build-Depends: debhelper-compat (= 13),
                libnet-dbus-perl,
                libpve-apiclient-perl,
                libpve-cluster-perl,
-               libpve-common-perl (>= 9.0.3),
+               libpve-common-perl (>= 9.0.12),
                libpve-guest-common-perl (>= 5.2.2),
                libpve-network-perl,
                libpve-storage-perl (>= 9.0.16),
-- 
2.47.3

[PATCH qemu-server 2/6] tests: migration: get rid of mocking for removed PVE::QemuMigrate::read_tunnel()

[Fiona Ebner]

The PVE::QemuMigrate::read_tunnel() function was removed by commit
e594231b ("migrate: move tunnel-helpers to pve-guest-common").

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/test/MigrationTest/QemuMigrateMock.pm | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/QemuMigrateMock.pm
index 8cd2da12..df8b575a 100644
--- a/src/test/MigrationTest/QemuMigrateMock.pm
+++ b/src/test/MigrationTest/QemuMigrateMock.pm
@@ -75,9 +75,6 @@ $qemu_migrate_module->mock(
     fork_tunnel => sub {
         die "fork_tunnel (mocked) - implement me\n"; # currently no call should lead here
     },
-    read_tunnel => sub {
-        die "read_tunnel (mocked) - implement me\n"; # currently no call should lead here
-    },
     start_remote_tunnel => sub {
         my ($self, $raddr, $rport, $ruri, $unix_socket_info) = @_;
         $expected_calls->{'finish_tunnel'} = 1;
-- 
2.47.3

[PATCH qemu-server 3/6] qm: mtunnel: avoid using deprecated check_running() helper

[Fiona Ebner]

Calling check_running() with $nocheck=1 is equivalent to using
vm_running_locally().

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/qm.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index bdae9641..2b42c3b4 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -465,9 +465,9 @@ __PACKAGE__->register_method({
                 last;
             } elsif ($line =~ /^resume (\d+)$/) {
                 my $vmid = $1;
-                # check_running and vm_resume with nocheck, since local node
-                # might not have processed config move/rename yet
-                if (PVE::QemuServer::check_running($vmid, 1)) {
+                if (PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+                    # vm_resume with nocheck, since local node might not have processed config
+                    # move/rename yet
                     eval { PVE::QemuServer::RunState::vm_resume($vmid, 1, 1); };
                     if ($@) {
                         $tunnel_write->("ERR: resume failed - $@");
-- 
2.47.3

[PATCH qemu-server 4/6] mtunnel: add 'conf' command to do strict configuration parsing

[Fiona Ebner]

Will be requested by the source of the migration before the
configuration is moved, so there is a parameter for the node where
the configuration resides.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/qm.pm | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index 2b42c3b4..8498d0c5 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -18,6 +18,7 @@ use URI::Escape;
 use PVE::APIClient::LWP;
 use PVE::Cluster;
 use PVE::Exception qw(raise_param_exc);
+use PVE::File;
 use PVE::GuestHelpers;
 use PVE::GuestImport::OVF;
 use PVE::INotify;
@@ -477,6 +478,18 @@ __PACKAGE__->register_method({
                 } else {
                     $tunnel_write->("ERR: resume failed - VM $vmid not running");
                 }
+            } elsif ($line =~ /^config (\d+) (\S+)$/) {
+                my ($vmid, $node) = ($1, $2);
+                eval {
+                    my $conf_fn = PVE::QemuConfig->config_file($vmid, $node);
+                    my $raw = PVE::File::file_get_contents($conf_fn);
+                    PVE::QemuServer::parse_vm_config($conf_fn, $raw, 1);
+                };
+                if (my $err = $@) {
+                    $tunnel_write->("ERR: strict config check for target node failed - $err");
+                } else {
+                    $tunnel_write->("OK");
+                }
             }
         }
 
-- 
2.47.3

[PATCH qemu-server 5/6] qm: mtunnel: reply when a command is unknown

[Fiona Ebner]

Otherwise, the other endpoint cannot distinguish between an unknown
command and a command which takes a long time.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/CLI/qm.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index 8498d0c5..9b7e5ab8 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -490,6 +490,8 @@ __PACKAGE__->register_method({
                 } else {
                     $tunnel_write->("OK");
                 }
+            } else {
+                $tunnel_write->("ERR: unknown command '$line'");
             }
         }
 
-- 
2.47.3

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

[Fiona Ebner]

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

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                    | 23 +++++++++++++++++++++++
 src/test/MigrationTest/QemuMigrateMock.pm | 11 ++++++++++-
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index c2e185a6..6828b1fc 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -5393,7 +5393,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..0ea6385a 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -355,6 +355,29 @@ 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
+                die "$err - use --force to migrate regardless\n" if $err !~ m/^no reply to command/;
+            }
+            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