[pull] master from haproxy:master#542
Merged
pull[bot] merged 12 commits intoMu-L:masterfrom Nov 29, 2022
Merged
Conversation
…" output Stick-tables support sharding to multiple peers but there was no way to know to what shard an entry was going to be sent. Let's display this in the "show table" output to ease debugging.
ncb_blk structure is used to represent a block of data or a gap in a non-contiguous buffer. This is used in several functions for ncbuf implementation. Before this patch, ncb_blk was passed by value, which is sub-optimal. Replace this by const pointer arguments. This has the side-effect of suppressing a compiler warning reported in older GCC version : CC src/http_conv.o src/ncbuf.c: In function 'ncb_blk_next': src/ncbuf.c:170: warning: 'blk.end' may be used uninitialized in this function This should be backported up to 2.6.
ncbuf API relies on lot of small functions. Mark these functions as inline to reduce call invocations and facilitate compiler optimizations to reduce code size. This should be backported up to 2.6.
ncbuf can be compiled for haproxy or standalone to run unit test suite. For the latest mode, BUG_ON() macro has been re-implemented in a simple version. The inclusion of the default or the redefined macro relied on DEBUG_DEV. Change this to now rely on DEBUG_STRICT as this is activated for the default build. This change is safe as only BUG_ON_HOT() macro is used in ncbuf code, which is activated only with the default value DEBUG_STRICT=2. This should be backported up to 2.6.
qc_dgrams_retransmit() could reuse the same local list and could splice it two times to the packet number space list of frame to be send/resend. This creates a loop in this list and makes qc_build_frms() possibly endlessly loop when trying to build frames from the packet number space list of frames. Then haproxy aborts. This issue could be easily reproduced patching qc_build_frms() function to set <dlen> variable value to 0 after having built at least 10 CRYPTO frames and using ngtcp2 as client with 30% packet loss in both direction. Thank you to @gabrieltz for having reported this issue in GH #1903. Must be backported to 2.6.
Some issues such as #1929 seem to involve a task without timeout but we can't find the condition to reproduce this in the code. However, not having this info in the output doesn't help, so this patch adds the task pointer and its timeout (when the task is non-null). It may be useful to backport it.
We need to initialize the shard value in __stksess_init() because there is not necessarily a key to make it happen later, resulting in an uninitialized shard value appearing in the entry, typically when entries are learned from peers. This fixes commit 36d1565 ("MINOR: peers: Support for peer shards"), no backport is needed. Note however that it is not sufficient to completely fix the peers code, the shard value remains zero because the setting of the key was open-coded in the peers code and these parts were not identified when adding support for shards.
When I added commit 16b282f ("MINOR: stick-table: show the shard number in each entry's "show table" output"), I don't know how but I managed to mess up my reg tests since everything worked fine, most likely by running it on a binary built in the wrong branch. Several reg tests include some table outputs that were upset by the new "shard=" field. This test added them and revealed at the same time that entries learned over peers are not properly initialized, which will be fixed in a future series of fixes. This commit requires previous fix "BUG/MINOR: peers: always initialize the stksess shard value" so as not to trip on entries learned from peers.
If the client closes the connection while there is no pending outgoing data, the H1 connection must be released. However, it was switched to CLOSING state instead. Thus the client connection was closed on client timeout. It is side effect of the commif d1b5730 ("MINOR: mux-h1: Avoid useless call to h1_send() if no error is sent"). Before, the extra call to h1_send() was able to fix the H1C state. To fix the bug and make switch to close state (CLOSING or CLOSED) less errorprone, h1_close() helper function is systematically used. It is a 2.7-specific bug. No backport needed.
…the shard The function used to calculate the shard number currently requires a stktable_key on input for this. Unfortunately, it happens that peers currently miss this calculation and they do not provide stktable_key at all, instead they're open-coding all the low-level stick-table work (hence why it's missing). Thus we'll need to be able to calculate the shard number in keys coming from peers as well but the current API does not make it possible. This commit addresses this by inverting the order where the length and the shard number are used. Now the low-level function is independent on stksess and stktable_key, it takes a table, pointer and length and does all the job. The upper function takes care of the type and key to get the its length, and is for use only from stick-table code. This doesn't change anything except that the low-level one will be usable from outside (hence why it's exported now).
In peer_treat_updatemsg(), the lower layers of the stick-table code are reimplemented, and the key length is never really known for an entry being processed, it depends on the type being parsed and the moment where it's done. This makes it quite difficult to stuff some shard number calculation there. This patch adds a keylen local variable that is always set to the length of the current key depending on its type. It takes this opportunity for reducing redudant expressions involving this length and always using the new variable instead, limiting the risk of errors. Arguably that code would have been way simpler by creating a dummy stktable_key and passing it to stksess_new() as done anywhere else, but let's not change all that a few days before the release.
…updates If shards are in use, we must fill the shard number on incoming updates, otherwise some entries are assigned shard number zero, and may be broadcast everywhere once updated, instead of being sent only to the peers having the same shard number. This fixes commit 36d1565 ("MINOR: peers: Support for peer shards"). No backport is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )