cache: implement the circuit breaker pattern for asynchronous set operations in the cache client #7010

damnever · 2023-12-26T11:43:52Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Wrap SetAsync with a circuit breaker to dynamically limit the cache backfilling requests. Both the memcached and redis clients have added the same circuit breaker configuration.

Verification

Added some unit tests.

yeya24

Overall it is great work and it makes a lot of sense. Thanks.
We need to update docs.

pkg/cacheutil/memcached_client.go

pkg/cacheutil/memcached_client_test.go

yeya24 · 2023-12-27T07:52:02Z

pkg/cacheutil/memcached_client.go

+		SetAsyncCircuitBreakerOpenDuration:        5 * time.Second,
+		SetAsyncCircuitBreakerMinRequests:         50,
+		SetAsyncCircuitBreakerConsecutiveFailures: 5,
+		SetAsyncCircuitBreakerFailurePercent:      0.05,


We might need to keep an eye on this to make sure those values sane.
So far, it looks good to me. Would be better to have another pair of eyes on it

GiedriusS

Please update docs with new fields + add a paragraph explaining how this works 😄

@damnever

… in the cache client Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

@damnever

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

@damnever

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

yeya24

Thanks!

yeya24 · 2024-01-28T04:39:46Z

17:08:51 redis-redis: 1:C 26 Jan 2024 17:08:51.802 # Configuration loaded
17:08:51 redis-redis: 1:M 26 Jan 2024 17:08:51.802 * monotonic clock: POSIX clock_gettime
17:08:51 redis-redis: 1:M 26 Jan 2024 17:08:51.803 * Running mode=standalone, port=6379.
17:08:51 redis-redis: 1:M 26 Jan 2024 17:08:51.803 # Server initialized
17:08:51 redis-redis: 1:M 26 Jan 2024 17:08:51.803 # WARNING overcommit_memory is set to 0! Background save may fail under low memory condition. To fix this issue add 'vm.overcommit_memory = 1' to /etc/sysctl.conf and then reboot or run the command 'sysctl vm.overcommit_memory=1' for this to take effect.
17:08:51 redis-redis: 1:M 26 Jan 2024 17:08:51.803 * Ready to accept connections
17:08:52 static: 172.30.0.8 - - [26/Jan/2024:17:08:52 +0000] "GET /metrics HTTP/1.1" 200 172 "-" "Prometheus/2.41.0" "-"
17:08:52 Ports for container redis-client-redis-redis >> Local ports: map[redis:6379] Ports available from host: map[redis:32919]
    store_gateway_test.go:1001: store_gateway_test.go:1001: ""
        
         unexpected error: set async circuit breaker: consecutive failures must be greater than 0
        
17:08:52 Killing redis-redis
=== CONT  TestRangeQueryShardingWithRandomData
--- FAIL: TestRedisClient_Rueidis (2.90s)

We need to fix E2E test. @damnever https://github.com/thanos-io/thanos/actions/runs/7665969811/job/20908734057?pr=7010

@damnever

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

yeya24 · 2024-02-01T03:45:08Z

@damnever There is one more lint to fix before we can merge. Thanks

@damnever

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

yeya24 · 2024-02-01T17:46:33Z

@GiedriusS Wanna take another look before I merge this code change?

GiedriusS · 2024-02-09T13:55:55Z

pkg/cacheutil/redis_client.go

+			MaxRequests: config.SetAsyncCircuitBreakerHalfOpenMaxRequests,
+			Interval:    10 * time.Second,
+			Timeout:     config.SetAsyncCircuitBreakerOpenDuration,
+			ReadyToTrip: func(counts gobreaker.Counts) bool {


Should this be factored out into another function? I think it's exactly the same in memcached_client.

I have separated the entire configuration and logic, including this function. Please take another look.

GiedriusS · 2024-02-09T13:56:32Z

docs/components/store.md

@@ -340,6 +346,12 @@ While the remaining settings are **optional**:
 - `max_async_concurrency`: maximum number of concurrent asynchronous operations can occur.
 - `max_async_buffer_size`: maximum number of enqueued asynchronous operations allowed.
 - `max_get_multi_concurrency`: maximum number of concurrent connections when fetching keys. If set to `0`, the concurrency is unlimited.
+- `set_async_circuit_breaker_enabled`: `true` to enable circuite breaker for asynchronous operations. The circuit breaker consists of three states: closed, half-open, and open. It begins in the closed state. When the total requests exceed `set_async_circuit_breaker_min_requests`, and either consecutive failures occur or the failure percentage is excessively high according to the configured values, the circuit breaker transitions to the open state. This results in the rejection of all asynchronous operations. After `set_async_circuit_breaker_open_duration`, the circuit breaker transitions to the half-open state, where it allows `set_async_circuit_breaker_half_open_max_requests` asynchronous operations to be processed in order to test if the conditions have improved. If they have not, the state transitions back to open; if they have, it transitions to the closed state. Following each 10 seconds interval in the closed state, the circuit breaker resets its metrics and repeats this cycle.
+- `set_async_circuit_breaker_half_open_max_requests`: maximum number of requests allowed to pass through when the circuit breaker is half-open. If set to 0, the circuit breaker allows only 1 request.
+- `set_async_circuit_breaker_open_duration`: the period of the open state after which the state of the circuit breaker becomes half-open. If set to 0, the circuit breaker resets it to 60 seconds.


If set to 0, the circuit breaker resets it to 60 seconds.

What does it mean? That the default value of this is 60 seconds? 🤔

Yes, github.com/sony/gobreaker uses this default value.

@damnever

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

yeya24 · 2024-02-20T21:18:53Z

E2E test failure

=== NAME  TestRuleNativeHistograms
    native_histograms_test.go:391: native_histograms_test.go:391: ""
        
         unexpected error: unable to find metrics [prometheus_remote_storage_histograms_total] with expected values after 50 retries. Last error: <nil>. Last values: [0]

Doesn't seem like related. Ignoring for now

yeya24 · 2024-02-25T23:26:27Z

Hey @GiedriusS, I will merge this pr now. If you find anything else that's worth improving, feel free to create a separate issue and we can address later.

GiedriusS · 2024-02-26T16:06:29Z

👍 I am also thinking about whether it makes sense to only protect SET operations through the circuit breaker pattern? I mean if the cache is down then GETs won't work either.

yeya24 · 2024-02-26T18:43:58Z

I think what you said makes sense. @GiedriusS
Need to test this one out first and we can improve GETs later.
WDYT @damnever ?

damnever · 2024-02-27T13:24:30Z

I agree; I think GET is more sensitive to latency and should be assigned a higher priority.

GiedriusS

So maybe let's remove the set_ prefix from config and put gets under the circuit breaker too? Should be a small change. Ideally this would be done before the next release

damnever · 2024-02-28T03:11:06Z

Removing the set_ prefix sounds fine, but I recommend using a different circuit breaker for the GET operation. The intent of this PR is to try to limit the resources used by async set operations(#6975), subsequently benefiting other operations such as GETs.

@damnever

…rations in the cache client (thanos-io#7010) * Implement the circuit breaker pattern for asynchronous set operations in the cache client Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> * Add feature flag for circuitbreaker Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> * Sync docs Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> * Skip configuration validation if the circuit breaker is disabled Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> * Make lint happy Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> * Abstract the logic of the circuit breaker Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com> --------- Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

pull-request-size bot added the size/L label Dec 26, 2023

damnever force-pushed the cache/circuitbreaker branch from 8d5d504 to ec38a52 Compare December 26, 2023 11:44

yeya24 reviewed Dec 27, 2023

View reviewed changes

GiedriusS reviewed Dec 27, 2023

View reviewed changes

Implement the circuit breaker pattern for asynchronous set operations…

4dd89b7

… in the cache client Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

damnever force-pushed the cache/circuitbreaker branch 3 times, most recently from 44ca87c to c69653b Compare January 25, 2024 10:54

Add feature flag for circuitbreaker

c9b1f7c

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

damnever force-pushed the cache/circuitbreaker branch 3 times, most recently from 2ad5763 to 7ae1164 Compare January 25, 2024 11:27

Sync docs

808bdc9

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

damnever force-pushed the cache/circuitbreaker branch from 7ae1164 to 808bdc9 Compare January 26, 2024 09:00

yeya24 previously approved these changes Jan 26, 2024

View reviewed changes

damnever dismissed yeya24’s stale review via 53d805e January 29, 2024 10:55

damnever force-pushed the cache/circuitbreaker branch 2 times, most recently from 1d56ca5 to b00e6ed Compare January 29, 2024 10:57

Skip configuration validation if the circuit breaker is disabled

f76994e

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

damnever force-pushed the cache/circuitbreaker branch from bed1dea to f76994e Compare January 30, 2024 09:02

Make lint happy

07d9134

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

yeya24 previously approved these changes Feb 1, 2024

View reviewed changes

GiedriusS reviewed Feb 9, 2024

View reviewed changes

Abstract the logic of the circuit breaker

e299abe

Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>

damnever dismissed yeya24’s stale review via e299abe February 19, 2024 07:25

yeya24 approved these changes Feb 20, 2024

View reviewed changes

yeya24 requested a review from GiedriusS February 20, 2024 21:19

yeya24 merged commit f72b767 into thanos-io:main Feb 25, 2024
19 of 20 checks passed

yeya24 mentioned this pull request Feb 26, 2024

support set async circuit breaker for memcached and redis client cortexproject/cortex#5789

Merged

3 tasks

GiedriusS reviewed Feb 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache: implement the circuit breaker pattern for asynchronous set operations in the cache client #7010

cache: implement the circuit breaker pattern for asynchronous set operations in the cache client #7010

damnever commented Dec 26, 2023 •

edited

yeya24 left a comment

yeya24 Dec 27, 2023

GiedriusS left a comment

yeya24 left a comment

yeya24 commented Jan 28, 2024

yeya24 commented Feb 1, 2024

yeya24 commented Feb 1, 2024

GiedriusS Feb 9, 2024

damnever Feb 19, 2024

GiedriusS Feb 9, 2024

damnever Feb 19, 2024

yeya24 commented Feb 20, 2024

yeya24 commented Feb 25, 2024

GiedriusS commented Feb 26, 2024

yeya24 commented Feb 26, 2024

damnever commented Feb 27, 2024

GiedriusS left a comment

damnever commented Feb 28, 2024

cache: implement the circuit breaker pattern for asynchronous set operations in the cache client #7010

cache: implement the circuit breaker pattern for asynchronous set operations in the cache client #7010

Conversation

damnever commented Dec 26, 2023 • edited

Changes

Verification

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 Dec 27, 2023

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 commented Jan 28, 2024

yeya24 commented Feb 1, 2024

yeya24 commented Feb 1, 2024

GiedriusS Feb 9, 2024

Choose a reason for hiding this comment

damnever Feb 19, 2024

Choose a reason for hiding this comment

GiedriusS Feb 9, 2024

Choose a reason for hiding this comment

damnever Feb 19, 2024

Choose a reason for hiding this comment

yeya24 commented Feb 20, 2024

yeya24 commented Feb 25, 2024

GiedriusS commented Feb 26, 2024

yeya24 commented Feb 26, 2024

damnever commented Feb 27, 2024

GiedriusS left a comment

Choose a reason for hiding this comment

damnever commented Feb 28, 2024

damnever commented Dec 26, 2023 •

edited