David Carlier [Sat, 20 Apr 2024 06:18:48 +0000 (07:18 +0100)]
MEDIUM: shctx: Naming shared memory context
From Linux 5.17, anonymous regions can be name via prctl/PR_SET_VMA
so caches can be identified when looking at HAProxy process memory
mapping.
The most possible error is lack of kernel support, as a result
we ignore it, if the naming fails the mapping of memory context
ought to still occur.
BUILD: stick-tables: silence build warnings when threads are disabled
Since 3.0-dev7 with commit 1a088da7c2 ("MAJOR: stktable: split the keys
across multiple shards to reduce contention"), building without threads
yields a warning about the shard not being used. This is because the
locks API does nothing of its arguments, which is the only place where
the shard is being used. We cannot modify the lock API to pretend to
consume its argument because quite often it's not even instantiated.
Let's just pretend we consume shard using an explict ALREADY_CHECKED()
statement instead. While we're at it, let's make sure that XXH32() is
not called when there is a single bucket!
BUG/MEDIUM: applet: Let's applets decide if they have more data to deliver
Unlike the muxes, the applets have the responsibility to notify the SC if
they have more data to deliver to the stream. The same is done to notify the
SC that applets must be woken up ASAP to continue some processing. When an
applet is woken up, we pretend it has no more data to deliver by setting
SE_FL_HAVE_NO_DATA flag. If the applet removes this flag, we must take care
to not set it again just after. Otherwise, the applet may remain blocked if
there is no other condition to wake it up.
It is an issue for the applets using their own buffers because
SE_FL_HAVE_NO_DATA is erroneously set in sc_applet_recv() function, after
the applet execution. For instance, it happens for the cli applet when a
huge map is cleared. No data are delivered to the stream but we pretend it
is the case to clear the map per batches.
This patch should fix the issue #2543. No Backported needed.
Some flags are defined during statistics generation and output. They use
the prefix STAT_* which is also used for other purposes. Rename them
with the new prefix STAT_F_* to differentiate them from the other
usages.
MINOR: stats: use stricter naming stats/field/line
Several unique names were used for different purposes under statistics
implementation. This caused the code to be difficult to understand.
* stat/stats name is removed when a more specific name could be used
* restrict field usage to purely refer to <struct field> which
represents a raw stat value.
* use "line" naming to represent an array of <struct field>
Info are used to expose haproxy global metrics. It is similar to proxy
statistics and any other module. As such, rename info indexes using
SI_I_INF_* prefix. Also info variable is renamed stat_line_info.
Thanks to this, naming is now consistent between info and other
statistics. It will help to integrate it as a "global" statistics
module.
MINOR: stats: rename ambiguous stat_l and stat_count
Statistics were extended with the introduction of stats module. This
mechanism allows to expose various metrics for several haproxy
components. As a consequence of this, some static variables were
transformed to dynamic ones to be able to regroup all statistics
definition.
Rename these variables with more explicit naming :
* stat_lines can be used to generate one line of statistics for any
module using struct field as value
* metrics and metrics_len are used to stored description of metrics
indexed by module
Note that info is not integrated in the statistics module mechanism.
However, it could be done in the future to better reflect its purpose.
This commit is the first one of a serie which adjust naming convention
for stats module. The objective is to remove ambiguity and better
reflect how stats are implemented, especially since the introduction of
stats module.
This patch renames elements related to proxies statistics. One of the
main change is to rename ST_F_* statistics indexes prefix with the new
name ST_I_PX_*. This remove the reference to field which represents
another concept in the stats module. In the same vein, global
stat_fields variable is renamed metrics_px.
This commit is part of a series to align counters usage between
frontends/listeners on one side and backends/servers on the other.
On frontend side, "stot" is the total count of sessions for both proxies
and listeners. For proxies, fe_counters <cum_sess> is correctely used.
The bug is on listeners where <cum_conn> value is returned, which
instead indicates a number of connection. This commit fixes this by
returning <cum_sess> counter value for "stot" metric.
Along this fixes, use the opportunity to report "conn_tot" for listeners
using <cum_conn> value, as for frontend proxies.
This commit fixes a bug but must not be backported as stats output is
changed.
BUG/MINOR: backend: use cum_sess counters instead of cum_conn
This commit is part of a serie to align counters usage between
frontends/listeners on one side and backends/servers on the other.
"stot" metric refers to the total number of sessions. On backend side,
it is interpreted as a number of streams. Previously, this was accounted
using <cum_sess> be_counters field for servers, but <cum_conn> instead
for backend proxies.
Adjust this by using <cum_sess> for both proxies and servers. As such,
<cum_conn> field can be removed from be_counters.
Note that several diagnostic messages which reports total frontend and
backend connections were adjusted to use <cum_sess>. However, this is an
outdated and misleading information as it does reports streams count on
backend side. These messages should be fixed in a separate commit.
32bits build was broken because of wrong printf length modifier.
src/ssl_ckch.c:4144:66: error: format specifies type 'long' but the argument has type 'unsigned int' [-Werror,-Wformat]
4143 | memprintf(err, "parsing [%s:%d] : cannot parse '%s' value '%s', too long, max len is %ld.\n",
| ~~~
| %u
4144 | file, linenum, args[cur_arg], args[cur_arg + 1], sizeof(alias_name));
| ^~~~~~~~~~~~~~~~~~
src/ssl_ckch.c:4217:64: error: format specifies type 'long' but the argument has type 'unsigned int' [-Werror,-Wformat]
4216 | memprintf(err, "parsing [%s:%d] : cannot parse '%s' value '%s', too long, max len is %ld.\n",
| ~~~
| %u
4217 | file, linenum, args[cur_arg], args[cur_arg + 1], sizeof(alias_name));
| ^~~~~~~~~~~~~~~~~~
2 errors generated.
make: *** [Makefile:1034: src/ssl_ckch.o] Error 1
make: *** Waiting for unfinished jobs....
Released version 3.0-dev8 with the following main changes :
- BUG/MINOR: cli: Don't warn about a too big command for incomplete commands
- BUG/MINOR: listener: always assign distinct IDs to shards
- BUG/MINOR: log: fix lf_text_len() truncate inconsistency
- BUG/MINOR: tools/log: invalid encode_{chunk,string} usage
- BUG/MINOR: log: invalid snprintf() usage in sess_build_logline()
- CLEANUP: log: lf_text_len() returns a pointer not an integer
- MINOR: quic: simplify qc_send_hdshk_pkts() return
- MINOR: quic: uniformize sending methods for handshake
- MINOR: quic: improve sending API on retransmit
- MINOR: quic: use qc_send_hdshk_pkts() in handshake IO cb
- MEDIUM: quic: remove duplicate hdshk/app send functions
- OPTIM: quic: do not call qc_send() if nothing to emit
- OPTIM: quic: do not call qc_prep_pkts() if everything sent
- BUG/MEDIUM: http-ana: Deliver 502 on keep-alive for fressh server connection
- BUG/MINOR: http-ana: Fix TX_L7_RETRY and TX_D_L7_RETRY values
- BUILD: makefile: warn about unknown USE_* variables
- BUILD: makefile: support USE_xxx=0 as well
- BUG/MINOR: guid: fix crash on invalid guid name
- BUILD: atomic: fix peers build regression on gcc < 4.7 after recent changes
- BUG/MINOR: debug: make sure DEBUG_STRICT=0 does work as documented
- BUILD: cache: fix non-inline vs inline declaration mismatch to silence a warning
- BUILD: debug: make DEBUG_STRICT=1 the default
- BUILD: pools: make DEBUG_MEMORY_POOLS=1 the default option
- CI: update the build options to get rid of unneeded DEBUG options
- BUILD: makefile: get rid of the config CFLAGS variable
- BUILD: makefile: allow to use CFLAGS to append build options
- BUILD: makefile: drop the SMALL_OPTS settings
- BUILD: makefile: move -O2 from CPU_CFLAGS to OPT_CFLAGS
- BUILD: makefile: get rid of the CPU variable
- BUILD: makefile: drop the ARCH variable and better document ARCH_FLAGS
- BUILD: makefile: extract ARCH_FLAGS out of LDFLAGS
- BUILD: makefile: move the fwrapv option to STD_CFLAGS
- BUILD: makefile: make the ERR variable also support 0
- BUILD: makefile: add FAILFAST to select the -Wfatal-errors behavior
- BUILD: makefile: extract -Werror/-Wfatal-errors from automatic CFLAGS
- BUILD: makefile: split WARN_CFLAGS from SPEC_CFLAGS
- BUILD: makefile: rename SPEC_CFLAGS to NOWARN_CFLAGS
- BUILD: makefile: do not pass warnings to VERBOSE_CFLAGS
- BUILD: makefile: also drop DEBUG_CFLAGS
- CLEANUP: makefile: make the output of the "opts" target more readable
- DOC: install: clarify the build process by splitting it into subsections
- BUG/MINOR: server: fix slowstart behavior
- BUG/MEDIUM: cache/stats: Handle inbuf allocation failure in the I/O handler
- MINOR: ssl: add the section parser for 'crt-store'
- DOC: configuration: Add 3.12 Certificate Storage
- REGTESTS: ssl: test simple case of crt-store
- MINOR: ssl: rename ckchs_load_cert_file to new_ckch_store_load_files_path
- MINOR: ssl/crtlist: alloc ssl_conf only when a valid keyword is found
- BUG/MEDIUM: stick-tables: fix the task's next expiration date
- CLEANUP: stick-tables: always respect the to_batch limit when trashing
- BUG/MEDIUM: peers/trace: fix crash when listing event types
- BUG/MAJOR: stick-tables: fix race with peers in entry expiration
- DEBUG: pool: improve decoding of corrupted pools
- REORG: pool: move the area dump with symbol resolution to tools.c
- DEBUG: pools: report the data around the offending area in case of mismatch
- MINOR: listener/protocol: add proto name in alerts
- MINOR: proto_quic: add proto name in alert
- BUG/MINOR: lru: fix the standalone test case for invalid revision
- DOC: management: fix typos
- CI: revert kernel addr randomization introduced in 3a0fc864
- MINOR: ring: clarify the usage of ring_size() and add ring_allocated_size()
- BUG/MAJOR: ring: use the correct size to reallocate startup_logs
- MINOR: ring: always check that the old ring fits in the new one in ring_dup()
- CLEANUP: ssl: remove dead code in cfg_parse_crtstore()
- MINOR: ssl: supports crt-base in crt-store
- MINOR: ssl: 'key-base' allows to load a 'key' from a specific path
- MINOR: net_helper: Add support for floats/doubles.
- BUG/MEDIUM: grpc: Fix several unaligned 32/64 bits accesses
- MINOR: peers: Split resync process function to separate running/stopping states
- MINOR: peers: Add 2 peer flags about the peer learn status
- MINOR: peers: Add flags to report the peer state to the resync task
- MINOR: peers: sligthly adapt part processing the stopping signal
- MINOR: peers: Add functions to commit peer changes from the resync task
- BUG/MINOR: peers: Report a resync was explicitly requested from a thread-safe manner
- BUG/MAJOR: peers: Update peers section state from a thread-safe manner
- MEDIUM: peers: Only lock one peer at a time in the sync process function
- MINOR: peer: Restore previous peer flags value to ease debugging
- BUG/MEDIUM: stconn: Don't forward channel data if input data must be filtered
- BUILD: cache: fix a build warning with gcc < 7
- BUILD: xxhash: silence a build warning on Solaris + gcc-5.5
- CI: reduce ASAN log redirection umbrella size
- CLEANUP: assorted typo fixes in the code and comments
- BUG/MEDIUM: evports: do not clear returned events list on signal
- MEDIUM: evports: permit to report multiple events at once
- MEDIUM: ssl: support aliases in crt-store
- BUG/MINOR: ssl: check on forbidden character on wrong value
- BUG/MINOR: ssl: fix crt-store load parsing
- BUG/MEDIUM: applet: Fix applet API to put input data in a buffer
- BUG/MEDIUM: spoe: Always retry when an applet fails to send a frame
- BUG/MEDIUM: peers: Fix exit condition when max-updates-at-once is reached
- BUILD: linuxcap: Properly declare prepare_caps_from_permitted_set()
- BUG/MEDIUM: peers: fix localpeer regression with 'bind+server' config style
- MINOR: peers: stop relying on srv->addr to find peer port
- MEDIUM: ssl: support a named crt-store section
- MINOR: stats: remove implicit static trash_chunk usage
- REORG: stats: extract HTML related functions
- REORG: stats: extract JSON related functions
- MEDIUM: ssl: crt-base and key-base local keywords for crt-store
- MINOR: stats: Get the right prototype for stats_dump_html_end().
- MAJOR: ssl: use the msg callback mecanism for backend connections
- MINOR: ssl: implement keylog fetches for backend connections
- BUG/MINOR: stconn: Fix sc_mux_strm() return value
- MINOR: mux-pt: Test conn flags instead of sedesc ones to perform a full close
- MINOR: stconn/connection: Move shut modes at the SE descriptor level
- MINOR: stconn: Rewrite shutdown functions to simplify the switch statements
- MEDIUM: stconn: Use only one SC function to shut connection endpoints
- MEDIUM: stconn: Explicitly pass shut modes to shut applet endpoints
- MEDIUM: stconn: Use one function to shut connection and applet endpoints
- MEDIUM: muxes: Use one callback function to shut a mux stream
- BUG/MINOR: sock: handle a weird condition with connect()
- BUG/MINOR: fd: my_closefrom() on Linux could skip contiguous series of sockets
- BUG/MEDIUM: peers: Don't set PEERS_F_RESYNC_PROCESS flag on a peer
- BUG/MEDIUM: peers: Fix state transitions of a peer
- MINOR: init: use RLIMIT_DATA instead of RLIMIT_AS
- CI: modernize macos matrix
let's stick to macos-13 for stable branches and macos-14 for development branches.
since macos-14 is available for Apple Silicon, some modifications are required
for VTest (should be ported to VTest later)
Limiting total allocatable process memory (VSZ) via setting RLIMIT_AS limit is
no longer effective, in order to restrict memory consumption at run time.
We can see from process memory map below, that there are many holes within
the process VA space, which bumps its VSZ to 1.5G. These holes are here by
many reasons and could be explaned at first by the full randomization of
system VA space. Now it is usually enabled in Linux kernels by default. There
are always gaps around the process stack area to trap overflows. Holes before
and after shared libraries could be explained by the fact, that on many
architectures libraries have a 'preferred' address to be loaded at; putting
them elsewhere requires relocation work, and probably some unshared pages.
Repetitive holes of 65380K are most probably correspond to the header that
malloc has to allocate before asked a claimed memory block. This header is
used by malloc to link allocated chunks together and for its internal book
keeping.
This exceeded VSZ makes impossible to start an haproxy process with 200M
memory limit, set at its initialization stage as RLIMIT_AS. We usually
have in this case such cryptic output at stderr:
At the same time the process RSS (a memory really used) is only 75,5M.
So to make process memory accounting more realistic let's base the memory
limit, set by -m option, on RSS measurement and let's use RLIMIT_DATA instead
of RLIMIT_AS.
RLIMIT_AS was used before, because earlier versions of haproxy always allocate
memory buffers for new connections, but data were not written there
immediately. So these buffers were not instantly counted in RSS, but were
always counted in VSZ. Now we allocate new buffers only in the case, when we
will write there some data immediately, so using RLIMIT_DATA becomes more
appropriate.
BUG/MEDIUM: peers: Fix state transitions of a peer
The commit 9425aeaffb ("BUG/MAJOR: peers: Update peers section state from a
thread-safe manner") introduced regressions about state transitions of a
peer.
A peer may be in a connected, accepted or released state. Before, changes for
these states were performed synchronously. Since the commit above, changes
are mainly performed in the sync process task.
The first regression was about the released then accepted state transition,
called the renewed state. In reality the state was always crushed by the
accepted state. After some review, the state was just removed to always
perform the cleanup in the sync process task before acknowledging the
connected or accepted states.
Then, a wakeup of the peer applet was missing from the sync process task
after the ack of connected or accepted states, blocking the applet.
Finally, when a peer is in released, connected or accepted state, we must
take care to wait the sync process task wakeup before trying to receive or
send messages.
This patch must only be backported if the above commit is backported.
BUG/MEDIUM: peers: Don't set PEERS_F_RESYNC_PROCESS flag on a peer
The bug was introduced by commit 9425aeaffb ("BUG/MAJOR: peers: Update peers
section state from a thread-safe manner"). A peers flags was set on a peer
by error. Just remove it.
This patch must only be backported if the above commit is backported.
BUG/MINOR: fd: my_closefrom() on Linux could skip contiguous series of sockets
We got a detailed report analysis showing that our optimization consisting
in using poll() to detect already closed FDs within a 1024 range has an
issue with the case where 1024 consecutive FDs are open (hence do not show
POLLNVAL) and none of them has any activity report. In this case poll()
returns zero update and we would just skip the loop that inspects all the
FDs to close the valid ones. One visible effect is that the called programs
might occasionally see some FDs being exposed in the low range of their fd
space, possibly making the process run out of FDs when trying to open a
file for example.
Note that this is actually a fix for commit b8e602cb1b ("BUG/MINOR: fd:
make sure my_closefrom() doesn't miss some FDs") that already faced a
more common form of this problem (incomplete but non-empty FDs reported).
BUG/MINOR: sock: handle a weird condition with connect()
As reported on github issue #2491, there's a very strange situation where
epoll_wait() appears to be reported EPOLLERR only (and not IN/OUT/HUP etc
as normally happens with EPOLLERR), and when connect() is called again to
check the state of the ongoing connection, it returns EALREADY, basically
saying "no news, please wait". This obviously triggers a wakeup loop. For
now it has remained impossible to reproduce this issue outside of the
reporter's environment, but that's definitely something that is impossible
to get out from.
The workaround here is to address the lowest level cause we can act on,
which is to avoid returning to wait if EPOLLERR was returned. Indeed, in
this case we know it will loop, so we must definitely take this one into
account. We only do that after connect() asks us to wait, so that a
properly established connection with a queued error at the end of an
exchange will not be diverted and will be handled as usual.
This should be backported to approximately all versions, at least as far
as 2.4 according to the reporter who observed it there.
Thanks to @donnyxray for their useful captures isolating the problem.
MEDIUM: muxes: Use one callback function to shut a mux stream
mux-ops .shutr and .shutw callback functions are merged into a unique
functions, called .shut. The shutdown mode is still passed as argument,
muxes are responsible to test it. Concretly, .shut() function of each mux is
now the content of the old .shutw() followed by the content of the old
.shutr().
MEDIUM: stconn: Use one function to shut connection and applet endpoints
se_shutdown() function is now used to perform a shutdown on a connection
endpoint and an applet endpoint. The same function is used for
both. sc_conn_shut() function was removed and appctx_shut() function was
updated to only deal with the applet stuff.
MEDIUM: stconn: Explicitly pass shut modes to shut applet endpoints
It is the same than the previous patch but for applets. Here there is
already only one function. But with this patch, appctx_shut() function was
modified to explicitly get shutdown mode as parameter. In addition
appctx_shutw() was removed.
MEDIUM: stconn: Use only one SC function to shut connection endpoints
The SC API to perform shutdowns on connection endpoints was unified to have
only one function, sc_conn_shut(), with read/write shut modes passed
explicitly. It means sc_conn_shutr() and sc_conn_shutw() were removed. The
next step is to do the same at the mux level.
MINOR: stconn/connection: Move shut modes at the SE descriptor level
CO_SHR_* and CO_SHW_* modes are in fact used by the stream-connectors to
instruct the muxes how streams must be shut done. It is then the mux
responsibility to decide if it must be propagated to the connection layer or
not. And in this case, the modes above are only tested to pass a boolean
(clean or not).
So, it is not consistant to still use connection related modes for
information set at an upper layer and never used by the connection layer
itself.
These modes are thus moved at the sedesc level and merged into a single
enum. Idea is to add more modes, not necessarily mutually exclusive, to pass
more info to the muxes. For now, it is a one-for-one renaming.
MINOR: mux-pt: Test conn flags instead of sedesc ones to perform a full close
In .shutr and .shutw callback functions, we must rely on the connection
flags (CO_FL_SOCK_RD_SH/WR_SH) to decide to fully close the connection
instead of using sedesc flags. At the end, for the PT multiplexer, it is
equivalent. But it is more logicial and consistent this way.
Since the begining, this function returns a pointer on an appctx while it
should be a void pointer. It is the caller responsibility to cast it to the
right type, the corresponding mux stream in this case.
However, it is not a big deal because this function is unused for now. Only
the unsafe one is used.
MINOR: stats: Get the right prototype for stats_dump_html_end().
When the stat code was reorganized, and the prototype to
stats_dump_html_end() was moved to its own header, it missed the function
arguments. Fix that.
MEDIUM: ssl: crt-base and key-base local keywords for crt-store
Add support for crt-base and key-base local keywords for the crt-store.
current_crtbase and current_keybase are filed with a copy of the global
keyword argument when a crt-store is declared, and updated with a new
path when the keywords are in the crt-store section.
The ckch_conf_kws[] array was updated with ¤t_crtbase and
¤t_keybase instead of the global_ssl ones so the parser can use
them.
The keyword must be used before any "load" line in a crt-store section.
Example:
crt-store web
crt-base /etc/ssl/certs/
key-base /etc/ssl/private/
load crt "site3.crt" alias "site3"
load crt "site4.crt" key "site4.key"
Extract functions related to HTML stats webpage from stats.c into a new
module named stats-html. This allows to reduce stats.c to roughly half
of its original size.
A static variable trash_chunk was used as implicit buffer in most of
stats output function. It was a oneline buffer uses as temporary storage
before emitting to the final applet or CLI buffer.
Replaces it by a buffer defined in show_stat_ctx structure. This allows
to retrieve it in most of stats output function. An additional parameter
was added for the function where context was not already used. This
renders the code cleaner and will allow to split stats.c in several
source files.
As a result of a new member into show_stat_ctx, per-command context max
size has increased. This forces to increase APPLET_MAX_SVCCTX to ensure
pool size is big enough. Increase it to 128 bytes which includes some
extra room for the future.
MINOR: peers: stop relying on srv->addr to find peer port
Now that peers entirely rely on peer->srv for connection settings, and
that it was confirmed that it works properly thanks to previous commit,
let's finish what we started in f6ae258 ("MINOR: peers: rely on srv->addr
and remove peer->addr") and stop using srv->addr to find out peers port
and instead rely on srv->svc_port as it's already done for other proxy
types.
BUG/MEDIUM: peers: fix localpeer regression with 'bind+server' config style
A dumb mistake was made in f6ae25858 ("MINOR: peers: rely on srv->addr
and remove peer->addr"). I completely overlooked the part where the bind
address settings are used as implicit server's address settings when the
peers are declared using the new bind+server config style (which is the
new recommended method to declare peers as it follows the same logic as
the one used in other proxy sections).
As such, the peers synchro fails to work between previous and new process
(localpeer mechanism) upon reload when declaring peers with way:
global
localpeer local
peers mypeers
bind 127.0.0.1:10001
server local
And one has to use the 'old' config style to make it work:
global
localpeer local
peers mypeers
peer local 127.0.0.1:10001
--
To fix the issue, let's explicitly set the server's addr:port
according to the bind's address settings (only the first listener is
considered) when local peer was declared using the 'bind+server' method.
Expected arguments were not specified in the
prepare_caps_from_permitted_set() function declaration. It is an issue for
some compilers, for instance clang. But at the end, it is unexpected and
deprecated.
No backport needed, except if f0b6436f57 ("MEDIUM: capabilities: check
process capabilities sets") is backported.
BUG/MEDIUM: peers: Fix exit condition when max-updates-at-once is reached
When a peer applet is pushing updates, we limit the number of update sent at
once via a global parameter to not spend too much time in the applet. On
interrupt, we claimed for more room to be woken up quickly. However, this
statement is only true if something was pushed in the buffer. Otherwise,
with an empty buffer, if the stream itself is not woken up, the applet
remains also blocked because there is no send activity on the other side to
unblock it.
In this case, instead of requesting more room, it is sufficient to state the
applet have more data to send.
BUG/MEDIUM: spoe: Always retry when an applet fails to send a frame
This bug is related to the previous one ("BUG/MEDIUM: spoe: Always retry
when an applet fails to send a frame"). applet_putblk() function retruns -1
on error and it should always be interpreted as a missing of room in the
buffer. However, on the spoe, this was processed as an I/O error.
BUG/MEDIUM: applet: Fix applet API to put input data in a buffer
applet_putblk and co were added to simplify applets. In 2.8, a fix was
pushed to deal with all errors as a room error because the vast majority of
applets didn't expect other kind of errors. The API was changed with the
commit 389b7d1f7b ("BUG/MEDIUM: applet: Fix API for function to push new
data in channels buffer").
Unfortunately and for unknown reason, the fix was totally failed. Checks on
channel functions were just wrong and not consistent. applet_putblk()
function is especially affected because the error is returned but no flag
are set on the SC to request more room. Because of this bug, applets relying
on it may be blocked, waiting for more room, and never woken up.
The crt-store load line parser relies on offsets of member of the
ckch_conf struct. However the new "alias" keyword as an offset to
-1, because it does not need to be used. Plan was to handle it that way
in the parser, but it wasn't supported yet. So -1 was still used in an
offset computation which was not used, but ASAN could see the problem.
This patch fixes the issue by using a signed type for the offset value,
so any negative value would be skipped. It also introduced a
PARSE_TYPE_NONE for the parser.
The crt-store load line now allows to put an alias. This alias is used
as the key in the ckch_tree instead of the certificate. This way an
alias can be referenced in the configuration with the '@/' prefix.
MEDIUM: evports: permit to report multiple events at once
Since the beginning in 2.0 the nevlist parameter was set to 1 before
calling port_getn(), which means that a single FD event will be reported
per polling loop. This is extremely inefficient, and all the code was
designed to use global.tune.maxpollevents. It looks like it's a leftover
of a temporary debugging change. No apparent issues were found by setting
it to a higher value, so better do that.
That code is not much used nowadays with Solaris disappearing from the
landscape, so even if this definitely was a bug, it's preferable not to
backport that fix as it could uncover other subtle bugs that were never
raised yet.
BUG/MEDIUM: evports: do not clear returned events list on signal
Since 2.0 with commit 0ba4f483d2 ("MAJOR: polling: add event ports
support (Solaris)"), the polling system on Solaris suffers from a
signal handling problem. It turns out that this API is very bizarre,
as reported events are automatically unregistered and their counter
is updated in the same variable that was used to pass the count on
input, making it difficult to handle certain error codes (how should
one handle ENOSYS for example?). And to complete everything, the API
is able to return both EINTR and an event if a signal is reported.
The code tries to deal with certain such cases (e.g. ETIME for timeout
can also report an event), otherwise it defaults to clearing the
event counter upon error. This has the effect that EINTR clears the
list of events, which are also automatically cleared from the set by
the system.
This is visible when using external checks where the SIGCHLD of the
leaving child causes a wakeup that ruins the event counter and causes
endless loops, apparently due to the queued inter-thread byte in the
pipe used to wake threads up that never gets removed in this case.
Note that extcheck would also deserve deeper investigation because it
can immediately re-trigger a check in such a case, which is not normal.
Removing the wiping of the nevlist variable fixes the problem.
This can be backported to all versions since it affects 2.0.
BUILD: xxhash: silence a build warning on Solaris + gcc-5.5
Testing an undefined macro emits warnings due to -Wundef, and we have
exactly one such case in xxhash:
include/import/xxhash.h:3390:42: warning: "__cplusplus" is not defined [-Wundef]
#if ((defined(sun) || defined(__sun)) && __cplusplus) /* Solaris includes __STDC_VERSION__ with C++. Tested with GCC 5.5 */
Let's just prepend "defined(__cplusplus) &&" before __cplusplus to
resolve the problem. Upstream is still affected apparently.
Gcc before 7 does really not like direct operations on cast pointers
such as "((struct a*)b)->c += d;". It turns our that we have exactly
that construct in 3.0 since commit 5baa9ea168 ("MEDIUM: cache: Save
body size of cached objects and track it on delivery").
It's generally sufficient to use an intermediary variable such as :
"({ (struct a*) _ = b; _; })->c +=d;" but that's ugly. Fortunately
DISGUISE() implicitly does something very similar and works fine, so
let's use that.
BUG/MEDIUM: stconn: Don't forward channel data if input data must be filtered
Once data are received and placed in a channel buffer, if it is possible,
outgoing data are immediately forwarded. But we must take care to not do so
if there is also pending input data and a filter registered on the
channel. It is especially important for HTX streams because the HTX may be
altered, especially the extra field. And it is indeed an issue with the HTTP
compression filter and the H1 multiplexer. The wrong chunk size may be
announced leading to an internal error.
This patch should fix the issue #2530. It must be backported to all stable
versions.
MINOR: peer: Restore previous peer flags value to ease debugging
The last fixes on the peers to improve the locking mechanism introduced new
peer flags and the value of some old flags was changed. This was done in the
commit 9b78e33837 ("MINOR: peers: Add 2 peer flags about the peer learn
status"). But, to ease the debugging of the peers team, old values are
restored.
This patch must be backported with the commit above.
MEDIUM: peers: Only lock one peer at a time in the sync process function
Thanks to all previous changes, it is now possible to stop locking all peers
at once in the resync process function. Peer are locked one after the
other. Wen a peer is locked, another one may be locked when all peer sharing
the same shard must be updated. Otherwise, at anytime, at most one peer is
locked. This should significantly improve the situation.
This patch depends on the following patchs:
* BUG/MAJOR: peers: Update peers section state from a thread-safe manner
* BUG/MINOR: peers: Report a resync was explicitly requested from a thread-safe manner
* MINOR: peers: Add functions to commit peer changes from the resync task
* MINOR: peers: sligthly adapt part processing the stopping signal
* MINOR: peers: Add flags to report the peer state to the resync task
* MINOR: peers: Add 2 peer flags about the peer learn status
* MINOR: peers: Split resync process function to separate running/stopping states
It may be good to backport it to 2.9. All the seris should fix the issue #2470.
BUG/MAJOR: peers: Update peers section state from a thread-safe manner
It is the main part of this series. In the peer applet, only the peer flags
are updated. It is now the responsibility of the resync process function to
check changes on each peer to update the peers section state accordingly.
Concretly, changes on the connection state (accepted, connected, released or
renewed) are first reported at the peer level and then handled in
__process_peer_state() function.
In the same manner, when the learn status of a peer changes, the peers
section state is no longer updated immediately. The resync task is woken up
to deal with this changes.
Thanks to these changes, the peers should be now really thread-safe.
This patch relies on the following ones:
* BUG/MINOR: peers: Report a resync was explicitly requested from a thread-safe manner
* MINOR: peers: Add functions to commit peer changes from the resync task
* MINOR: peers: sligthly adapt part processing the stopping signal
* MINOR: peers: Add flags to report the peer state to the resync task
* MINOR: peers: Add 2 peer flags about the peer learn status
* MINOR: peers: Split resync process function to separate running/stopping states
No bug was reported about the thread-safety of peers. Only a performance
issue was encountered with a huge number of peers (> 50). So there is no
reason to backport all these patches further than 2.9.
BUG/MINOR: peers: Report a resync was explicitly requested from a thread-safe manner
Flags on the peers section state must be updated from a thread-safe manner.
It is not true today. With this patch we take care PEERS_F_RESYNC_REQUESTED
flag is only set by the resync task. To do so, a peer flag is used. This
flag is only set once and never removed. It is juste used for debugging
purpose. So it is enough to set it on a peer and be sure to report it on the
peers section when the sync task is executed.
This patch relies on previous ones:
* MINOR: peers: Add functions to commit peer changes from the resync task
* MINOR: peers: sligthly adapt part processing the stopping signal
* MINOR: peers: Add flags to report the peer state to the resync task
* MINOR: peers: Add 2 peer flags about the peer learn status
* MINOR: peers: Split resync process function to separate running/stopping states
MINOR: peers: Add functions to commit peer changes from the resync task
For now, nothing is done in these functions. It is only a patch to prepare
the huge part of the refactoring about the locking mechanism of the peers.
These functions will be responsible to check peers state and their learn
status to update the peers section flags accordingly.
MINOR: peers: sligthly adapt part processing the stopping signal
The signal and the PEERS_F_DONOTSTOP flag are now handled in the loop on peers
to force sessions shutdown. We will need to loop on all peers to update their
state. It is easier this way.
MINOR: peers: Add flags to report the peer state to the resync task
As the previous patch, this patch is also part of the refactoring of peer
locking mechanisme. Here we add flags to represent a transitional state for
a peer. It will be the resync task responsibility to update the peers state
accordingly.
A peer may be in 4 transitional states:
* accepted : a connection was accepted from a peer
* connected: a connection to a peer was established
* release : a peer session was released
* renewed : a peer session was released because it was replaced by a new
one. Concretly, this will be equivalent to released+accepted
If none of these flags is set, it means the transition, if any, was
processed by the resync task, or no transition happened.
MINOR: peers: Add 2 peer flags about the peer learn status
PEER_F_LEARN_PROCESS and PEER_F_LEARN_FINISHED flags are added to help to
fix locking issue about peers. Indeed, a peer is able to update the peers
"section" state under its own lock. Because the resync task locks all peers
at once, there is no conflict at this level. But there is nothing to prevent
2 peers to update the peers state in same time. So it seems there is no real
issue here, but there is a theorical thread-safety issue here. And it means
the locking mechanism of the peers must be reviewed.
In this context, the 2 flags above will help to move all update of the peers
state in the scope of resync task. Each peer will be able to update its own
state and the resync task will be responsible to update the peers state
accordingly.
MINOR: peers: Split resync process function to separate running/stopping states
The function responsible to deal with resynchro between all peers is now split
in two subfunctions. The first one is used when HAProxy is running while the
other one is used in soft-stop case.
This patch is required to be able to refactor locking mechanism of the peers.
BUG/MEDIUM: grpc: Fix several unaligned 32/64 bits accesses
There were several places in grpc and its dependency protobuf where unaligned
accesses were done. Read accesses to 32 (resp. 64) bits values should be performed
by read_u32() (resp. read_u64()).
Replace these unligned read accesses by correct calls to these functions.
Same fixes for doubles and floats.
Such unaligned read accesses could lead to crashes with bus errors on CPU
archictectures which do not fix them at run time.
This patch depends on this previous commit: 861199fa71 MINOR: net_helper: Add support for floats/doubles.
Add crt-base support for "crt-store". It will be used by 'crt', 'ocsp',
'issuer', 'sctl' load line parameter.
In order to keep compatibility with previous configurations and scripts
for the CLI, a crt-store load line will save its ckch_store using the
absolute crt path with the crt-base as the ckch tree key. This way, a
`show ssl cert` on the CLI will always have the completed path.
BUG/MAJOR: ring: use the correct size to reallocate startup_logs
In 3.0-dev, with commit 7c9ce715c9 ("MINOR: ring: make callers use
ring_data() and ring_size(), not ring->buf"), we made startup_logs_dup()
use ring_size() to get the old ring size and pass it to ring_new() to
create a new ring. But due to the ambiguity of the allocate vs usable
size, this resulted in slightly shrinking the buffer compared to the
previous one, occasionally causing crashes if the first one was already
full of warnings, as seen in GH issue #2529. We need to use the allocated
size instead, thanks to the function brought by previous commit.
No backport is needed, this only affects 3.0-dev. Thanks to @felipewd
for the detailed report that allowed to spot the problem.
MINOR: ring: clarify the usage of ring_size() and add ring_allocated_size()
There's currently an abiguity around ring_size(), it's said to return
the allocated size but returns the usable size. We can't change it as
it's used everywhere in the code like this. Let's fix the comment and
add ring_allocated_size() instead for anything related to allocation.
BUG/MINOR: lru: fix the standalone test case for invalid revision
In 2.6, a build issue for LRU in standalone test mode was addressed by
commit bf9c07fd9 ("BUILD/DEBUG: lru: update the standalone code to
support the revision"), but using revision 1 while looking up rev 0
results in 100% misses. Let's fix this and commit with revision 0 as
well.
No backport is needed, this only happens when hacking on the code.
MINOR: listener/protocol: add proto name in alerts
Frontend and listen sections allow unlimited number of bind statements, it is
often, when there is a bind statement per supported protocol, like below:
listen test
mode http
bind quic4@0.0.0.0:443 name quic ssl crt ...
bind 0.0.0.0:443 name https ssl alpn http/1.1,h2 crt ...
bind 0.0.0.0:8080 ...
...
It seems useful to show corresponded protocol name in alerts and warnings,
when problem occures with port binding, connection resuming or sharding. This
helps to figure out immediately, which bind statement has a wrong setting or
which protocol module is the root cause of the issue.
DEBUG: pools: report the data around the offending area in case of mismatch
When the integrity check fails, it's useful to get a dump of the area
around the first faulty byte. That's what this patch does. For example
it now shows this before reporting info about the tag itself:
REORG: pool: move the area dump with symbol resolution to tools.c
This function is particularly useful to dump unknown areas watching
for opportunistic symbols, so let's move it to tools.c so that we can
reuse it a little bit more.
When a corruption was detected in an object, it's often said that the
tag doesn't match the pool, but it should also check if it matches the
location of an earlier pool_free() call, which happens when -dMcaller
is used. That's what we're doing now.
BUG/MAJOR: stick-tables: fix race with peers in entry expiration
In 2.9 with commit 7968fe3889 ("MEDIUM: stick-table: change the ref_cnt
atomically") we significantly relaxed the stick-tables locking when
dealing with peers by adjusting the ref_cnt atomically and moving it
out of the lock.
However it opened a tiny window that became problematic in 3.0-dev7
when the table's contention was lowered by commit 1a088da7c2 ("MAJOR:
stktable: split the keys across multiple shards to reduce contention").
What happens is that some peers may access the entry for reading at
the moment it's about to expire, and while the read accesses to push
the data remain unnoticed (possibly that from time to time we push
crap), but the releasing of the refcount causes a new write that may
damage anything else. The scenario is the following:
Here it's clear that the bottom part of peer_send_teachmsgs() believes
to be protected but may act on freed data.
This is more visible when enabling -dMtag,no-merge,integrity because
the ATOMIC_DEC(&ref_cnt) decrements one byte in the area, that makes
the eviction check fail while the tag has the address of the left
__stksess_free(), proving a completed pool_free() before the decrement,
and the anomaly there is pretty visible in the crash dump. Changing
INC()/DEC() with ADD(2)/DEC(2) shows that the byte is now off by two,
confirming that the operation happened there.
The solution is not very hard, it consists in checking for the ref_cnt
on the left after grabbing the lock, and doing both before deleting the
element, so that we have the guarantee that either the peer will not
take it or that it has already started taking it.
This was proven to be sufficient, as instead of crashing after 3s of
injection with 4 peers, 16 threads and 130k RPS, it survived for 15mn.
In order to stress the setup, a config involving 4+ peers, tracking
HTTP request with randoms and applying a bwlim-out filter with a
random key, with a client made of 160 h2 conns downloading 10 streams
of 4MB objects in parallel managed to trigger it within a few seconds:
backend bk
server s1 198.18.0.30:8000
server s2 198.18.0.34:8000
backend tbl
stick-table type ip size 1000k expire 1s store http_req_cnt,bytes_in_rate(1s),bytes_out_rate(1s) peers peers
This seems to be very dependent on the timing and setup though.
This will need to be backported to 2.9. This part of the code was
reindented with shards but the block should remain mostly unchanged.
The logic to apply is the same.
BUG/MEDIUM: peers/trace: fix crash when listing event types
Sending "trace peers event" on the CLI crashes because the event list
in the peers is not finished. This was introduced in 2.4 by commit d865935f32 ("MINOR: peers: Add traces to peer_treat_updatemsg().")
so this must be backported to 2.4.
CLEANUP: stick-tables: always respect the to_batch limit when trashing
When adding the shards support to tables with commit 1a088da7c ("MAJOR:
stktable: split the keys across multiple shards to reduce contention"),
the condition to stop eliminating entries based on the batch size being
reached is based on a pre-decrement of the max_search counter, but now
it goes back into the outer loop which doesn't check it, so next time
it does it when entering the next shard, it will become even more
negative and will properly stop, but at first glance it looks like an
int overflow (which it is not). Let's make sure the outer loop stops
on this condition so that we don't continue searching when the limit
is reached.
BUG/MEDIUM: stick-tables: fix the task's next expiration date
While changing the stick-table indexing that led to commit 1a088da7c
("MAJOR: stktable: split the keys across multiple shards to reduce
contention"), I met a problem with the task's expiration date being
incorrectly updated, I fixed it and apparently I committed the wrong
version :-/
The effect is that the task's date is only correctly reset if the
table is empty, otherwise the task wakes up again and is queued at
the previous date, eating 100% CPU. The tick_isfirst() must not be
used when storing the last result.
No backport is needed as this was only merged in 3.0-dev7.
MINOR: ssl/crtlist: alloc ssl_conf only when a valid keyword is found
crt-list will be enhanced with ckch_conf keywords, however these keywords
does not fill the 'ssl_conf' structure. So we don't need to allocate the
ssl_conf for every options between [ ] but only when we found a relevant
one.
BUG/MEDIUM: cache/stats: Handle inbuf allocation failure in the I/O handler
When cache and stats applets were changed to use their own buffers, a change
was also performed to no longer access the stream from the I/O
handller. Among other things, the HTTP start-line of the request is now
retrieved to get the method. But, when these changes were brought, the inbuf
buffer allocation failures were not handled.
It is of course not so common. But if this happens, a crash may be
experienced. To fix the issue, we now check for inbuf allocation failures
before accessing it.
We observed that a dynamic server which health check is down for longer
than slowstart delay at startup doesn't trigger the warmup phase, it
receives full traffic immediately. This has been confirmed by checking
haproxy UI, weight is immediately the full one (e.g. 75/75), without any
throttle applied. Further tests showed that it was similar if it was in
maintenance, and even when entering a down or maintenance state after
being up.
Another issue is that if the server is down for less time than
slowstart, when it comes back up, it briefly has a much higher weight
than expected for a slowstart.
An easy way to reproduce is to do the following:
- Add a server with e.g. a 20s slowstart and a weight of 10 in config
file
- Put it in maintenance using CLI (set server be1/srv1 state maint)
- Wait more than 20s, enable it again (set server be1/srv1 state ready)
- Observe UI, weight will show 10/10 immediately.
If server was down for less than 20s, you'd briefly see a weight and
throttle value that is inconsistent, e.g. 50% throttle value and a
weight of 5 if server comes back up after 10s before going back to
6% after a second or two.
Code analysis shows that the logic in server_recalc_eweight stops the
warmup task by setting server's next state to SRV_ST_RUNNING if it
didn't change state for longer than the slowstart duration, regardless
of its current state. As a consequence, a server being down or disabled
for longer than the slowstart duration will never enter the warmup phase
when it will be up again.
Regarding the weight when server comes back up, issue is that even if
the server is down, we still compute its next weight as if it was up,
hence when it comes back up, it can briefly have a much higher weight
than expected during slowstart, until the warmup task is called again
after last_change is updated.
DOC: install: clarify the build process by splitting it into subsections
The doc about the build process has grown to a point where it was painful
to read when searching a specific element. This commit cuts it into a few
sub-categories for ease of searching, and it also adds a summary of the
most commonly used makefile variables, their usage and default settings.
CLEANUP: makefile: make the output of the "opts" target more readable
"make opts" is nice because it shows all options being used, but it
does so in a copy-pastable way that aggregates everything on a single
line, rendering very poorly and making it hard to spot the relevant
variables.
Let's break lines and append a trailing backslash to them instead. This
gives something much more readable which remains copy-pastable. Options
can be inspected and more easily replicated. It was also verified that
copy-pasting the whole block after "make" does continue to work like
before and produces the same output.
This one is often used as a gateway to inject regular CFLAGS, even though
not designed for this. It's now ignored, but any attempt at setting it
reports a warning suggesting to use CFLAGS or ARCH_FLAGS instead.
BUILD: makefile: do not pass warnings to VERBOSE_CFLAGS
The VERBOSE_CFLAGS variable is used to report the CFLAGS used in the
output of "haproxy -vv". It currently contains all -Wxxx and -Wno-xxx
because there was no other possibility till now, but these warnings
are highly specific to the compiler version in use, are automatically
detected and do not bring value being presented here. Worse, by
encouraging users to copy-paste them, they can end up with warnings
that are not relevant to their build environment.
In addition, their presence there doesn't give indication of whether
or not they triggered, so they cannot vouch for the output code quality.
Better just not report them there. However, as part of VERBOSE_CFLAGS
they were also used to detect whether or not to rebuild, via build_opts,
so they still have to be explicitly mentioned there if we want to make
sure that changing warning options triggers a rebuild to see their
effect.
Now we can have more useful outputs like this one which indicate precisely
what to use to safely rebuild the executable:
BUILD: makefile: split WARN_CFLAGS from SPEC_CFLAGS
It's currently not possible to only set some -Wno... without breaking
the -W... and conversely. Let's split both sets apart so that it's now
possible to set -W... alone in WARN_CFLAGS to enable only some warnings,
and pass the -Wno... in SPEC_CFLAGS without losing the enabled ones.
BUILD: makefile: extract -Werror/-Wfatal-errors from automatic CFLAGS
The compiler-specific CFLAGS that are automatically detected (SPEC_CFLAGS)
are currently the ones affected by ERR and FAILFAST. Not only this makes
these options useless as soon as SPEC_CFLAGS is forced, but it also means
that any change to them causes a rebuild, so disabling FAILFAST or
enabling ERR to get more info on a faulty object causes a full rebuild
of all others. Let's just move them to ERROR_CFLAGS that only contains
these two. It's not documented outside of the makefile because it's not
supposed to be touched.
BUILD: makefile: add FAILFAST to select the -Wfatal-errors behavior
-Wfatal-errors is set by default and is not supported on older compilers.
Since it's part of all the automatically detected flags, it's painful to
remove when needed. Also it's a matter of taste, some developers might
prefer to get a long list of all errors at once, others prefer that the
build stops immediately after the root cause.
The default is now back to no -Wfatal-errors, and when FAILFAST is set to
any non-empty non-zero value, -Wfatal-errors is added:
BUILD: makefile: make the ERR variable also support 0
It's among the options that change a lot on the developer's side and it's
tempting to change from ERR=1 to ERR=0 on the make command line by reusing
the history, except it doesn't work. Let's explictily permit ERR=0 to
disable -Werror like ERR= does.
projects / thirdparty / haproxy.git / log
