Re: [PATCH 2/2] test/passt_in_ns: Consistent sleep commands before starting socat client

[Stefano Brivio <sbrivio@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6013 bytes --]

On Mon, 19 Sep 2022 11:48:33 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Sat, Sep 17, 2022 at 01:28:42PM +0200, Stefano Brivio wrote:
> > On Sat, 17 Sep 2022 20:19:21 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >   
> > > On Sat, Sep 17, 2022 at 10:44:41AM +0200, Stefano Brivio wrote:  
> > > > On Sat, 17 Sep 2022 13:32:45 +1000
> > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Sat, Sep 17, 2022 at 01:55:34AM +0200, Stefano Brivio wrote:    
> > > > > > There are some 'sleep 1' commands between starting the socat server
> > > > > > and its corresponding client to avoid races due to the server not
> > > > > > being ready as we start sending data.
> > > > > > 
> > > > > > However, those don't cover all the cases where we might need them,
> > > > > > and in some cases the sleep command actually ended up being before
> > > > > > the server even starts.
> > > > > > 
> > > > > > This fixes occasional failures in TCP and UDP simple transfer tests,
> > > > > > that became apparent with the new command dispatch mechanism.
> > > > > > 
> > > > > > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>      
> > > > > 
> > > > > Heh, I have a very similar patch in my upcoming series.  I used sleep
> > > > > 0.1 though, to avoid taking so long, which seems to be sufficient in
> > > > > practice.    
> > > > 
> > > > Just mind POSIX only specifies integers as argument for sleep(1), a
> > > > pattern I commonly use is:
> > > > 
> > > > 	sleep 0.1 || sleep 1    
> > > 
> > > Ah, right.
> > >   
> > > > > I did look for a proper way to wait for the socat server to be ready,
> > > > > but I couldn't find anything workable.    
> > > > 
> > > > I guess we could enable logging and look for "starting accept loop",
> > > > from _xioopen_listen() in xio-listen.c.    
> > > 
> > > Yeah, I suppose.
> > >   
> > > > Anyway, right now, I'm just trying to get the tests to complete with
> > > > all your pending series, because it's been a while. This doesn't look
> > > > too bad, we can try to improve it later.    
> > > 
> > > Yeah, fair enough.  When I rebase I'll see if there's any refinements
> > > I can make relatively easily.  
> > 
> > Okay, thanks.
> >   
> > > > > I thought I could retry on the
> > > > > client side, but that doesn't work (at least for host to guest
> > > > > connections) because passt is always listening on the forwarded ports,
> > > > > so the client won't see a connection refused from the actual server.
> > > > > I don't think there's any sane way to propagate connection refused in
> > > > > that way, although we could and probably should forward connection
> > > > > resets outwards.    
> > > > 
> > > > They should be already, and (manual) retries actually worked for me,    
> > > 
> > > I'm not entirely sure what you mean by manual retries.  I was trying
> > > to use socat's retry option, but that only seems to fire on
> > > ECONNREFUSED.  
> > 
> > I was watching the text execution, then as I saw the socat server
> > process getting stuck, I just re-run the client with ssh -F ... with the
> > original command, and that worked for me every time.  
> 
> Ah, yeah, I'd expect that to work.  The difficulty is automatically
> detecting the failure.
> 
> > So, the behaviour I'm inferring is:
> > 
> > - server not listening, no retry option: connect() goes
> >   through, but anything else will get ECONNRESET (RST from passt), plus
> >   there should be EPOLLERR if socat checks that: client exits  
> 
> That doesn't seem to be the case.  We *eventually* get an ECONNRESET,
> but it seems like we can send some data before it happens - sometimes
> quite a lot.  I'm assuming this is because quite a lot can fit into
> socket buffers before the RST makes its way back.

Well, yes, it might simply be that socat relies on ECONNRESET from
send(), and the first send()-like call includes the full buffer.

Or, even if it's already checking for EPOLLERR at that point, it could
be delivered too late.

> > - server not listening, retry option: client exits without retry because
> >   it doesn't get ECONNREFUSED  
> 
> Right.
> 
> > ...I guess a cleaner behaviour on passt's side would be to delay the
> > accept() (that's in tcp_conn_from_sock()) until we get SYN, ACK from
> > the guest. But:  
> 
> I thought about that, but I don't think its workable.  For starters, I
> don't think there's a way of explicitly refusing a connection from
> userspace.  Not accept()ing, then closing and re-opening the listening
> socket *might* do it.
> 
> But.. more fundamentally, due to the connection backlog, I think by
> the time we get the notification in userspace, the kernel has probably
> already completed the TCP handshake, so it's too late to signal a
> connection refused to the client.

Oops, right, listen() by itself already warrants a handshake, so I
guess there's no point in trying if we can't ensure a given behaviour.

In real-world applications:

- the user forwards a set of ports because they expect a server to be
  reachable on those ports, fine

- or all ports are forwarded (this is the case of KubeVirt) so that the
  user doesn't have to configure them. In that case, we can always have
  handshake, client sending data, and receiving a RST before any of it
  is acknowledged -- this is guaranteed by passt as it won't dequeue
  data from buffers before the real server accepted the connection

...the only alternatives I see sound a lot like a port scan, which is
definitely not desirable.

I guess we just have to... accept this behaviour. The important fact, I
think, is that we don't acknowledge data.

Whether closing and re-opening the listening socket causes ICMP
messages to be sent: from kernel code I'd say it's not the case, but it
might be worth an actual test, as that would be a marginal improvement.

-- 
Stefano