allocator: replace read amp with io thresh

[kvoli]

We previously checked stores' L0-sublevels to exclude IO overloaded
stores from being allocation targets (#78608). This commit replaces the signal
with the normalized IO overload score instead, which also factors in the
L0-filecount. We started gossiping this value as of #83720. We continue
gossiping L0-sublevels for mixed-version compatibility; we can stop doing this
in 23.2.

Resolves: #85084

Release note (ops change): We've deprecated two cluster settings:

• kv.allocator.l0_sublevels_threshold
• kv.allocator.l0_sublevels_threshold_enforce.
  The pair of them were used to control rebalancing and upreplication behavior in
  the face of IO overloaded stores. This has been now been replaced by other
  internal mechanisms.

[cockroach-teamcity]

This change is 

[irfansharif]

Since most of the code changes are in the allocator package, consider using allocator: in your {PR,commit} title prefix. Also, do bear with my warring against the passive tense, but I think it would help you write better comments/commit messages.

[irfansharif]

Typo.

[kvoli]

Fixed.

[irfansharif]

[nit, throughout, feel free to ignore] "Store health" seems vague. Why not call it what it is -- storeIOOverloadTracker?

[irfansharif]

Something smells fishy here. Are we stamping on follower pausing by updating this store capacity IO threshold state with a rolling 10m max? We're using this same state for follower pausing here:

cockroach/pkg/kv/kvserver/store_raft.go

Lines 787 to 791 in 2111f9b

	ioThresholdMap := map[roachpb.StoreID]*admissionpb.IOThreshold{}
	for _, sd := range s.cfg.StorePool.GetStores() {
	ioThreshold := sd.Capacity.IOThreshold // need a copy
	ioThresholdMap[sd.StoreID] = &ioThreshold
	}

Which I take is updated for every store every 15s:

cockroach/pkg/util/admission/grant_coordinator.go

Lines 100 to 116 in 2cac11b

	ticker := time.NewTicker(ioTokenTickDuration)
	done := false
	for !done {
	select {
	case <-ticker.C:
	ticks++
	if ticks%ticksInAdjustmentInterval == 0 {
	metrics := sgc.pebbleMetricsProvider.GetPebbleMetrics()
	if len(metrics) != sgc.numStores {
	log.Warningf(ctx,
	"expected %d store metrics and found %d metrics", sgc.numStores, len(metrics))
	}
	for _, m := range metrics {
	if unsafeGc, ok := sgc.gcMap.Load(int64(m.StoreID)); ok {
	gc := (*GrantCoordinator)(unsafeGc)
	gc.pebbleMetricsTick(ctx, m)
	iotc.UpdateIOThreshold(roachpb.StoreID(m.StoreID), gc.ioLoadListener.ioThreshold)

[kvoli]

That is a problem and I didn't check where the follower pausing was using the storepool to get the descriptors. The easiest way to swap in IOThreshold was to use the store descriptor capacity. The store descriptor is taken directly from the storepool.

I am going to add a new type to encapsulate the descriptor and derived capacity information. Then replace calls to desc.Capacity with the derived capacity everywhere in the allocator and storepool update code.

[andrewbaptist]

@kvoli and I had a long conversation on this issue this afternoon. The high-level idea is

1. Remove the rolling window on storing this stat. It changes slowly enough that this isn't really necessary
2. Change the handling of suspect state so that a store that is down for a while transitions through suspect before being healthy. (should be a separate PR)
3. Consider making a store that hits IO overload (>1.0) to move it to suspect. This will accomplish many of the same goals as having a rolling window, but be more understandable.

[irfansharif]

LGTM. I've tried to rewrite the commit message/release note to avoid referencing these non-public settings. It's still a good idea to note the deprecation of these old settings, like you've done.

We previously checked stores' L0-sublevels to exclude IO overloaded
stores from being allocation targets (#78608). This commit replaces the signal
with the normalized IO overload score instead, which also factors in the
L0-filecount. We started gossiping this value as of #83720. We continue
gossiping L0-sublevels for mixed-version compatibility; we can stop doing this
in 23.2.

Resolves: cockroachdb#85084

Release note (ops change): We've deprecated two cluster settings:
- kv.allocator.l0_sublevels_threshold
- kv.allocator.l0_sublevels_threshold_enforce.
The pair of them were used to control rebalancing and upreplication behavior in
the face of IO overloaded stores. This has been now been replaced by other
internal mechanisms.

[irfansharif]

Should we get rid of this? It's only used for metrics, AFAICT, and is surfacing a value we're no longer using in production code.

[kvoli]

The gossiped value is taken from this. It isn't exposed to metrics. We can remove it in 23.2.

[kvoli]

LGTM. I've tried to rewrite the commit message/release note to avoid referencing these non-public settings. It's still a good idea to note the deprecation of these old settings, like you've done.

I've updated the commit message to be this, thanks!

TYFTR

[kvoli]

bors r=irfansharif

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)