Reject PP/SR HTTP requests if cross shard semaphore has been exhausted #15977

michael-redpanda · 2024-01-05T21:26:14Z

Cross shard calls in SR/PP utilize an SMP resource group to limit the number of cross shard calls that
are made. However, if the semaphore has been fully consumed, the call simply hangs. Customers have
experienced issues where, under heavy load, SR appears to hang and not response. Now, instead, once the
semaphore has been completely exhausted, new HTTP requests that attempt cross shard calls will be
rejected with a 500 error.

Backports Required

Release Notes

Improvements

SR/PP will now reply with a 500 error if an internal service semaphore has been completely exhausted

michael-redpanda · 2024-01-05T21:27:31Z

src/v/config/configuration.cc

+  , pp_sr_smp_max_non_local_requests(
+      *this,
+      "pp_sr_smp_max_non_local_requests",
+      "Maximum number of x-core requests pending in Panda Proxy and Schema "
+      "Registry seastar::smp group.  (for more details look at "
+      "`seastar::smp_service_group` documentation)",
+      {.needs_restart = needs_restart::yes, .visibility = visibility::user},
+      std::nullopt)


I don't believe we backport things if there's a new cluster config (right?). If we do want this backported, I can drop the commits that add this and submit the change in a separate PR.

src/v/config/configuration.cc

Signed-off-by: Michael Boquard <michael@redpanda.com>

graphcareful · 2024-01-10T22:03:15Z

Do we need the SMP resource group anymore?

src/v/pandaproxy/schema_registry/util.h

michael-redpanda · 2024-01-11T00:08:52Z

Do we need the SMP resource group anymore?

I'd like @BenPope 's take on this question, I think it's a valid one.

src/v/pandaproxy/schema_registry/util.h

BenPope · 2024-01-11T09:57:30Z

Do we need the SMP resource group anymore?

I'd like @BenPope 's take on this question, I think it's a valid one.

I'm not sure what the suggested alternative is?

src/v/pandaproxy/server.h

BenPope

This doesn't seem to limit the number of in-flight requests; so does it fix #14116?

I.e., would it be better to limit the number of in-flight requests by applying backpressure rather than rejecting valid requests?

michael-redpanda · 2024-01-11T14:05:04Z

This doesn't seem to limit the number of in-flight requests; so does it fix #14116?

The issue that we had in relation to the incident in #14116 was that new requests were just hanging out in the semaphores wait list but we had no idea that that was happening. By checking whether the semaphore has been exhausted before initiating the cross-shard call, we can reject the new request.

I.e., would it be better to limit the number of in-flight requests by applying backpressure rather than rejecting valid requests?

I'm not sure how the behavior to the client would be any different. How would you limit the number of in-flight requests without rejecting new ones?

BenPope · 2024-01-11T14:41:36Z

This doesn't seem to limit the number of in-flight requests; so does it fix #14116?

The issue that we had in relation to the incident in #14116 was that new requests were just hanging out in the semaphores wait list but we had no idea that that was happening. By checking whether the semaphore has been exhausted before initiating the cross-shard call, we can reject the new request.

It's not just new requests, though, it's requests that may be half-processed.

I.e., would it be better to limit the number of in-flight requests by applying backpressure rather than rejecting valid requests?

I'm not sure how the behavior to the client would be any different. How would you limit the number of in-flight requests without rejecting new ones?

By not processing them until semaphore can be obtained (or timed-out), Unfortunately, we don't yet stream the body, so it's not ideal for something like a produce right now.

Signed-off-by: Michael Boquard <michael@redpanda.com>

If inflight semaphore is exhausted, then return a 429 error. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-01-12T21:26:06Z

Force push 5a4d0ed:

Refactored solution to add new adjustable sempahore that prevents too many inflight requests, per shard

graphcareful · 2024-01-16T16:03:52Z

Do we need the SMP resource group anymore?

I'd like @BenPope 's take on this question, I think it's a valid one.

I'm not sure what the suggested alternative is?

Within the smp resource config there is an option:

max_nonlocal_requests: The maximum number of non-local requests that execute on a shard concurrently

Correct me if i'm wrong but doesn't this configurable option solve the same issue that Mike is attempting to solve with the adjustable semaphore?

EDIT: I guess i'll try to answer my own question, maybe the logical difference here is the smp group enforces the max is number of futures vs what this PR is attempting to solve which is the max number of "requests"

BenPope · 2024-01-16T16:12:56Z

Within the smp resource config there is an option:
max_nonlocal_requests: The maximum number of non-local requests that execute on a shard concurrently
Correct me if i'm wrong but doesn't this configurable option solve the same issue that Mike is attempting to solve with the adjustable semaphore?

EDIT: I guess i'll try to answer my own question, maybe the logical difference here is the smp group enforces the max is number of futures vs what this PR is attempting to solve which is the max number of "requests"

I suspect that the problem is that cross-core requests are being made several times without "unwinding" first; a request can get stuck half-way waiting on the semaphore. Timing it out isn't ideal as only some of the work may have been applied.

Limiting requests much lower than the nonlocal limit should resolve this problem.

BenPope

Looks good.

BenPope · 2024-01-16T17:11:08Z

src/v/pandaproxy/rest/proxy.cc

+    _inflight_config_binding.watch(
+      [this]() { _inflight_sem.set_capacity(_inflight_config_binding()); });


_inflight_sem could be destructed at this point (I don't see another mechanism that unsubscribes the watch, or otherwise sequences the destruction order.

BenPope · 2024-01-16T17:11:22Z

src/v/pandaproxy/schema_registry/service.cc

+    _inflight_config_binding.watch(
+      [this]() { _inflight_sem.set_capacity(_inflight_config_binding()); });


_inflight_sem could be destructed at this point (I don't see another mechanism that unsubscribes the watch, or otherwise sequences the destruction order.

Isn't destruction sequence order dictated by the class? _inflight_config_binding is constructed after _inflight_sem and therefor should be destructed before the semaphore?

BTW I find relying on destructor order is incredibly sneaky and usually deserves a comment somewhere

Isn't destruction sequence order dictated by the class? _inflight_config_binding is constructed after _inflight_sem and therefor should be destructed before the semaphore?

You're correct.

dotnwat

lgtm

BenPope · 2024-01-17T11:39:22Z

Why is this considered "not a bug fix" and not backported?

michael-redpanda self-assigned this Jan 5, 2024

michael-redpanda requested review from BenPope and oleiman January 5, 2024 21:26

github-actions bot added the area/redpanda label Jan 5, 2024

michael-redpanda requested review from dotnwat and graphcareful January 5, 2024 21:26

michael-redpanda commented Jan 5, 2024

View reviewed changes

michael-redpanda marked this pull request as draft January 6, 2024 00:08

michael-redpanda removed request for dotnwat, BenPope, oleiman and graphcareful January 6, 2024 00:15

rockwotj reviewed Jan 7, 2024

View reviewed changes

src/v/config/configuration.cc Outdated Show resolved Hide resolved

michael-redpanda force-pushed the issues/14116 branch from 47f1acc to 3359a67 Compare January 10, 2024 21:22

redpanda-data deleted a comment from vbotbuildovich Jan 10, 2024

michael-redpanda requested review from BenPope, oleiman, graphcareful and rockwotj January 10, 2024 21:23

michael-redpanda added 2 commits January 10, 2024 16:27

config: Added config to set SG for proxy

9b11ec3

Signed-off-by: Michael Boquard <michael@redpanda.com>

app: Wire up PP SR x-shard config

9bedb5e

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda force-pushed the issues/14116 branch from 3359a67 to b58bba6 Compare January 10, 2024 21:28

michael-redpanda marked this pull request as ready for review January 10, 2024 21:28

rockwotj previously approved these changes Jan 10, 2024

View reviewed changes

src/v/pandaproxy/schema_registry/util.h Outdated Show resolved Hide resolved

BenPope reviewed Jan 11, 2024

View reviewed changes

src/v/pandaproxy/schema_registry/util.h Outdated Show resolved Hide resolved

BenPope reviewed Jan 11, 2024

View reviewed changes

src/v/pandaproxy/server.h Outdated Show resolved Hide resolved

BenPope reviewed Jan 11, 2024

View reviewed changes

michael-redpanda added 6 commits January 12, 2024 11:05

config: Added config for in flight PP/SR restriction

f7707d2

Signed-off-by: Michael Boquard <michael@redpanda.com>

utils: Added try_get_units to adjustable_semaphore

e088ac3

Signed-off-by: Michael Boquard <michael@redpanda.com>

pp: Wire in in-flight semaphore

260d1d5

Signed-off-by: Michael Boquard <michael@redpanda.com>

sr: Wired in in-flight sempahore

b9c0384

Signed-off-by: Michael Boquard <michael@redpanda.com>

pp/sr: Added adjustable inflight semaphore to context

dc0d6cf

Signed-off-by: Michael Boquard <michael@redpanda.com>

pp/sr: Added handling of inflight semaphore

5a4d0ed

If inflight semaphore is exhausted, then return a 429 error. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda dismissed rockwotj’s stale review via 5a4d0ed January 12, 2024 21:25

michael-redpanda force-pushed the issues/14116 branch from b58bba6 to 5a4d0ed Compare January 12, 2024 21:25

michael-redpanda requested review from BenPope and rockwotj January 12, 2024 21:25

BenPope reviewed Jan 16, 2024

View reviewed changes

BenPope self-requested a review January 16, 2024 19:32

BenPope approved these changes Jan 16, 2024

View reviewed changes

michael-redpanda merged commit a99419e into redpanda-data:dev Jan 16, 2024
19 checks passed

dotnwat reviewed Jan 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject PP/SR HTTP requests if cross shard semaphore has been exhausted #15977

Reject PP/SR HTTP requests if cross shard semaphore has been exhausted #15977

michael-redpanda commented Jan 5, 2024

michael-redpanda Jan 5, 2024

graphcareful commented Jan 10, 2024

michael-redpanda commented Jan 11, 2024

BenPope commented Jan 11, 2024

BenPope left a comment

michael-redpanda commented Jan 11, 2024 •

edited

Loading

BenPope commented Jan 11, 2024

michael-redpanda commented Jan 12, 2024 •

edited

Loading

graphcareful commented Jan 16, 2024 •

edited

Loading

BenPope commented Jan 16, 2024 •

edited

Loading

BenPope left a comment

BenPope Jan 16, 2024

BenPope Jan 16, 2024

michael-redpanda Jan 16, 2024

rockwotj Jan 17, 2024

BenPope Jan 17, 2024

dotnwat left a comment

BenPope commented Jan 17, 2024

		_inflight_config_binding.watch(
		[this]() { _inflight_sem.set_capacity(_inflight_config_binding()); });

Reject PP/SR HTTP requests if cross shard semaphore has been exhausted #15977

Reject PP/SR HTTP requests if cross shard semaphore has been exhausted #15977

Conversation

michael-redpanda commented Jan 5, 2024

Backports Required

Release Notes

Improvements

michael-redpanda Jan 5, 2024

Choose a reason for hiding this comment

graphcareful commented Jan 10, 2024

michael-redpanda commented Jan 11, 2024

BenPope commented Jan 11, 2024

BenPope left a comment

Choose a reason for hiding this comment

michael-redpanda commented Jan 11, 2024 • edited Loading

BenPope commented Jan 11, 2024

michael-redpanda commented Jan 12, 2024 • edited Loading

graphcareful commented Jan 16, 2024 • edited Loading

BenPope commented Jan 16, 2024 • edited Loading

BenPope left a comment

Choose a reason for hiding this comment

BenPope Jan 16, 2024

Choose a reason for hiding this comment

BenPope Jan 16, 2024

Choose a reason for hiding this comment

michael-redpanda Jan 16, 2024

Choose a reason for hiding this comment

rockwotj Jan 17, 2024

Choose a reason for hiding this comment

BenPope Jan 17, 2024

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

BenPope commented Jan 17, 2024

michael-redpanda commented Jan 11, 2024 •

edited

Loading

michael-redpanda commented Jan 12, 2024 •

edited

Loading

graphcareful commented Jan 16, 2024 •

edited

Loading

BenPope commented Jan 16, 2024 •

edited

Loading