Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's
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
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"
Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's
Hi again Lukas, please have a look at this patch proposal. I think it could do the job, but I have only build-tested it, nothing more. 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 bc38c5710..95b7cd4fa 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) - return chash_get_server_hash(px, hash); + return chash_get_server_hash(px, hash, avoid); else return map_get_server_hash(px, hash); } @@ -396,7 +396,7 @@ static struct server *get_server_ph_post(struct stream *s) * 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_hh(struct stream *s) +static struct server *get_server_hh(struct stream *s, const struct server *avoid) { unsigned int hash = 0; struct http_txn *txn = s->txn; @@ -466,13 +466,13 @@ static struct server *get_server_hh(struct stream *s) hash = full_hash(hash); hash_done: if (px->lbprm.algo & BE_LB_LKUP_CHTREE) - return chash_get_server_hash(px, hash); +
Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's
Hi Lukas, On Wed, Dec 26, 2018 at 11:07:27PM +0100, Lukas Tribus wrote: > > redispatch never worked for hash based alghoritms, as the code > > (BE_LB_LKUP_CHTREE -> chash_get_next_server()) would only have been > > called for BE_LB_KIND_RR, which doesn't make sense. Fix this by also > > going down this code path when the BE_LB_KIND is BE_LB_KIND_HI. > > > > Reported by Oskar Stenman on discourse: > > https://discourse.haproxy.org/t/balance-uri-consistent-hashing-redispatch-3-not-redispatching/3344 > > > > Can be backported to 1.6. > > Actually my intention was to set this as RFC, with the following > comments, but git send-mail was faster: > > 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
Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's
Hi, On Wed, 26 Dec 2018 at 23:04, Lukas Tribus wrote: > > redispatch never worked for hash based alghoritms, as the code For the commit: s/alghoritms/algorithms/ Lukas
Re: [PATCH] BUG/MINOR: lb: fix redispatch for hash based lb-algo's
Hello, On Wed, 26 Dec 2018 at 23:04, Lukas Tribus wrote: > > redispatch never worked for hash based alghoritms, as the code > (BE_LB_LKUP_CHTREE -> chash_get_next_server()) would only have been > called for BE_LB_KIND_RR, which doesn't make sense. Fix this by also > going down this code path when the BE_LB_KIND is BE_LB_KIND_HI. > > Reported by Oskar Stenman on discourse: > https://discourse.haproxy.org/t/balance-uri-consistent-hashing-redispatch-3-not-redispatching/3344 > > Can be backported to 1.6. Actually my intention was to set this as RFC, with the following comments, but git send-mail was faster: 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. However, since option redispatch is configurable per backend, it doesn't make a lot of sense to exclude hash based algorithms from redispatch functionality. 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. Backporting this could change behavior as redispatch suddenly begins to work in such a situation; however this is expected and a correct configuration fixes it immediately. Regards, Lukas