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.limb...@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();
>>
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
