Re: [RFC qemu-server 1/1] fix #7053: api: migrate: save and restore migration params for HA managed VMs

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

Am 25.02.26 um 3:35 PM schrieb Daniel Kral:
> If a HA-managed VM's migrate API endpoint is called from a web API or
> CLI environment, it is first relayed to the HA Manager by queueing a
> 'migrate' CRM command with `ha-manager migrate vm:$vmid $target_node`.
> This command doesn't take any additional migration parameters though.
> 
> As soon as the HA Manager reads the CRM command in the next HA Manager
> round, it passes the migration request - if valid - to the HA resource
> state. This migration request is then picked up by the LRM, where the HA
> resource is assigned to and calls the migrate_vm API endpoint, which
> will then initial the VM migration.

Nit: according to Wiktionary, 'initial' can't be used as a verb in this
context [0]

> 
> As the migrate_vm API request is proxied to the node, where the HA
> resource is assigned to, this allows the migrate parameters to stored
> locally while the migration request is passed through the HA stack.
> 

I'm fine with this approach :)

> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  src/PVE/API2/Qemu.pm | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index 1f0864f5..027896a3 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -3,6 +3,7 @@ package PVE::API2::Qemu;
>  use strict;
>  use warnings;
>  use Cwd 'abs_path';
> +use File::stat qw();
>  use Net::SSLeay;
>  use IO::Socket::IP;
>  use IO::Socket::UNIX;
> @@ -5367,6 +5368,52 @@ __PACKAGE__->register_method({
>      },
>  });
>  
> +my sub migrate_params_filename {

These helpers are closely connected to the API call, but I'm still
wondering if it's not better to put them in the
PVE::QemuMigrate::Helpers module.

> +    my ($vmid) = @_;
> +    return "/run/qemu-server/$vmid.migrate_params";
> +}
> +
> +my sub save_migrate_params {
> +    my ($vmid, $params) = @_;
> +
> +    my $migrate_params_file = migrate_params_filename($vmid);
> +
> +    warn "existing migration parameters file for '$vmid' will be overwritten\n"
> +        if -f $migrate_params_file;
> +
> +    PVE::Tools::file_set_contents($migrate_params_file, encode_json($params), 0640);

Please use PVE::File for new usages

> +}
> +
> +my sub try_to_restore_migrate_params {
> +    my ($vmid, $params) = @_;
> +
> +    my $migrate_params_file = migrate_params_filename($vmid);
> +    my @migrate_params_denylist = qw(node vmid target online force with-local-disks targetstorage);

Nit: it's rather 'skip' than 'deny' and using a hash and avoiding the
grep would be slightly simpler

> +
> +    if (-f $migrate_params_file) {
> +        my $stat = File::stat::lstat($migrate_params_file);
> +        # prevent that non-root users could write the migrate parameters file
> +        my $has_correct_perms = $stat->uid == 0 && ($stat->mode & 037) == 0;

Usually, we rely on the fact that the containing directory already has
the correct permissions to prevent unprivileged users from writing files
there and when you create the file, you also don't allow writes from
other users, so it should already be fine. If we do go for this, then
not having correct permissions should produce a dedicated warning.

> +
> +        if (PVE::HA::Config::vm_is_ha_managed($vmid) && $has_correct_perms) {

I'd prefer if we could check for the rpcenv type here too

> +            my $migration_params = {};
> +            eval {
> +                my $data = PVE::Tools::file_get_contents($migrate_params_file);

Please use PVE::File for new usages

> +                $migration_params = decode_json($data) // {};
> +            };

Missing error handling here.

> +            for my $key (keys %$migration_params) {
> +                next if grep { $key eq $_ } @migrate_params_denylist;
> +
> +                $params->{$key} = $migration_params->{$key};
> +            }
> +        } else {
> +            warn "remove orphan migration parameters file\n";
> +        }
> +
> +        unlink $migrate_params_file;

Nit: the return value could be checked while ignoring ENOENT, we have
quite a few examples of this

> +    }
> +}
> +
>  __PACKAGE__->register_method({
>      name => 'migrate_vm',
>      path => '{vmid}/migrate',
> @@ -5464,6 +5511,13 @@ __PACKAGE__->register_method({
>  
>          my $vmid = extract_param($param, 'vmid');
>  
> +        if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
> +            save_migrate_params($vmid, $param);
> +        } else {
> +            # always try to restore to remove oprhaned migration parameters files

Typo: 'oprhaned'

> +            try_to_restore_migrate_params($vmid, $param);

I'd prefer if the naming would mention that it can only have an effect
for HA, maybe restore_migrate_params_for_ha()?

Not sure if a dedicated helper for removing makes sense to better
organize it? Then we could call the restore one only if ha_managed and
rpcenv type eq 'ha'

> +        }
> +
>          raise_param_exc({ force => "Only root may use this option." })
>              if $param->{force} && $authuser ne 'root@pam';
>  

[0]: https://en.wiktionary.org/wiki/initial#Verb