[PATCH qemu-server v2 2/5] remote migration: collect preconditions as structured findings

[Erik Fastermann <e.fastermann@proxmox.com>]

Extract the remote-migration precondition checks into a
validate_remote_migrate_preconditions helper that records each problem
as a {severity, type, message} finding and returns the derived migration
parameters. This prepares for a precondition endpoint that returns the
full findings list instead of dying on errors.

User-visible effect is minimal and successful migrations are unchanged.
Some precondition error messages are reworded and the "Establishing
API connection" info line is no longer printed, as this would be noise
in the precondition endpoint. Precondition warnings now use log_warn and
the collected precondition errors are all included in the die message.
The HA check no longer uses raise_param_exc, which unifies the
interface.

Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 280 +++++++++++++++++++++++++++++--------------
 1 file changed, 189 insertions(+), 91 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 56c5f08c..b16a1365 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -1063,6 +1063,157 @@ sub assert_scsi_feature_compatibility {
     }
 }
 
+my sub validate_remote_migrate_preconditions {
+    my ($param, $findings) = @_;
+
+    my $add_finding = sub {
+        my ($severity, $type, $message, %extra_info) = @_;
+        chomp($message);
+
+        push @$findings,
+            {
+                severity => $severity,
+                type => $type,
+                message => $message,
+                %extra_info,
+            };
+    };
+
+    my $add_error = sub { $add_finding->('error', @_); };
+    my $add_warning = sub { $add_finding->('warning', @_); };
+
+    my $source_vmid = extract_param($param, 'vmid');
+    my $target_endpoint = extract_param($param, 'target-endpoint');
+    my $remote = PVE::JSONSchema::parse_property_string('proxmox-remote', $target_endpoint);
+    my $target_vmid = extract_param($param, 'target-vmid') // $source_vmid;
+
+    my $target_storage = extract_param($param, 'target-storage');
+    my $storagemap = eval { PVE::JSONSchema::parse_idmap($target_storage, 'pve-storage-id') };
+    raise_param_exc({ 'target-storage' => "failed to parse storage map: $@" })
+        if $@;
+
+    my $target_bridge = extract_param($param, 'target-bridge');
+    my $bridgemap = eval { PVE::JSONSchema::parse_idmap($target_bridge, 'pve-bridge-id') };
+    raise_param_exc({ 'target-bridge' => "failed to parse bridge map: $@" })
+        if $@;
+
+    my $delete = extract_param($param, 'delete') // 0;
+
+    PVE::Cluster::check_cfs_quorum();
+
+    # test if VM exists
+    my $conf = eval { PVE::QemuConfig->load_config($source_vmid) };
+    if (my $err = $@) {
+        $add_error->('load-vm-config', $err);
+        return;
+    }
+
+    if (PVE::QemuConfig->has_lock($conf)) {
+        $add_error->('vm-locked', "VM is locked ($conf->{lock})");
+    }
+
+    if (PVE::HA::Config::service_is_configured("vm:$source_vmid")) {
+        $add_error->('vm-ha-configured', 'cannot remote-migrate VM that is configured for HA');
+    }
+
+    my $conn_args = {
+        protocol => 'https',
+        host => $remote->{host},
+        port => $remote->{port} // 8006,
+        apitoken => $remote->{apitoken},
+    };
+
+    if ($remote->{fingerprint}) {
+        $conn_args->{cached_fingerprints} = { uc($remote->{fingerprint}) => 1 };
+    }
+
+    my $api_client = PVE::APIClient::LWP->new(%$conn_args);
+
+    # check connection once at the start
+    eval { $api_client->get("/version") };
+    if (my $err = $@) {
+        $add_error->('remote-query', $err);
+        return;
+    }
+
+    my $repl_conf = PVE::ReplicationConfig->new();
+    my $is_replicated = $repl_conf->check_for_existing_jobs($source_vmid, 1);
+    $add_error->('vm-replicated', "cannot remote-migrate replicated VM")
+        if $is_replicated;
+
+    if (PVE::QemuServer::check_running($source_vmid)) {
+        $add_error->(
+            'offline-migration-vm-running',
+            "cannot migrate running VM without online option",
+        ) if !$param->{online};
+    } else {
+        $add_warning->(
+            'online-migration-vm-not-running',
+            "VM isn't running, migrating offline instead",
+        ) if $param->{online};
+        $param->{online} = 0;
+    }
+
+    my $check_custom_cpu = sub {
+        return if !defined($conf->{cpu});
+
+        my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu});
+        my $cputype = $cpu->{cputype};
+        return if !defined($cputype) || !PVE::QemuServer::CPUConfig::is_custom_model($cputype);
+
+        my $custom_cpu = PVE::QemuServer::CPUConfig::get_custom_model($cputype);
+
+        my $remote_custom_cpu = eval {
+            $api_client->get(
+                "/cluster/qemu/custom-cpu-models/" . URI::Escape::uri_escape_utf8($cputype));
+        };
+
+        if (my $err = $@) {
+            $add_error->(
+                'custom-cpu-validation',
+                "could not validate custom CPU model compatibility: $err",
+            );
+            return;
+        }
+
+        my $cpu_schema = {
+            type => 'object',
+            properties => PVE::QemuServer::CPUConfig->options(),
+        };
+        eval { PVE::JSONSchema::validate($remote_custom_cpu, $cpu_schema); };
+
+        if (my $err = $@) {
+            $add_error->(
+                'custom-cpu-validation',
+                "could not validate custom CPU model compatibility: $err",
+            );
+            return;
+        }
+
+        eval {
+            PVE::QemuServer::CPUConfig::assert_custom_model_compatibility(
+                $custom_cpu, $remote_custom_cpu,
+            );
+        };
+
+        $add_error->('custom-cpu-mismatch', $@) if $@;
+    };
+    $check_custom_cpu->();
+
+    $add_error->('storage-mapping', "remote migration requires explicit storage mapping")
+        if $storagemap->{identity};
+
+    return {
+        conn_args => $conn_args,
+        api_client => $api_client,
+        source_vmid => $source_vmid,
+        target_vmid => $target_vmid,
+        storagemap => $storagemap,
+        bridgemap => $bridgemap,
+        delete => $delete,
+    };
+}
+
 __PACKAGE__->register_method({
     name => 'vmlist',
     path => '',
@@ -5725,106 +5876,36 @@ __PACKAGE__->register_method({
         my $rpcenv = PVE::RPCEnvironment::get();
         my $authuser = $rpcenv->get_user();
 
-        my $source_vmid = extract_param($param, 'vmid');
-        my $target_endpoint = extract_param($param, 'target-endpoint');
-        my $target_vmid = extract_param($param, 'target-vmid') // $source_vmid;
-
-        my $delete = extract_param($param, 'delete') // 0;
-
-        PVE::Cluster::check_cfs_quorum();
-
-        # test if VM exists
-        my $conf = PVE::QemuConfig->load_config($source_vmid);
-
-        PVE::QemuConfig->check_lock($conf);
+        my $findings = [];
+        my $migration_info = validate_remote_migrate_preconditions($param, $findings);
 
-        raise_param_exc({ vmid => "cannot remote-migrate VM that is configured for HA" })
-            if PVE::HA::Config::service_is_configured("vm:$source_vmid");
-
-        my $remote = PVE::JSONSchema::parse_property_string('proxmox-remote', $target_endpoint);
-
-        # TODO: move this as helper somewhere appropriate?
-        my $conn_args = {
-            protocol => 'https',
-            host => $remote->{host},
-            port => $remote->{port} // 8006,
-            apitoken => $remote->{apitoken},
-        };
+        my $errors = '';
+        my $error_count = 0;
 
-        if ($remote->{fingerprint}) {
-            $conn_args->{cached_fingerprints} = { uc($remote->{fingerprint}) => 1 };
+        for my $finding (@$findings) {
+            next if $finding->{severity} ne 'error';
+            $errors .= "- $finding->{message}\n";
+            $error_count++;
         }
 
-        print "Establishing API connection with remote at '$remote->{host}'\n";
-
-        my $api_client = PVE::APIClient::LWP->new(%$conn_args);
-
-        my $repl_conf = PVE::ReplicationConfig->new();
-        my $is_replicated = $repl_conf->check_for_existing_jobs($source_vmid, 1);
-        die "cannot remote-migrate replicated VM\n" if $is_replicated;
-
-        if (PVE::QemuServer::check_running($source_vmid)) {
-            die "can't migrate running VM without --online\n" if !$param->{online};
-
-        } else {
-            warn "VM isn't running. Doing offline migration instead.\n" if $param->{online};
-            $param->{online} = 0;
-        }
-
-        if (defined($conf->{cpu})) {
-            my $cpu = PVE::JSONSchema::parse_property_string('pve-vm-cpu-conf', $conf->{cpu});
-            my $cputype = $cpu->{cputype};
-            if (defined($cputype) && PVE::QemuServer::CPUConfig::is_custom_model($cputype)) {
-                my $custom_cpu = PVE::QemuServer::CPUConfig::get_custom_model($cputype);
-
-                my $remote_custom_cpu = eval {
-                    $api_client->get("/cluster/qemu/custom-cpu-models/"
-                        . URI::Escape::uri_escape_utf8($cputype));
-                };
-                die "could not validate custom CPU model compatibility: $@\n" if $@;
-
-                my $cpu_schema = {
-                    type => 'object',
-                    properties => PVE::QemuServer::CPUConfig->options(),
-                };
-                eval { PVE::JSONSchema::validate($remote_custom_cpu, $cpu_schema); };
-                die "could not validate custom CPU model compatibility: $@\n" if $@;
-
-                PVE::QemuServer::CPUConfig::assert_custom_model_compatibility(
-                    $custom_cpu, $remote_custom_cpu,
-                );
-            }
+        if ($errors) {
+            die "detected $error_count error(s) preventing remote migration:\n" . $errors;
         }
 
-        my $storecfg = PVE::Storage::config();
-        my $target_storage = extract_param($param, 'target-storage');
-        my $storagemap =
-            eval { PVE::JSONSchema::parse_idmap($target_storage, 'pve-storage-id') };
-        raise_param_exc({ 'target-storage' => "failed to parse storage map: $@" })
-            if $@;
-
-        my $target_bridge = extract_param($param, 'target-bridge');
-        my $bridgemap = eval { PVE::JSONSchema::parse_idmap($target_bridge, 'pve-bridge-id') };
-        raise_param_exc({ 'target-bridge' => "failed to parse bridge map: $@" })
-            if $@;
-
-        die "remote migration requires explicit storage mapping!\n"
-            if $storagemap->{identity};
-
-        $param->{storagemap} = $storagemap;
-        $param->{bridgemap} = $bridgemap;
+        $param->{storagemap} = $migration_info->{storagemap};
+        $param->{bridgemap} = $migration_info->{bridgemap};
         $param->{remote} = {
-            conn => $conn_args, # re-use fingerprint for tunnel
-            client => $api_client,
-            vmid => $target_vmid,
+            conn => $migration_info->{conn_args}, # re-use fingerprint for tunnel
+            client => $migration_info->{api_client},
+            vmid => $migration_info->{target_vmid},
         };
         $param->{migration_type} = 'websocket';
         $param->{'with-local-disks'} = 1;
-        $param->{delete} = $delete if $delete;
+        $param->{delete} = $migration_info->{delete} if $migration_info->{delete};
 
-        my $cluster_status = $api_client->get("/cluster/status");
+        my $cluster_status = $migration_info->{api_client}->get("/cluster/status");
         my $target_node;
-        foreach my $entry (@$cluster_status) {
+        for my $entry (@$cluster_status) {
             next if $entry->{type} ne 'node';
             if ($entry->{local}) {
                 $target_node = $entry->{name};
@@ -5836,14 +5917,31 @@ __PACKAGE__->register_method({
             if !defined($target_node);
 
         my $realcmd = sub {
-            PVE::QemuMigrate->migrate($target_node, $remote->{host}, $source_vmid, $param);
+            PVE::QemuMigrate->migrate(
+                $target_node,
+                $migration_info->{conn_args}->{host},
+                $migration_info->{source_vmid},
+                $param,
+            );
         };
 
         my $worker = sub {
-            return PVE::GuestHelpers::guest_migration_lock($source_vmid, 10, $realcmd);
+            # warn only in the worker, so the messages reach the task log and
+            # the task's warning count matches what is logged
+            for my $finding (@$findings) {
+                log_warn($finding->{message}) if $finding->{severity} eq 'warning';
+            }
+
+            return PVE::GuestHelpers::guest_migration_lock(
+                $migration_info->{source_vmid},
+                10,
+                $realcmd,
+            );
         };
 
-        return $rpcenv->fork_worker('qmigrate', $source_vmid, $authuser, $worker);
+        return $rpcenv->fork_worker(
+            'qmigrate', $migration_info->{source_vmid}, $authuser, $worker,
+        );
     },
 });
 
-- 
2.47.3