cloud_storage_clients: stricter but simpler invariants for the pool #15681

nvartolomei · 2023-12-15T14:08:55Z

Backports Required

Release Notes

Bug Fixes

Fix a rare bug where http client connections would vanish from the connection pool leading to various operations hanging while waiting for an http client.

nvartolomei · 2023-12-15T14:09:23Z

/dt

nvartolomei · 2023-12-15T16:00:18Z

/dt

vbotbuildovich · 2023-12-15T19:08:21Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42883#018c6e74-8533-4bd2-a43d-d89958b8fb16

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42883#018c6e64-011b-4532-88b3-39986c08cb68

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42948#018c7740-e6bc-4146-b15d-1d9732355bb9

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42952#018c785b-7b37-4bdd-adf0-44baa5c07c93

nvartolomei · 2023-12-15T20:23:36Z

/cdt

nvartolomei · 2023-12-17T08:54:11Z

/dt

vbotbuildovich · 2023-12-17T11:20:52Z

new failures in https://buildkite.com/redpanda/redpanda/builds/42948#018c7740-e6c2-40d6-91d1-d17fedf2ace6:

"rptest.tests.e2e_iam_role_test.AWSRoleFetchTests.test_write"

nvartolomei · 2023-12-17T14:09:19Z

/dt

src/v/cloud_storage_clients/tests/client_pool_test.cc

src/v/cloud_storage_clients/client_pool.cc

can be used for synchronization in testing

Simplify the lease/borrow logic by never adding borrowed clients to the local pool.

nvartolomei · 2023-12-19T11:33:05Z

Force-push with dropping the unnecessary condition variable wait change and improvements for one of the tests.

andijcr · 2023-12-19T13:43:31Z

src/v/cloud_storage_clients/client_pool.cc

@@ -442,7 +442,7 @@ void client_pool::populate_client_pool() {
    _cvar.signal();
 }

-client_pool::http_client_ptr client_pool::make_client() const {
+client_pool::http_client_ptr client_pool::make_client() const noexcept {


Q: this work with us because an allocation failure gets handled with an abort and a stacktrace directly by the allocator, right?

That’s my assumption. The code using this method doesn’t seem to be exception safe so I made make_client fail hard rather than cause surprising behaviour.

why it's not exception safe? it doesn't change any state in client_pool
it's OK for this method to be noexcept, I'm just curious

One example is when we’re borrowing from another shard. If make_client throws an exception there, then we would never return the borrow.

got it, so it's the code in a different place is not exception safe if this method could throw

andijcr · 2023-12-19T14:01:19Z

src/v/cloud_storage_clients/client_pool.cc

+    vassert(
+      _pool.size() < _capacity,
+      "tried to release a client but the pool is at capacity");
+    _pool.emplace_back(std::move(leased));


that's a pretty big change of behavior, is this why the test was hanging?

It is related to this, yes. The change in behavior doesn’t have a material effect. Used ManyPartitionsTest with tiered storage to confirm that.

src/v/cloud_storage_clients/client_pool.cc

andijcr · 2023-12-19T14:22:39Z

seems good, but should we wait for the LRC to have a chance to test this and in case backport on 23.3?

nvartolomei · 2023-12-19T14:37:33Z

@andijcr can you elaborate on the LRC bit? My plan is to backport this to 23.3 and 23.2. You mean to delay backporting to 23.2?

andijcr · 2023-12-19T14:52:55Z

@andijcr can you elaborate on the LRC bit? My plan is to backport this to 23.3 and 23.2. You mean to delay backporting to 23.2?

I meant delay the backport to 23.3 to have a chance to test that the new vassert in release_one is not triggered, if this pr is not a fix for an incident. not sure if we plan to run LRC with dev, was just a suggestion

Lazin · 2023-12-19T19:43:22Z

src/v/cloud_storage_clients/client_pool.h

+    // A gate for background operations. Most useful in testing where we want
+    // to wait all async housekeeping to complete before asserting state
+    // invariants.
+    ss::gate _bg_gate;


is it useful for anything other than tests?

Only for testing.

Lazin · 2023-12-19T19:45:03Z

src/v/cloud_storage_clients/client_pool.cc

@@ -442,7 +442,7 @@ void client_pool::populate_client_pool() {
    _cvar.signal();
 }

-client_pool::http_client_ptr client_pool::make_client() const {
+client_pool::http_client_ptr client_pool::make_client() const noexcept {


why it's not exception safe? it doesn't change any state in client_pool
it's OK for this method to be noexcept, I'm just curious

vbotbuildovich · 2023-12-19T22:29:50Z

/backport v23.3.x

vbotbuildovich · 2023-12-19T22:29:51Z

/backport v23.2.x

vbotbuildovich · 2023-12-19T22:30:51Z

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-15681-v23.2.x-515 remotes/upstream/v23.2.x
git cherry-pick -x 286004247fd07bd2060c441e52a6ef257d96267c 8f68873e774dbb88a359d3ca561748892827fc00 a2141244a682c329a2e7d1c8a92be09bad48bd83 e2a762dd2c4db34d2ea18fad88d7ef82b8db5cb3 e16fe06f533f3ff210b1f778d4b7656236254ad7 a6111c0d45751422a301100c906185292ad74d90

Workflow run logs.

dotnwat · 2023-12-22T05:43:05Z

src/v/cloud_storage_clients/client_pool.cc

+                co_await ssx::with_timeout_abortable(
+                  _cvar.wait(), model::no_timeout, as);


@nvartolomei i don't think we should do this. when the abort source is delivered the waiting future is backgrounded. that's hard to analyze and maintain.

it looks like it might be safe in this case, but i'm still not 100% sure. in stop() when sem::broken() is called all of the waiter promises have their values set. but the continuation returned from wait() captures a reference to the condition variable and broken() doesn't wait on anything. what if seastar changed and inserted a continuation in that chain that captured a reference to _cvar to do something like implement some debugging feature? that'd be a potential use after free.

at the very least it looks like the _cvar.wait() future should be wrapped in a gate.

you could also use the sleeping variant of wait() and wake up periodically to poll the abort source. not as efficient, but it is clear and simple.

you could also do something like

as.subscribe([this] { _cvar.signal(); }); _cvar.wait(); if (_abort_source.abort_requested()) { ... }

it's also not entirely clear why the abort source is needed. we already have a condition variable. whatever is requesting the abort, could just broadcast on the condition variable and then each fiber waiting on the cvar can figure out the state of the system and what to do.

there are probably other formulations too, but let's reserve future backgrounding for when it's really necessary.

I think that the future returned by cvar.wait() can safely outlive the cvar. So it shouldn't be a problem to background it.

Signaling _cvar from as.subscribe callback looks safe though.

I think you are onto something. We'd have to _cvar.broadcast() instead of signal as we need to wake up a particular waiter. Since we don't know which one, we have to wake them all. I don't like waking up all waiters.

Re backgrounding: I'm pretty sure it is safe to do. In the worst case we'd end up with a broken_promise exception from there. Agree that it is not desirable. Couldn't come up with anything better at this moment.

Shall condition_variable::wait have a method which accepts an abort_source?

I think that the future returned by cvar.wait() can safely outlive the cvar

maybe, but we should not have background futures that are not protected by a gate. is there an exception to this rule?

Couldn't come up with anything better at this moment.

let's background it under a gate, and think about a fix. we really shouldn't have backgrounded futures for cases like this. we can maybe integrate something into upstream seastar--i think that the continued safety of this approach is dependent on seastar cv implementation co-design--and that's a risk.

polling is a common thing we do (using the timeout variant of wait). it is very light weight and makes any concern of background and lifetime completely vanish.

dotnwat · 2023-12-22T05:53:42Z

src/v/cloud_storage_clients/client_pool.cc

@@ -18,6 +18,7 @@
 #include <seastar/core/smp.hh>


Simplify the lease/borrow logic by never adding borrowed clients to the
local pool.

it would be nice if this commit message explained why things are simpler. the changes in the commit are complicated enough that it's not obvious that it is now some how simpler.

As I have mentioned elsewhere, noted to myself to write less abstract commit messages in the future. Thank you for feedback! :thanks:

github-actions bot added the area/redpanda label Dec 15, 2023

nvartolomei force-pushed the nv/client-pool-metastable branch from be0c504 to dec87fd Compare December 15, 2023 16:00

nvartolomei force-pushed the nv/client-pool-metastable branch 2 times, most recently from 2025be0 to 3b2479e Compare December 16, 2023 20:22

nvartolomei changed the title ~~cloud_storage_clients: stricter invariants for the pool~~ cloud_storage_clients: stricter but simpler invariants for the pool Dec 16, 2023

nvartolomei marked this pull request as ready for review December 16, 2023 20:24

nvartolomei requested review from Lazin, dotnwat and andijcr December 16, 2023 20:24

nvartolomei force-pushed the nv/client-pool-metastable branch from 3b2479e to 8f474ba Compare December 17, 2023 14:09

Lazin reviewed Dec 18, 2023

View reviewed changes

src/v/cloud_storage_clients/tests/client_pool_test.cc Outdated Show resolved Hide resolved

src/v/cloud_storage_clients/client_pool.cc Outdated Show resolved Hide resolved

nvartolomei added 3 commits December 19, 2023 11:19

cloud_storage_clients: separate gate for housekeeping

2860042

can be used for synchronization in testing

cloud_storage_clients: noexcept make_client

8f68873

cloud_storage_clients: stricter but simpler invariants for the pool

a214124

Simplify the lease/borrow logic by never adding borrowed clients to the local pool.

piyushredpanda added this to the v23.3.1-rc5 milestone Dec 19, 2023

nvartolomei added 3 commits December 19, 2023 11:20

cloud_storage_clients: abortable acquire

e2a762d

cloud_storage_clients: keep the most recently used clients

e16fe06

cloud_storage_clients: fix test race conditions causing failures

a6111c0

nvartolomei force-pushed the nv/client-pool-metastable branch from 8f474ba to a6111c0 Compare December 19, 2023 11:30

andijcr reviewed Dec 19, 2023

View reviewed changes

andijcr approved these changes Dec 19, 2023

View reviewed changes

Lazin approved these changes Dec 19, 2023

View reviewed changes

nvartolomei merged commit 037380b into redpanda-data:dev Dec 19, 2023
19 checks passed

vbotbuildovich mentioned this pull request Dec 19, 2023

[v23.3.x] cloud_storage_clients: stricter but simpler invariants for the pool #15784

Merged

vbotbuildovich mentioned this pull request Dec 19, 2023

[v23.2.x] cloud_storage_clients: stricter but simpler invariants for the pool #15785

Closed

nvartolomei mentioned this pull request Dec 21, 2023

[v23.2.x] cloud_storage_clients: stricter but simpler invariants for the pool #15834

Merged

dotnwat reviewed Dec 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud_storage_clients: stricter but simpler invariants for the pool #15681

cloud_storage_clients: stricter but simpler invariants for the pool #15681

nvartolomei commented Dec 15, 2023 •

edited

nvartolomei commented Dec 15, 2023

nvartolomei commented Dec 15, 2023

vbotbuildovich commented Dec 15, 2023 •

edited

nvartolomei commented Dec 15, 2023

nvartolomei commented Dec 17, 2023

vbotbuildovich commented Dec 17, 2023

nvartolomei commented Dec 17, 2023

nvartolomei commented Dec 19, 2023 •

edited

andijcr Dec 19, 2023

nvartolomei Dec 19, 2023

Lazin Dec 19, 2023

nvartolomei Dec 19, 2023

Lazin Dec 22, 2023

andijcr Dec 19, 2023

nvartolomei Dec 19, 2023

andijcr commented Dec 19, 2023

nvartolomei commented Dec 19, 2023

andijcr commented Dec 19, 2023

Lazin Dec 19, 2023

nvartolomei Dec 19, 2023

Lazin Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

dotnwat Dec 22, 2023

Lazin Dec 22, 2023

nvartolomei Dec 22, 2023

nvartolomei Dec 22, 2023

dotnwat Dec 23, 2023

dotnwat Dec 23, 2023 •

edited

dotnwat Dec 23, 2023

dotnwat Dec 22, 2023

nvartolomei Dec 22, 2023

dotnwat Dec 23, 2023

		co_await ssx::with_timeout_abortable(
		_cvar.wait(), model::no_timeout, as);

cloud_storage_clients: stricter but simpler invariants for the pool #15681

cloud_storage_clients: stricter but simpler invariants for the pool #15681

Conversation

nvartolomei commented Dec 15, 2023 • edited

Backports Required

Release Notes

Bug Fixes

nvartolomei commented Dec 15, 2023

nvartolomei commented Dec 15, 2023

vbotbuildovich commented Dec 15, 2023 • edited

nvartolomei commented Dec 15, 2023

nvartolomei commented Dec 17, 2023

vbotbuildovich commented Dec 17, 2023

nvartolomei commented Dec 17, 2023

nvartolomei commented Dec 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andijcr commented Dec 19, 2023

nvartolomei commented Dec 19, 2023

andijcr commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat Dec 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvartolomei commented Dec 15, 2023 •

edited

vbotbuildovich commented Dec 15, 2023 •

edited

nvartolomei commented Dec 19, 2023 •

edited

dotnwat Dec 23, 2023 •

edited