public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC qemu-server 1/1] partially fix #4501: migration: start vm: move port reservation and usage closer together
Date: Wed, 15 Nov 2023 11:12:45 +0100	[thread overview]
Message-ID: <gkypmq6cjf4c5dddd3a5c66crwmv3m4d4ssaodfqupi6bzhfjj@vwrkzvhkqb32> (raw)
In-Reply-To: <1700038325.l2cibe7y7o.astroid@yuna.none>

On Wed, Nov 15, 2023 at 09:55:22AM +0100, Fabian Grünbichler wrote:
> 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

What about adding an option to `next_migrate_port()` to actually return
the open socket to keep the reservation?

Also, did we consider passing the file descriptor through to qemu via
`-incoming fd:$number`?

> 
> > +	    $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




  reply	other threads:[~2023-11-15 10:12 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-14 14:02 [pve-devel] [RFC qemu-server/common] fix #4501: improve port reservation for QEMU TCP migration Fiona Ebner
2023-11-14 14:02 ` [pve-devel] [RFC qemu-server 1/1] partially fix #4501: migration: start vm: move port reservation and usage closer together Fiona Ebner
2023-11-15  8:55   ` Fabian Grünbichler
2023-11-15 10:12     ` Wolfgang Bumiller [this message]
2023-11-15 10:22       ` Fiona Ebner
2023-11-15 11:21         ` Wolfgang Bumiller
2023-11-14 14:02 ` [pve-devel] [RFC common 1/2] partially fix #4501: next unused port: bump port reservation expiretime Fiona Ebner
2023-11-15  8:51   ` Fabian Grünbichler
2023-11-14 14:02 ` [pve-devel] [RFC common 2/2] fix #4501: next unused port: work around issue with too short expiretime Fiona Ebner
2023-11-14 14:13   ` Fiona Ebner
2023-11-15  8:51     ` Fabian Grünbichler
2023-11-15 10:16       ` Fiona Ebner
2023-11-15 10:27         ` Fabian Grünbichler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=gkypmq6cjf4c5dddd3a5c66crwmv3m4d4ssaodfqupi6bzhfjj@vwrkzvhkqb32 \
    --to=w.bumiller@proxmox.com \
    --cc=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal