Repository navigation
Conversation
CAS now waits with the existing backoff configuration before retrying failed reads or writes, preserving the backoff across successful reads when writes continue to fail. Conflicts and callback retries do not trigger error backoff. Retry waits honor context cancellation, with additional cancellation checks before reads, writes, and the final error return. A regression test checks that two reads receiving HTTP 429 are separated by at least one second and never invoke the callback. Fixes cortexproject#4697 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
| index := uint64(0) | ||
| // Keep error backoff across attempts: a successful read doesn't imply | ||
| // that writes have recovered. Conflicts and callback retries don't wait. | ||
| errorBackoff := backoff.New(ctx, backoffConfig) |
There was a problem hiding this comment.
backoffConfig is the 1s→1m backoff meant for the endless watch loops. Inside CAS, with the default 10 attempts, it adds up to 159–302s of waiting when Consul fails fast: against an httptest server that always returns 429, one CAS took 3m27s here vs ~2ms before. Several CAS callers pass context.Background() (Lifecycler.stopping for LEAVING/heartbeats/unregister, BasicLifecycler.stopping, autoJoin, initRing), so the new ctx handling doesn't bound them, and a shutdown during a KV outage can hang for minutes per call. A heartbeat can also still be in a 32–60s wait after Consul recovers, which is close to the default 1m heartbeat timeout.
Could CAS get its own much shorter backoff (e.g. sub-second min and a few seconds max, or a total budget around the heartbeat period)? Throttling just the HA tracker path, which is what #4697 is about, would be another option.
There was a problem hiding this comment.
Agreed, 5996434 gives CAS its own backoff: 100ms min, 2s max. The watch loops keep the 1s to 1m backoffConfig. At the default 10 attempts that is at most 9 waits, roughly 9.5s to 13s in total, well under the 1m heartbeat timeout. That bounds the context.Background() callers you listed too, and a heartbeat is never more than 2s behind a recovered Consul.
|
|
||
| ## master / unreleased | ||
|
|
||
| * [BUGFIX] Consul: Back off CAS retries after failed reads or writes to reduce request amplification during outages, and honor context cancellation while waiting to retry. |
There was a problem hiding this comment.
Could you move this below the existing [BUGFIX] entries (the order is CHANGE, FEATURE, ENHANCEMENT, BUGFIX), end it with #7893, and drop the extra blank line after it?
There was a problem hiding this comment.
Done in 5996434: the entry is now the last [BUGFIX] item, ends with #7893, and the extra blank line is gone.
| return nil | ||
| } | ||
|
|
||
| func TestCASBackoff(t *testing.T) { |
There was a problem hiding this comment.
This only covers failed reads. With the same httptest server it'd be cheap to also cover failed writes after a successful read (the delay should keep growing), ctx cancellation during the wait (CAS should return context.Canceled well before the backoff elapses), and a CAS conflict being retried without waiting.
There was a problem hiding this comment.
Added in 5996434, all against the same httptest server:
TestCASBackoffAfterFailedWrites: reads succeed, writes return 429. It checks the gaps between writes grow (at least 100ms, 200ms, 400ms).TestCASBackoffContextCanceledDuringWait: with a 1m CAS backoff, cancelling 200ms in returnscontext.Canceledin about 0.2s after one request.TestCASConflictRetriedWithoutBackoff: three conflicts and then success complete without waiting, under a 1m backoff.
go test ./pkg/ring/kv/consul/ -race passes, and the CAS tests passed 5 runs in a row with -race.
The 1s to 1m backoff is meant for the endless watch loops. Inside CAS, with the default 10 attempts, it could add minutes of waiting when Consul fails fast, and several CAS callers (lifecycler shutdown, heartbeats, autoJoin, initRing) pass context.Background(), so the context does not bound it. CAS now uses its own backoff (100ms min, 2s max). With the default 10 attempts there are at most 9 waits, totalling less than 13s, well below the default 1m ring heartbeat timeout. The watch loops keep the existing backoff. Add tests for failed writes after successful reads (delay keeps growing), context cancellation during the wait, and CAS conflicts being retried without waiting. Move the CHANGELOG entry below the existing BUGFIX entries and reference the PR. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
sandy2008
left a comment
sandy2008
left a comment
There was a problem hiding this comment.
Re-checked at 5996434. CAS now has its own 100ms to 2s backoff, so with the default 10 attempts the worst case is 9 waits adding up to 9.5–13s: against a server that always returns 429 I measured about 11s per CAS (it was 3m27s before), and a heartbeat-style 5s deadline returns context deadline exceeded at 5s. The CHANGELOG entry is now the last BUGFIX under master / unreleased and ends with #7893. Of the new tests, TestCASBackoff, the failed-writes test and the cancellation test fail with the CAS changes reverted, the conflict test guards against over-waiting, and the package passes with -race -count=3. One tiny nit on a comment inline.
…onsul-cas-error-backoff Signed-off-by: Matt Van Horn <mvanhorn@gmail.com> # Conflicts: # CHANGELOG.md
The jitter range starts at [100ms, 200ms) and doubles, so the upper bounds of the first four waits are 200ms, 400ms, 800ms and 1.6s, then 2s for the remaining five, which totals 13s. Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
|
Merged master into the branch to clear the CHANGELOG conflict. The Consul entry stays as the last BUGFIX under master / unreleased, after the new querier entry. Also took the comment nit in 0a53279. |
…onsul-cas-error-backoff Signed-off-by: Matt Van Horn <mvanhorn@gmail.com>
Motivation
During Consul outages, distributors can send rapid bursts of retries that amplify load and prolong rate limiting, delaying recovery.
Failed reads and writes in the Consul CAS loop immediately continued to the next attempt without error backoff. The optional retry delay also used an unconditional sleep that could not be interrupted by context cancellation.
What this changes
CAS now waits with the existing backoff configuration before retrying failed reads or writes, preserving the backoff across successful reads when writes continue to fail. Conflicts and callback retries do not trigger error backoff. Retry waits honor context cancellation, with additional cancellation checks before reads, writes, and the final error return. A regression test checks that two reads receiving HTTP 429 are separated by at least one second and never invoke the callback.
How it was tested
gofmt -l pkg/ring/kv/consul/client.go pkg/ring/kv/consul/client_test.goreported no formatting differences.go vet ./pkg/ring/kv/consul/...reported no diagnostics.go test ./pkg/ring/kv/consul/...passed.repo_lintran three times with no output shown.make BUILD_IN_CONTAINER=false lintran with no output shown.make BUILD_IN_CONTAINER=false mod-checkran with no output shown.make BUILD_IN_CONTAINER=false check-protosran with no output shown.make BUILD_IN_CONTAINER=false check-docran with no output shown.make BUILD_IN_CONTAINER=false check-white-noiseran with no output shown. The tests inpkg/ring/kv/consul/client_test.gofail without the fix in the cortex change (checked by reverting the source changes).Fixes #4697
AI was used for assistance.