Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's

[Willy Tarreau]

Hi Lukas,

On Sun, Dec 30, 2018 at 09:52:33PM +0100, Lukas Tribus wrote:
> In my tests your patch does the job and redispatch does work for
> consistent source or uri hashing, but only if hash-balance-factor is
> set. If I comment hash-balance-factor out, the redispatch does not
> occur:
> 
> #balance uri
> balance source
> hash-type consistent
> hash-balance-factor 150
> server primary-fail 10.0.0.199:80 maxconn 200 weight 256 # unreachable
> server backup-ok 10.0.0.254:80 maxconn 200 weight 1
> 
> 
> Not sure why this happens.

Bah, of course, guess who is as stupid at the end of the year as at the
beginning ? Me of course!

The issue is here :
  while (p->lbprm.chash.balance_factor && (nsrv == avoid || 
!chash_server_is_eligible(nsrv))) {

It should be this :
  while (nsrv == avoid || (p->lbprm.chash.balance_factor && 
!chash_server_is_eligible(nsrv))) {

I'm attaching an updated patch. If it's OK, feel free to adjust it
according to your taste and to put your initial commit message on
it and your previous doc updates. I think we can safely backport it
this way.

Thanks!
Willy
diff --git a/include/proto/lb_chash.h b/include/proto/lb_chash.h
index a0ebf696e..679dff363 100644
--- a/include/proto/lb_chash.h
+++ b/include/proto/lb_chash.h
@@ -28,7 +28,7 @@
 
 void chash_init_server_tree(struct proxy *p);
 struct server *chash_get_next_server(struct proxy *p, struct server 
*srvtoavoid);
-struct server *chash_get_server_hash(struct proxy *p, unsigned int hash);
+struct server *chash_get_server_hash(struct proxy *p, unsigned int hash, const 
struct server *avoid);
 
 #endif /* _PROTO_LB_CHASH_H */
 
diff --git a/src/backend.c b/src/backend.c
index 3c1620bbb..c92e76172 100644
--- a/src/backend.c
+++ b/src/backend.c
@@ -165,7 +165,7 @@ void update_backend_weight(struct proxy *px)
  * If any server is found, it will be returned. If no valid server is found,
  * NULL is returned.
  */
-static struct server *get_server_sh(struct proxy *px, const char *addr, int 
len)
+static struct server *get_server_sh(struct proxy *px, const char *addr, int 
len, const struct server *avoid)
 {
unsigned int h, l;
 
@@ -186,7 +186,7 @@ static struct server *get_server_sh(struct proxy *px, const 
char *addr, int len)
h = full_hash(h);
  hash_done:
if (px->lbprm.algo & BE_LB_LKUP_CHTREE)
-   return chash_get_server_hash(px, h);
+   return chash_get_server_hash(px, h, avoid);
else
return map_get_server_hash(px, h);
 }
@@ -203,7 +203,7 @@ static struct server *get_server_sh(struct proxy *px, const 
char *addr, int len)
  * algorithm out of a tens because it gave him the best results.
  *
  */
-static struct server *get_server_uh(struct proxy *px, char *uri, int uri_len)
+static struct server *get_server_uh(struct proxy *px, char *uri, int uri_len, 
const struct server *avoid)
 {
unsigned int hash = 0;
int c;
@@ -239,7 +239,7 @@ static struct server *get_server_uh(struct proxy *px, char 
*uri, int uri_len)
hash = full_hash(hash);
  hash_done:
if (px->lbprm.algo & BE_LB_LKUP_CHTREE)
-   return chash_get_server_hash(px, hash);
+   return chash_get_server_hash(px, hash, avoid);
else
return map_get_server_hash(px, hash);
 }
@@ -253,7 +253,7 @@ static struct server *get_server_uh(struct proxy *px, char 
*uri, int uri_len)
  * is returned. If any server is found, it will be returned. If no valid server
  * is found, NULL is returned.
  */
-static struct server *get_server_ph(struct proxy *px, const char *uri, int 
uri_len)
+static struct server *get_server_ph(struct proxy *px, const char *uri, int 
uri_len, const struct server *avoid)
 {
unsigned int hash = 0;
const char *start, *end;
@@ -296,7 +296,7 @@ static struct server *get_server_ph(struct proxy *px, const 
char *uri, int uri_l
hash = full_hash(hash);
 
if (px->lbprm.algo & BE_LB_LKUP_CHTREE)
-   return chash_get_server_hash(px, hash);
+   return chash_get_server_hash(px, hash, 
avoid);
else
return map_get_server_hash(px, hash);
}
@@ -315,7 +315,7 @@ static struct server *get_server_ph(struct proxy *px, const 
char *uri, int uri_l
 /*
  * this does the same as the previous server_ph, but check the body contents
  */
-static struct server *get_server_ph_post(struct stream *s)
+static struct server *get_server_ph_post(struct stream *s, const struct server 
*avoid)
 {
unsigned int hash = 0;
struct http_txn *txn  = s->txn;
@@ -370,7 +370,7 @@ static struct server *get_server_ph_post(struct stream *s)
hash = full_hash(hash);
 
if (px->lbprm.algo & BE_LB_LKUP_CHTREE)
-

Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's

[Lukas Tribus]

Hi Willy,


On Thu, 27 Dec 2018 at 15:54, Willy Tarreau  wrote:
> > I'm not 100% sure whether "option redispatch" was only intended to
> > break cookie persistence, but not other lb algorithms. Docs are
> > ambiguous about this.
>
> I think you meant hashing instead of cookie persistence. But indeed,
> hashing is one way to enforce strict affinity, and as long as a server
> is seen up, a request targetting this server must never ever be sent
> to another one. It could cause quite some trouble in a number of setups
> (some users don't even implement health checks to guarantee stickiness).
> You can think for example about a bunch of database servers where objects
> names are hashed to decide what server to visit, it could be dramatic if
> they were mixed.
>
> > However, since option redispatch is configurable per backend, it
> > doesn't make a lot of sense to exclude hash based algorithms from
> > redispatch functionality.
>
> Yes it does because there is not much way to safely fall back to a
> working alternative. If you have hashing, you want all requests for
> the same element to go to the same place. There is hardly a safe way
> to fall back to a given server, and doing this when the server is
> still seen as up could have much more damaging effects than reporting
> a temporary failure.
>
> However in the thread above, I'm seeing that the repoerter is using
> consistent hashing. Consistent hashing is different, as we expect it
> to be much less deterministic. Thus we could imagine implementing the
> redispatch there avoiding picking the same server. In this case I
> think we could slightly modify your patch to permit this case to work.
>
> By the way, looking deeper, I think your patch totally breaks hashing
> by replacing it with round robin, as I don't see how we can reach the
> hashing code anymore, though I might have overlooked something.
>
> I haven't looked as deeply as you did, but I think that there might
> be something wrong with the way the consistent hashing works when trying
> to avoid a server. Indeed, we enter the switch/case here :
>
>638  case BE_LB_LKUP_CHTREE:
>639  case BE_LB_LKUP_MAP:
>640  if ((s->be->lbprm.algo & BE_LB_KIND) == 
> BE_LB_KIND_RR) {
>
> the algo is not round-robin so we continue :
>
>648  }
>649  else if ((s->be->lbprm.algo & BE_LB_KIND) != 
> BE_LB_KIND_HI) {
>
> it's indeed a hash so we skip this block and end up in the place where
> we're hashing the values :
>
>655  switch (s->be->lbprm.algo & BE_LB_PARM) {
>656  case BE_LB_HASH_SRC:
> (...)
>697  case BE_LB_HASH_HDR:
>698  /* Header Parameter hashing */
>699  if (!s->txn || s->txn->req.msg_state 
> < HTTP_MSG_BODY)
>700  break;
>701  srv = get_server_hh(s);
>702  break;
>
> So here for example, in order to hash a header we call the get_server_hh()
> function passing it the stream and never passing the previous server. It
> is this function (and its cousins for other params) which finally calls
> either chash_get_server_hash() or map_get_server_hash(). And there we
> don't have any information to decide what server to avoid.
>
> But looking at chash_get_server_hash(), it would be trivial to pass the
> pointer to the server to avoid :
>
>318  struct server *chash_get_server_hash(struct proxy *p, unsigned int 
> hash)
>319  {
> (...)
>370  while (p->lbprm.chash.balance_factor && 
> !chash_server_is_eligible(nsrv)) {
>371  next = eb32_next(next);
>372  if (!next) {
>373  next = eb32_first(root);
>374  if (++loop > 1) // protection against 
> accidental loop
>375  break;
>376  }
>377  nsrv = eb32_entry(next, struct tree_occ, 
> node)->server;
>378  }
>379
>
> So in short we could pass "avoid" as an argument and change the loop
> condition like this :
>
>370  while (p->lbprm.chash.balance_factor && nsrv != avoid && 
> !chash_server_is_eligible(nsrv)) {
>
> I think the patch will not be as small as expected however it should be
> quite trivial because it would only be an API change consisting in adding
> an extra argument to all these intermediary functions, thus there would be
> almost no risk of breakage and we could probably backport it to 1.9 and
> 1.8.
>
> > Also, the fact the code is already there, it's just never called
> > (s->be->lbprm.algo is never BOTH BE_LB_KIND_RR and BE_LB_LKUP_CHTREE)
> > makes me think that this was just an oversight.
>
> Apparently we do have this combination for the "random" 

State of 0-RTT TLS resumption with OpenSSL

[Janusz Dziemidowicz]

Hi,
I've been trying to get 0-RTT resumption working with haproxy 1.8.16
and OpenSSL 1.1.1a.
No matter what I put in configuration file, testing with openssl
s_client always results in:
Max Early Data: 0

OK, let's look at ssl_sock.c
The only thing that seems to try to enable 0-RTT is this:
#ifdef OPENSSL_IS_BORINGSSL
if (allow_early)
SSL_set_early_data_enabled(ssl, 1);
#else
if (!allow_early)
SSL_set_max_early_data(ssl, 0);
#endif

But I fail to see how this is supposed to work. OpenSSL has 0-RTT
disabled by default. To enable this one must call
SSL_set_max_early_data with the amount of bytes it is willing to read.
The above simply does... nothing.

Is it supposed to work at all or do I miss something? ;)

-- 
Janusz Dziemidowicz