On April 14, 2020 11:45 am, Mira Limbeck wrote:
> Looks good to me.
>
> Reviewed-By: Mira Limbeck <m.limb...@proxmox.com>
> Tested-By: Mira Limbeck <m.limb...@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.gruenbich...@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};
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
