This looks pretty good. The IO granter code is getting quite large, though I'm hesitant to suggest breaking it down in this PR.

Could you reword the release note a bit? It's a little choppy. Also, make sure to point out that the setting comes with a default of 0.5 (I guess 50 since we're not doing fractions) and so this protection is on by default.

TFTR!

Reworded the release note.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @joshimhoff, and @tbg)

pkg/kv/kvserver/metrics.go line 492 at r1 (raw file):

Are these for alerts based on prometheus metrics? I am not sure what threshold is reasonable for SRE. I don't think there should be any aggregation here. I am inclined towards waiting for some incidents where this metric turned out to be useful as an early signal of badness.
@joshimhoff do you have an opinion?

pkg/util/admission/granter.go line 61 at r1 (raw file):

Changed to a fraction.

pkg/util/admission/granter.go line 65 at r1 (raw file):

Done

pkg/util/admission/granter.go line 1635 at r1 (raw file):

Ack

pkg/util/admission/granter.go line 1637 at r1 (raw file):

Done

pkg/util/admission/granter.go line 2073 at r1 (raw file):

Done

pkg/util/admission/granter.go line 2111 at r1 (raw file):

Done

pkg/util/admission/granter.go line 2233 at r1 (raw file):

Done

pkg/util/admission/granter.go line 2237 at r1 (raw file):

Done

From an o11y standpoint, defining an alerting rule was the main point I wanted to bring up. Thanks @tbg for being an advocate!

I am inclined towards waiting for some incidents where this metric turned out to be useful as an early signal of badness.

SGTM from an SRE-specific alert perspective.

SRE should alert on != what the customer should alert on. Any signs of overload at all is a good thing for the customer to alert on.

I also agree with this - are we sure there's not a conservative alerting rule we can define here to indicate when things are going south? Or is storage.write-stall-nanos dependent enough on cluster configuration & workload where there's no clear "one size fits all"?

If there's some threshold that, in all cases, would indicate that investigation is required, I think an alerting rule would be a great way for us to provide guidance to customers on how to practically make use of this metric. I realize this isn't always the case, so if it doesn't make sense to do so, that's fine too.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @joshimhoff, and @tbg)

If we are talking about write stalls induced by resource overload, I expect the kvprober shadow write latency histogram will actually capture this fairly well. Right?

Yes, it should capture it.

I also agree with this - are we sure there's not a conservative alerting rule we can define here to indicate when things are going south? Or is storage.write-stall-nanos dependent enough on cluster configuration & workload where there's no clear "one size fits all"?

It's very possible that the write stall rate (seconds/second) that is tolerable is dependent on the workload. And that rate doesn't even give a good sense of the latency impact, which a prober or the customer measuring their own latency would do. Based on this discussion thread I am more inclined now towards treating this new metric as something that is useful primarily for diagnosis, and not for alerting.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @joshimhoff, and @tbg)

but looks like there's a rebase, could you make sure the rebase happens separately from any additions? For example, do the rebase next, and make any changes prompted by my comments in a separate fixup commit (which we can squash before merge). Thanks!

Reviewed 1 of 9 files at r1, 6 of 9 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @irfansharif, and @sumeerbhola)

-- commits line 78 at r3:
This could use a bit more context, like

Release note (ops change): I/O admission control now reduces the likelihood of storage layer write stalls which would result from memtable flushes becoming a bottleneck by suitably restricting admission tokens. The new cluster setting admission.min_flush_util_fraction disables or tunes this mechanism; it is enabled by default and can be disabled by setting to a large value such as 10. It should not be changed without recommendation by a domain expert.

pkg/storage/engine.go line 922 at r2 (raw file):

	// calls to this method. Hence, this should be used with care, with only one
	// caller, which is currently the admission control subsystem.
	GetInternalIntervalMetrics() *pebble.InternalIntervalMetrics

This seems a little odd, why don't we return a cumulative metric here? The timeseries would then look at the non-neg rate of this and this seems to sidestep the frailty of this method.

pkg/util/admission/granter.go line 65 at r1 (raw file):

Sorry to nitpick again, but should the explanations be in the setting help and not in a comment? After all, they will be harder to reach if they are only in the source.

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

-- commits line 78 at r3:

Done

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier, @irfansharif, and @tbg)

pkg/storage/engine.go line 922 at r2 (raw file):

It's the histogram (which we don't currently use) -- see the following comment from pebble/metrics.go.

// InternalIntervalMetrics exposes metrics about internal subsystems, that can
// be useful for deep observability purposes, and for higher-level admission
// control systems that are trying to estimate the capacity of the DB. These
// are experimental and subject to change, since they expose internal
// implementation details, so do not rely on these without discussion with the
// Pebble team.
// These represent the metrics over the interval of time from the last call to
// retrieve these metrics. These are not cumulative, unlike Metrics. The main
// challenge in making these cumulative is the hdrhistogram.Histogram, which
// does not have the ability to subtract a histogram from a preceding metric
// retrieval.

pkg/util/admission/granter.go line 65 at r1 (raw file):

Done (duplicated more of the code comment into the setting help).

Reviewed 10 of 10 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier and @sumeerbhola)

pkg/storage/engine.go line 922 at r2 (raw file):

I don't understand, isn't the hdrhistogram returned in *InternalIntervalMetrics cumulative, then? Looking at the impl of GetInternalIntervalMetrics() it looks like we're just attaching a long-lived hdrhistogram, so no resetting is taking place.
However, we are resetting flushWriteThroughput but this seems unnecessary, since the caller knows how to subtract these from each other (it's just a collection of cumulative fields). So I don't think what is blocking this method from just being "canonical" in returning cumulatives entirely.

Besides, one can take deltas of hdrhistograms:

	var g, h *hdrhistogram.Histogram // assumed to use same parameters
	snapg, snaph := g.Export(), h.Export()
	for i := range snapg.Counts {
		snaph.Counts[i] -= snapg.Counts[i]
	}
	delta := hdrhistogram.Import(snaph) // delta == h-g

I'm okay with fixing up the PebbleIntervalMetrics separately.

I'm okay with fixing up the PebbleIntervalMetrics separately.
Ack

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier, @irfansharif, and @tbg)

pkg/storage/engine.go line 922 at r2 (raw file):

No, the hdrhistogram is not cumulative. Since once we grab d.mu.log.metrics we set it to nil

		m.LogWriter.SyncLatencyMicros = d.mu.log.metrics.SyncLatencyMicros
		d.mu.log.metrics = nil

Thanks for the pointer regarding deltas -- I should have looked more closely at the implementation (the sub-bucket stuff threw me off and I thought it was doing data dependent buckets, and not deterministic buckets based on the initial parameters).

pkg/util/admission/granter.go line 2285 at r6 (raw file):

I've modified this comment
// There is a dedicated flush thread. With a maximum compaction concurrency
// of C, we have up to C threads dedicated to handling the write-amp of W
// (caused by rewriting the same data). So C/(W-1) threads on average are
// reading the original data (that will be rewritten W-1 times). Since L0
// can have multiple overlapping files, and intra-L0 compactions are usually
// avoided, we can assume (at best) that the original data (in L0) is being
// read only when compacting to levels lower than L0. That is, C/(W-1)
// threads are reading from L0 to compact to levels lower than L0. Since W
// can be 20+ and C defaults to 3 (we plan to dynamically adjust C but one
// can expect C to be <= 10), C/(W-1) < 1. So the main reason we are
// considering flush tokens is transient flush bottlenecks, and workloads
// where W is small.

pkg/util/admission/granter.go line 2364 at r6 (raw file):

If someone finds it useful. To understand what is really happening with these tokens, I needed to look at multiple numbers, hence the log statement printed every 15s.

pkg/util/admission/granter.go line 2375 at r6 (raw file):

This is talking about flushUtilIgnoreThreshold. It that is too low we may have cases of intFlushUtilization > flushUtilIgnoreThreshold that result in poor quality token computation. Whether that poor quality errs towards fewer tokens than appropriate or more tokens than appropriate, I don't know.

pkg/util/admission/testdata/io_load_listener line 320 at r6 (raw file):
It's sometimes hard to understand a datadriven test if each directive is not self contained -- one has to look at the whole test history. Currently one has to look at the previous directive, since many of these values are cumulative. One could make a case to take the delta of these cumulative values in the directive, to make it more self contained. I'm going to leave such debatable changes for a future PR.

With these test cases below, what most specifically is of interest? The fact that multiplier observes steady decrements with each write stall? If there was a way to only emit the delta from the last printout that would make these tests easier to follow.

The lack of comments was an oversight. I've added them now.

TFTRs!

I've squashed the commits -- I plan to merge when CI is green.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier, @irfansharif, and @tbg)