Re: Haproxy running on 100% CPU and slow downloads

[Sachin Shetty]

We deployed the latest and we saw throughput still dropped around peak
hours a bit, then we swithed to nbproc 4 which is holding up ok. Note that
4 Cpus was not sufficient earlier, so I believe the latest version is
scaling better. 

Thanks Lukas and Willy.


On 4/29/16, 11:09 AM, "Willy Tarreau"  wrote:

>Hi guys,
>
>On Tue, Apr 26, 2016 at 08:46:37AM +0200, Lukas Tribus wrote:
>> Hi Sachin,
>> 
>> 
>> there is another fix Willy recently committed, its ff9c7e24fb [1]
>> and its in the snapshots [2] since 1.6.4-20160426.
>> 
>> This is supposed to fix the issue altogether.
>> 
>> Please let us know if this works for you.
>
>Yes it should fix this. Please note that I've got one report in 1.5 of
>some huge transfers (multi-GB) stalling after this patch, and since I
>can't find any case where it could be wrong nor can I reproduce it, I
>suspect we may have a bug somewhere else (at least in 1.5) that was
>hidden by the bug this series of patches fix. We had no such report on
>1.6 however.
>
>There's another case of high CPU usage which Cyril managed to isolate.
>The issue has been present since 1.4 and is *very* hard to reproduce,
>I even had to tweek some sysctls on my laptop to see it and am careful
>not to reboot it. It is triggered by *some* pipelined requests. We're
>currently working on fixing it, there are several ways to fix it but
>all of them come with their downsides for now (one of them being a
>different code path between 1.7 and 1.6/1.5/1.4 which doesn't appeal
>me much).
>
>This is why I'm still waiting before issuing a new series of versions.
>
>In the mean time, feel free to test latest 1.6 snapshot and report any
>issues you may face. I've really committed into getting these issues
>fixed once for all, it's getting irritating to see such bugs surviving
>but I never give up the fight :-)
>
>Best regards,
>Willy
>

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi Maxime,

On Tue, May 10, 2016 at 12:07:15AM +0200, Maxime de Roucy wrote:
> > > I'm not sure to like this feature in its current implementation.
> > > I fear it will also create some new issues depending on how people
> > > will use it.
> 
> Indeed it should be use with care.
> But for me it's as dangerous as the '--' option as '--' had clearly
> been implemented to be used with bash globling.
> However with '--' the sysadmin can filter himself the files list ; it
> can't with my patch.

That's exactly the point.

(...)
> > > Other use cases I immediately see :
> > > - some configurations provide the crt files in the same directory,
> > > which will break things
> > > - some others will store map files in the same directory also
> > > - what about configurations with a README or similar in the
> > > directory ? or swap files because someone else is editing a file at
> > > the same time ?
> 
> The last point is a very good one I think, swap files can easily be
> forgotten.
> The previous ones are more controversial for me as with my patch the
> directory itself become a configuration "file". If sysadmin put "trash"
> in it, it's the same as if he put "trash" in a configuration file??? it's
> not haproxy's fault if it fail.
>
> However, I agree that we should implement some safeguards.

User error can be addressed with good documentation, and stupidity with a
baseball bat. But we need to protect the user against issues he's not aware
of (swap files, backup files, core files if the editor dies, etc).

> > I think that before going further we should start by trying to
> > define what we want to see and what we don't want.
> >  Maybe a solution could be to at least impose a file name extension
> > (eg: .cfg), and I'm not sure it's enough. For example what should we
> > do with symlinks, some will prefer to follow them, others not to.
> > Most likely we should skip all dot files, etc.
> > I think the discussion should go on before we go further with the
> > code.
> 
> OK.
> 
> For me we should add a filter on file name ; keeping only ending with
> ".cfg" and not starting with ".".

I do think that it could be sufficient but I have not thought about it
for a long time, so I could be missing some cases.

> I vote for following symlinks, as the implementation currently does.
> It means less work for me :-) ??? Joking aside, I don't see any point
> against it and sysadmins will assume it does except if it's explicitly
> said in the docs (docs which I should complet regarding this point).

Yes I think you're right. Also it matches the output of "grep" when admins
search for something (eg: a domain name). And it allows to share some
elements between multiple directories if needed (eg: defaults sections).

Regards,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Tue, May 10, 2016 at 12:54:40AM +0200, maxime de Roucy wrote:
> I forgot to free the memory allocated at 'filename = calloc' (why valgrind
> didn't warn...). Forget this patch. I will send another one tomorow.

Yes I noticed, and there's this one as well :

> > +   wlt = calloc(1, sizeof(*wlt));
> > +   tmp_str = strdup(filename);
> > +   if (!wlt || !tmp_str) {
> > +   Alert("Cannot load configuration
> > files %s : out of memory.\n",
> > + filename);
> > +   exit(1);
> > +   }

If tmp_str fails and wlt succeeds, wlt is not freed. Overall I still think
that writing this into a dedicated function will make the controls and
unrolling easier. I'd suggest something like this which is much more
conventional, auditable and maintainable :

/* loads config file/dir , returns 0 on failure with errmsg
 * filled with an error message.
 */
int load_config_file(const char *arg, char **errmsg)
{
...
wlt = calloc(1, sizeof(*wlt));
if (!wlt) {
 memprintf(errmsg, "out of memory");
 goto fail_wlt;
}

tmp_str = strdup(filename);
if (!tmp_str) {
 memprintf(errmsg, "out of memory");
 goto fail_tmp_str;
}
...
return 1;
...
fail_smp_str:
free(wlt);
fail_wlt:
...
return 0;
}

Then in init() :

if (!load_config_from_file(argv, )) {
Alert("Cannot load configuration files %s : %s.\n", filename, errmsg);
exit(1);
}

Also, please don't use sprintf() but snprintf() as I mentionned in the
earlier mail. Some platforms will systematically emit a warning when
sprintf() is used, for the same reason as strcpy() and strcat(), and
we managed to get rid of the last one already :

> > +   struct dirent *dir_entry = dir_entries[dir_entries_it];
> > +   char *filename;
> > +   int filename_size = strlen(wl->s)
> > +   + 1 /* for '/' */
> > +   + strlen(dir_entry->d_name);
> > +
> > +   filename = calloc(filename_size + 1 /* for \0 */, 
> > sizeof(*filename));
(...)
> > +   sprintf(filename, "%s/%s", wl->s, dir_entry->d_name);

At first I thought there was a bug in the calloc call but it's OK. It's not
much common to use it with anything but "1" in the element size so it confused
me. You don't need to justify that +1 is for \0 since that's usual in every
string allocation.

Thanks,
Willy

update LED Floodlight/T5T8 Tube/LED Srtip price list 170lm/w

[Karen-.yoshine lighting]

If you do not wish to receive this message, please click here to unsubscribe！Dear Purchaseing manager,Have good day Very glad to tell you that:Our T5 T8 LED Tube Lumen Efficacy can be : 170LM/W  PF>0.9. You can spend only a little bit more time to know us in detail, you won't disappointed about what you see there.Your kindly feedback is highly appreciated. thanksKarenShenzhen Yoshine Lighitng Co.,LtdSkype: yoshinelighting         WhatsApp:15507583778E-mall: i...@yoshinelighting.com    Tel:+0086-755-33131087Web: www.yoshinelighting.com      Fax:+0086-755-33131087Add: 6B.Jiayi Industrial area.Longhua disctrict.Shenzhen.Guangdong.China

SUPERLIGHTINGS @ Guangzhou lighting fair June 2016 (AD)

[superlightings vincent]

Newsletter@superlightings,yourpart=nerforaluminumlinearlightingsystems!Topic:Guangzhoulightingfair2016Duetotheconfidentialagreementswit=hcustomers,wecouldnotexhibitatGuangzhoulightingfair2016whic=hwillstartfromJune9untilJune12.However,wearestillgladtomeetyou=thereduringthefair,demonstratingour=A0mostpopular=A0products=A0=forlinearlightingsystems.Alsowelcometoourfactory,andyouw=illhavecomprehensiveunderstandings=A0tothequalityandflexibility=ofourlinearlightingsolutions.Forproductdatasheets,pleasegoto=www.super-lightings.comKindRegards,VincentShumSalesDirectorSUPERLIGHTINGSCO.,LTDNO.4,SANXIANG,SHANGCUNDONGSTREET,T=ANGGE,SHIJING,BAIYUN,GUANGZHOU,ZIPCODE:=510450SKYPE:common.heartwechat:ShumKingTseungWHATSAPP:+8613570439296Mobilenumber:+8613570439296vinc...@superlightings.com=shum.vinc...@outlook.comwww.super-lightings.com

---
Avast 防毒软件已对此电子邮件执行病毒检查。
https://www.avast.com/antivirus

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[maxime de Roucy]

I forgot to free the memory allocated at 'filename = calloc' (why valgrind
didn't warn...). Forget this patch. I will send another one tomorow.

Sorry
Le 9 mai 2016 11:28 PM, "Maxime de Roucy"  a
écrit :

> If -f argument is a directory add all the files (and only files) it
> containes to the config files list.
> These files are added in lexical order (man alphasort).
> The -f order is still respected:
>
> $ tree rootdir
> rootdir
> ├── root1
> ├── root2
> ├── root3
> ├── superdir1
> │   ├── aaa1
> │   ├── aaa2
> │   ├── aaa3
> │   ├── bbb1 -> aaa1
> │   └── wrong
> │   └── wrongfile
> └── superdir2
> ├── ccc1
> └── wronglink -> .
>
> $ ./haproxy -C rootdir -f root2 -f superdir2 \
>-f root3 -f superdir1 -f root1
> root2
> superdir2/ccc1
> root3
> superdir1/aaa1
> superdir1/aaa2
> superdir1/aaa3
> superdir1/bbb1
> root1
>
> This can be useful on systemd where you can't change the haproxy
> commande line options on service reload.
> ---
>  doc/haproxy.1  |   8 +++--
>  doc/management.txt |  23 ++--
>  src/haproxy.c  | 103
> ++---
>  3 files changed, 116 insertions(+), 18 deletions(-)
>
> diff --git a/doc/haproxy.1 b/doc/haproxy.1
> index a836d5d..6017efa 100644
> --- a/doc/haproxy.1
> +++ b/doc/haproxy.1
> @@ -6,7 +6,7 @@ HAProxy \- fast and reliable http reverse proxy and load
> balancer
>
>  .SH SYNOPSIS
>
> -haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\
> maxconn] [\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\
> ] [\-dk] [\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ]
> [{\-sf|\-st}\ pidlist...]
> +haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\
> maxconn] [\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\
> ] [\-dk] [\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ]
> [{\-sf|\-st}\ pidlist...]
>
>  .SH DESCRIPTION
>
> @@ -33,8 +33,10 @@ instances without risking the system's stability.
>  .SH OPTIONS
>
>  .TP
> -\fB\-f \fP
> -Specify configuration file path.
> +\fB\-f \fP
> +Specify configuration file or directory path. If the argument is a
> directory
> +the files (and only files) it containes are added in lexical order (man
> +alphasort).
>
>  .TP
>  \fB\-L \fP
> diff --git a/doc/management.txt b/doc/management.txt
> index e0469aa..3e51d49 100644
> --- a/doc/management.txt
> +++ b/doc/management.txt
> @@ -134,16 +134,19 @@ list of options is :
>  one of "global", "defaults", "peers", "listen", "frontend",
> "backend", and
>  so on. A file cannot contain just a server list for example.
>
> -  -f  : adds  to the list of configuration files to be
> -loaded. Configuration files are loaded and processed in their
> declaration
> -order. This option may be specified multiple times to load multiple
> files.
> -See also "--". The difference between "--" and "-f" is that one "-f"
> must
> -be placed before each file name, while a single "--" is needed before
> all
> -file names. Both options can be used together, the command line
> ordering
> -still applies. When more than one file is specified, each file must
> start
> -on a section boundary, so the first keyword of each file must be one
> of
> -"global", "defaults", "peers", "listen", "frontend", "backend", and so
> -on. A file cannot contain just a server list for example.
> +  -f  : adds  to the list of configuration files
> to be
> +loaded. If  is a directory, all the files (and only files) it
> +containes are added in lexical order (man alphasort) to the list of
> +configuration files to be loaded. Configuration files are loaded and
> +processed in their declaration order. This option may be specified
> multiple
> +times to load multiple files. See also "--". The difference between
> "--"
> +and "-f" is that one "-f" must be placed before each file name, while
> a
> +single "--" is needed before all file names. Both options can be used
> +together, the command line ordering still applies. When more than one
> file
> +is specified, each file must start on a section boundary, so the first
> +keyword of each file must be one of "global", "defaults", "peers",
> +"listen", "frontend", "backend", and so on. A file cannot contain
> just a
> +server list for example.
>
>-C  : changes to directory  before loading configuration
>  files. This is useful when using relative paths. Warning when using
> diff --git a/src/haproxy.c b/src/haproxy.c
> index 0c223e5..9824b7c 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -33,10 +33,12 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> 

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Pavlos Parissis]

On 10/05/2016 12:07 πμ, Maxime de Roucy wrote:
> Hi Willy and Cyril,
> 
> I just send a new version of the patch.
> I made some changes following the remarks of Willy.
> 
>>> I'm not sure to like this feature in its current implementation.
>>> I fear it will also create some new issues depending on how people
>>> will use it.
> 
> Indeed it should be use with care.
> But for me it's as dangerous as the '--' option as '--' had clearly
> been implemented to be used with bash globling.
> However with '--' the sysadmin can filter himself the files list ; it
> can't with my patch.
> 
>>> For example, I know lots of sysadmin who have the (bad) habit to
>>> make backup of the configuration files in the same directory,
>>> without cleaning it up. We may see some directories like this :
>>>
>>> service1.cfg
>>> service2.cfg
>>> service2.cfg~
>>> service3.cfg.20160509
>>> service4.cfg-19980101
>>> service5.cfg
>>> service5.cfg.old
>>> service6.cfg.disabled
>>>
>>> ...and so on.
>>>
>>> When several sysadmins share the same haproxy instance, it can
>>> quickly
>>> become annoying.
>>>
>>> Other use cases I immediately see :
>>> - some configurations provide the crt files in the same directory,
>>> which will break things
>>> - some others will store map files in the same directory also
>>> - what about configurations with a README or similar in the
>>> directory ? or swap files because someone else is editing a file at
>>> the same time ?
> 
> The last point is a very good one I think, swap files can easily be
> forgotten.
> The previous ones are more controversial for me as with my patch the
> directory itself become a configuration "file". If sysadmin put "trash"
> in it, it's the same as if he put "trash" in a configuration file… it's
> not haproxy's fault if it fail.
> 

IMHO: If as operator configure haproxy to source all files from a
directory and at the same time I leave broken configuration in that
directory then I blame myself and I don't expect from HAProxy to be
smart enough to do right job for me.

Some times having craftsmanship as one of the main competence of your
coworkers is the right solution, rather having technical solutions to
compensate the absence of craftsmanship.

My 2 cents,
Pavlos



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hi Willy,


> > Instead I create Alert_exit which embedded the exit call inside
> > Alert.
> > I just sent the patchs.
> > 
> > Is this solution good for you ?
> 
> To be honnest, no, I really don't like it. There are very few such
> locations that benefit from it and 2/3 of them even had the code
> ordering changed to be able to use the function (unrolling done
> before the message instead of after), which basically means that if
> any error or warning is printed in any of these functions (or if any
> crashes) the error output will be confusing or missing.

Indeed… let's forget about it.
-- 
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hi Willy and Cyril,

I just send a new version of the patch.
I made some changes following the remarks of Willy.

> > I'm not sure to like this feature in its current implementation.
> > I fear it will also create some new issues depending on how people
> > will use it.

Indeed it should be use with care.
But for me it's as dangerous as the '--' option as '--' had clearly
been implemented to be used with bash globling.
However with '--' the sysadmin can filter himself the files list ; it
can't with my patch.

> > For example, I know lots of sysadmin who have the (bad) habit to
> > make backup of the configuration files in the same directory,
> > without cleaning it up. We may see some directories like this :
> > 
> > service1.cfg
> > service2.cfg
> > service2.cfg~
> > service3.cfg.20160509
> > service4.cfg-19980101
> > service5.cfg
> > service5.cfg.old
> > service6.cfg.disabled
> > 
> > ...and so on.
> > 
> > When several sysadmins share the same haproxy instance, it can
> > quickly
> > become annoying.
> > 
> > Other use cases I immediately see :
> > - some configurations provide the crt files in the same directory,
> > which will break things
> > - some others will store map files in the same directory also
> > - what about configurations with a README or similar in the
> > directory ? or swap files because someone else is editing a file at
> > the same time ?

The last point is a very good one I think, swap files can easily be
forgotten.
The previous ones are more controversial for me as with my patch the
directory itself become a configuration "file". If sysadmin put "trash"
in it, it's the same as if he put "trash" in a configuration file… it's
not haproxy's fault if it fail.

However, I agree that we should implement some safeguards.

> I think that before going further we should start by trying to
> define what we want to see and what we don't want.
>  Maybe a solution could be to at least impose a file name extension
> (eg: .cfg), and I'm not sure it's enough. For example what should we
> do with symlinks, some will prefer to follow them, others not to.
> Most likely we should skip all dot files, etc.
> I think the discussion should go on before we go further with the
> code.

OK.

For me we should add a filter on file name ; keeping only ending with
".cfg" and not starting with ".".
I vote for following symlinks, as the implementation currently does.
It means less work for me :-) … Joking aside, I don't see any point
against it and sysadmins will assume it does except if it's explicitly
said in the docs (docs which I should complet regarding this point).
-- 
Regards
Maxime de Roucy

signature.asc
Description: This is a digitally signed message part

[PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

If -f argument is a directory add all the files (and only files) it
containes to the config files list.
These files are added in lexical order (man alphasort).
The -f order is still respected:

$ tree rootdir
rootdir
├── root1
├── root2
├── root3
├── superdir1
│   ├── aaa1
│   ├── aaa2
│   ├── aaa3
│   ├── bbb1 -> aaa1
│   └── wrong
│   └── wrongfile
└── superdir2
├── ccc1
└── wronglink -> .

$ ./haproxy -C rootdir -f root2 -f superdir2 \
   -f root3 -f superdir1 -f root1
root2
superdir2/ccc1
root3
superdir1/aaa1
superdir1/aaa2
superdir1/aaa3
superdir1/bbb1
root1

This can be useful on systemd where you can't change the haproxy
commande line options on service reload.
---
 doc/haproxy.1  |   8 +++--
 doc/management.txt |  23 ++--
 src/haproxy.c  | 103 ++---
 3 files changed, 116 insertions(+), 18 deletions(-)

diff --git a/doc/haproxy.1 b/doc/haproxy.1
index a836d5d..6017efa 100644
--- a/doc/haproxy.1
+++ b/doc/haproxy.1
@@ -6,7 +6,7 @@ HAProxy \- fast and reliable http reverse proxy and load 
balancer
 
 .SH SYNOPSIS
 
-haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\ maxconn] 
[\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\ ] [\-dk] 
[\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ] [{\-sf|\-st}\ 
pidlist...]
+haproxy \-f  [\-L\ ] [\-n\ maxconn] [\-N\ 
maxconn] [\-C\ ] [\-v|\-vv] [\-d] [\-D] [\-q] [\-V] [\-c] [\-p\ ] 
[\-dk] [\-ds] [\-de] [\-dp] [\-db] [\-dM[]] [\-m\ ] [{\-sf|\-st}\ 
pidlist...]
 
 .SH DESCRIPTION
 
@@ -33,8 +33,10 @@ instances without risking the system's stability.
 .SH OPTIONS
 
 .TP
-\fB\-f \fP
-Specify configuration file path.
+\fB\-f \fP
+Specify configuration file or directory path. If the argument is a directory
+the files (and only files) it containes are added in lexical order (man
+alphasort).
 
 .TP
 \fB\-L \fP
diff --git a/doc/management.txt b/doc/management.txt
index e0469aa..3e51d49 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -134,16 +134,19 @@ list of options is :
 one of "global", "defaults", "peers", "listen", "frontend", "backend", and
 so on. A file cannot contain just a server list for example.
 
-  -f  : adds  to the list of configuration files to be
-loaded. Configuration files are loaded and processed in their declaration
-order. This option may be specified multiple times to load multiple files.
-See also "--". The difference between "--" and "-f" is that one "-f" must
-be placed before each file name, while a single "--" is needed before all
-file names. Both options can be used together, the command line ordering
-still applies. When more than one file is specified, each file must start
-on a section boundary, so the first keyword of each file must be one of
-"global", "defaults", "peers", "listen", "frontend", "backend", and so
-on. A file cannot contain just a server list for example.
+  -f  : adds  to the list of configuration files to be
+loaded. If  is a directory, all the files (and only files) it
+containes are added in lexical order (man alphasort) to the list of
+configuration files to be loaded. Configuration files are loaded and
+processed in their declaration order. This option may be specified multiple
+times to load multiple files. See also "--". The difference between "--"
+and "-f" is that one "-f" must be placed before each file name, while a
+single "--" is needed before all file names. Both options can be used
+together, the command line ordering still applies. When more than one file
+is specified, each file must start on a section boundary, so the first
+keyword of each file must be one of "global", "defaults", "peers",
+"listen", "frontend", "backend", and so on. A file cannot contain just a
+server list for example.
 
   -C  : changes to directory  before loading configuration
 files. This is useful when using relative paths. Warning when using
diff --git a/src/haproxy.c b/src/haproxy.c
index 0c223e5..9824b7c 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -33,10 +33,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -423,7 +425,7 @@ void usage(char *name)
 {
display_version();
fprintf(stderr,
-   "Usage : %s [-f ]* [ -vdV"
+   "Usage : %s [-f ]* [ -vdV"
"D ] [ -n  ] [ -N  ]\n"
"[ -p  ] [ -m  ] [ -C  ] [-- 
*]\n"
"-v displays version ; -vv shows known build options.\n"
@@ -551,6 +553,94 @@ void dump(struct sig_handler *sh)
pool_gc2();
 }
 
+/* This function check if 

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Cyril Bonté]

Hi Maxime and Willy,

Le 09/05/2016 11:17, Maxime de Roucy a écrit :

Hello,

Thanks for your remarks !


I think this is a nice addition, it completes well the ability to
load an arbitrary file list.


Good to hear that :)


I'm not sure to like this feature in its current implementation.
I fear it will also create some new issues depending on how people will 
use it.


For example, I know lots of sysadmin who have the (bad) habit to make 
backup of the configuration files in the same directory, without 
cleaning it up. We may see some directories like this :


service1.cfg
service2.cfg
service2.cfg~
service3.cfg.20160509
service4.cfg-19980101
service5.cfg
service5.cfg.old
service6.cfg.disabled

...and so on.

When several sysadmins share the same haproxy instance, it can quickly 
become annoying.


Other use cases I immediately see :
- some configurations provide the crt files in the same directory, which 
will break things

- some others will store map files in the same directory also
- what about configurations with a README or similar in the directory ? 
or swap files because someone else is editing a file at the same time ?




--
Cyril Bonté

Unsubscribe

[Errol Neal]

Unsubscribe

Re: Stale UNIX sockets after reload

[Pavlos Parissis]

On 09/05/2016 02:26 μμ, Christian Ruppert wrote:
> Hi,
> 
> it seems that HAProxy does not remove the UNIX sockets after reloading
> (also restarting?) even though they have been removed from the
> configuration and thus are stale afterwards.
> At least 1.6.4 seems to be affected. Can anybody else confirm that? It's
> a multi-process setup in this case but it also happens with binds bound
> to just one process.
> 

I can confirm this behavior. I don't think it is easy for haproxy to
clean up stale UNIX socket files as their names can change or stored in
a directory which is shared with other services.

Cheers,
Pavlos




signature.asc
Description: OpenPGP digital signature

Stale UNIX sockets after reload

[Christian Ruppert]

Hi,

it seems that HAProxy does not remove the UNIX sockets after reloading 
(also restarting?) even though they have been removed from the 
configuration and thus are stale afterwards.
At least 1.6.4 seems to be affected. Can anybody else confirm that? It's 
a multi-process setup in this case but it also happens with binds bound 
to just one process.


--
Regards,
Christian Ruppert

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

On Mon, May 09, 2016 at 01:30:51PM +0200, Willy Tarreau wrote:
> Hello Maxime,
> 
> On Mon, May 09, 2016 at 11:17:25AM +0200, Maxime de Roucy wrote:
> > > I have another comment here, please try to factor the error messages
> > > using a goto, this block appears at least 3 times :
> > 
> > I tried to use goto but there is always a slight difference in the
> > Alert(...) calls, so there is quite as many label as Alert call.
> 
> I had the impression that the error messages were the same, maybe I
> have not seen well.

I've rechecked and indeed there are some tiny differences, and the only two
identical ones will be merged once you switch to strdup(), so that's fine.

Regards,
Willy

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hello Maxime,

On Mon, May 09, 2016 at 11:17:25AM +0200, Maxime de Roucy wrote:
> > I have another comment here, please try to factor the error messages
> > using a goto, this block appears at least 3 times :
> 
> I tried to use goto but there is always a slight difference in the
> Alert(...) calls, so there is quite as many label as Alert call.

I had the impression that the error messages were the same, maybe I
have not seen well.

> After some lines it seams error-prone and not very readable.

I understand.

> Instead I create Alert_exit which embedded the exit call inside Alert.
> I just sent the patchs.
> 
> Is this solution good for you ?

To be honnest, no, I really don't like it. There are very few such locations
that benefit from it and 2/3 of them even had the code ordering changed to
be able to use the function (unrolling done before the message instead of
after), which basically means that if any error or warning is printed in
any of these functions (or if any crashes) the error output will be confusing
or missing.

Thanks,
Willy

Re: AWS ELB with SSL backend adds proxy protocol inside SSL stream

[Hector Rivas Gandara]

On 5 May 2016 at 23:27, Igor Cicimov  wrote:
>
>
> On 5 May 2016 10:39 pm, "Hector Rivas Gandara" 
>  wrote:
> > > https://jve.linuxwall.info/ressources/taf/haproxy-aws/
> > Thank you for your answer, but this article describes a configuration where 
> > the ELB is setup in plain TCP mode
> (no SSL), so it does not do reencryption but passes the stream to HAProxy.
> >
> > But my case is  different ELB terminates SSL and opens a SSL connection to 
> > backend (see my original mail).
>
> Maybe you should think then why do you need tproxy at all.

I am not sure what you refer with 'tproxy' but:

 * If 'tproxy' is ELB, as said: We want to use ELB because they
scalability and HA features provided by AWS, SSL  terminatation and to
restrict access to the end user certificates to only some specific
roles.

 * If 'tproxy' is HAProxy, we want to use use HAProxy to be able to do
some HTTP request rewriting.

 * If 'tproxy' is ELB in TCP/SSL mode, rather than HTTP/HTTPS mode, we
need that because we must support websockets, and ELB does not support
websockets.

Thx


-- 
Regards
Hector Rivas | GDS / Multi-Cloud PaaS

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Maxime de Roucy]

Hello,

Thanks for your remarks !

> I think this is a nice addition, it completes well the ability to
> load an arbitrary file list.

Good to hear that :)

> However I cannot merge it as is, there is a huge buffer overflow
> …

Ok, I see. I will try to patch that this evening.

> I have another comment here, please try to factor the error messages
> using a goto, this block appears at least 3 times :

I tried to use goto but there is always a slight difference in the
Alert(...) calls, so there is quite as many label as Alert call.
After some lines it seams error-prone and not very readable.

Instead I create Alert_exit which embedded the exit call inside Alert.
I just sent the patchs.

Is this solution good for you ?
-- 
Thanks
Maxime

signature.asc
Description: This is a digitally signed message part

[PATCH 2/2] MINOR: log: use Alert_exit in src/haproxy.c

[Maxime de Roucy]

This patch replace
  Alert(...); exit(1);
with
  Alert(1, ...)
when it's possible.
---
 src/haproxy.c | 176 +-
 1 file changed, 87 insertions(+), 89 deletions(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index 0c223e5..c1acea6 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -698,10 +698,9 @@ void init(int argc, char **argv)
 
while (argc > 1 && argv[1][0] != '-') {
oldpids = realloc(oldpids, (nb_oldpids 
+ 1) * sizeof(int));
-   if (!oldpids) {
-   Alert("Cannot allocate old pid 
: out of memory.\n");
-   exit(1);
-   }
+   if (!oldpids)
+   Alert_exit(1, "Cannot allocate 
old pid : out of memory.\n");
+
argc--; argv++;
oldpids[nb_oldpids] = atol(*argv);
if (oldpids[nb_oldpids] <= 0)
@@ -714,10 +713,11 @@ void init(int argc, char **argv)
argv++; argc--;
while (argc > 0) {
wl = calloc(1, sizeof(*wl));
-   if (!wl) {
-   Alert("Cannot load 
configuration file %s : out of memory.\n", *argv);
-   exit(1);
-   }
+   if (!wl)
+   Alert_exit(1,
+  "Cannot load 
configuration file %s : out of memory.\n",
+  *argv);
+
wl->s = *argv;
LIST_ADDQ(_cfgfiles, >list);
argv++; argc--;
@@ -737,10 +737,11 @@ void init(int argc, char **argv)
case 'L' : strncpy(localpeer, *argv, 
sizeof(localpeer) - 1); break;
case 'f' :
wl = calloc(1, sizeof(*wl));
-   if (!wl) {
-   Alert("Cannot load 
configuration file %s : out of memory.\n", *argv);
-   exit(1);
-   }
+   if (!wl)
+   Alert_exit(1,
+  "Cannot load 
configuration file %s : out of memory.\n",
+  *argv);
+
wl->s = *argv;
LIST_ADDQ(_cfgfiles, >list);
break;
@@ -761,10 +762,11 @@ void init(int argc, char **argv)
if (LIST_ISEMPTY(_cfgfiles))
usage(progname);
 
-   if (change_dir && chdir(change_dir) < 0) {
-   Alert("Could not change to directory %s : %s\n", change_dir, 
strerror(errno));
-   exit(1);
-   }
+   if (change_dir && chdir(change_dir) < 0)
+   Alert_exit(1,
+  "Could not change to directory %s : %s\n",
+  change_dir,
+  strerror(errno));
 
global.maxsock = 10; /* reserve 10 fds ; will be incremented by socket 
eaters */
 
@@ -774,13 +776,15 @@ void init(int argc, char **argv)
int ret;
 
ret = readcfgfile(wl->s);
-   if (ret == -1) {
-   Alert("Could not open configuration file %s : %s\n",
- wl->s, strerror(errno));
-   exit(1);
-   }
+   if (ret == -1)
+   Alert_exit(1,
+  "Could not open configuration file %s : 
%s\n",
+  wl->s,
+  strerror(errno));
+
if (ret & (ERR_ABORT|ERR_FATAL))
Alert("Error(s) found in configuration file : %s\n", 
wl->s);
+
err_code |= ret;
if (err_code & ERR_ABORT)
exit(1);
@@ -792,10 +796,8 @@ void init(int argc, char **argv)
 #endif
 
err_code |= check_config_validity();
-   if (err_code & (ERR_ABORT|ERR_FATAL)) {
-   Alert("Fatal errors found in configuration.\n");
-   exit(1);
-   }
+   if (err_code & (ERR_ABORT|ERR_FATAL))
+   Alert_exit(1, "Fatal errors found in 

[PATCH 1/2] MINOR: log: Alert_exit function creation

[Maxime de Roucy]

Alert is often followed by exit.
Alert_exit embedded the exit call.
---
 include/proto/log.h |  9 +
 src/log.c   | 31 ++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/include/proto/log.h b/include/proto/log.h
index e606a3c..7afa2a0 100644
--- a/include/proto/log.h
+++ b/include/proto/log.h
@@ -90,8 +90,17 @@ void parse_logformat_string(const char *str, struct proxy 
*curproxy, struct list
  * Displays the message on stderr with the date and pid. Overrides the quiet
  * mode during startup.
  */
+void Alert_va(const char *fmt, va_list argp);
+/*
+ * Same as Alert_va with printf argument style
+ */
 void Alert(const char *fmt, ...)
__attribute__ ((format(printf, 1, 2)));
+/*
+ * Give the possibility to exit just after Alert.
+ */
+void Alert_exit(int exit_code, const char *fmt, ...)
+   __attribute__ ((format(printf, 2, 3)));
 
 /*
  * Displays the message on stderr with the date and pid.
diff --git a/src/log.c b/src/log.c
index 2d02247..949436a 100644
--- a/src/log.c
+++ b/src/log.c
@@ -622,23 +622,44 @@ void parse_logformat_string(const char *fmt, struct proxy 
*curproxy, struct list
  * Displays the message on stderr with the date and pid. Overrides the quiet
  * mode during startup.
  */
-void Alert(const char *fmt, ...)
+void Alert_va(const char *fmt, va_list argp)
 {
-   va_list argp;
struct tm tm;
 
if (!(global.mode & MODE_QUIET) || (global.mode & (MODE_VERBOSE | 
MODE_STARTING))) {
-   va_start(argp, fmt);
-
get_localtime(date.tv_sec, );
fprintf(stderr, "[ALERT] %03d/%02d%02d%02d (%d) : ",
tm.tm_yday, tm.tm_hour, tm.tm_min, tm.tm_sec, 
(int)getpid());
vfprintf(stderr, fmt, argp);
fflush(stderr);
-   va_end(argp);
}
 }
 
+/*
+ * Same as Alert_va with printf argument style
+ */
+void Alert(const char *fmt, ...)
+{
+   va_list argp;
+
+   va_start(argp, fmt);
+   Alert_va(fmt, argp);
+   va_end(argp);
+}
+
+/*
+ * Give the possibility to exit just after Alert.
+ */
+void Alert_exit(int exit_code, const char *fmt, ...)
+{
+   va_list argp;
+
+   va_start(argp, fmt);
+   Alert_va(fmt, argp);
+   va_end(argp);
+   if (exit_code)
+   exit(exit_code);
+}
 
 /*
  * Displays the message on stderr with the date and pid.
-- 
2.8.2

Re: [PATCH] MEDIUM: init: allow directory as argument of -f

[Willy Tarreau]

Hi,

On Sun, May 08, 2016 at 11:41:17AM +0200, Maxime de Roucy wrote:
> If -f argument is a directory add all the files (and only files) it
> containes to the config files list.

I think this is a nice addition, it completes well the ability to load
an arbitrary file list.

However I cannot merge it as is, there is a huge buffer overflow here :

> + char filename[MAX_CLI_DIRFILE_NAME];
> + struct dirent *dir_entry = dir_entries[dir_entries_it];
> +
> + strcpy(filename, wl->s);
> + strcat(filename, "/");
> + strcat(filename, dir_entry->d_name);

You'd rather use snprintf("%s/%s") and check the result.

As a hint, you should always consider that strcpy() with a variable on
input is almost always a bug, and that strcat() is always a bug.


I have another comment here, please try to factor the error messages
using a goto, this block appears at least 3 times :

> +
> + if (stat(filename, _stat)) {
> + Alert("Cannot open configuration file %s : 
> %s\n",
> +   filename,
> +   strerror(errno));
> + exit(1);
> + }

I think it would be easier to move all this patch into a dedicated function.

Here instead of calloc+strcpy, you can simply use strdup() :

> + wlt->s = calloc(strlen(filename) + 1, 
> sizeof(*wlt->s));
> + if (!wlt->s) {
> + Alert("Cannot load configuration files 
> %s : out of memory.\n",
> +   filename);
> + exit(1);
> + }
> + strcpy(wlt->s, filename);

Otherwise it looks good.

Thanks,
Willy