Re: [pve-devel] [RFC qemu-server 1/1] partially fix #4501: migration: start vm: move port reservation and usage closer together

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 14, 2023 3:02 pm, Fiona Ebner wrote:
> Currently, volume activation, PCI reservation and resetting systemd
> scope happen in between and the 5 second expiretime used for port
> reservation might not be enough.
> 
> Still not ideal, because entering systemd scope and maybe starting
> swtpm still happen after reservation before the QEMU binary can be
> invoked and actually use the port, but the reservation needs to happen
> outside of the fork, because the result is used there too.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Acked-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

we could move the whole statefile handling further down, but then some
additional side-effects need to be taken care of/refactored, this seems
like a minimal-invasive version for the uncommon (insecure) case.

> ---
>  PVE/QemuServer.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index c465fb6f..aeaea8eb 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5697,6 +5697,9 @@ sub vm_start_nolock {
>  	return $migration_ip;
>      };
>  
> +    # helper to move port reservation and usage closer together to avoid expiry (bug #4501)
> +    my $append_tcp_migration_cmdline;
> +
>      if ($statefile) {
>  	if ($statefile eq 'tcp') {
>  	    my $migrate = $res->{migrate} = { proto => 'tcp' };
> @@ -5717,12 +5720,13 @@ sub vm_start_nolock {
>  		$migrate->{addr} = "[$migrate->{addr}]" if Net::IP::ip_is_ipv6($migrate->{addr});
>  	    }
>  
> -	    my $pfamily = PVE::Tools::get_host_address_family($nodename);
> -	    $migrate->{port} = PVE::Tools::next_migrate_port($pfamily);
> -	    $migrate->{uri} = "tcp:$migrate->{addr}:$migrate->{port}";
> -	    push @$cmd, '-incoming', $migrate->{uri};
> -	    push @$cmd, '-S';
> -

nit: I'd maybe add another comment here, maybe something like

# delay migration port reservation to prevent expiry before binding

> +	    $append_tcp_migration_cmdline = sub {
> +		my $pfamily = PVE::Tools::get_host_address_family($nodename);
> +		$migrate->{port} = PVE::Tools::next_migrate_port($pfamily);
> +		$migrate->{uri} = "tcp:$migrate->{addr}:$migrate->{port}";
> +		push @$cmd, '-incoming', $migrate->{uri};
> +		push @$cmd, '-S';
> +	    };
>  	} elsif ($statefile eq 'unix') {
>  	    # should be default for secure migrations as a ssh TCP forward
>  	    # tunnel is not deterministic reliable ready and fails regurarly
> @@ -5840,6 +5844,10 @@ sub vm_start_nolock {
>      $systemd_properties{timeout} = 10 if $statefile; # setting up the scope shoul be quick
>  
>      my $run_qemu = sub {
> +	# sets the port+uri for $res->{migrate} which is printed below and part of the result, so
> +	# needs to happen outside of the fork.
> +	$append_tcp_migration_cmdline->() if $append_tcp_migration_cmdline;
> +
>  	PVE::Tools::run_fork sub {
>  	    PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid", %systemd_properties);
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>