This is a generic health check which can be used to match a
banner or send a request and analyse a server response.
It works in a send/expect ways and many exchange can be done between
HAProxy and a server to decide the server status, making HAProxy able to
speak the server's protocol.
It can send arbitrary regular or binary strings and match content as a
regular or binary string or a regex.
Willy Tarreau [Tue, 3 Dec 2013 16:50:47 +0000 (17:50 +0100)]
MINOR: tools: add a generic binary hex string parser
We currently use such an hex parser in pat_parse_bin() to parse hex
string patterns. We'll need another generic one so let's move it to
standard.c and have pat_parse_bin() make use of it.
This patch permits to use the same struct pattern for two indentical maps.
This permits to preserve memory, and permits to update only one
"struct pattern" when the dynamic map update is supported.
BUG/MINOR: acl: acl parser does not recognize empty converter list
Commit 348971e (MEDIUM: acl: use the fetch syntax 'fetch(args),conv(),conv()'
into the ACL keyword) introduced a regression in the ACL parser. The second
argument of an ACL keyword is now mistakenly confused with a converter.
This bug is post-dev19 and does not require any backport.
Willy Tarreau [Thu, 5 Dec 2013 01:36:25 +0000 (02:36 +0100)]
OPTIM: checks: avoid setting SO_LINGER twice
We happened to preform this call twice on some checks, once in the
recv event handler, and another one in the main function. Remove
the one from the event handler which does not make any more sense
there.
Willy Tarreau [Thu, 5 Dec 2013 00:53:08 +0000 (01:53 +0100)]
OPTIM: checks: don't poll on recv when using plain TCP connects
When pure TCP checks are used, we see a useless call to recvfrom()
in strace resulting from an inconditional poll on recv after the
connect() succeeds. Let's remove this one and properly report
connection success in the write events.
Willy Tarreau [Wed, 4 Dec 2013 10:17:05 +0000 (11:17 +0100)]
MEDIUM: checks: centralize error reporting
Error reporting in health checks is unreliable as the number of recent
patch shows. The main reason is that the code required to detect the
exact situation where the error occurred is not simple, and the errors
have to be handled closer to where they occur in order to be accurate
(rely on getsockopt(SO_ERROR) and errno).
To solve this, we introduce chk_report_conn_err(). It does its best to
consider a possible errno passed in argument, a possible timeout passed
as well, then it completes this with getsockopt() if needed, and takes
into account the current status of the connection. The result is that
by simply calling this function with errno when it's known, we can emit
accurate log messages from every location. We can now see a messages
like "Connection error during SSL handshake (No route to host)" which
were not previously possible.
Willy Tarreau [Wed, 4 Dec 2013 23:31:46 +0000 (00:31 +0100)]
BUG/MINOR: checks: do not trust errno in write event before any syscall
The only case where errno is supposed to be valid is when the connection
has just got the CO_FL_ERROR flag and errno is not zero, because it will
have been set by the same function that has set the flag. For all other
situations, we need to check the socket using getsockopt(), but only do
it once, since it clears the pending error code. For this reason, we
assign the error code to errno in order not to lose it. The same call
is made at the entry of event_srv_chk_r(), event_srv_chk_w(), and
wake_srv_chk() so that we get a chance to collect errors reported by
the poller or by failed syscalls.
Note that this fix relies on the 4 previous patches, so backporters
must be very careful.
Willy Tarreau [Wed, 4 Dec 2013 23:49:40 +0000 (00:49 +0100)]
MINOR: connection: clear errno prior to checking for errors
At some places, we report an error by just detecting FD_POLL_ERR.
The problem is that the caller never knows if it must use errno or
call getsockopt(SO_ERROR). And since this last one clears the
pending error from the queue, it cannot be used inconditionally.
An elegant solution consists in clearing errno prior to inspecting
FD_POLL_ERR. The caller then knows that if it gets CO_FL_ERROR and
errno == 0, it must call getsockopt().
Willy Tarreau [Thu, 5 Dec 2013 01:19:58 +0000 (02:19 +0100)]
BUG/MEDIUM: acl: fix regression introduced by latest converters support
Since commit 348971e (MEDIUM: acl: use the fetch syntax
'fetch(args),conv(),conv()' into the ACL keyword), ACLs wait on input
that may change. This is visible in the configuration below :
tcp-request inspect-delay 3s
tcp-request content accept if REQ_CONTENT
Nothing will pass before the end of the timer. This is because
historically, sample_process() was dedicated to stick tables where
it was absolutely necessary to wait for a stable sample. Now samples
are used by many other things and we can't afford this. So let's move
this check to the stick tables after the call to sample_process()
instead.
This is post-1.5-dev19 work, no backport is required.
Willy Tarreau [Wed, 4 Dec 2013 22:44:10 +0000 (23:44 +0100)]
MEDIUM: connection: set the socket shutdown flags on socket errors
When we get a hard error from a syscall indicating the socket is dead,
it makes sense to set the CO_FL_SOCK_WR_SH and CO_FL_SOCK_RD_SH flags
to indicate that the socket may not be used anymore. It will ease the
error processing in health checks where the state of socket is very
important. We'll also be able to avoid some setsockopt(nolinger) after
an error.
For now, the rest of the code is not impacted because CO_FL_ERROR is
always tested prior to these flags.
Willy Tarreau [Wed, 4 Dec 2013 22:37:56 +0000 (23:37 +0100)]
BUG/MINOR: connection: check EINTR when sending a PROXY header
PROXY protocol header was not tolerant to signals, so it might cause a
connection to report an error if a signal comes in at the exact same
moment the send is done.
This is 1.5-specific and does not need any backport.
Willy Tarreau [Wed, 4 Dec 2013 15:11:04 +0000 (16:11 +0100)]
BUG/MINOR: tcp: check that no error is pending during a connect probe
The tcp_connect_probe() function may be called upon I/O activity when
no recv/send callbacks were called (eg: recv not possible, nothing to
send). It only relies on connect() to observe the connection establishment
progress but that does not work when some network errors are pending on
the socket (eg: a delayed connection refused).
For this reason we need to run a getsockopt() in the case where the
poller reports FD_POLL_ERR on the socket. We use this opportunity to
update errno so that the conn->data->wake() function has all relevant
info when it sees CO_FL_ERROR.
At the moment no code is impacted by this bug because recv polling is
always enabled during a connect, so recvfrom() always sees the error
first. But this may change with the health check cleanup.
Godbach [Wed, 4 Dec 2013 09:24:06 +0000 (17:24 +0800)]
OPTIM: stream_interface: return directly if the connection flag CO_FL_ERROR has been set
The connection flag CO_FL_ERROR will be tested in the functions both
si_conn_recv_cb() and si_conn_send_cb(). If CO_FL_ERROR has been set, out_error
branch will be executed. But the only job of out_error branch is to set
CO_FL_ERROR on connection flag. So it's better return directly than goto
out_error branch under such conditions. As a result, out_error branch becomes
needless and can be removed.
In addition, the return type of si_conn_send_loop() is also changed to void.
The caller should check conn->flags for errors just like stream_int_chk_snd_conn()
does as below:
Godbach [Wed, 4 Dec 2013 08:08:22 +0000 (16:08 +0800)]
DOC: stick-table: modify the description
The stickiness table can be declared in such sections as frontend, listen
and backend, but the original manual only mentioned backend. Modify the
description simply as below:
"current backend" -> "current section"
Willy Tarreau [Tue, 3 Dec 2013 23:43:21 +0000 (00:43 +0100)]
MINOR: stats: remove some confusion between the DRAIN state and NOLB
We now have to report 2 conflicting information on the stats page :
- NOLB = server which returns 404 and stops load balancing ;
- DRAIN = server with a weight forced to zero
The DRAIN state was previously detected from eweight==0 and represented in
blue so that a temporarily disabled server was noticed. This was done by
commit cc8bb92 (MINOR: stats: show soft-stopped servers in different color).
This choice suffered from a small defect however, which is that a server
with a zero weight was reported in this color whatever its state (even down
or switching).
Also, one of the motivations for the color above was because the NOLB state
is barely detectable as it's very close to the UP state.
Since commit 8c3d0be (MEDIUM: Add DRAIN state and report it on the stats page),
we have the new DRAIN state to show servers with a zero weight. The colors are
unfortunately very close to those of the MAINT state, and some users were
confused by the disappearance of the blue bars.
Additionally, the NOLB state had precedence over DRAIN, which could be an
issue since DRAIN is the only thing the admin can act on, so once NOLB was
shown, there was nothing to indicate that the weight was forced to zero.
By switching the two priorities we can report DRAIN (forced mode) before
NOLB (detected mode).
The best solution to fix all this is to reuse the previous blue color for
all cases where weight == 0, whether it's set by config / agent / cli (DRAIN)
or detected by a 404 response (NOLB). However we only use this color when the
server is 100% UP. If it's going down we switch to the usual yellow color
showing failed checks, and when it's down it keeps its usual red color.
That way, a blue bar on the display indicates a server not taking new
sessions but perfectly up. And other colors keep their usual meaning.
Willy Tarreau [Tue, 3 Dec 2013 23:05:29 +0000 (00:05 +0100)]
BUG/MEDIUM: checks: also update the DRAIN state from the web interface
In commit 8c3d0be (MEDIUM: Add DRAIN state and report it on the stats page),
the drain state was updated on every weight change except those that can be
sent via the web interface. This caused inconsistent state combinations to
be reported in the stats depending on the sequence (web then cli vs cli
then web).
It would seem that a call to set_server_drain_state() from within
server_recalc_eweight() would simplify things but that's not completely
certain yet.
Willy Tarreau [Tue, 3 Dec 2013 21:48:23 +0000 (22:48 +0100)]
BUG/MINOR: checks: don't consider errno and use conn->err_code
The last fix on checks (02b0f58: BUG/MEDIUM: checks: fix a long-standing
issue with reporting connection errors) tried to isolate error codes
retrieved from the socket in order to report appropriate messages. The
only thing is that we must not pre-initialize err to errno since we're
not in I/O context anymore and errno will be the one of the last syscall
(whatever it was). However we can complete the message with more info
from the transport layer (eg: SSL can inform us we were in a handshake).
Also add a catch-all case for CO_FL_ERROR when the connection was
established. No check currently seem to leave this case open, but better
catch it because it's hard to find all possible cases.
Error handling in checks is complex because some stuff must be done in
the central task (mandatory at least for timeouts) and other stuff is
done closer to the data.
Since checks have their own buffers now, we could move everything to
the main task and only keep the low-level I/O for sending/retrieving
data to/from this buffer. It would also avoid sending logs from the
I/O context!
Willy Tarreau [Tue, 3 Dec 2013 14:42:33 +0000 (15:42 +0100)]
BUG/MEDIUM: checks: fix a long-standing issue with reporting connection errors
In 1.5-dev14 we fixed a bug induced by the new connection system which caused
handshake failures not to be reported in health checks. It was done with
commit 6c560da (BUG/MEDIUM: checks: report handshake failures). This fix
caused another issue which is that every check getting a TCP RST after a
valid response was flagged as error. This was fixed using commit c5c61fc
(BUG/MEDIUM: checks: ignore late resets after valid responses).
But because of this, we completely miss the status report. These two fixes
only set the check result as failed and did not call set_server_check_status()
to pass the information to upper layers.
The impact is that some failed checks are reported as INI or are simply not
updated if they happen fast enough (eg: TCP RST in response to connect()
without data in a pure TCP check). So the server appears down but the check
status says "L4OK".
After commit 6c560da, the handshake failures have been correctly dealt with
and every error causes process_chk() to be called with the appropriate
information still present on the socket. So let's get the error code in
process_chk() instead and stop mangling it in wake_srv_chk().
Now both L4 and L6 checks are correctly reported.
This bug was first introduced in 1.5-dev12 so no backport is needed.
Willy Tarreau [Tue, 3 Dec 2013 10:11:34 +0000 (11:11 +0100)]
BUG/MEDIUM: checks: fix health check regression causing them to depend on declaration order
Since commit 4a74143 (MEDIUM: Paramatise functions over the check of a
server), the check type is inherited from the current proxy's check type
at the moment where the server is declared instead of when reviewing
server configs. This causes an issue where a health check is disabled
when the server is declared before the checks. In fact the server will
inherit the last known check type declared before the "server" line :
backend foo
# this server is not checked at all
server s1 1.1.1.1:80 check
option tcpchk
# this server is tcp-checked :
server s2 1.1.1.2:80 check
option httpchk
# this server is http-checked :
server s3 1.1.1.3:80 check
The fix consists in assigning the check type during the config review
phase where the config is stable. No backport is nedeed.
Willy Tarreau [Mon, 2 Dec 2013 23:51:09 +0000 (00:51 +0100)]
BUILD: log: silent a warning about isblank() with latest patches
Recent commit 06d97f9 (MEDIUM: log-format: relax parsing of '%' followed
by unsupported characters) caused the following warning on some compilers
since isblank is not always present :
src/log.c: In function 'parse_logformat_string':
src/log.c:453: warning: implicit declaration of function 'isblank'
As usual, replace it with the two values (space and tab).
Willy Tarreau [Mon, 2 Dec 2013 23:48:45 +0000 (00:48 +0100)]
BUG/MINOR: http: usual deinit stuff in last commit
We need to initialize the rdr_fmt list inconditionally. Using only
a redirect rule without an http-redirect may cause a crash during
deinit because of the list iterating from null.
Thierry FOURNIER [Fri, 29 Nov 2013 11:15:45 +0000 (12:15 +0100)]
MEDIUM: http: The redirect strings follows the log format rules.
We handle "http-request redirect" with a log-format string now, but we
leave "redirect" unaffected.
Note that the control of the special "/" case is move from the runtime
execution to the configuration parsing. If the format rule list is
empty, the build_logline() function does nothing.
Willy Tarreau [Mon, 2 Dec 2013 16:45:48 +0000 (17:45 +0100)]
MEDIUM: log-format: relax parsing of '%' followed by unsupported characters
At the moment when a '%' character is followed by any unhandled character,
it is considered as a variable name, and if it cannot be resolved, a warning
is emitted and the configuration goes on.
When we start using log-format for redirect rules, it may happen that some
people accidently use '%' instead of '%%' without understanding the cause
of the issue. Thus we do two things here :
- if a single '%' is followed by a blank or a digit, we fix it and emit a
warning explaining how this should be done ; this ensures that existing
configs continue to work ;
- if a single '%' is followed by an unknown variable name, we report it
and explain how to emit a verbatim '%' in case this is what the user
desired.
It searches the for input value from <map_file> using the <match_type>
matching method, and return the associated value converted to the type
<output_type>. If the input value cannot be found in the <map_file>,
the converter returns the <default_value>. If the <default_value> is
not set, the converter fails and acts as if no input value could be
fetched. If the <match_type> is not set, it defaults to "str".
Likewise, if the <output_type> is not set, it defaults to "str". For
convenience, the "map" keyword is an alias for "map_str" and maps a
string to another string. The following array contains contains the
list of all the map* converters.
+----+----------+---------+-------------+------------+
| `-_ out | | | |
| input `-_ | str | int | ip |
| / match `-_ | | | |
+---------------+---------+-------------+------------+
| str / str | map_str | map_str_int | map_str_ip |
| str / sub | map_sub | map_sub_int | map_sub_ip |
| str / dir | map_dir | map_dir_int | map_dir_ip |
| str / dom | map_dom | map_dom_int | map_dom_ip |
| str / end | map_end | map_end_int | map_end_ip |
| str / reg | map_reg | map_reg_int | map_reg_ip |
| int / int | map_int | map_int_int | map_int_ip |
| ip / ip | map_ip | map_ip_int | map_ip_ip |
+---------------+---------+-------------+------------+
The names are intentionally chosen to reflect the same match methods
as ACLs use.
Thierry FOURNIER [Tue, 26 Nov 2013 19:47:54 +0000 (20:47 +0100)]
MEDIUM: sample: let the cast functions set their output type
This patch allows each sample cast function to specify the sample
output type. The goal is to be able to emit an output type IPv4 or
IPv6 depending on what is found in the input if the next converter
is able to process them both.
The patch also adds a new pseudo type called "ADDR". This type is an
alias for IPV4 and IPV6 which is only used as an input type by converters
who want to express their compatibility with both address formats. It may
not be emitted.
The goal is to unify as much as possible the processing of IPv4 and IPv6
in order not to add extra keywords for the maps which act as converters,
but will match samples like ACLs do with their patterns.
Willy Tarreau [Mon, 2 Dec 2013 22:17:27 +0000 (23:17 +0100)]
MEDIUM: stick-tables: support automatic conversion from ipv4<->ipv6
Make the stick-table key converter automatically adapt to the address
family of the input sample. Samples such as "src" will return an address
with a sample type depending on the input family. We'll have to support
such combinations when we add support for maps because the output type
will not necessarily be fixed.
Thierry FOURNIER [Thu, 28 Nov 2013 17:22:00 +0000 (18:22 +0100)]
MEDIUM: pattern: rename "acl" prefix to "pat"
This patch just renames functions, types and enums. No code was changed.
A significant number of files were touched, especially the ACL arrays,
so it is likely that some external patches will not apply anymore.
One important thing is that we had to split ACL_PAT_* into two groups :
- ACL_TEST_{PASS|MISS|FAIL}
- PAT_{MATCH|UNMATCH}
A future patch will enforce enums on all these places to avoid confusion.
Thierry FOURNIER [Thu, 28 Nov 2013 10:05:19 +0000 (11:05 +0100)]
REORG: acl/pattern: extract pattern matching from the acl file and create pattern.c
This patch just moves code without any change.
The ACL are just the association between sample and pattern. The pattern
contains the match method and the parse method. These two things are
different. This patch cleans the code by splitting it.
Thierry FOURNIER [Fri, 22 Nov 2013 18:14:42 +0000 (19:14 +0100)]
MEDIUM: acl: associate "struct sample_storage" to each "struct acl_pattern"
This will be used later with maps. Each map will associate an entry with
a sample_storage value.
This patch changes the "parse" prototype and all the parsing methods.
The goal is to associate "struct sample_storage" to each entry of
"struct acl_pattern". Only the "parse" function can add the sample value
into the "struct acl_pattern".
Thierry FOURNIER [Tue, 26 Nov 2013 09:21:51 +0000 (10:21 +0100)]
MINOR: sample: Define new struct sample_storage
This struct is used to store a sample constant. The size of this
struct is less than the struct sample. This struct only contains
a constant and doesn't need the "ctx" nor the "flags".
Thierry FOURNIER [Fri, 22 Nov 2013 16:33:27 +0000 (17:33 +0100)]
MINOR: acl: Extract the pattern parsing and indexation from the "acl_read_patterns_from_file()" function
With this split, the pattern indexation can apply to any source. The map
feature needs this functionality because the map cannot be loaded with the
same file format as the ones supported by acl_read_patterns_from_file().
The code was only moved to its own function, no functional changes were made.
Thierry FOURNIER [Fri, 22 Nov 2013 15:16:59 +0000 (16:16 +0100)]
MINOR: tools: Add a function to convert buffer to an ipv6 address
The inet_pton function needs an input string with a final \0. This
function copies the input string to a temporary buffer, adds the final
\0 and converts to address.
Willy Tarreau [Tue, 26 Nov 2013 18:02:32 +0000 (19:02 +0100)]
DOC: add some information about how to apply converters to samples
We've had the feature for log-format, unique-id-format and add-header for
a while now. It has just been implemented for ACLs but some doc was still
lacking.
Thierry FOURNIER [Thu, 21 Nov 2013 09:50:10 +0000 (10:50 +0100)]
MEDIUM: acl: use the fetch syntax 'fetch(args),conv(),conv()' into the ACL keyword
If the acl keyword is a "fetch", the dedicated parsing function
"sample_parse_expr()" is used. Otherwise, the acl parsing function
"parse_acl_expr()" is extended to understand the syntax of a series
of converters placed after the "fetch" keyword.
Before this patch, each acl uses a "struct sample_fetch" and executes
it with the "<fetch>->process()" function. Now, the dedicated function
"sample_process()" is called.
These syntax are now avalaible:
acl bad req.hdr(host),lower -m str www
http-request redirect prefix /go-away if bad
acl bad hdr_beg(host),lower www
http-request redirect prefix /go-away if bad
Willy Tarreau [Mon, 2 Dec 2013 11:24:54 +0000 (12:24 +0100)]
BUG/MINOR: log: fix log-format parsing errors
Some errors were still reported as log-format instead of their respective
contexts (acl, request header, stick, ...). This is harmless and does not
require any backport.
Willy Tarreau [Mon, 2 Dec 2013 22:29:05 +0000 (23:29 +0100)]
BUG/MINOR: config: report the correct track-sc number in tcp-rules
When parsing track-sc* actions in tcp-request rules, we now automatically
compute the track-sc identifier number using %d when displaying an error
message. But the ID has become wrong since we introduced sc0, we continue
to report id+1 in error messages causing some confusion.
Willy Tarreau [Sun, 1 Dec 2013 20:46:24 +0000 (21:46 +0100)]
BUG/MINOR: backend: fix target address retrieval in transparent mode
A very old bug resulting from some code refactoring causes
assign_server_address() to refrain from retrieving the destination
address from the client-side connection when transparent mode is
enabled and we're connecting to a server which has address 0.0.0.0.
The impact is low since such configurations are unlikely to ever
be encountered. The fix should be backported to older branches.
Willy Tarreau [Thu, 28 Nov 2013 09:50:06 +0000 (10:50 +0100)]
BUG/MINOR: stats: do not report "via" on tracking servers in maintenance
When a server tracks another one, its state on the stats page always reports
"via xx/yy". That's convenient to know what server to act on to change the
state. But it is also possible to force the tracking server itself into
maintenance mode and in this case we should not report "via xx/yy" because
the tracked server can't do anything to change the server's state, which
is confusing. In practice there is nothing wrong in leaving it as-is,
except that it's highly misleading when looking at the stats page.
Note that we only change the HTML output, not the CSV one. The states are
already different : "MAINT" vs "MAINT(via)" and we expect anyone coding a
monitoring system based on the CSV output to know the differences between
all possible states.
Willy Tarreau [Thu, 28 Nov 2013 10:27:16 +0000 (11:27 +0100)]
BUG/MAJOR: check: fix haproxy crash during soft-stop/soft-start
This is the continuation of previous fix bc16cd8 "BUG/MAJOR: fix haproxy
crash when using server tracking instead of checks", the soft-stop/start
states were not addressed by this fix.
Willy Tarreau [Wed, 27 Nov 2013 15:52:23 +0000 (16:52 +0100)]
BUG/MAJOR: fix haproxy crash when using server tracking instead of checks
Igor at owind reported a very recent bug (just present in latest snapshot).
Commit "4a741432 MEDIUM: Paramatise functions over the check of a server"
causes up/down to die with tracked servers due to a typo.
The following call in set_server_down causes the server to put itself
down recurseively because "check" is the current server's check, so once
fed to the function again, it will pass through the exact same path (note
we have the exact symmetry in set_server_up) :
for (srv = s->tracknext; srv; srv = srv->tracknext)
if (!(srv->state & SRV_MAINTAIN))
/* Only notify tracking servers that are not already in maintenance. */
set_server_down(check);
Instead we should stop the tracking server being visited in the loop :
for (srv = s->tracknext; srv; srv = srv->tracknext)
if (!(srv->state & SRV_MAINTAIN))
/* Only notify tracking servers that are not already in maintenance. */
set_server_down(&srv->check);
But that's not exactly enough because srv->check->server is only set when
checks are enabled, so ->server is NULL for tracking servers, still causing a
crash upon first iteration. The fix is easy and consists in always initializing
check->server when creating a new server, which is what was already done a few
patches later by 69d29f9 (MEDIUM: cfgparse: Factor out check initialisation).
With the fix above alone on top of current version or snapshot 20131122, the
problem disappears.
Thanks to Igor for testing and reporting the issue.
Willy Tarreau [Mon, 25 Nov 2013 22:02:37 +0000 (23:02 +0100)]
MINOR: peers: accept to learn strings of different lengths
While analysing old bug (9d9179b) with Emeric, we first believed
that the fix was wrong and that there was a potential for learning
one extra character in the peers learning code for strings due to
the use of table->key_size instead of table->key_size-1. In fact it
cannot happen with a normally behaving sender because the key sizes
are compared when synchronizing the table.
But this unveiled a suboptimal handling of strings. It can be quite
common to see admins reload haproxy to increase some key sizes when
seeing that user agents or cookies get truncated, or conversely to
reduce them after seeing they take too much memory and are never full.
The problem is that this will get rid of the table's contents because
of the size mismatch. While this is understandable for properly
formatted data (eg: IP addresses, integers, SSLIDs...) it's too bad
for strings.
So instead, make an exception to accept string of incompatible lengths
and let the synchronization code truncate them to the appropriate size
just as if the keys were learned normally.
Thanks to this change, it is now possible to change the "len" parameter
of a string stick-table and restart without losing its contents.
Willy Tarreau [Mon, 25 Nov 2013 07:41:15 +0000 (08:41 +0100)]
OPTIM: connection: fold the error handling with handshake handling
Both of them are rare and are detected from the same flags source, so
let's detect errors in the handshake loop and remove two tests in the
fast path. This seems to improve overall performance by less than 0.5%
on connection-bound workloads.
These commands allow temporarily stopping and subsequently
re-starting an auxiliary agent check. The effect of this is as follows:
New checks are only initialised when the agent is in the enabled. Thus,
disable agent will prevent any new agent checks from begin initiated until
the agent re-enabled using enable agent.
When an agent is disabled the processing of an auxiliary agent check that
was initiated while the agent was set as enabled is as follows: All
results that would alter the weight, specifically "drain" or a weight
returned by the agent, are ignored. The processing of agent check is
otherwise unchanged.
The motivation for this feature is to allow the weight changing effects
of the agent checks to be paused to allow the weight of a server to be
configured using set weight without being overridden by the agent.
Simon Horman [Mon, 25 Nov 2013 01:46:38 +0000 (10:46 +0900)]
MEDIUM: Set rise and fall of agent checks to 1
This is achieved by moving rise and fall from struct server to struct check.
After this move the behaviour of the primary check, server->check is
unchanged. However, the secondary agent check, server->agent now has
independent rise and fall values each of which are set to 1.
The result is that receiving "fail", "stopped" or "down" just once from the
agent will mark the server as down. And receiving a weight just once will
allow the server to be marked up if its primary check is in good health.
This opens up the scope to allow the rise and fall values of the agent
check to be configurable, however this has not been implemented at this
stage.
Simon Horman [Mon, 25 Nov 2013 01:46:36 +0000 (10:46 +0900)]
MEDIUM: checks: Add supplementary agent checks
Allow an auxiliary agent check to be run independently of the
regular a regular health check. This is enabled by the agent-check
server setting.
The agent-port, which specifies the TCP port to use for the agent's
connections, is required.
The agent-inter, which specifies the interval between agent checks and
timeout of agent checks, is optional. If not set the value for regular
checks is used.
e.g.
server web1_1 127.0.0.1:80 check agent-port 10000
If either the health or agent check determines that a server is down
then it is marked as being down, otherwise it is marked as being up.
An agent health check performed by opening a TCP socket and reading an
ASCII string. The string should have one of the following forms:
* An ASCII representation of an positive integer percentage.
e.g. "75%"
Values in this format will set the weight proportional to the initial
weight of a server as configured when haproxy starts.
* The string "drain".
This will cause the weight of a server to be set to 0, and thus it
will not accept any new connections other than those that are
accepted via persistence.
* The string "down", optionally followed by a description string.
Mark the server as down and log the description string as the reason.
* The string "stopped", optionally followed by a description string.
This currently has the same behaviour as "down".
* The string "fail", optionally followed by a description string.
Simon Horman [Mon, 25 Nov 2013 01:46:35 +0000 (10:46 +0900)]
MEDIUM: Remove option lb-agent-chk
Remove option lb-agent-chk and thus the facility to configure
a stand-alone agent health check. This feature was added by
"MEDIUM: checks: Add agent health check". It will be replaced
by subsequent patches with a features to allow an agent check
to be run as either a secondary check, along with any of the existing
checks, or as part of an http check with the status returned
in an HTTP header.
This patch does not entirely revert "MEDIUM: checks: Add agent health
check". The infrastructure it provides to parse the results of an
agent health check remains and will be re-used by the planned features
that are mentioned above.
Simon Horman [Mon, 25 Nov 2013 01:46:34 +0000 (10:46 +0900)]
MEDIUM: Log agent fail, stopped or down as info
In the case where an agent check returns fail, stopped or down,
log this as info when logging the server status along with any
trailing message returned by the agent after fail, stopped or down.
Previously only the trailing message was logged as info and
if omitted no info was logged.
MEDIUM: systemd-wrapper: Kill child processes when interrupted
Send SIGINT to child processes when killed. This ensures that
the haproxy process managed by the systemd-wrapper is stopped
when "systemctl stop haproxy.service" is called.
Willy Tarreau [Thu, 21 Nov 2013 14:30:45 +0000 (15:30 +0100)]
MINOR: stats: report correct throttling percentage for servers in slowstart
The column used to report the throttle percentage when a server is in
slowstart is based on the time only. This is wrong, because server weights
in slowstart are updated at most once a second, so the reported value is
wrong at least fo rone second during each step, which means all the time
when using short delays (< 20s).
The second point is that it's disturbing to see a weight < 100% without
any throttle at the end of the period (during the last second), because
the effective weight has not yet been updated.
Instead, we now compute the exact ratio between eweight and uweight and
report it. It's always accurate and describes the value being used instead
of using only the date.
It can be backported to 1.4 though it's not particularly important.
Willy Tarreau [Thu, 21 Nov 2013 10:22:01 +0000 (11:22 +0100)]
BUG/MAJOR: server: weight calculation fails for map-based algorithms
A crash was reported by Igor at owind when changing a server's weight
on the CLI. Lukas Tribus could reproduce a related bug where setting
a server's weight would result in the new weight being multiplied by
the initial one. The two bugs are the same.
The incorrect weight calculation results in the total farm weight being
larger than what was initially allocated, causing the map index to be out
of bounds on some hashes. It's easy to reproduce using "balance url_param"
with a variable param, or with "balance static-rr".
It appears that the calculation is made at many places and is not always
right and not always wrong the same way. Thus, this patch introduces a
new function "server_recalc_eweight()" which is dedicated to this task
of computing ->eweight from many other elements including uweight and
current time (for slowstart), and all users now switch to use this
function.
The patch is a bit large but the code was not trivially fixable in a way
that could guarantee this situation would not occur anymore. The fix is
much more readable and has been verified to work with all algorithms,
with both consistent and map-based hashes, and even with static-rr.
Slowstart was tested as well, just like enable/disable server.
The same bug is very likely present in 1.4 as well, so the patch will
probably need to be backported eventhough it will not apply as-is.
Thanks to Lukas and Igor for the information they provided to reproduce it.
Willy Tarreau [Thu, 21 Nov 2013 10:50:50 +0000 (11:50 +0100)]
BUG/MEDIUM: checks: fix slow start regression after fix attempt
Commit 2e99390 (BUG/MEDIUM: checks: fix slowstart behaviour when server
tracking is in use) moved the slowstart task initialization within the
health check code and leaves it unset when checks are disabled. The
problem is that it's possible to trigger slowstart from the CLI by
issuing "disable server XXX / enable server XXX" even when checks are
disabled. The result is a crash when trying to wake up the slowstart
task of that server.
Move the task initialization earlier so that it is done even if the
checks are disabled.
This patch should be backported to 1.4 since the commit above was
backported there.
Godbach [Thu, 21 Nov 2013 02:21:22 +0000 (10:21 +0800)]
MINOR: buffer: align the last output line if there are less than 8 characters left
Commit c08057c does the align job for buffer_dump(), but it has not fixed the
issue that less than 8 characters are left in the last line as below:
Dumping contents from byte 0 to byte 119
0 1 2 3 4 5 6 7 8 9 a b c d e f
0000: 47 45 54 20 2f 69 6e 64 - 65 78 2e 68 74 6d 20 48 GET /index.htm H
0010: 54 54 50 2f 31 2e 30 0d - 0a 55 73 65 72 2d 41 67 TTP/1.0..User-Ag
...
0060: 6e 65 63 74 69 6f 6e 3a - 20 4b 65 65 70 2d 41 6c nection: Keep-Al
0070: 69 76 65 0d 0a 0d 0a ive....
The last line of the hex column is still overlapped by the text column. Since
there will be additional "- " for the output line which has no less than 8
characters, two additional spaces should be present when there is less than 8
characters in order to do alignment. The result after being fixed is as below:
Dumping contents from byte 0 to byte 119
0 1 2 3 4 5 6 7 8 9 a b c d e f
0000: 47 45 54 20 2f 69 6e 64 - 65 78 2e 68 74 6d 20 48 GET /index.htm H
0010: 54 54 50 2f 31 2e 30 0d - 0a 55 73 65 72 2d 41 67 TTP/1.0..User-Ag
...
0060: 6e 65 63 74 69 6f 6e 3a - 20 4b 65 65 70 2d 41 6c nection: Keep-Al
0070: 69 76 65 0d 0a 0d 0a ive....
Simon Horman [Sun, 24 Feb 2013 08:23:38 +0000 (17:23 +0900)]
MEDIUM: Add state to struct check
Add state to struct check. This is currently used to store one bit,
CHK_RUNNING, which is set if a check is running and clear otherwise.
This bit was previously SRV_CHK_RUNNING of the state element of struct
server.
This is in preparation for associating a agent check
with a server which runs as well as the server's existing check.
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Generally the server parameter of these functions has been removed.
Where it is still needed it is obtained using check->server.
This is in preparation for associating a agent check
with a server which runs as well as the server's existing check.
By paramatising these functions they may act on each of the checks
without further significant modification.
Explanation of the SSP_O_HCHK portion of this change:
* Prior to this patch SSP_O_HCHK serves a single purpose which
is to tell server_status_printf() weather it should print
the details of the check of a server or not.
With the paramatisation that this patch adds there are two cases.
1) Printing the details of the check in which case a
valid check parameter is needed.
2) Not printing the details of the check in which case
the contents check parameter are unused.
In case 1) we could pass SSP_O_HCHK and a valid check and;
In case 2) we could pass !SSP_O_HCHK and any value for check
including NULL.
If NULL is used for case 2) then SSP_O_HCHK becomes supurfulous
and as NULL is used for case 2) SSP_O_HCHK has been removed.
Simon Horman [Sat, 23 Feb 2013 01:16:43 +0000 (10:16 +0900)]
MEDIUM: Split up struct server's check element
This is in preparation for associating a agent check
with a server which runs as well as the server's existing check.
The split has been made by:
* Moving elements of struct server's check element that will
be shared by both checks into a new check_common element
of struct server.
* Moving the remaining elements to a new struct check and
making struct server's check element a struct check.
* Adding a server element to struct check, a back-pointer
to the server element it is a member of.
- At this time the server could be obtained using
container_of, however, this will not be so easy
once a second struct check element is added to struct server
to accommodate an agent health check.
Willy Tarreau [Mon, 18 Nov 2013 17:33:22 +0000 (18:33 +0100)]
BUG/MINOR: http: fix build warning introduced with url32/url32_src
commit 39c63c5 "url32+src - like base32+src but whole url including parameters"
was missing the last argument "const char *kw", resulting in the build warning
below :
src/proto_http.c:10351:2: warning: initialization from incompatible pointer type [enabled by default]
src/proto_http.c:10351:2: warning: (near initialization for 'sample_fetch_keywords.kw[50].process') [enabled by default]
src/proto_http.c:10352:2: warning: initialization from incompatible pointer type [enabled by default]
src/proto_http.c:10352:2: warning: (near initialization for 'sample_fetch_keywords.kw[51].process') [enabled by default]
Willy Tarreau [Mon, 18 Nov 2013 17:04:25 +0000 (18:04 +0100)]
BUG/MEDIUM: http: fix possible parser crash when parsing erroneous "http-request redirect" rules
Baptiste Assmann reported a bug affecting the "http-request redirect"
parser. It may randomly crash when reporting an error message if the
syntax is not OK. It happens that this is caused by the output error
message pointer which was not initialized to NULL.
This bug is 1.5-specific (introduced in dev17), no backport is needed.
url32+src - like base32+src but whole url including parameters
I have a need to limit traffic to each url from each source address. much
like base32+src but the whole url including parameters (this came from
looking at the recent 'Haproxy rate limit per matching request' thread)
attached is patch that seems to do the job, its a copy and paste job of the
base32 functions
the url32 function seems to work too and using 2 machines to request the
same url locks me out of both if I abuse from either with the url32 key
function and only the one if I use url32_src.
Willy Tarreau [Sat, 16 Nov 2013 09:28:23 +0000 (10:28 +0100)]
CLEANUP: http: merge error handling for req* and http-request *
The reqdeny/reqtarpit and http-request deny/tarpit were using
a copy-paste of the error handling code because originally the
req* actions used to maintain their own stats. This is not the
case anymore so we can use the same error blocks for both.
The http-request rulesets still has precedence over req* so no
functionality was changed.
Willy Tarreau [Sat, 16 Nov 2013 09:13:35 +0000 (10:13 +0100)]
CLEANUP: http: homogenize processing of denied req counter
The reqdeny/reqideny and reqtarpit/reqitarpit rules used to maintain
the stats counters themselves while http-request deny/tarpit and
rspdeny/rspideny used to centralize them at the point where the
error is processed.
Thus, let's do the same for reqdeny/reqtarpit so that the functions
which iterate over the rules do not have to deal with these counters
anymore.
When a connection is tarpitted, a denied req is counted once when the
action is applied, and then a failed req is counted when the tarpit
timeout expires. This is completely wrong as the tarpit is exactly
equivalent to a deny since it's a disguised deny.
So let's not increment the failed req anymore.
This fix may be backported to 1.4 which has the same issue.
Willy Tarreau [Fri, 15 Nov 2013 23:15:40 +0000 (00:15 +0100)]
OPTIM/MINOR: mark the source address as already known on accept()
Commit 986a9d2d12 moved the source address from the stream interface
to the session, but it did not set the flag on the connection to
report that the source address is known. Thus when logs are enabled,
we had a call to getpeername() which is redundant with the result
from accept(). This patch simply sets the flag.
The reason for the failed recv() call is that the reading was marked
as speculative while we already have a polled I/O there. So we already
know when removing send write poll that the read is pending. Thus,
let's improve this by merging speculative I/O into polled I/O when
polled state changes. The result is now the following as expected :
This is specific to epoll(), it doesn't make much sense at the moment
to do so for other pollers, because the cost of updating them is very
small.
The average performance gain on small requests is of 1.6% in TCP mode,
which is easily explained with the syscall stats below for 10000 forwarded
connections :
Willy Tarreau [Thu, 14 Nov 2013 13:30:35 +0000 (14:30 +0100)]
MEDIUM: backend: add support for the wt6 hash
This function was designed for haproxy while testing other functions
in the past. Initially it was not planned to be used given the not
very interesting numbers it showed on real URL data : it is not as
smooth as the other ones. But later tests showed that the other ones
are extremely sensible to the server count and the type of input data,
especially DJB2 which must not be used on numeric input. So in fact
this function is still a generally average performer and it can make
sense to merge it in the end, as it can provide an alternative to
sdbm+avalanche or djb2+avalanche for consistent hashing or when hashing
on numeric data such as a source IP address or a visitor identifier in
a URL parameter.
Bhaskar Maddala [Tue, 5 Nov 2013 16:54:02 +0000 (11:54 -0500)]
MEDIUM: backend: Implement avalanche as a modifier of the hashing functions.
Summary:
Avalanche is supported not as a native hashing choice, but a modifier
on the hashing function. Note that this means that possible configs
written after 1.5-dev4 using "hash-type avalanche" will get an informative
error instead. But as discussed on the mailing list it seems nobody ever
used it anyway, so let's fix it before the final 1.5 release.
The default values were selected for backward compatibility with previous
releases, as discussed on the mailing list, which means that the consistent
hashing will still apply the avalanche hash by default when no explicit
algorithm is specified.
Examples
(default) hash-type map-based
Map based hashing using sdbm without avalanche
(default) hash-type consistent
Consistent hashing using sdbm with avalanche
Additional Examples:
(a) hash-type map-based sdbm
Same as default for map-based above
(b) hash-type map-based sdbm avalanche
Map based hashing using sdbm with avalanche
(c) hash-type map-based djb2
Map based hashing using djb2 without avalanche
(d) hash-type map-based djb2 avalanche
Map based hashing using djb2 with avalanche
(e) hash-type consistent sdbm avalanche
Same as default for consistent above
(f) hash-type consistent sdbm
Consistent hashing using sdbm without avalanche
(g) hash-type consistent djb2
Consistent hashing using djb2 without avalanche
(h) hash-type consistent djb2 avalanche
Consistent hashing using djb2 with avalanche
Bhaskar [Wed, 30 Oct 2013 03:30:51 +0000 (23:30 -0400)]
MEDIUM: backend: Enhance hash-type directive with an algorithm options
Summary:
In testing at tumblr, we found that using djb2 hashing instead of the
default sdbm hashing resulted is better workload distribution to our backends.
This commit implements a change, that allows the user to specify the hash
function they want to use. It does not limit itself to consistent hashing
scenarios.
The supported hash functions are sdbm (default), and djb2.
For a discussion of the feature and analysis, see mailing list thread
"Consistent hashing alternative to sdbm" :
Note: This change does NOT make changes to new features, for instance,
applying an avalance hashing always being performed before applying
consistent hashing.
Willy Tarreau [Thu, 14 Nov 2013 15:00:12 +0000 (16:00 +0100)]
BUG/MINOR: acl: remove patterns from the tree before freeing them
A call to free_pattern_tree() upon exit() is made to free all ACL
patterns allocated in a tree (strings or IP addresses). Unfortunately
it happens that this function has been bogus from the beginning, it
walks over the whole tree, frees the nodes but forgets to remove them
from the tree prior to freeing them. So after visiting a leaf, the
next eb_next() call will require to revisit some of the upper nodes
that were just freed. This can remain unnoticed for a long time because
free() often just marks the area as free. But in cases of aggressive
memory freeing, the location will not be mapped anymore and the process
segfaults.
Note that the bug has no impact other than polluting kernel logs and
frightening sysadmins, since it happens just before exit().
Simply adding the debug code below makes it easier to reproduce the
same bug :
while (node) {
next = eb_next(node);
+ node->node_p = (void *)-1;
free(node);
node = next;
}
Many thanks to the StackExchange team for their very detailed bug report
that permitted to quickly understand this non-obvious bug!
This fix should be backported to 1.4 which introduced the bug.
Willy Tarreau [Mon, 4 Nov 2013 17:09:12 +0000 (18:09 +0100)]
MINOR: acl: add a warning when an ACL keyword is used without any value
It's quite common to write directives like the following :
tcp-request reject if WAIT_END { sc0_inc_gpc0 }
This one will never reject, because sc0_inc_gpc0 is provided no value
to compare against. The proper form should have been something like this :
tcp-request reject if WAIT_END { sc0_inc_gpc0 gt 0 }
or :
tcp-request reject if WAIT_END { sc0_inc_gpc0 -m found }
Now we detect the absence of any argument on the command line and emit
a warning suggesting alternatives or the use of "--" to really avoid
matching anything (might be used when debugging).