Re: [pve-devel] [PATCH guest-common 1/1] migrate: handle migration_network with remote migration

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

Am 05.11.21 um 14:03 schrieb Fabian Grünbichler:
> we only want to use an explicitly provided migration network, not one
> for the local cluster.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   src/PVE/AbstractMigrate.pm | 51 +++++++++++++++++++++++---------------
>   1 file changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/src/PVE/AbstractMigrate.pm b/src/PVE/AbstractMigrate.pm
> index af2be38..ec60b82 100644
> --- a/src/PVE/AbstractMigrate.pm
> +++ b/src/PVE/AbstractMigrate.pm
> @@ -115,22 +115,27 @@ sub migrate {
>   
>       $class = ref($class) || $class;
>   
> -    my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
> +    my ($ssh_info, $rem_ssh);
> +    if (!$opts->{remote}) {
> +	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>   
> -    my $migration_network = $opts->{migration_network};
> -    if (!defined($migration_network)) {
> -	$migration_network = $dc_conf->{migration}->{network};
> -    }
> -    my $ssh_info = PVE::SSHInfo::get_ssh_info($node, $migration_network);
> -    $nodeip = $ssh_info->{ip};
> -
> -    my $migration_type = 'secure';
> -    if (defined($opts->{migration_type})) {
> -	$migration_type = $opts->{migration_type};
> -    } elsif (defined($dc_conf->{migration}->{type})) {
> -        $migration_type = $dc_conf->{migration}->{type};
> +	my $migration_network = $opts->{migration_network};
> +	if (!defined($migration_network)) {
> +	    $migration_network = $dc_conf->{migration}->{network};
> +	}
> +	$ssh_info = PVE::SSHInfo::get_ssh_info($node, $migration_network);
> +	$nodeip = $ssh_info->{ip};
> +
> +	my $migration_type = 'secure';
> +	if (defined($opts->{migration_type})) {
> +	    $migration_type = $opts->{migration_type};
> +	} elsif (defined($dc_conf->{migration}->{type})) {
> +	    $migration_type = $dc_conf->{migration}->{type};
> +	}
> +	$opts->{migration_type} = $migration_type;
> +	$opts->{migration_network} = $migration_network;
> +	$rem_ssh = PVE::SSHInfo::ssh_info_to_command($ssh_info);
>       }
> -    $opts->{migration_type} = $migration_type;
>   
>       my $self = {
>   	delayed_interrupt => 0,
> @@ -139,7 +144,7 @@ sub migrate {
>   	node => $node,
>   	ssh_info => $ssh_info,
>   	nodeip => $nodeip,
> -	rem_ssh => PVE::SSHInfo::ssh_info_to_command($ssh_info)
> +	rem_ssh => $rem_ssh,
>       };
>   
>       $self = bless $self, $class;
> @@ -162,15 +167,21 @@ sub migrate {
>   	&$eval_int($self, sub { $self->{running} = $self->prepare($self->{vmid}); });
>   	die $@ if $@;
>   
> -	if (defined($migration_network)) {
> +	if (defined($self->{opts}->{migration_network})) {
>   	    $self->log('info', "use dedicated network address for sending " .
>   	               "migration traffic ($self->{nodeip})");
>   
>   	    # test if we can connect to new IP
> -	    my $cmd = [ @{$self->{rem_ssh}}, '/bin/true' ];
> -	    eval { $self->cmd_quiet($cmd); };
> -	    die "Can't connect to destination address ($self->{nodeip}) using " .
> -	        "public key authentication\n" if $@;
> +	    if ($self->{opts}->{remote}) {
> +		eval { $self->{opts}->{remote}->{client}->get("/") };

Shouldn't the check here happen regardless of whether a 
migration_network is defined? Actually, isn't the same true for the 
existing check too?

> +		die "Can't connect to destination address ($self->{nodeip}) using " .
> +		    "API connection - $@\n" if $@;
> +	    } else {
> +		my $cmd = [ @{$self->{rem_ssh}}, '/bin/true' ];
> +		eval { $self->cmd_quiet($cmd); };
> +		die "Can't connect to destination address ($self->{nodeip}) using " .
> +		    "public key authentication\n" if $@;
> +	    }
>   	}
>   
>   	&$eval_int($self, sub { $self->phase1($self->{vmid}); });
>