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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Sep 19, 2022 at 08:41:50AM +0200, Stefano Brivio wrote:
> 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.

Yeah, that appears to be the case.  When it did fail, it seemed to be
because it was getting an ECONNRESET on one of the send()s or
write()s.

> 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

Uuuhh.. does that actually guarantee that?  Does the kernel send an
ack only once userspace has read the data, or just once it is queued
in-kernel?

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

Right.

> I guess we just have to... accept this behaviour.

Yeah, I think so.

> 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.

Ok.  It's not really clear to me what ICMP messages would be sent to
where in that scenario.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]