Creating 24.2.x branch #109
Merged
Conversation
This file contains 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
Seastar http server implementation supports multiple listeners. It may be required for the handler logic to know which listener the connection is coming from. Added listener_idx field to `httpd::request` to allow handler recognize listener. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Since an exception carries some text for the response body text, the raising site might like to specify the content type if it's e.g. json. Signed-off-by: John Spray <jcs@vectorized.io>
This enables throwing a base_exception from a json request handler with a json payload inside it. Signed-off-by: John Spray <jcs@vectorized.io>
Signed-off-by: John Spray <jcs@vectorized.io>
Prior to this patch seastar only exposes one global metrics::impl::impl object which holds all metric related data for one application. This patch changes the implementation details such that multiple metrics::impl::impl objects can exist for any given application. Said objects are stored into a map on each shard and created dinamically whenever requested. A metrics::impl::impl is identified by an integer handle that acts as the key for the storage map. Implementation note: in order to avoid issues caused by the ordering of static thread_local objects I had to declare the storage in reactor.cc. (cherry picked from commit 585a8af)
This patch extends the metrics internal apis to use a specific metrics::impl::impl object identified by its integer handle. (cherry picked from commit 6ee4af7)
Add a public method to metric_groups_impl that exposes the handle of the internal implementation it is using. This is required in order for the metric_groups class to be able to reset itself to the configured implementation handle.
This patch extends the metrics user facing apis to use a specific metrics::impl::impl object identified by its integer handle. Note that the constructor of 'metric_groups' is marked explicit in this patch and updates two call sites where the constructor was used implicitly.
This patch removes two subsequent calls to `get_local_impl` and reuses the returned handle in that scope.
This patch extends the user facing prometheus apis allowing the user to specify the internal metrics implementation to be used through a handle. Additionally, 'add_prometheus_routes' now takes an argument that specifies the route on which to advertise the metrics. This enables different metrics "namespaces" to be served by different endpoints in isolation. (cherry picked from commit 6189522)
This patch extends the scollectd apis with the ability to select the internal metrics implementation to be used by providing a handle. (cherry picked from commit d4331d1)
This patch adds a 'get_skip_when_empy' getter to the 'registered_metric' class. It is used by follow-up patches in order to replicate metrics.
This patch adds private methods to the 'metrics::impl' class that deal with the creation of replicated metrics. They will be used to build the public api in future commits.
This patch adds private helpers to 'metrics::impl' that deal with the removal of replicated metric families from their destintation implementation. These methods will be used in subsequent commits to manage the lifetime of replicated metrics.
This patch adds a public method to the 'metrics::impl' class: 'set_metric_families_to_replicate'. When this method is called the families that match any of the specifications will be replicated on the specified destinations.
This patch extends the metric registration and unregistration processes to make them aware of metric replication. In the case of metric registration, if the new metric belongs to a family that matches one of the replication specs, then a replicated metric is created accordingly. For unregistration of a metric, the replicated metric is unregistered too if one exists.
This patch exposes a method in the public interface of the metrics
module ('replicate_metric_families'), which enables metric replication
internally for the requested metric families.
This commit extends the public interface of scheduling_group to expose usage statistics (e.g. runtime).
The problem with DNS queries is caused by the fact that the UDP socket is used concurrently i.e. write is scheduled while the other one is pending. DNS queries are based on c-ares library. The library is initialized with a set of functions (callbacks) that are called whenever there is a need to send, receive, create socket, connect or close. Fixed processing of send requests leading to assertion being triggered in reactor. The fix involves checking if send is already pending and if so returning an error. The error returned from send callback will instruct c-ares library to retry the query. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Redpanda uses this logger name for its http client and we choose to change the logger name in Seastar to avoid duplicate logger registration exceptions.
We sort of inadvertently picked up the change to io_uring when rebasing our seastar fork, but for the coming release we'd like to keep using aio to reduce risk and give sufficient time to do performance tests on io_uring. This effectively rolls back the upstream commit: eedca15 We simply put aio and epoll above io_uring in the available list, the default backend is the first one in that list Issue redpanda-data/redpanda#10105.
This change is to allow for the timer code in the stall detectors to be used in a profiler implementation. The hope is to be able to reuse a lot of the stall_detector codebase in the profiler without complicating the existing stall_detector implementation.
Moves some functions and classes defined in the stall_detector test to a separate header file so they can be reused in other tests.
Adds an include for <algorithm> as it's where `move_backward` is defined.
The check_for_work() function is called in the core reactor loop (and not just in the path where we are about to enter idle) and is assigned to a std::function object outside the loop. This forces all invocations of this function to go through the std::function machinery, which means that among other things they cannot be inlined. This does not appear to be intentional, rather the intent seems to have been to DRY this small function as it is called from two places in the reactor loop. Instead, use auto type to infer the lambda type and avoid assignment to std::function, leading to something akin to a direct call.
Before the reactor main loop a local pure_check_for_work std::function is created on the stack. This is only ever passed to the _idle_cpu_handler, which expects a ss::noncopyable_function. The type mismatch means we need to clone the std::function every time, creating a temporary object to bind to the seastar function. This causes the construction and destruction methods to show up as a non-trivial portion of performance profiles whenever the idle check occurs frequently enough. Instead, just change the type to non-copyable function so the reference can be passed directly without any copy.
In libgcc there is a critical section where the stack is being modified so execution can return to an exceptions landing pad. However, this partially modified state isn’t valid or specified by the dwarf info in the eh_frames. So a seg fault tends to occur when `backtrace()` tries to unwind through this partially modified stack and follows an invalid pointer in it.
Extends the metrics api to allow changing the aggregation labels of a metrics family. Otherwise one had to un-register every single metric instance in a metric family and then re-register with the changed aggregation labels. For metric families with thousands of instances (e.g.: histograms with lots of different labels) this is quite expensive. With this change we avoid the full reconstruction of the metrics family and all its metrics. Only the work associated with marking the metrics `dirty()` is needed then.
GnuTLS provides serial numbers not as a legible string of hex digits but as a byte array corresponding to an integer value up to 20 octets wide. The cert_info interface should reflect this. To that end, this commit updates the type of `cert_info::serial` to an non-null-terminated basic_sstring of uint8_t. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
As seen in scylladb#1784, having flush retry `io_submit` in a loop might be futile if the kernel internal queue is full and we need to reap completions to free up space for more iocb:s. This change breaks out of the loop as soon as `io_submit` fails and just moves the remaining entries in the iocbs queue into the front so more entries can be queued (otherwise, we'd need a circular queue that will penalize the error-free path for the unlikely event of an error) Fixes scylladb#1784 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
EAGAIN is expected here when "Insufficient resources are available to queue any iocbs" (see io_submit(2)). Abort on any other error, as those indicate an internal error on our side. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
The `aio_general_context` had the implicit assumption that in a single tick we would never queue more than `--max-networking-io-control-blocks` events/iocbs. This however ignores situations such as queuing multiple iocbs per socket per tick, having left over iocbs in the queue from previous ticks via the new EAGAIN handling or simply because a lot more sockets are in use which isn't prevented anywhere else. If this condition was hit (`last > end`) the reactor would just assert out and crash. To avoid this situation this patch introduces a backlog into which elements are being enqueued when the original array is full and which can grow unbounded. This mirrors how the `aio_storage_context` works which uses the `io_sink` for the same purpose. To avoid oversized allocations after startup the split into two separate data structures is needed (instead of just regrowing the array). Further the datastructure from which the iocbs are passed into `io_submit` needs to be in contigiuous memory (and also provide an API to use it which most containers don't). `std::deque` is used in the backlog to avoid oversized allocations in the backlog itself. The existing array solution for `iocbs` is kept to fulfill the contigiuous memory requirement. Further we slightly change how EAGAIN is handled. Instead of backshifting the array we keep the array as is and just track the `begin` of the array across `flush` calls. This is possible now as the backlog handling is in place. This introduces "batching" and prevents degenerate cases where only a single element is being submitted which would then result in repeated shifting of the whole array on each `flush` call. Given we use a chunked data structure like `std::deque` erasing from the front of the backlog is relatively cheap and does not require shifting all the elements in the backlog. Hence, the per-iocb overhead is amortized constant. Note that in general we try to submit as many iocbs per `io_submit` call. Given the new behaviour of not backshifting the iocb array and immediately backfilling from the backlog we might introduce `io_submit` calls that don't try to submit the max amount of iocbs. However we assume that if we ran into EAGAIN then either: - We are still behind the next time around: it's unlikely we would succeed in submitting all the iocbs anyway - We have now caugt up: we have introduced a single additional `io_submit` call which only submits `max_poll()/2` iocbs on average. The backlog will be drained at full `max_poll` per `io_submit`.
Signed-off-by: Ben Pope <ben@redpanda.com>
Passed up to application code as a tls::blob (basic_string_view<char>). Also provide the ability to fetch this blob directly via credentials_builder::get_trust_file_blob() Updates unit tests to accommodate the new reload callback signature. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Add the ability to disable the CPU profiler in regions with known issues. Specifically for Redpanda this is to disable stacktraces while Wasmtime is running, as JIT doesn't generate stack trace entries (for other reasons). Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
graphcareful
requested review from
Lazin,
dotnwat,
BenPope,
StephanDollberg,
ballard26,
rockwotj,
andrwng,
mmaslankaprv,
michael-redpanda,
pgellert,
oleiman and
travisdowns
|
@graphcareful - The ones w/ my name on them lgtm. |
|
Note that in
|
BenPope
reviewed
Apr 26, 2024
BenPope
reviewed
Apr 26, 2024
Comment on lines
+352
to
+356
| /// This is a low level method for use cases where it is very important to | ||
| /// avoid any allocations. The \arg writer will be passed a | ||
| /// internal::log_buf::inserter_iterator that allows it to write into the log | ||
| /// buffer directly, avoiding the use of any intermediary buffers. | ||
| void log(log_level level, log_writer& writer, format_info_t<> fmt = {}) noexcept { |
There was a problem hiding this comment.
Where is this used and can its use be avoided?
There was a problem hiding this comment.
BenPope
reviewed
Apr 26, 2024
| @@ -22,6 +22,7 @@ | |||
| #pragma once | |||
|
|
|||
| #ifndef SEASTAR_MODULE | |||
| #include <atomic> | |||
There was a problem hiding this comment.
Approved. This code is not upstream.
This reverts commit 9dc6e81.
graphcareful
force-pushed
the
ss-master-scratch
branch
from
c782d14
to
bcaac50
Compare
michael-redpanda
approved these changes
Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR creates a new branch with all of the upstream changes from seastar/master along with all of the changes that haven't yet been merged upstream that exist in our current fork, specifically the diff between seastar/master and 24.1.x. To achieve this normally we just perform a plain git rebase, however due to the fact that the order of the commits merged upstream did not always match the order of the commits as they entered our fork, the rebase was quiet difficult. Instead I opted to start with master and apply the missing commits on top. This is partly why the base of 24.2.x is seastar/master, that way reviewers can see only the differences I've applied between the forks.
How this was performed
The negative of such a process is that a commit may be missed due to human error. As such I've devised a strategy , using a master list of the delta between the two branches using the git diff command, I could determine which commits could be dropped (as they were already upstreamed) and which had to be cherry-picked on.
Diff between redpanda/24.1.x and upstream/master
on_signaldoesn't allochttps://redpandadata.atlassian.net/browse/CORE-2434