kvflowcontrol: set kvadmission.flow_control.mode default to apply_to_all

[kvoli]

The cluster setting kvadmission.flow_control.mode is used to control whether replication admission control applies to only elastic work, or applies to all work which sets an admission priority.

The cluster setting was prior to #132125, metamorphic in tests, selecting either value with equal probability. #132125 disabled the metamorphism in order to prevent the newly introduced send queue / pull mode from being run unexpectedly, while still under testing.

With testing now in place, re-enable the metamorphism, leaving the default value unchanged.

Resolves: #132364

---

With testing now in place, this commit changes the default cluster
setting value of kvadmission.flow_control.mode from
apply_to_elastic, to apply_to_all.

Now by default, regular work will be subject to replication admission
control, whereby a quorum will be allowed to proceed, queuing entries
to any non-quorum required replica, when send tokens are exhausted.

For more details, see #123509.

Resolves: #133838
Release note (performance improvement): Regular writes are now subject
to admission control by default, meaning that non-quorum required
replicas may not be told about new writes from the leader if they are
unable to keep up. This brings a large performance improvement during
instances where there is a large backlog of replication work towards a
subset of node(s), such as node restarts. The setting can be reverted to
the <=24.3 default by setting kvadmission.flow_control.mode to
apply_to_elastic.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[kvoli]

Hmm, I'm not sure why dev gen updates the metrics, perhaps it does some code reachability static analysis and they were never being updated (it thinks).

[kvoli]

Not unexpectedly, some tests are failing. I suspect these tests exhaust tokens and delay admission. We should confirm whats happening.

[sumeerbhola]

I can't repro the TestReplicateQueueDownReplicate failure.

[sumeerbhola]

The copy/bank/rows=1000000,nodes=9,txn=false failure seems like a SQL issue.

run_180714.916987178_n1_cockroach-workload-f: 2024/12/18 18:07:14 cluster.go:2505: > ./cockroach workload fixtures load bank --rows=1000000 --payload-bytes=100 --seed 1 {pgurl:1}
I241218 18:07:15.720586 1 ccl/workloadccl/cliccl/fixtures.go:269  [-] 1  starting restore of 1 tables
I241218 18:07:15.720661 40 ccl/workloadccl/fixture.go:630  [-] 2  Restoring from gs://cockroach-fixtures-us-east1/workload/bank/version=1.0.0,payload-bytes=100,ranges=10,rows=1000000,seed=1/bank?AUTH=implicit
Error: restoring fixture: sql: expected 4 destination arguments in Scan, not 6
run_180714.916987178_n1_cockroach-workload-f: 2024/12/18 18:07:21 cluster.go:2518: > result: COMMAND_PROBLEM: exit status 1

[kvoli]

Closed out the PR which enabled just the metamorphism. If anything fails, it will be equally as disruptive, metamorphic or not. So lets just merge both at once, then rollback as necessary. I'll also make an announcement in (internal) slack.

• kvflowcontrol: re-enable kvadmission.flow_control.mode metamorphism #133854 (comment)

[sumeerbhola]

Reviewed 2 of 3 files at r2, 2 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

---

docs/generated/metrics/metrics.html line 22 at r5 (raw file):

<tr><td>STORAGE</td><td>admission.admitted.kv-stores.locking-normal-pri</td><td>Number of requests admitted</td><td>Requests</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>
<tr><td>STORAGE</td><td>admission.admitted.kv-stores.normal-pri</td><td>Number of requests admitted</td><td>Requests</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>
<tr><td>STORAGE</td><td>admission.admitted.kv-stores.user-high-pri</td><td>Number of requests admitted</td><td>Requests</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>

why these changes to metrics.html?

[kvoli]

TYFTR!!

bors r=sumeerbhola

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

docs/generated/metrics/metrics.html line 22 at r5 (raw file):

Previously, sumeerbhola wrote…

why these changes to metrics.html?

See earlier comment, there is a liveness/reachability analysis conducted via running some tests, in order to decide which metrics to generate.

#133860 (comment)

[craig]

Merge conflict.

[kvoli]

Going to rebase and try again.

[kvoli]

Rebased to resolve merge conflicts. Given we were green before, I'll bors this again as soon as the required checks are passed.

[kvoli]

Pushed a small diff to resolve a flaky, test-only issue on TestRaftTracing/lease-type=LeaseEpoch:

diff --git a/pkg/kv/kvserver/client_raft_log_queue_test.go b/pkg/kv/kvserver/client_raft_log_queue_test.go
index abaadb9d5bb..10a70e2d288 100644
--- a/pkg/kv/kvserver/client_raft_log_queue_test.go
+++ b/pkg/kv/kvserver/client_raft_log_queue_test.go
@@ -201,8 +201,10 @@ func TestRaftTracing(t *testing.T) {
 				expectedMessages := []string{
 					`replica_proposal_buf.* flushing proposal to Raft`,
 					`replica_proposal_buf.* registering local trace`,
+					// Note that we don't assert that the 1->3 MsgApp goes through, as
+					// the ordering may change between 1->2 and 1->3. It should be
+					// sufficient to just check one of them for tracing.
 					`replica_raft.* 1->2 MsgApp`,
-					`replica_raft.* 1->3 MsgApp`,
 					`replica_raft.* AppendThread->1 MsgStorageAppendResp`,
 					`ack-ing replication success to the client`,
 				}

Passes 1k/1k now, where previously it would fail quite quickly:

$ dev test pkg/kv/kvserver -v -f TestRaftTracing/lease-type=LeaseEpoch --stress
...
INFO: Found 1 test target...
Target //pkg/kv/kvserver:kvserver_test up-to-date:
  _bazel/bin/pkg/kv/kvserver/kvserver_test_/kvserver_test
INFO: Elapsed time: 308.212s, Critical Path: 43.41s
INFO: 1006 processes: 1 internal, 1005 darwin-sandbox.
INFO: Build completed successfully, 1006 total actions
//pkg/kv/kvserver:kvserver_test                                          PASSED in 4.3s
  Stats over 1000 runs: max = 4.3s, min = 1.1s, avg = 1.3s, dev = 0.2s

[kvoli]

Stacked a commit to resolve the flaky behavior on testcluster startup. Left a todo to investigate further and linked the known related failures, which were all pre-existing.

[kvoli]

bors r=sumeerbhola

[craig]

Build succeeded:

• local_roachtest_fips
• lint
• macos_amd64_build
• acceptance
• macos_arm64_build
• check_generated_code
• linux_amd64_build
• linux_arm64_build
• docker_image_amd64
• unit_tests
• local_roachtest
• linux_amd64_fips_build
• windows_build
• examples_orms