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

[Mira Limbeck]

On 4/14/20 12:49 PM, Fabian Grünbichler wrote:
> 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
Yes, right. Should be fine as we don't support PVE 4.X and PVE 6.X in 
the same cluster. There's no way live migration could be used then.
>
>> 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};