Re: Propagating agent-check weight change to tracking servers

[Willy Tarreau]

On Sat, Apr 15, 2017 at 02:24:41PM +0200, Michal wrote:
> Hi,
> Maybe my email wasn't nice enough, but breaking compilation

You were the first one to experience the build breakage, it worked for
most of us, but you didn't even give the smallest hints about the error(s)
you met, making it much harder to fix the problem. Fortunately others were
much more constructive and reported it the normal way.

> and simplest
> config with server using "source" got me very angry. I didn't send any
> reproducer, because even simple
> "server name 1.1.1.1:80 source 1.2.3.4 track other"
> wasn't parsing.

It's much easier when the proper info is sent, you can imagine as many
test combinations as you want, you'll always find one that does not work.
I tested several configs myself before merging Fred's patches and all of
them worked so I was confident enough.

In this project we try very hard not to break existing setups. Some configs
have never been modified since version 1.1 emitted 16 years ago and still
work fine. We even implemented emulation for some older settings that are
not natively implemented anymore, so we know pretty well that breaking
setups is not nice. But this is a development branch and during development
it's expected that things break from time to time, this is why nobody is
supposed to run this in production, and everyone is welcome to report
issues.

> I'm posting this patch as open source committer, private person with private
> e-mail address and let's be honest - @haproxy.com guy didn't check
> simplest and very important thing as compilation on different platforms.
> Even -dev branch shouldn't break such thing.

Please stop saying that bullshit. Tests were made and I tested myself. It's
just that you had the first config revealing a regression and you would
have been welcome to report it as anyone else usually does here. If you're
too narrow-minded to stand a development cycle, please go pollute another
project, we don't need people who complain against those who do the work.
And feel free to call me a clueless incompetent bastard if that makes you
feel better. Let me tell you something : I couldn't have brought this project
where it is alone without all the contributions we received, so I do have
a lot of respect for contributors and their work and I will not let anyone
insult those who devote time to improve the project. And that's true for
your contributions as well. If there's something you don't like, propose
improvements but do not tell people they're doing a bad work. I hope this
is clear.

> Please don't mess allegrogroup here, I'm posting those patches from my
> private e-mail.

This is the "From" you put in the commit author. As you see it's never nice
to see your work criticized when you thought you did a nice job and it
has an impact on others, and constructive bug reports are generally nicer
for everyone than broad accusations.

Now let's turn constructive again and try to find how to deal with this
weight propagation issue.

Willy

[PATCH]: CLEANUP

[Jim Freeman]

trivial typo in log.c
From 6b51be4bc3b71eda400a7c0012a4642f393acaae Mon Sep 17 00:00:00 2001
From: Jim Freeman 
Date: Sat, 15 Apr 2017 08:01:59 -0600
Subject: [PATCH] CLEANUP - typo: simgle => single

---
 src/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/log.c b/src/log.c
index be1ebdc..003e42b 100644
--- a/src/log.c
+++ b/src/log.c
@@ -575,7 +575,7 @@ int parse_logformat_string(const char *fmt, struct proxy *curproxy, struct list
 cformat = LF_TEXT;
 pformat = LF_TEXT; /* finally we include the previous char as well */
 sp = str - 1; /* send both the '%' and the current char */
-memprintf(err, "unexpected variable name near '%c' at position %d line : '%s'. Maybe you want to write a simgle '%%', use the syntax ''",
+memprintf(err, "unexpected variable name near '%c' at position %d line : '%s'. Maybe you want to write a single '%%', use the syntax ''",
   *str, (int)(str - backfmt), fmt);
 return 0;
 
-- 
2.1.4

Re: Propagating agent-check weight change to tracking servers

[Michał]

Hi,
I didn't want to continue this discussion, but there is one thing that's
totally not true.

2017-04-13 13:50 GMT+02:00 Frederic Lecaille :

> Hello Michal,
>
> On 04/11/2017 04:41 PM, Michał wrote:
>
>> Hello Willy,
>>
>> So I'm fighting with dba97077 made by Frédéric Lécaille - it broke many
>> things.
>>
>
> This patch broke haproxy non-transparent builds. Thanks to Steven
> Davidovitz, Pavlos Parissis and David Carlier for having promptly helped in
> fixing this.
>
> Excepted this building issue, AFAIK this patch may break only server  or
> default server 'source' setting parsing. How much other things did it break?
>
> Do not forgive that as far as your configuration files do not use any
> 'default-server' line with new supported settings (I agree they are
> numerous), they should be parsed as before.
>
>
That's the case. My config don't use this functionality and the parsing
broke.
That's why I got so angry, because breaking existing configs it's not best
practice IMHO.


> So, if you want to have more chances not to be impacted by my
> "default-server patches", please do not use any 'default-server' new
> supported setting as a first workaround, or no 'default-server' line at all
> as a second workaround. This should help you to continue and develop your
> features.
>
> E.g. you can't use "source", because that patch broke it. I'm curious how
>> many other stuff got broken with those patches around default-server.
>>
>>
> [snipped rude/useless/unproductive published anonymous comments,
> especially towards my colleagues]
>
>
>> In attachment there is patch fixed after your (Willy) review. Sorry for
>> loop,
>> I was focused on fixing all those problems with Frédérics patch I just
>> didn't
>> think how to replace do..while (which obviously works) with this cleaner
>> version - thanks for this. The patch got even simpler.
>>
>
> As Willy wrote before, please feel free to contact me to fix asap any
> issue I alone am responsible. I am keeping an eye on HAproxy ML, and I
> would still be glad to help you and consequently any other haproxy users.
>
> Regards,
> Fred.
>
>

Re: Propagating agent-check weight change to tracking servers

[Michał]

Hi,
Maybe my email wasn't nice enough, but breaking compilation and simplest
config with server using "source" got me very angry. I didn't send any
reproducer, because even simple
"server name 1.1.1.1:80 source 1.2.3.4 track other"
wasn't parsing.

2017-04-13 15:38 GMT+02:00 Willy Tarreau :

> Hi again Michal,
>
> So in the end I already had to revert your latest patch, I should have
> been more careful before merging it.
>
> > We need some CI (even if they will only build haproxy) and IMHO people
> with
> > @haproxy.com mails should test their code before posting and merging :(
>
> Thus please let me reuse your own words above :
>
>   You need some CI, and IMHO people with @allegrogroup.com mails should
>   test their code before posting.
>
> but I won't generalize and as I previously said it's development so we
> are allowed to break a bit for the sake of trying to make something better,
> so case closed.
>

I'm posting this patch as open source committer, private person with private
e-mail address and let's be honest - @haproxy.com guy didn't check
simplest and very important thing as compilation on different platforms.
Even
-dev branch shouldn't break such thing.


>
> The problem is immediately spotted in configs like this one :
>
>   global
> stats timeout 1h
> stats socket /var/run/haproxy.sock level admin mode 600
>
>   defaults
> maxconn 100
> log 127.0.0.1 local0
> option log-health-checks
> mode http
> timeout client 3s
> timeout server 1s
> timeout connect 10s
> default-server maxconn 50 weight 100
>
>   listen main
> bind :8000
> stats uri /stat
> use_backend customer1 if { req.hdr(host) -i customer1 }
> use_backend customer2 if { req.hdr(host) -i customer2 }
> use_backend customer3 if { req.hdr(host) -i customer3 }
>
>   backend servers
> server s1 127.0.0.1:8001 check inter 1s
>
>   backend customer1
> balance uri
> hash-type consistent
> server s1 127.0.0.1:8001 track servers/s1
>
>   backend customer2
> balance uri
> server s1 127.0.0.1:8001 track servers/s1
>
>   backend customer3
> balance uri
> hash-type consistent
> server s1 127.0.0.1:8001 track servers/s1
>
> There are 3 backends, one per customer. Two customers use a consistent
> hash on the URI while the other one uses a standard hash (map-based).
> But all point to the same server, it's just a matter of choice. In fact
> an even more common setup with reverse caches looks like this when
> users want to use one farm under moderate load and another one under
> high load :
>
>backend leastconn
>balance leastconn
>server s1 127.0.0.1:8001 track cache/s1
>server s2 127.0.0.2:8001 track cache/s2
>server s3 127.0.0.3:8001 track cache/s3
>
>backend cache
>balance uri
>server s1 127.0.0.1:8001 check
>server s2 127.0.0.2:8001 check
>server s3 127.0.0.3:8001 check
>
> The problem faced here is when a non-integral weight change happens, such
> as "50%". Since it can only be applied to backends using a dynamic LB algo,
> it will be rejected on others. In the first example above, doing so will
> lead to server "s1" in farms "servers" and "customer3" to turn to 50%,
> farm "customer2" to generate an error and to abort the processing, and
> farm "customer1" not to be changed despite being dynamic thus compatible.
>
> At the very least it's very important that the change remains consistent
> across multiple similar farms. Having both customer1 and customer3
> identical
> and tracking "servers" with different states after a change is not
> acceptable.
>
> Now what's the best approach here ? I don't really know. We could refuse
> to apply the change at all and roll everything back, but that means that
> you are supposed to save the respective servers weights in order to restore
> them. And it also means that having one new farm tracking a central one and
> making use of a non-compatible balance algorithm could prevent all the
> other
> ones from adjusting their weights. Or we can decide that after all, a
> change
> performed to a map-based farm normally only triggers an error and is
> ignored,
> so probably we could simply report the first error, continue, then report
> the
> number of errors at the end of the operation and the rate of success.
>
> Still another problem will remain :
>
>   defaults
> balance uri
>
>   backend servers
> server s1 127.0.0.1:8001 check inter 1s
>
>   backend customer1
> hash-type consistent
> server s1 127.0.0.1:8001 track servers/s1
>
>   backend customer2
> hash-type consistent
> server s1 127.0.0.1:8001 track servers/s1
>
>   backend customer3
> hash-type consistent
> server s1 127.0.0.1:8001 track servers/s1
>
> This one will always fail to use non-integral weigths because of
> 

Re: BUG/MINOR : wrong error message during 'default-server' lines

[Willy Tarreau]

On Fri, Apr 14, 2017 at 03:59:40PM +0200, Frederic Lecaille wrote:
> Hello,
> 
> This patch fixes a minor bug, a wrong error message during 'usesrc' keyword
> parsing on 'default-server' lines.
> 
> 'usesrc' was displayed as unknown if not used after 'source' keyword.

Applied, thank you Fred.
Willy