Re: [RFC qemu-server 2/4] remote migrate: collect preconditions as structured findings

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

Am 21.07.26 um 1:58 PM schrieb Erik Fastermann:
> Extract the remote-migration precondition checks into a
> validate_remote_migrate_preconditions helper that records each problem
> as a {severity, code, message} finding and returns the derived
> migration parameters. The endpoint fails on the first error finding,
> preserving the previous abort behavior.
> 
> It also pulls the checks that previously lived only in the qm CLI into
> the endpoint. Direct API callers such as the web UI now run them too,
> so fewer migrations start only to fail partway. This prepares for a
> precondition endpoint that returns the full findings list instead of
> dying on the first error.

Nit: that part could be it's own patch

> User-visible effect is minimal: successful migrations are unchanged.
> Some precondition error messages are reworded and the "Establishing
> API connection" info line is no longer printed.

Do we gain anything from not printing it? Such changes can also be nicer
in separate patches to ease reviewability and discussion.

> 
> Signed-off-by: Erik Fastermann <e.fastermann@proxmox.com>
> ---
>  src/PVE/API2/Qemu.pm | 304 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 211 insertions(+), 93 deletions(-)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index 68f1800c..9b487457 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -1042,6 +1042,195 @@ sub assert_scsi_feature_compatibility {
>      }
>  }
>  
> +my sub validate_remote_migrate_preconditions {
> +    my ($param, $findings) = @_;
> +
> +    my $add_error = sub {

Style nit: please collect the subroutine arguments as named arguments
like usual. In particular, you can use a hash like %extra_info as the
last one.

> +        push @$findings,
> +            {
> +                severity => 'error',
> +                code => $_[0],

Nit: for the {storage}/import-metadata endpoint we use 'type' rather
than 'code'.

> +                message => ("$_[1]" =~ s/\s+$//r),

Please use chomp() rather than such a regex (or PVE::Tools::trim, but I
don't think there'll be trailing whitespace, else we should rather fix
it where the messsage originates).

> +                @_[2 .. $#_],
> +            };
> +    };
> +
> +    my $add_warning = sub {

Style nit: please collect the subroutine arguments as named arguments
like usual.

> +        push @$findings,
> +            {
> +                severity => 'warning',
> +                code => $_[0],
> +                message => ("$_[1]" =~ s/\s+$//r),

Same as above

> +                @_[2 .. $#_],
> +            };
> +    };
> +
> +    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 ($@) {

Style nit: please use if (my $err = $@) { to immediately assign the
error to a variable. Same for the other instances.

> +        $add_error->('load-vm-config', $@);
> +        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 ($@) {
> +        $add_error->('remote-conn', $@);
> +        return;
> +    }
> +
> +    my $resources = $api_client->get("/cluster/resources", { type => 'vm' });
> +    if (grep { defined($_->{vmid}) && $_->{vmid} eq $target_vmid } @$resources) {
> +        $add_error->('target-vmid-exists', "remote: guest with ID '$target_vmid' already exists");
> +    }
> +
> +    my $storages = $api_client->get("/nodes/localhost/storage", { enabled => 1 });
> +
> +    my $check_remote_storage = sub {
> +        my ($storage) = @_;
> +        my $found = [grep { $_->{storage} eq $storage } @$storages];
> +
> +        if (!@$found) {
> +            $add_error->(
> +                'storage-missing',
> +                "remote: storage '$storage' does not exist (or missing permission)",
> +                storage => $storage,
> +            );
> +            return;
> +        }
> +
> +        $found = $found->[0];
> +        my $content_types = [PVE::Tools::split_list($found->{content})];
> +        $add_error->(
> +            'storage-no-images',
> +            "remote: storage '$storage' cannot store images",
> +            storage => $storage,
> +        ) if !grep { $_ eq 'images' } @$content_types;
> +    };
> +
> +    foreach my $target_sid (values %{ $storagemap->{entries} }) {

Style nit: please use for instead of foreach, please use post deref ->%*

> +        $check_remote_storage->($target_sid);
> +    }
> +

---snip 8<---

> @@ -5713,104 +5902,26 @@ __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);
> -
> -        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},
> -        };
> -
> -        if ($remote->{fingerprint}) {
> -            $conn_args->{cached_fingerprints} = { uc($remote->{fingerprint}) => 1 };
> -        }
> -
> -        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;
> +        my $findings = [];
> +        my $plan = validate_remote_migrate_preconditions($param, $findings);

Style nit: why pass in $findings like this? You can just return multiple
values instead. I'd prefer a name like $migration_info rather than $plan.

>  
> -        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;
> +        foreach my $finding (@$findings) {

Style nit: please use for instead of foreach

> +            warn "$finding->{message}\n" if $finding->{severity} eq 'warning';
> +            die "$finding->{message}\n" if $finding->{severity} eq 'error';

We can print all warnings and errors and then die with a message saying
how many warnings and errors there are.

>          }
>  
> -        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,
> -                );
> -            }
> -        }
> -
> -        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} = $plan->{storagemap};
> +        $param->{bridgemap} = $plan->{bridgemap};
>          $param->{remote} = {
> -            conn => $conn_args, # re-use fingerprint for tunnel
> -            client => $api_client,
> -            vmid => $target_vmid,
> +            conn => $plan->{conn_args}, # re-use fingerprint for tunnel
> +            client => $plan->{api_client},
> +            vmid => $plan->{target_vmid},
>          };
>          $param->{migration_type} = 'websocket';
>          $param->{'with-local-disks'} = 1;
> -        $param->{delete} = $delete if $delete;
> +        $param->{delete} = $plan->{delete} if $plan->{delete};
>  
> -        my $cluster_status = $api_client->get("/cluster/status");
> +        my $cluster_status = $plan->{api_client}->get("/cluster/status");
>          my $target_node;
>          foreach my $entry (@$cluster_status) {
>              next if $entry->{type} ne 'node';
> @@ -5824,14 +5935,21 @@ __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,
> +                $plan->{conn_args}->{host},
> +                $plan->{source_vmid},
> +                $param,
> +            );
>          };
>  
>          my $worker = sub {
> -            return PVE::GuestHelpers::guest_migration_lock($source_vmid, 10, $realcmd);
> +            return PVE::GuestHelpers::guest_migration_lock($plan->{source_vmid}, 10, $realcmd);
>          };
>  
> -        return $rpcenv->fork_worker('qmigrate', $source_vmid, $authuser, $worker);
> +        return $rpcenv->fork_worker(
> +            'qmigrate', $plan->{source_vmid}, $authuser, $worker,
> +        );
>      },
>  });
>