Hi Bartosz, [CCing the ML again]
Interestingly I initially couldn't manage to reproduce it your way, but that's common so I didn't worry nor did I give up, I've just let it cool down seeking an idea. And last night I had an idea about it that made me get up just to note it and I could confirm it this morning :-) I suspect that your server is using keep-alive with a short idle timeout and the working connection expires during the connect retry, so that the working connection suddenly becomes the last one in the pool and is reused at this precise instant. I could reproduce it with my server working in close mode a bit differently, I connect to the faulty server, and during the retry attempts, I immediately try the working one, then the next retry is performed on the working one's address and succeeds. The issue lies with the flag SN_ADDR_SET which is not cleared when killing the connection and which indicates that the connection's address is still valid. It's much easier to observe it when running with -dM as the next connect attempts are not made (socket family unknown). I'm attaching the fix that I'm going to merge and to backport to 1.6 and 1.5. I definitely need to improve the internal architecture to make this more robust but now I see how to do that. Thanks for your help, Willy
>From 51be109e844ea2b7d13b74c89b6e31af1e3d2adb Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Mon, 29 Aug 2016 14:46:54 +0200 Subject: BUG/MAJOR: stream: properly mark the server address as unset on connect retry Bartosz Koninski reported that recent commit 568743a ("BUG/MEDIUM: stream-int: completely detach connection on connect error") introduced a nasty side effect during the connection retry, causing a reconnect attempt to be performed on a random address, possibly another server from another backend as shown in his reproducer. The real reason is in fact that by calling si_release_endpoint() after a failed connect attempt and not clearing the SN_ADDR_SET flag on the stream, we indicate that we continue to trust the connection's current address. So upon next connect attempt, a connection is picked from the pool and the last target address is reused. It is not very easy to reproduce it 100% reliably out of production traffic, but it's easier to see when haproxy is started with -dM to force the pools to be filled with junk, and where subsequent connections are not even attempted due to their family being incorrect. The correct fix consists in clearing the SN_ADDR_SET flag after calling si_release_endpoint() during a retry. But it outlines a deeper issue which is in fact that the target address is *stored* in the connection and its validity is stored in the stream. Until we have a better solution to store target addresses, it would be better to rely on the connection flags (CO_FL_ADDR_TO_SET) for this purpose. But it also outlines the fact that the same issue still exists in Lua sockets and in idle sockets, which fortunately are not affected by this issue. Thanks to Bartosz for providing all the elements needed to understand the problem. This fix needs to be backported to 1.6 and 1.5. --- src/stream.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/stream.c b/src/stream.c index 3982daa..151bcb0 100644 --- a/src/stream.c +++ b/src/stream.c @@ -580,6 +580,7 @@ static int sess_update_st_con_tcp(struct stream *s) si->state = SI_ST_CER; si_release_endpoint(si); + s->flags &= ~SF_ADDR_SET; if (si->err_type) return 0; -- 1.7.12.1
