Merge pull request #123211 from cockroachdb/blathers/backport-release…

…-24.1-123118

docs/generated/settings/settings-for-tenants.txt

@@ -54,10 +54,10 @@ kv.closed_timestamp.side_transport_interval	duration	200ms	the interval at which
 kv.closed_timestamp.target_duration	duration	3s	if nonzero, attempt to provide closed timestamp notifications for timestamps trailing cluster time by approximately this duration	system-visible
 kv.dist_sender.circuit_breaker.cancellation.enabled	boolean	true	when enabled, in-flight requests will be cancelled when the circuit breaker trips	application
 kv.dist_sender.circuit_breaker.cancellation.write_grace_period	duration	10s	how long after the circuit breaker trips to cancel write requests (these can't retry internally, so should be long enough to allow quorum/lease recovery)	application
-kv.dist_sender.circuit_breaker.enabled	boolean	true	enable circuit breakers for failing or stalled replicas	application
 kv.dist_sender.circuit_breaker.probe.interval	duration	3s	interval between replica probes	application
 kv.dist_sender.circuit_breaker.probe.threshold	duration	3s	duration of errors or stalls after which a replica will be probed	application
 kv.dist_sender.circuit_breaker.probe.timeout	duration	3s	timeout for replica probes	application
+kv.dist_sender.circuit_breakers.mode	enumeration	no ranges	set of ranges to trip circuit breakers for failing or stalled replicas [no ranges = 0, liveness range only = 1, all ranges = 2]	application
 kv.protectedts.reconciliation.interval	duration	5m0s	the frequency for reconciling jobs with protected timestamp records	system-visible
 kv.rangefeed.client.stream_startup_rate	integer	100	controls the rate per second the client will initiate new rangefeed stream for a single range; 0 implies unlimited	application
 kv.rangefeed.closed_timestamp_refresh_interval	duration	3s	the interval at which closed-timestamp updatesare delivered to rangefeeds; set to 0 to use kv.closed_timestamp.side_transport_interval	system-visible

docs/generated/settings/settings.html

@@ -69,10 +69,10 @@
 <tr><td><div id="setting-kv-closed-timestamp-target-duration" class="anchored"><code>kv.closed_timestamp.target_duration</code></div></td><td>duration</td><td><code>3s</code></td><td>if nonzero, attempt to provide closed timestamp notifications for timestamps trailing cluster time by approximately this duration</td><td>Serverless/Dedicated/Self-Hosted (read-only)</td></tr>
 <tr><td><div id="setting-kv-dist-sender-circuit-breaker-cancellation-enabled" class="anchored"><code>kv.dist_sender.circuit_breaker.cancellation.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when enabled, in-flight requests will be cancelled when the circuit breaker trips</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-dist-sender-circuit-breaker-cancellation-write-grace-period" class="anchored"><code>kv.dist_sender.circuit_breaker.cancellation.write_grace_period</code></div></td><td>duration</td><td><code>10s</code></td><td>how long after the circuit breaker trips to cancel write requests (these can&#39;t retry internally, so should be long enough to allow quorum/lease recovery)</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-kv-dist-sender-circuit-breaker-enabled" class="anchored"><code>kv.dist_sender.circuit_breaker.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enable circuit breakers for failing or stalled replicas</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-dist-sender-circuit-breaker-probe-interval" class="anchored"><code>kv.dist_sender.circuit_breaker.probe.interval</code></div></td><td>duration</td><td><code>3s</code></td><td>interval between replica probes</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-dist-sender-circuit-breaker-probe-threshold" class="anchored"><code>kv.dist_sender.circuit_breaker.probe.threshold</code></div></td><td>duration</td><td><code>3s</code></td><td>duration of errors or stalls after which a replica will be probed</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-dist-sender-circuit-breaker-probe-timeout" class="anchored"><code>kv.dist_sender.circuit_breaker.probe.timeout</code></div></td><td>duration</td><td><code>3s</code></td><td>timeout for replica probes</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-kv-dist-sender-circuit-breakers-mode" class="anchored"><code>kv.dist_sender.circuit_breakers.mode</code></div></td><td>enumeration</td><td><code>no ranges</code></td><td>set of ranges to trip circuit breakers for failing or stalled replicas [no ranges = 0, liveness range only = 1, all ranges = 2]</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-lease-transfer-read-summary-global-budget" class="anchored"><code>kv.lease_transfer_read_summary.global_budget</code></div></td><td>byte size</td><td><code>0 B</code></td><td>controls the maximum number of bytes that will be used to summarize the global segment of the timestamp cache during lease transfers and range merges. A smaller budget will result in loss of precision.</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-lease-transfer-read-summary-local-budget" class="anchored"><code>kv.lease_transfer_read_summary.local_budget</code></div></td><td>byte size</td><td><code>4.0 MiB</code></td><td>controls the maximum number of bytes that will be used to summarize the local segment of the timestamp cache during lease transfers and range merges. A smaller budget will result in loss of precision.</td><td>Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-kv-log-range-and-node-events-enabled" class="anchored"><code>kv.log_range_and_node_events.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>set to true to transactionally log range events (e.g., split, merge, add/remove voter/non-voter) into system.rangelogand node join and restart events into system.eventolog</td><td>Dedicated/Self-Hosted</td></tr>

pkg/kv/kvclient/kvcoord/dist_sender_circuit_breaker.go

@@ -31,12 +31,54 @@ import (
 	"github.com/cockroachdb/redact"
 )
 
+// DistSenderCircuitBreakersMode controls if and to what level we trip circuit
+// breakers for replicas in the DistSender when they are failed or stalled.
+type DistSenderCircuitBreakersMode int64
+
+const (
+	// DistSenderCircuitBreakersNoRanges indicates we should never trip circuit
+	// breakers.
+	DistSenderCircuitBreakersNoRanges DistSenderCircuitBreakersMode = iota
+	// DistSenderCircuitBreakersLivenessRangeOnly indicates we should only trip
+	// circuit breakers if a replica belonging to node liveness experiences a
+	// failure or stall.
+	//
+	// Typically, a replica belonging to node liveness has a high potential to
+	// disrupt a cluster. All nodes need to write to the liveness range -- either
+	// to heartbeat their liveness record to keep their leases, or to increment
+	// another node's epoch before acquiring theirs. As such, a stalled or failed
+	// replica belonging to the liveness range that these requests get stuck on is
+	// no good -- nothing in the cluster will make progress.
+	//
+	// TODO(arul): currently, this doesn't do what it claims -- it just behaves like
+	// off. We should fix this. See
+	// https://github.com/cockroachdb/cockroach/issues/123117.
+	DistSenderCircuitBreakersLivenessRangeOnly
+	// DistSenderCircuitBreakersAllRanges indicates that we should trip circuit
+	// breakers for any replica that experiences a failure or a stall, regardless
+	// of the range it belongs to.
+	//
+	// Tripping a circuit breaker involves launching a probe to eventually un-trip
+	// the thing. This per-replica probe launches a goroutine. As such, a node
+	// level issue that causes a large number of circuit breakers to be tripped on
+	// a client has the potential of causing a big goroutine spike on the client.
+	// We currently don't have good scalability numbers to measure the impact of
+	// this -- once we do, we can revaluate the default mode we run in if the
+	// numbers look good. Else we may want to make these things more scalable.
+	DistSenderCircuitBreakersAllRanges
+)
+
 var (
-	CircuitBreakerEnabled = settings.RegisterBoolSetting(
+	CircuitBreakersMode = settings.RegisterEnumSetting(
 		settings.ApplicationLevel,
-		"kv.dist_sender.circuit_breaker.enabled",
-		"enable circuit breakers for failing or stalled replicas",
-		true,
+		"kv.dist_sender.circuit_breakers.mode",
+		"set of ranges to trip circuit breakers for failing or stalled replicas",
+		"no ranges",
+		map[int64]string{
+			int64(DistSenderCircuitBreakersNoRanges):          "no ranges",
+			int64(DistSenderCircuitBreakersLivenessRangeOnly): "liveness range only",
+			int64(DistSenderCircuitBreakersAllRanges):         "all ranges",
+		},
 		settings.WithPublic,
 	)
 
@@ -263,7 +305,7 @@ func (d *DistSenderCircuitBreakers) probeStallLoop(ctx context.Context) {
 		}
 
 		// Don't do anything if circuit breakers have been disabled.
-		if !CircuitBreakerEnabled.Get(&d.settings.SV) {
+		if d.Mode() == DistSenderCircuitBreakersNoRanges {
 			continue
 		}
 
@@ -361,7 +403,7 @@ func (d *DistSenderCircuitBreakers) ForReplica(
 	rangeDesc *roachpb.RangeDescriptor, replDesc *roachpb.ReplicaDescriptor,
 ) *ReplicaCircuitBreaker {
 	// If circuit breakers are disabled, return a nil breaker.
-	if !CircuitBreakerEnabled.Get(&d.settings.SV) {
+	if d.Mode() == DistSenderCircuitBreakersNoRanges {
 		return nil
 	}
 
@@ -381,6 +423,17 @@ func (d *DistSenderCircuitBreakers) ForReplica(
 	return v.(*ReplicaCircuitBreaker)
 }
 
+func (d *DistSenderCircuitBreakers) Mode() DistSenderCircuitBreakersMode {
+	mode := DistSenderCircuitBreakersMode(CircuitBreakersMode.Get(&d.settings.SV))
+	// TODO(arul): Currently, even when configured for just the liveness range, we
+	// treat dist sender circuit breakers as off. See
+	// https://github.com/cockroachdb/cockroach/issues/123117.
+	if mode == DistSenderCircuitBreakersLivenessRangeOnly {
+		return DistSenderCircuitBreakersNoRanges
+	}
+	return mode
+}
+
 // ReplicaCircuitBreaker is a circuit breaker for an individual replica.
 type ReplicaCircuitBreaker struct {
 	d        *DistSenderCircuitBreakers
@@ -824,7 +877,7 @@ func (r *ReplicaCircuitBreaker) launchProbe(report func(error), done func()) {
 
 		for {
 			// Untrip the breaker and stop probing if circuit breakers are disabled.
-			if !CircuitBreakerEnabled.Get(&r.d.settings.SV) {
+			if r.d.Mode() == DistSenderCircuitBreakersNoRanges {
 				report(nil)
 				return
 			}

pkg/kv/kvclient/kvcoord/dist_sender_circuit_breaker_test.go

@@ -48,7 +48,9 @@ func TestDistSenderReplicaStall(t *testing.T) {
 		// speed up the test by reducing various intervals and timeouts.
 		st := cluster.MakeTestingClusterSettings()
 		kvserver.ExpirationLeasesOnly.Override(ctx, &st.SV, true)
-		kvcoord.CircuitBreakerEnabled.Override(ctx, &st.SV, true)
+		kvcoord.CircuitBreakersMode.Override(
+			ctx, &st.SV, int64(kvcoord.DistSenderCircuitBreakersAllRanges),
+		)
 		kvcoord.CircuitBreakerCancellation.Override(ctx, &st.SV, true)
 		kvcoord.CircuitBreakerProbeThreshold.Override(ctx, &st.SV, time.Second)
 		kvcoord.CircuitBreakerProbeInterval.Override(ctx, &st.SV, time.Second)
@@ -128,7 +130,9 @@ func BenchmarkDistSenderCircuitBreakersForReplica(b *testing.B) {
 
 	ambientCtx := log.MakeTestingAmbientCtxWithNewTracer()
 	st := cluster.MakeTestingClusterSettings()
-	kvcoord.CircuitBreakerEnabled.Override(ctx, &st.SV, true)
+	kvcoord.CircuitBreakersMode.Override(
+		ctx, &st.SV, int64(kvcoord.DistSenderCircuitBreakersAllRanges),
+	)
 
 	cbs := kvcoord.NewDistSenderCircuitBreakers(
 		ambientCtx, stopper, st, nil, kvcoord.MakeDistSenderMetrics())
@@ -186,15 +190,23 @@ func BenchmarkDistSenderCircuitBreakersTrack(b *testing.B) {
 }
 
 func benchmarkCircuitBreakersTrack(
-	b *testing.B, enable, cancel, alone bool, errType string, conc int,
+	b *testing.B, enable bool, cancel bool, alone bool, errType string, conc int,
 ) {
 	ctx := context.Background()
 	stopper := stop.NewStopper()
 	defer stopper.Stop(ctx)
 
 	ambientCtx := log.MakeTestingAmbientCtxWithNewTracer()
 	st := cluster.MakeTestingClusterSettings()
-	kvcoord.CircuitBreakerEnabled.Override(ctx, &st.SV, enable)
+	if enable {
+		kvcoord.CircuitBreakersMode.Override(
+			ctx, &st.SV, int64(kvcoord.DistSenderCircuitBreakersAllRanges),
+		)
+	} else {
+		kvcoord.CircuitBreakersMode.Override(
+			ctx, &st.SV, int64(kvcoord.DistSenderCircuitBreakersNoRanges),
+		)
+	}
 	kvcoord.CircuitBreakerCancellation.Override(ctx, &st.SV, cancel)
 
 	cbs := kvcoord.NewDistSenderCircuitBreakers(