[pve-devel] [PATCH qemu-server] migrate: cleanup forwarding code

[Fabian Grünbichler]

On April 14, 2020 11:45 am, Mira Limbeck wrote:
> Looks good to me.
> 
> Reviewed-By: Mira Limbeck <m.limbeck at proxmox.com>
> Tested-By: Mira Limbeck <m.limbeck at proxmox.com>
> 
> On 4/14/20 10:51 AM, Fabian Grünbichler wrote:
>> fixing the following two issues:
>> - the legacy code path was never converted to the new fork_tunnel
>> signature (which probably means that nothing triggers it in practice
>> anymore?)
> 
> Do you mean the 'my $tunnel_addr' in the TCP case? That was an oversight 
> on my part, the 'my' should have been removed.

not only the 'my' - in that code path, $tunnel_addr was a plain string, 
not a list of strings which fork_tunnel expects. so migration to a 
qemu-server that does localhost TCP migration was broken since that 
change. it only affects new->rather old[1] though, which is probably why 
noone noticed ;)

we could probably also drop that codepath altogether on both sides now?

1: the switch to UNIX sockets happened in 2016

> But it should not be triggered in practice and not together with nbd 
> unix socket forwarding.
> 
>> - the NBD Unix socket got forwarded multiple times if more than one disk
>> was migrated via NBD (this is harmless, but wrong)
>>
>> for the second issue I opted to keep the code compatible with the
>> possibility that Qemu starts supporting multiple NBD servers in the
>> future (and the target node could thus return multiple UNIX socket
>> paths). currently we can only start one NBD server on one socket, and
>> each drive-mirror simply starts a new connection over that single
>> socket.
>>
>> I took the liberty of renaming the variables/keys since I found
>> 'tunnel_addr' and 'sock_addr' rather confusing.
>>
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
>> ---
>> tested:
>> - migration with multiple disks with and without replication
>> - same, but forced migration and NBD to use TCP (stateuri == tcp,
>>    nbd_protocol_version == 0)
>> - insecure migration
>>
>> alternatively, we could push the whole unix socket handling including
>> waiting for readyness into fork_tunnel, and just pass it the list of
>> unix sockets, and a list of extra forwarding strings (the latter being
>> used for the legacy TCP migration uri), with both being optional?
>>
>>   PVE/QemuMigrate.pm | 41 ++++++++++++++++++++++-------------------
>>   1 file changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 0a6277d..829d84b 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -146,10 +146,10 @@ sub write_tunnel {
>>   }
>>   
>>   sub fork_tunnel {
>> -    my ($self, $tunnel_addr) = @_;
>> +    my ($self, $ssh_forward_info) = @_;
>>   
>>       my @localtunnelinfo = ();
>> -    foreach my $addr (@$tunnel_addr) {
>> +    foreach my $addr (@$ssh_forward_info) {
>>   	push @localtunnelinfo, '-L', $addr;
>>       }
>>   
>> @@ -191,9 +191,9 @@ sub finish_tunnel {
>>   
>>       $self->finish_command_pipe($tunnel, 30);
>>   
>> -    if ($tunnel->{sock_addr}) {
>> +    if (my $unix_sockets = $tunnel->{unix_sockets}) {
>>   	# ssh does not clean up on local host
>> -	my $cmd = ['rm', '-f', @{$tunnel->{sock_addr}}]; #
>> +	my $cmd = ['rm', '-f', @$unix_sockets];
>>   	PVE::Tools::run_command($cmd);
>>   
>>   	# .. and just to be sure check on remote side
>> @@ -701,8 +701,7 @@ sub phase2 {
>>       }
>>   
>>       my $spice_port;
>> -    my $tunnel_addr = [];
>> -    my $sock_addr = [];
>> +    my $unix_socket_info = {};
>>       # version > 0 for unix socket support
>>       my $nbd_protocol_version = 1;
>>       # TODO change to 'spice_ticket: <ticket>\n' in 7.0
>> @@ -755,8 +754,7 @@ sub phase2 {
>>   
>>   	    $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
>>   	    $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
>> -	    push @$tunnel_addr, "$nbd_unix_addr:$nbd_unix_addr";
>> -	    push @$sock_addr, $nbd_unix_addr;
>> +	    $unix_socket_info->{$nbd_unix_addr} = 1;
>>   	} elsif ($line =~ m/^re-using replicated volume: (\S+) - (.*)$/) {
>>   	    my $drive = $1;
>>   	    my $volid = $2;
>> @@ -782,22 +780,28 @@ sub phase2 {
>>       if ($migration_type eq 'secure') {
>>   
>>   	if ($ruri =~ /^unix:/) {
>> -	    unlink $raddr;
>> -	    push @$tunnel_addr, "$raddr:$raddr";
>> -	    $self->{tunnel} = $self->fork_tunnel($tunnel_addr);
>> -	    push @$sock_addr, $raddr;
>> +	    my $ssh_forward_info = ["$raddr:$raddr"];
>> +	    $unix_socket_info->{$raddr} = 1;
>> +
>> +	    my $unix_sockets = [ keys %$unix_socket_info ];
>> +	    for my $sock (@$unix_sockets) {
>> +		push @$ssh_forward_info, "$sock:$sock";
>> +		unlink $sock;
>> +	    }
>> +
>> +	    $self->{tunnel} = $self->fork_tunnel($ssh_forward_info);
>>   
>>   	    my $unix_socket_try = 0; # wait for the socket to become ready
>>   	    while ($unix_socket_try <= 100) {
>>   		$unix_socket_try++;
>>   		my $available = 0;
>> -		foreach my $sock (@$sock_addr) {
>> +		foreach my $sock (@$unix_sockets) {
>>   		    if (-S $sock) {
>>   			$available++;
>>   		    }
>>   		}
>>   
>> -		if ($available == @$sock_addr) {
>> +		if ($available == @$unix_sockets) {
>>   		    last;
>>   		}
>>   
>> @@ -808,17 +812,18 @@ sub phase2 {
>>   		$self->finish_tunnel($self->{tunnel});
>>   		die "Timeout, migration socket $ruri did not get ready";
>>   	    }
>> +	    $self->{tunnel}->{unix_sockets} = $unix_sockets if (@$unix_sockets);
>>   
>>   	} elsif ($ruri =~ /^tcp:/) {
>> -	    my $tunnel_addr;
>> +	    my $ssh_forward_info = [];
>>   	    if ($raddr 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);
>> -		$tunnel_addr = "$lport:localhost:$rport";
>> +		push @$ssh_forward_info, "$lport:localhost:$rport";
>>   	    }
>>   
>> -	    $self->{tunnel} = $self->fork_tunnel($tunnel_addr);
>> +	    $self->{tunnel} = $self->fork_tunnel($ssh_forward_info);
>>   
>>   	} else {
>>   	    die "unsupported protocol in migration URI: $ruri\n";
>> @@ -827,8 +832,6 @@ sub phase2 {
>>   	#fork tunnel for insecure migration, to send faster commands like resume
>>   	$self->{tunnel} = $self->fork_tunnel();
>>       }
>> -    $self->{tunnel}->{sock_addr} = $sock_addr if (@$sock_addr);
>> -
>>       my $start = time();
>>   
>>       my $opt_bwlimit = $self->{opts}->{bwlimit};
>