rac2,kvserver: do not quiesce if send tokens held

[pav-kv]

This PR prevents range quiescence if RACv2 holds any send tokens for this range. Quiescence would prevent MsgApp pings which ensure that the leader reliably learns about the follower store admitting log entries, and causes it to release tokens accordingly. We do not want to end up holding tokens permanently.

Resolves #129581

[pav-kv]

@sumeerbhola @kvoli The 4th commit is the interesting one. The rest is clean-ups.

[cockroach-teamcity]

This change is 

[pav-kv]

TestFlowControlQuiescedRangeV2 is failing (the range never quiesces), need to fix. Upd: this was expected. Fixed.

[sumeerbhola]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 9 of 9 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli and @pav-kv)

---

pkg/kv/kvserver/replica_raft_quiesce.go line 446 at r3 (raw file):

	}
	slices.SortFunc(lagging, func(a, b livenesspb.Liveness) int {
		return cmp.Compare(a.NodeID, b.NodeID)

is this sorting for determinism in tests, since sorting by NodeID seems unnecessary?

---

pkg/kv/kvserver/replica_raft_quiesce.go line 216 at r4 (raw file):

	// TODO(pav-kv): this method is a one-off holding raftMu. It should be able to
	// do its job with only Replica.mu held.
	hasSendTokensRaftMuLocked() bool

If I am reading correctly, shouldReplicaQuiesce is only ever called with raftMu and Replica.mu held, so this seems unnecessary to fix. There is a performance cost to adding locking in RangeController where it currently manages with raftMu already being held (mutex locking shows up in cpu profiles), so I'd rather not add another Mutex.

---

pkg/kv/kvserver/replica_raft_quiesce.go line 284 at r4 (raw file):

//
// NOTE: The last 3 conditions are fairly, but not completely, overlapping.
func shouldReplicaQuiesce(

suggest adding RaftMuLockedReplicaMuLocked suffix to this method.

---

pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 1169 at r4 (raw file):

	// TODO(pav-kv): in Replica.tick, we can call MaybeSendPingsRaftMuLocked
	// first, which does the same checks. It can return whether any tokens are
	// held, and this signal can be then used to prevent quiescence.

Would adding this logic into MaybeSendPingsRaftMuLocked help with code clarity (in the kvserver code) or performance?
Just trying to understand why this should be a TODO.

[pav-kv]

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

---

pkg/kv/kvserver/replica_raft_quiesce.go line 446 at r3 (raw file):

Previously, sumeerbhola wrote…

is this sorting for determinism in tests, since sorting by NodeID seems unnecessary?

I think so, but prefer not removing the sort in this PR. Looking at paths that use this lagging slice in the callers, they end up being sent over the wire. I didn't find any usage of this proto field though... Might remove in another PR.

---

pkg/kv/kvserver/replica_raft_quiesce.go line 216 at r4 (raw file):

Previously, sumeerbhola wrote…

If I am reading correctly, shouldReplicaQuiesce is only ever called with raftMu and Replica.mu held, so this seems unnecessary to fix. There is a performance cost to adding locking in RangeController where it currently manages with raftMu already being held (mutex locking shows up in cpu profiles), so I'd rather not add another Mutex.

The fact that raftMu is held in tick is odd and may change. Looking at the tick code, it is not used for anything (now used for RACv2 check). I am just documenting the interface irregularity here, but there is a bigger thing behind it.

The fact that RACv2 needs to hold raftMu everywhere in Processer is thus also a lucky coincidence. We might need to revisit that at some point, because we should be needing raftMu only for raft IO.

---

pkg/kv/kvserver/replica_raft_quiesce.go line 284 at r4 (raw file):

Previously, sumeerbhola wrote…

suggest adding RaftMuLockedReplicaMuLocked suffix to this method.

Done.

---

pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 1169 at r4 (raw file):

Previously, sumeerbhola wrote…

Would adding this logic into MaybeSendPingsRaftMuLocked help with code clarity (in the kvserver code) or performance?
Just trying to understand why this should be a TODO.

Performance. We're doing the same loop over rc.replicasMap + locking in each state, so we could both send pings and check whether we can quiesce (none/any of holdsTokens returned false).

Clarified the comment.

[pav-kv]

TFTR!

bors r=sumeerbhola

[pav-kv]

bors cancel

[craig]

Canceled.

[pav-kv]

bors r=sumeerbhola

[craig]

Build failed (retrying...):

• unit_tests

[craig]

Build failed:

• docker_image_amd64

[pav-kv]

One more test had to be fixed.

bors r=sumeerbhola

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• examples_orms

[craig]

Build succeeded:

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