Here are the adjustments I'm proposing (only 2 and 3 were slightly
touched). I'm not committing them without your consent since they
were signed-off :-) Just let me know.

Thanks,
Willy
>From 7f36164b38965c4c8a8df8d5aadbc31cafd39986 Mon Sep 17 00:00:00 2001
From: William Dauchy <w.dau...@criteo.com>
Date: Sun, 27 Oct 2019 20:08:09 +0100
Subject: MINOR: doc: fix busy-polling performance reference

busy-polling parameter was forgotten in the list of performance tuning

Signed-off-by: William Dauchy <w.dau...@criteo.com>
---
 doc/configuration.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 77f95572f8..8a86dbcaa8 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -631,6 +631,7 @@ The following keywords are supported in the "global" 
section :
    - wurfl-cache-size
 
  * Performance tuning
+   - busy-polling
    - max-spread-checks
    - maxconn
    - maxconnrate
-- 
2.20.1

>From 68a04b32fec881e79715a9b5208badaf3f66f068 Mon Sep 17 00:00:00 2001
From: William Dauchy <w.dau...@criteo.com>
Date: Sun, 27 Oct 2019 20:08:10 +0100
Subject: MINOR: config: allow no set-dumpable config option

in global config parsing, we currently expect to have a possible no
keyword (KWN_NO), but we never allow it in config parsing.
another patch could have been to simply remove the code handling a
possible KWN_NO.
take this opportunity to update documentation of set-dumpable.

Signed-off-by: William Dauchy <w.dau...@criteo.com>
---
 doc/configuration.txt | 33 +++++++++++++++++----------------
 src/cfgparse.c        |  9 ++++++---
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 8a86dbcaa8..bdb02a1aa8 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1141,22 +1141,23 @@ setenv <name> <value>
 
 set-dumpable
   This option is better left disabled by default and enabled only upon a
-  developer's request. It has no impact on performance nor stability but will
-  try hard to re-enable core dumps that were possibly disabled by file size
-  limitations (ulimit -f), core size limitations (ulimit -c), or "dumpability"
-  of a process after changing its UID/GID (such as /proc/sys/fs/suid_dumpable
-  on Linux). Core dumps might still be limited by the current directory's
-  permissions (check what directory the file is started from), the chroot
-  directory's permission (it may be needed to temporarily disable the chroot
-  directive or to move it to a dedicated writable location), or any other
-  system-specific constraint. For example, some Linux flavours are notorious
-  for replacing the default core file with a path to an executable not even
-  installed on the system (check /proc/sys/kernel/core_pattern). Often, simply
-  writing "core", "core.%p" or "/var/log/core/core.%p" addresses the issue.
-  When trying to enable this option waiting for a rare issue to re-appear, it's
-  often a good idea to first try to obtain such a dump by issuing, for example,
-  "kill -11" to the haproxy process and verify that it leaves a core where
-  expected when dying.
+  developer's request. If it has been enabled, it may still be forcibly
+  disabled by prefixing it with the "no" keyword. It has no impact on
+  performance nor stability but will try hard to re-enable core dumps that were
+  possibly disabled by file size limitations (ulimit -f), core size limitations
+  (ulimit -c), or "dumpability" of a process after changing its UID/GID (such
+  as /proc/sys/fs/suid_dumpable on Linux). Core dumps might still be limited by
+  the current directory's permissions (check what directory the file is started
+  from), the chroot directory's permission (it may be needed to temporarily
+  disable the chroot directive or to move it to a dedicated writable location),
+  or any other system-specific constraint. For example, some Linux flavours are
+  notorious for replacing the default core file with a path to an executable
+  not even installed on the system (check /proc/sys/kernel/core_pattern). 
Often,
+  simply writing "core", "core.%p" or "/var/log/core/core.%p" addresses the
+  issue. When trying to enable this option waiting for a rare issue to
+  re-appear, it's often a good idea to first try to obtain such a dump by
+  issuing, for example, "kill -11" to the haproxy process and verify that it
+  leaves a core where expected when dying.
 
 ssl-default-bind-ciphers <ciphers>
   This setting is only available when support for OpenSSL was built in. It sets
diff --git a/src/cfgparse.c b/src/cfgparse.c
index a621b5d51f..0cad16ba8e 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2147,9 +2147,12 @@ next_line:
                                args[arg] = args[arg+1];                // 
shift args after inversion
                }
 
-               if (kwm != KWM_STD && strcmp(args[0], "option") != 0 &&         
\
-                   strcmp(args[0], "log") != 0 && strcmp(args[0], 
"busy-polling")) {
-                       ha_alert("parsing [%s:%d]: negation/default currently 
supported only for options, log, and busy-polling.\n", file, linenum);
+               if (kwm != KWM_STD && strcmp(args[0], "option") != 0 &&
+                   strcmp(args[0], "log") != 0 && strcmp(args[0], 
"busy-polling") != 0 &&
+                   strcmp(args[0], "set-dumpable") != 0) {
+                       ha_alert("parsing [%s:%d]: negation/default currently "
+                                "supported only for options, log, busy-polling 
and "
+                                "set-dumpable.\n", file, linenum);
                        err_code |= ERR_ALERT | ERR_FATAL;
                }
 
-- 
2.20.1

>From 30479f27a7ea386cf9ce2dbdcd365610b0bca69b Mon Sep 17 00:00:00 2001
From: William Dauchy <w.dau...@criteo.com>
Date: Sun, 27 Oct 2019 20:08:11 +0100
Subject: MINOR: init: always fail when setrlimit fails

this patch introduces a strict-limits parameter which enforces the
setrlimit setting instead of a warning. This option can be forcingly
disable with the "no" keyword.
The general aim of this patch is to avoid bad surprises on a production
environment where you change the maxconn for example, a new fd limit is
calculated, but cannot be set because of sysfs setting. In that case you
might want to have an explicit failure to be aware of it before seeing
your traffic going down. During a global rollout it is also useful to
explictly fail as most progressive rollout would simply check the
general health check of the process.

As discussed, plan to use the strict by default mode starting from v2.3.

Signed-off-by: William Dauchy <w.dau...@criteo.com>
---
 doc/configuration.txt  |  9 +++++
 include/types/global.h |  2 +-
 src/cfgparse-global.c  |  8 ++++
 src/cfgparse.c         |  6 +--
 src/haproxy.c          | 88 ++++++++++++++++++++++++++++++++++--------
 5 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index bdb02a1aa8..1d4e696b0e 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -629,6 +629,7 @@ The following keywords are supported in the "global" 
section :
    - wurfl-information-list
    - wurfl-information-list-separator
    - wurfl-cache-size
+   - strict-limits
 
  * Performance tuning
    - busy-polling
@@ -1405,6 +1406,14 @@ wurfl-cache-size <size>
   Please note that this option is only available when haproxy has been compiled
   with USE_WURFL=1.
 
+strict-limits
+  Makes process fail at startup when a setrlimit fails. Haproxy is tries to set
+  the best setrlimit according to what has been calculated. If it fails, it
+  will emit a warning. Use this option if you want an explicit failure of
+  haproxy when those limits fail. This option is disabled by default. If it has
+  been enabled, it may still be forcibly disabled by prefixing it with the "no"
+  keyword.
+
 3.2. Performance tuning
 -----------------------
 
diff --git a/include/types/global.h b/include/types/global.h
index df6614eb16..d5dbc7f90d 100644
--- a/include/types/global.h
+++ b/include/types/global.h
@@ -72,8 +72,8 @@
 #define GTUNE_BUSY_POLLING       (1<<11)
 #define GTUNE_LISTENER_MQ        (1<<12)
 #define GTUNE_SET_DUMPABLE       (1<<13)
-
 #define GTUNE_USE_EVPORTS        (1<<14)
+#define GTUNE_STRICT_LIMITS      (1<<15)
 
 /* SSL server verify mode */
 enum {
diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c
index b117ebc7c2..dd37559a53 100644
--- a/src/cfgparse-global.c
+++ b/src/cfgparse-global.c
@@ -1172,6 +1172,14 @@ int cfg_parse_global(const char *file, int linenum, char 
**args, int kwm)
                                env++;
                }
        }
+       else if (!strcmp(args[0], "strict-limits")) { /* "no strict-limits" or 
"strict-limits" */
+               if (alertif_too_many_args(0, file, linenum, args, &err_code))
+                       goto out;
+               if (kwm == KWM_NO)
+                       global.tune.options &= ~GTUNE_STRICT_LIMITS;
+               else
+                       global.tune.options |= GTUNE_STRICT_LIMITS;
+       }
        else {
                struct cfg_kw_list *kwl;
                int index;
diff --git a/src/cfgparse.c b/src/cfgparse.c
index 0cad16ba8e..eaad6c2cde 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2149,10 +2149,10 @@ next_line:
 
                if (kwm != KWM_STD && strcmp(args[0], "option") != 0 &&
                    strcmp(args[0], "log") != 0 && strcmp(args[0], 
"busy-polling") != 0 &&
-                   strcmp(args[0], "set-dumpable") != 0) {
+                   strcmp(args[0], "set-dumpable") != 0 && strcmp(args[0], 
"strict-limits") != 0) {
                        ha_alert("parsing [%s:%d]: negation/default currently "
-                                "supported only for options, log, busy-polling 
and "
-                                "set-dumpable.\n", file, linenum);
+                                "supported only for options, log, 
busy-polling, "
+                                "set-dumpable and strict-limits.\n", file, 
linenum);
                        err_code |= ERR_ALERT | ERR_FATAL;
                }
 
diff --git a/src/haproxy.c b/src/haproxy.c
index a7f294d6a0..cf23c396fc 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2829,14 +2829,24 @@ int main(int argc, char **argv)
                limit.rlim_max = MAX(rlim_fd_max_at_boot, limit.rlim_cur);
 
                if (setrlimit(RLIMIT_NOFILE, &limit) == -1) {
-                       /* try to set it to the max possible at least */
                        getrlimit(RLIMIT_NOFILE, &limit);
-                       limit.rlim_cur = limit.rlim_max;
-                       if (setrlimit(RLIMIT_NOFILE, &limit) != -1)
-                               getrlimit(RLIMIT_NOFILE, &limit);
+                       if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                               ha_alert("[%s.main()] Cannot raise FD limit to 
%d, limit is %d.\n",
+                                        argv[0], global.rlimit_nofile, 
(int)limit.rlim_cur);
+                               if (!(global.mode & MODE_MWORKER))
+                                       exit(1);
+                       }
+                       else {
+                               /* try to set it to the max possible at least */
+                               limit.rlim_cur = limit.rlim_max;
+                               if (setrlimit(RLIMIT_NOFILE, &limit) != -1)
+                                       getrlimit(RLIMIT_NOFILE, &limit);
 
-                       ha_warning("[%s.main()] Cannot raise FD limit to %d, 
limit is %d.\n", argv[0], global.rlimit_nofile, (int)limit.rlim_cur);
-                       global.rlimit_nofile = limit.rlim_cur;
+                               ha_warning("[%s.main()] Cannot raise FD limit 
to %d, limit is %d. "
+                                          "This will fail in >= v2.3\n",
+                                          argv[0], global.rlimit_nofile, 
(int)limit.rlim_cur);
+                               global.rlimit_nofile = limit.rlim_cur;
+                       }
                }
        }
 
@@ -2845,13 +2855,29 @@ int main(int argc, char **argv)
                        global.rlimit_memmax * 1048576ULL;
 #ifdef RLIMIT_AS
                if (setrlimit(RLIMIT_AS, &limit) == -1) {
-                       ha_warning("[%s.main()] Cannot fix MEM limit to %d 
megs.\n",
-                                  argv[0], global.rlimit_memmax);
+                       if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                               ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
+                                        argv[0], global.rlimit_memmax);
+                               if (!(global.mode & MODE_MWORKER))
+                                       exit(1);
+                       }
+                       else
+                               ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs."
+                                          "This will fail in >= v2.3\n",
+                                          argv[0], global.rlimit_memmax);
                }
 #else
                if (setrlimit(RLIMIT_DATA, &limit) == -1) {
-                       ha_warning("[%s.main()] Cannot fix MEM limit to %d 
megs.\n",
-                                  argv[0], global.rlimit_memmax);
+                       if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                               ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
+                                        argv[0], global.rlimit_memmax);
+                               if (!(global.mode & MODE_MWORKER))
+                                       exit(1);
+                       }
+                       else
+                               ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs.",
+                                          "This will fail in >= v2.3\n",
+                                          argv[0], global.rlimit_memmax);
                }
 #endif
        }
@@ -3050,13 +3076,31 @@ int main(int argc, char **argv)
                limit.rlim_cur = limit.rlim_max = RLIM_INFINITY;
 
 #if defined(RLIMIT_FSIZE)
-               if (setrlimit(RLIMIT_FSIZE, &limit) == -1)
-                       ha_warning("[%s.main()] Failed to set the raise the 
maximum file size.\n", argv[0]);
+               if (setrlimit(RLIMIT_FSIZE, &limit) == -1) {
+                       if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                               ha_alert("[%s.main()] Failed to set the raise 
the maximum "
+                                        "file size.\n", argv[0]);
+                               if (!(global.mode & MODE_MWORKER))
+                                       exit(1);
+                       }
+                       else
+                               ha_warning("[%s.main()] Failed to set the raise 
the maximum "
+                                          "file size. This will fail in >= 
v2.3\n", argv[0]);
+               }
 #endif
 
 #if defined(RLIMIT_CORE)
-               if (setrlimit(RLIMIT_CORE, &limit) == -1)
-                       ha_warning("[%s.main()] Failed to set the raise the 
core dump size.\n", argv[0]);
+               if (setrlimit(RLIMIT_CORE, &limit) == -1) {
+                       if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                               ha_alert("[%s.main()] Failed to set the raise 
the core "
+                                        "dump size.\n", argv[0]);
+                               if (!(global.mode & MODE_MWORKER))
+                                       exit(1);
+                       }
+                       else
+                               ha_warning("[%s.main()] Failed to set the raise 
the core "
+                                          "dump size. This will fail in >= 
v2.3\n", argv[0]);
+               }
 #endif
 
 #if defined(USE_PRCTL)
@@ -3069,8 +3113,20 @@ int main(int argc, char **argv)
        limit.rlim_cur = limit.rlim_max = 0;
        getrlimit(RLIMIT_NOFILE, &limit);
        if (limit.rlim_cur < global.maxsock) {
-               ha_warning("[%s.main()] FD limit (%d) too low for 
maxconn=%d/maxsock=%d. Please raise 'ulimit-n' to %d or more to avoid any 
trouble.\n",
-                          argv[0], (int)limit.rlim_cur, global.maxconn, 
global.maxsock, global.maxsock);
+               if (global.tune.options & GTUNE_STRICT_LIMITS) {
+                       ha_alert("[%s.main()] FD limit (%d) too low for 
maxconn=%d/maxsock=%d. "
+                                "Please raise 'ulimit-n' to %d or more to 
avoid any trouble.\n",
+                                argv[0], (int)limit.rlim_cur, global.maxconn, 
global.maxsock,
+                                global.maxsock);
+                       if (!(global.mode & MODE_MWORKER))
+                               exit(1);
+               }
+               else
+                       ha_alert("[%s.main()] FD limit (%d) too low for 
maxconn=%d/maxsock=%d. "
+                                "Please raise 'ulimit-n' to %d or more to 
avoid any trouble."
+                                "This will fail in >= v2.3\n",
+                                argv[0], (int)limit.rlim_cur, global.maxconn, 
global.maxsock,
+                                global.maxsock);
        }
 
        if (global.mode & (MODE_DAEMON | MODE_MWORKER | MODE_MWORKER_WAIT)) {
-- 
2.20.1

Reply via email to