Re: [pve-devel] [PATCH v5 qemu-server 08/11] migrate: refactor remote VM/tunnel start

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

Am 09.02.22 um 14:07 schrieb Fabian Grünbichler:
> no semantic changes intended, except for:
> - no longer passing the main migration UNIX socket to SSH twice for
> forwarding
> - dropping the 'unix:' prefix in start_remote_tunnel's timeout error message
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  PVE/QemuMigrate.pm | 158 ++++++++++++++++++++++++++++-----------------
>  PVE/QemuServer.pm  |  34 +++++-----
>  2 files changed, 113 insertions(+), 79 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 104e62ce..e6cb7e79 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -43,19 +43,24 @@ sub fork_tunnel {
>      return PVE::Tunnel::fork_ssh_tunnel($self->{rem_ssh}, $cmd, $ssh_forward_info, $log);
>  }
>  
> +# tunnel_info:
> +#   proto: unix (secure) or tcp (insecure/legacy compat)
> +#   addr: IP or UNIX socket path
> +#   port: optional TCP port
> +#   unix_sockets: additional UNIX socket paths to forward
>  sub start_remote_tunnel {
> -    my ($self, $raddr, $rport, $ruri, $unix_socket_info) = @_;
> +    my ($self, $tunnel_info) = @_;
>  
>      my $nodename = PVE::INotify::nodename();
>      my $migration_type = $self->{opts}->{migration_type};
>  
>      if ($migration_type eq 'secure') {
>  
> -	if ($ruri =~ /^unix:/) {
> -	    my $ssh_forward_info = ["$raddr:$raddr"];
> -	    $unix_socket_info->{$raddr} = 1;
> +	if ($tunnel_info->{proto} eq 'unix') {
> +	    my $ssh_forward_info = [];
>  
> -	    my $unix_sockets = [ keys %$unix_socket_info ];
> +	    my $unix_sockets = [ keys %{$tunnel_info->{unix_sockets}} ];
> +	    push @$unix_sockets, $tunnel_info->{addr};
>  	    for my $sock (@$unix_sockets) {
>  		push @$ssh_forward_info, "$sock:$sock";
>  		unlink $sock;
> @@ -82,23 +87,23 @@ sub start_remote_tunnel {
>  	    if ($unix_socket_try > 100) {
>  		$self->{errors} = 1;
>  		PVE::Tunnel::finish_tunnel($self->{tunnel});
> -		die "Timeout, migration socket $ruri did not get ready";
> +		die "Timeout, migration socket $tunnel_info->{addr} did not get ready";
>  	    }
>  	    $self->{tunnel}->{unix_sockets} = $unix_sockets if (@$unix_sockets);
>  
> -	} elsif ($ruri =~ /^tcp:/) {
> +	} elsif ($tunnel_info->{proto} eq 'tcp') {
>  	    my $ssh_forward_info = [];
> -	    if ($raddr eq "localhost") {
> +	    if ($tunnel_info->{addr} eq "localhost") {
>  		# for backwards compatibility with older qemu-server versions
>  		my $pfamily = PVE::Tools::get_host_address_family($nodename);
>  		my $lport = PVE::Tools::next_migrate_port($pfamily);
> -		push @$ssh_forward_info, "$lport:localhost:$rport";
> +		push @$ssh_forward_info, "$lport:localhost:$tunnel_info->{rport}";

Should be $tunnel_info->{port}

>  	    }
>  
>  	    $self->{tunnel} = $self->fork_tunnel($ssh_forward_info);
>  
>  	} else {
> -	    die "unsupported protocol in migration URI: $ruri\n";
> +	    die "unsupported protocol in migration URI: $tunnel_info->{proto}\n";
>  	}
>      } else {
>  	#fork tunnel for insecure migration, to send faster commands like resume
> @@ -650,52 +655,40 @@ sub phase1_cleanup {
>      }
>  }
>  
> -sub phase2 {
> -    my ($self, $vmid) = @_;
> +sub phase2_start_local_cluster {
> +    my ($self, $vmid, $params) = @_;
>  
>      my $conf = $self->{vmconf};
>      my $local_volumes = $self->{local_volumes};
>      my @online_local_volumes = $self->filter_local_volumes('online');
>  
>      $self->{storage_migration} = 1 if scalar(@online_local_volumes);
> +    my $start = $params->{start_params};
> +    my $migrate = $params->{migrate_opts};
>  
>      $self->log('info', "starting VM $vmid on remote node '$self->{node}'");
>  
> -    my $raddr;
> -    my $rport;
> -    my $ruri; # the whole migration dst. URI (protocol:address[:port])
> -    my $nodename = PVE::INotify::nodename();
> +    my $tunnel_info = {};
>  
>      ## start on remote node
>      my $cmd = [@{$self->{rem_ssh}}];
>  
> -    my $spice_ticket;
> -    if (PVE::QemuServer::vga_conf_has_spice($conf->{vga})) {
> -	my $res = mon_cmd($vmid, 'query-spice');
> -	$spice_ticket = $res->{ticket};
> -    }
> +    push @$cmd, 'qm', 'start', $vmid, '--skiplock';

Nit: the parameter $start->{skiplock} that's passed in is ignored
(although it is always 1 currently)

> +    push @$cmd, '--migratedfrom', $migrate->{migratedfrom};
>  
> -    push @$cmd , 'qm', 'start', $vmid, '--skiplock', '--migratedfrom', $nodename;
> +    push @$cmd, '--migration_type', $migrate->{type};
>  
> -    my $migration_type = $self->{opts}->{migration_type};
> +    push @$cmd, '--migration_network', $migrate->{network}
> +      if $migrate->{network};
>  
> -    push @$cmd, '--migration_type', $migration_type;
> +    push @$cmd, '--stateuri', $start->{statefile};
>  
> -    push @$cmd, '--migration_network', $self->{opts}->{migration_network}
> -      if $self->{opts}->{migration_network};
> -
> -    if ($migration_type eq 'insecure') {
> -	push @$cmd, '--stateuri', 'tcp';
> -    } else {
> -	push @$cmd, '--stateuri', 'unix';
> +    if ($start->{forcemachine}) {
> +	push @$cmd, '--machine', $start->{forcemachine};
>      }
>  
> -    if ($self->{forcemachine}) {
> -	push @$cmd, '--machine', $self->{forcemachine};
> -    }
> -
> -    if ($self->{forcecpu}) {
> -	push @$cmd, '--force-cpu', $self->{forcecpu};
> +    if ($start->{forcecpu}) {
> +	push @$cmd, '--force-cpu', $start->{forcecpu};
>      }
>  
>      if ($self->{storage_migration}) {