[pve-devel] [PATCH v2 qemu-server 4/4] add unix socket support for NBD storage migration

[Fabian Grünbichler]

On March 18, 2020 10:02 am, Stefan Reiter wrote:
> On 17/03/2020 20:56, Mira Limbeck wrote:
>> The reuse of the tunnel, which we're opening to communicate with the target
>> node and to forward the unix socket for the state migration, for the NBD unix
>> socket requires adding support for an array of sockets to forward, not just a
>> single one. We also have to change the $sock_addr variable to an array
>> for the cleanup of the socket file as SSH does not remove the file.
>> 
>> To communicate to the target node the support of unix sockets for NBD
>> storage migration, we're specifying an nbd_protocol_version which is set
>> to 1. This version is then passed to the target node via STDIN. Because
>> we don't want to be dependent on the order of arguments being passed
>> via STDIN, we also prefix the spice ticket with 'spice_ticket: '. The
>> target side handles both the spice ticket and the nbd protocol version
>> with a fallback for old source nodes passing the spice ticket without a
>> prefix.
>> All arguments are line based and require a newline in between.
>> 
>> When the NBD server on the target node is started with a unix socket, we
>> get a different line containing all the information required to start
>> the drive-mirror. This contains the unix socket path used on the target node
>> which we require for forwarding and cleanup.
>> 
>> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
>> ---
>> v2:
>>   - added 'spice_ticket: (...)' to input if $spice_ticket is defined
>>   - added waiting for all sockets that are used in the tunnel
>> 
>>   PVE/QemuMigrate.pm | 52 +++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 42 insertions(+), 10 deletions(-)
>> 
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 10c0ff2..50ebd77 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -142,7 +142,10 @@ sub write_tunnel {
>>   sub fork_tunnel {
>>       my ($self, $tunnel_addr) = @_;
>>   
>> -    my @localtunnelinfo = defined($tunnel_addr) ? ('-L' , $tunnel_addr ) : ();
>> +    my @localtunnelinfo = ();
>> +    foreach my $addr (@$tunnel_addr) {
>> +	push @localtunnelinfo, '-L', $addr;
>> +    }
>>   
>>       my $cmd = [@{$self->{rem_ssh}}, '-o ExitOnForwardFailure=yes', @localtunnelinfo, '/usr/sbin/qm', 'mtunnel' ];
>>   
>> @@ -184,7 +187,7 @@ sub finish_tunnel {
>>   
>>       if ($tunnel->{sock_addr}) {
>>   	# ssh does not clean up on local host
>> -	my $cmd = ['rm', '-f', $tunnel->{sock_addr}]; #
>> +	my $cmd = ['rm', '-f', @{$tunnel->{sock_addr}}]; #
>>   	PVE::Tools::run_command($cmd);
>>   
>>   	# .. and just to be sure check on remote side
>> @@ -594,10 +597,16 @@ sub phase2 {
>>       }
>>   
>>       my $spice_port;
>> +    my $tunnel_addr = [];
>> +    my $sock_addr = [];
>> +    # version > 0 for unix socket support
>> +    my $nbd_protocol_version = 1;
>> +    my $input = "nbd_protocol_version: $nbd_protocol_version\n";
>> +    $input .= "spice_ticket: $spice_ticket\n" if $spice_ticket;
> 
> I know it's already been applied, but this breaks new->old migration 
> (with SPICE) for no reason, doesn't it? I know we don't always support 
> it, but I don't see why we need the break here..
> 
> I.e. if we just send the spice-ticket w/o a prefix an old node will be 
> perfectly happy and just send us back the TCP migration URI (and with 
> the fallback in place a new node will also be happy), but if we do 
> prefix it the remote will pick up the entire "spice_ticket: xxx" as the 
> ticket.

yes, we could revert that back to the old behaviour and add a FIXME: add 
prefix 'spice_ticket: ' with 7.0?

> 
>>   
>>       # Note: We try to keep $spice_ticket secret (do not pass via command line parameter)
>>       # instead we pipe it through STDIN
>> -    my $exitcode = PVE::Tools::run_command($cmd, input => $spice_ticket, outfunc => sub {
>> +    my $exitcode = PVE::Tools::run_command($cmd, input => $input, outfunc => sub {
>>   	my $line = shift;
>>   
>>   	if ($line =~ m/^migration listens on tcp:(localhost|[\d\.]+|\[[\d\.:a-fA-F]+\]):(\d+)$/) {
>> @@ -626,7 +635,18 @@ sub phase2 {
>>   
>>   	    $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
>>   	    $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
>> +	} elsif ($line =~ m!^storage migration listens on nbd:unix:(/run/qemu-server/(\d+)_nbd\.migrate):exportname=(\S+) volume:(\S+)$!) {
>> +	    my $drivestr = $4;
>> +	    die "Destination UNIX socket's VMID does not match source VMID" if $vmid ne $2;
>> +	    my $nbd_unix_addr = $1;
>> +	    my $nbd_uri = "nbd:unix:$nbd_unix_addr:exportname=$3";
>> +	    my $targetdrive = $3;
>> +	    $targetdrive =~ s/drive-//g;
>>   
>> +	    $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;
>>   	} elsif ($line =~ m/^QEMU: (.*)$/) {
>>   	    $self->log('info', "[$self->{node}] $1\n");
>>   	}
>> @@ -645,20 +665,31 @@ sub phase2 {
>>   
>>   	if ($ruri =~ /^unix:/) {
>>   	    unlink $raddr;
>> -	    $self->{tunnel} = $self->fork_tunnel("$raddr:$raddr");
>> -	    $self->{tunnel}->{sock_addr} = $raddr;
>> +	    push @$tunnel_addr, "$raddr:$raddr";
>> +	    $self->{tunnel} = $self->fork_tunnel($tunnel_addr);
>> +	    push @$sock_addr, $raddr;
>>   
>>   	    my $unix_socket_try = 0; # wait for the socket to become ready
>> -	    while (! -S $raddr) {
>> +	    while ($unix_socket_try <= 100) {
>>   		$unix_socket_try++;
>> -		if ($unix_socket_try > 100) {
>> -		    $self->{errors} = 1;
>> -		    $self->finish_tunnel($self->{tunnel});
>> -		    die "Timeout, migration socket $ruri did not get ready";
>> +		my $available = 0;
>> +		foreach my $sock (@$sock_addr) {
>> +		    if (-S $sock) {
>> +			$available++;
>> +		    }
>> +		}
>> +
>> +		if ($available == @$sock_addr) {
>> +		    last;
>>   		}
>>   
>>   		usleep(50000);
>>   	    }
>> +	    if ($unix_socket_try > 100) {
>> +		$self->{errors} = 1;
>> +		$self->finish_tunnel($self->{tunnel});
>> +		die "Timeout, migration socket $ruri did not get ready";
>> +	    }
>>   
>>   	} elsif ($ruri =~ /^tcp:/) {
>>   	    my $tunnel_addr;
>> @@ -678,6 +709,7 @@ 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();
>>   
>> 
>