kvserver: circuit-break requests to unavailable ranges

[tbg]

Fixes #33007.
Closes #61311.

This PR uses the circuit breaker package introduced in #73641 to address
issue #33007: When a replica becomes unavailable, it should eagerly
refuse traffic that it believes would simply hang.

Concretely, every request to the Replica gets a cancelable context that
is sensitive to the circuit breaker tripping (relying on context
cancellation makes sure we don't end up with a second channel that needs
to be plumbed to everyone and their dog; Go makes it really difficult to
join two cancellation chains); if the breaker is tripped when the
request arrives, it is refused right away. In either case, the outgoing
error is augmented to carry information about the tripped breaker. This
isn't 100% trivial, since retreating from in-flight replication
typically causes an AmbiguousResultError, and while we could avoid it
in some cases we can't in all. A similar problem occurs when the
canceled request was waiting on a lease, in which case the result is a
NotLeaseholderError.

For any request that made it in, if it enters replication but does not
manage to succeed within base.SlowRequestThreshold (15s at time of
writing), the breaker is tripped, cancelling all inflight and future
requests until the breaker heals. Perhaps surprisingly, the existing
"slowness" detection (the informational "have been waiting ... for
proposal" warning in executeWriteBatch) was moved deeper into
replication (refreshProposalsLocked), where it now trips the breaker.
This has the advantage of providing a unified path for lease requests
(which don't go through executeWriteBatch) and pipelined writes (which
return before waiting on the inflight replication process). To make this
work, we need to pick a little fight with how leases are (not) refreshed
(#74711) and we need to remember the ticks at which a proposal was first
inserted (despite potential reproposals).

A tripped breaker deploys a background probe which sends a
ProbeRequest (introduced in #72972) to determine when to heal; this is
roughly the case whenever replicating a command through Raft is possible
for the Replica, either by appending to the log as the Raft leader, or
by forwarding to the Raft leader. A tiny bit of special casing allows
requests made by the probe to bypass the breaker.

As of this PR, all of this is opt-in via the
kv.replica_circuit_breakers.enabled cluster setting while we determine
which of the many follow-up items to target for 22.1 (see #74705). For
example, the breaker-sensitive context cancelation is a toy
implementation that comes with too much of a performance overhead (one
extra goroutine per request); #74707 will address that.

Other things not done here that we certainly want in the 22.1 release
are UI work (#74713) and metrics (#74505).

The release note is deferred to #74705 (where breakers are turned on).

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

@cockroachdb/repl-prs this is still a draft but it is ready for a high-level review. There is a lot going on and I can easily pull 5+ pieces of this into separate PRs and am planning to do so. However, before I start that I would like to get both of you on board with the basic approach. I think we've sufficiently discussed the circuit breaker API and are happy with it; I will rewrite the internals to avoid all of the unsafe stuff so ignore the internals but give the API another look and voice any larger concerns. For the NoopWrite, there is an alternative where we don't bypass the lease. I think bypassing the lease has worked out nicely and keeps NoopWrite simple and possibly reusable, but on the other hand the probe may pass but for some unknown reason a lease may never be acquired. My preference is to keep NoopWrite as is but make the probe use two stages, where it issues the noop write first and a lease-requiring write second but possibly this is a bad idea and I need to sleep over it and talk it through.

[erikgrinaker]

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @tbg)

---

pkg/util/circuit/circuitbreaker.go, line 28 at r1 (raw file):

// from Breaker.Err(), i.e. `errors.Is(err, ErrBreakerOpen()) can be
// used to check whether an error originated from some Breaker.
func ErrBreakerOpen() error {

not: why not simply export the variable?

---

pkg/util/circuit/circuitbreaker.go, line 56 at r1 (raw file):

	Name         string
	ShouldTrip   func(err error) error
	AsyncProbe   func(report func(error), done func())

Why does this need to take a done closure? Since the probe is expected to keep running, can't we just clean up when the closure returns?

Ah, I suppose it's because it's the probe's responsibility to spawn the async task. Is there a reason for that, i.e. why can't the prober spawn the task? The only reason I can see is for the caller to pass in its context, but that seems like an antipattern anyway?

[tbg]

Spent some time on this again today. While putting together an end-to-end test (lose quorum, try to write and see the failfast) I realized that we do need to make all parts of the read/write pipeline aware of the circuit breaker to avoid getting stuck. I knew that we needed lease proposals to be sensitive to the breaker (so that you wouldn't get stuck trying to acquire a lease over and over when there wasn't a valid lease to begin with) but I wasn't aware of a much more basic problem: that when one command fails fast after having proposed a command, the latches stick around. They have to; but this means that the next command will get stuck on the latches. So we need to short-circuit the latch acquisition as well when the breaker trips. Of course we don't want to plumb the circuit breaker everywhere, you'd really want to rely on context.Context for that sort of thing. I've hacked something together in the current version of the PR (as pushed right now) which I can polish to something that should get the job done for v1. Down the road, it would be interesting to explore #66884 as an ingredient here but I don't want this to become a prerequisite.

[erikgrinaker]

This should probably go into the next commit, "wip: add leaseholder-unavailable test".

[erikgrinaker]

when one command fails fast after having proposed a command, the latches stick around. They have to; but this means that the next command will get stuck on the latches. So we need to short-circuit the latch acquisition as well when the breaker trips. Of course we don't want to plumb the circuit breaker everywhere, you'd really want to rely on context.Context for that sort of thing

Ugh, this is somewhat annoying. Conceptually, cancelling the context in Send() when the breaker trips makes sense -- it's essentially what we want to happen. But the bookkeeping gets a bit unwieldy, as you point out. It'd be nice if we could inject a cancel trigger into the context somehow, without needing to keep track of the individual cancel functions.

I'll have to remind myself of how the latch guards are managed, but if the latches turn out to be the only problem here, is it terrible to come up with a solution at/below the Raft level that causes us to abandon proposals/commands and release latches without having to do context management higher up? Although I realize that the context approach generalizes better across failure modes.

[tbg]

Discussed this with Erik yesterday. Despite the potential perf overhead we decided that it was an important property that requests get canceled reliably on breaker tripping. This means making all ctxs cancelable and stashing the cancels. If we don't do that, we can sort of reliably refuse requests once the breaker has solidly tripped, but while the tripping is being detected, some requests might sneak in and get stuck on the latch manager (or really anywhere else that doesn't have a dedicated breaker check). We didn't want to plumb the breaker signal into the latch manager, as context cancellation is the "Go way" of doing what we're trying to do. As we have a few times before, we were dissatisfied with what the Go ecosystem provides in terms of joining multiple cancellation chains in a performant way; there are always more allocations involved than seems reasonable (related: we don't use Stopper.WithCancelOnQuiesce liberally because it's also expensive-ish).

We are abandoning our initial ambition to let reads that can be served succeed: this is just hard to build correctly. The main reason for reads to get stuck (no lease) is handled adequately by only circuit breaking on writes (lease acquisition is a write), but it's trickier if there is a stuck in-flight write that then prevents reads from getting their latches. We'd basically have to tell the latch manager to fail-fast a read but only if it is transitively waiting on a write's latch (write latches have to stick around on unavailable ranges, even if the client has long been fail-fasted). The "transitively" here is not something the latch manager is even concerned with today, see #73916 for a related discussion.

so:

• early in Replica.Send, check the breaker, make context cancelable and stash the cancel func somewhere on Replica
• on break, have the probe cancel everyone.