Re: [PATCH] treewide: By default, don't quit source after migration, keep sockets open

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 23 Jul 2025 10:27:30 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jul 22, 2025 at 11:12:49PM +0200, Stefano Brivio wrote:
> > On Tue, 22 Jul 2025 10:33:00 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Jul 22, 2025 at 12:12:58AM +0200, Stefano Brivio wrote:  
> > > > We are hitting an issue in the KubeVirt integration where some data is
> > > > still sent to the source instance even after migration is complete. As
> > > > we exit, the kernel closes our sockets and resets connections. The
> > > > resulting RST segments are sent to peers, effectively terminating
> > > > connections that were meanwhile migrated.
> > > > 
> > > > At the moment, this is not done intentionally, but in the future
> > > > KubeVirt might enable OVN-Kubernetes features where source and
> > > > destination nodes are explicitly getting mirrored traffic for a while,
> > > > in order to decrease migration downtime.
> > > > 
> > > > By default, don't quit after migration is completed on the source: the
> > > > previous behaviour can be enabled with the new --migrate-exit
> > > > option.    
> > > 
> > > Might be worth explicitly mentioning that the reason this helps is
> > > that having the sockets still around in repair mode prevents the
> > > source node from responding to any traffic for them.  
> > 
> > Added as separate paragraph, below.  
> 
> Thanks.
> 
> [snip]
> > > > diff --git a/conf.c b/conf.c
> > > > index 6c747aa..5e69014 100644
> > > > --- a/conf.c
> > > > +++ b/conf.c
> > > > @@ -864,6 +864,12 @@ static void usage(const char *name, FILE *f, int status)
> > > >  		FPRINTF(f,
> > > >  			"  --repair-path PATH	path for passt-repair(1)\n"
> > > >  			"    default: append '.repair' to UNIX domain path\n");
> > > > +		FPRINTF(f,
> > > > +			"  --migrate-exit	source quits after migration\n"
> > > > +			"    default: source keeps running after migration\n");
> > > > +		FPRINTF(f,
> > > > +			"  --migrate-no-linger	close sockets on migration\n"
> > > > +			"    default: keep sockets open, ignore data events\n");    
> > > 
> > > I'm a bit uncomfortable adding this as regular, supported options.
> > > I'm hoping we can eliminate them in the not too distant future.  In
> > > the medium term we can do that by improving our test code to simulate
> > > different hosts with namespaces.  Longer term we can allow local to
> > > local migration without these options by fixing the kernel: I don't
> > > believe bind() should fail if the address conflict is with a socket in
> > > repair mode.  
> > 
> > I see the point about --migrate-no-linger, but --migrate-exit looks
> > like something that depends on the migration workflow, not necessarily
> > something to cover up issues in our tests or in the kernel. In any
> > case...  
> 
> It does depend on the workflow, but I think this needs to be managed
> by qemu (or whatever upper level component is managing the overall
> migration).  --migrate-exit doesn't make us exit once the migration is
> complete - it makes us exit once *our part of* the migration is
> complete, the migration could still fail after that.  So in the happy
> path, yes, we'll exit, but it should be because qemu or libvirt or
> whatever is satisfied the migration is finished and kills us off
> 
> > > Further, as user accessible options these are kinda weird - what they
> > > do is defined in terms of some pretty obscure internals, not in terms
> > > of what the upshot is.  So, I think it would be better to introduce
> > > these as deprecated / debug-only / undocumented options.  
> > 
> > ...I marked both as deprecated in v2, to signal to any hypothetical
> > (I'm fairly sure not real) user that they shouldn't assume it's safe to
> > rely on them.
> > 
> > That's the only other category of options we have so far, we don't have
> > undocumented ones, and I think it's a feature. Especially as we use
> > them in the tests.  
> 
> Works for me.
> 
> > By the way, I would argue that testing is an important type of usage,
> > and changes in the test setup itself or in the kernel are non-trivial
> > effort, and it's rather uncertain whether we'll ever manage to commit
> > to that, so I don't quite feel that marking them as deprecated is
> > entirely warranted, but I see the point about possibly dropping that
> > migration model at some point in the future.
> > 
> > Regardless, the fact we *need* them at the moment should prove that
> > they are at least convenient, even if just for testing.  
> 
> Fair enough.
> 
> [snip]
> > > > diff --git a/passt.c b/passt.c
> > > > index 388d10f..f4c108f 100644
> > > > --- a/passt.c
> > > > +++ b/passt.c
> > > > @@ -308,6 +308,9 @@ loop:
> > > >  		      c.mode == MODE_PASTA ? "pasta" : "passt",
> > > >  		      EPOLL_TYPE_STR(ref.type), ref.fd, eventmask);
> > > >  
> > > > +		if (c.ignore_data_events && ref.type <= EPOLL_TYPE_DATA_MAX)
> > > > +			continue;
> > > > +    
> > > 
> > > This worries me slightly.  I think it will be fine if passt is killed
> > > not long after the migration.  However, if passt lingers, we're
> > > ignoring, but not clearing events.  For those events which are
> > > level-triggered this could make us chew CPU on a tight epoll_wait()
> > > loop.  
> > 
> > Ah, yes, I had that concern as well, but I conveniently forgot about it
> > at some point.
> >   
> > > I think a preferable approach would be to EPOLL_CTL_DEL each socket at
> > > the point we would close() them with --migrate-no-linger.  I'd be fine
> > > with that as a follow up improvement, though.  
> > 
> > I was pondering about adding this on top of the ignore_data_events
> > trick, but, actually, the whole thing as of this patch is somewhat
> > bogus because
> > 
> > - we're ignoring events on TCP sockets (intentional),
> > 
> > - we're ignoring events on the tap device (who cares, migration is only
> >   supported with vhost-user)
> > 
> > - but *not* ignoring events on the vhost-user kick descriptor
> >   (oversight).
> > 
> > On a second thought, it doesn't look safe to ignore events on the kick
> > descriptor, and in any case, with this change, we don't want to prevent
> > the guest to send out further packets. It's not expected anyway.
> > 
> > So I just replaced the whole thing with EPOLL_CTL_DEL (epoll_del()) as
> > we go through the sockets. It's simpler and arguably safer.  
> 
> Yes, that's what I had in mind as well (I thought I put that in the
> mail, but it looks like I didn't).  Just one additional concern that I
> don't think need hold up merge: do we also need to epoll_del() our
> listening sockets?

Right, I had that thought as well, and this was somewhat covered by the
first version because we'd ignore events on those. But that would still
come with the risk of epoll_wait() loops.

In the perspective of a simple implementation / fix mostly intended for
KubeVirt such as this one, we know that the guest is suspended at that
point, so we'll send a SYN to it, nothing comes back, we'll eventually
time out in 10 seconds and try to reset the connection (in the unlikely
case we're still running *and* getting traffic at that point).

And I guess that's fine because if we're still running and getting
traffic after that timeout something must have gone wrong, so sending a
RST segment doesn't look that bad to me. If the connection was meanwhile
established with the target node, I think our RST segment won't actually
reset the connection because sequence numbers won't match.

But surely it's not elegant and I think we should eventually have an
explicit implementation of the whole thing, perhaps with a new socket
state ("MIGRATED"?) and going through the whole list of listening
sockets and... closing them, I suppose?

-- 
Stefano