admission: change store write admission control to use byte tokens

[sumeerbhola]

This switch to byte tokens will result in better accounting for
large writes, including ingests based on whether their bytes land
in L0 or elsewhere. It is also a precurson to taking into account
flush capacity (in bytes).

The store write admission control path now uses a StoreWorkQueue
which wraps a WorkQueue and provides additional functionality:

• Work can specify WriteBytes and whether it is an IngestRequest.
  This is used to decide how many byte tokens to consume.
• Done work specifies how many bytes were ingested into L0, so
  token consumption can be adjusted.

The main framework change is that a single work item can consume
multiple (byte) tokens, which ripples through the various
interfaces including requester, granter. There is associated
cleanup: kvGranter that was handling both slots and tokens is
eliminated since in practice it was only doing one or the other.
Instead, for the slot case the slotGranter is reused. For the token
case there is a new kvStoreTokenGranter.

The main logic change is in ioLoadListener which computes byte
tokens and various estimates. The change is (mostly) neutral if
no write provides WriteBytes, since the usual estimation will
take over. The integration changes in this PR are superficial in
that the requests don't provide WriteBytes. Improvements to the
integration, along with experimental results, will happen in
future PRs.

Informs #79092
Informs #77357

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Just nits

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

-- commits line 6 at r1:
nit: precursor

Code quote:

precurson

---

pkg/util/admission/work_queue.go line 1581 at r1 (raw file):

}

type StoreWriteWorkInfo struct {

you'll need a comment here

---

pkg/util/admission/work_queue.go line 1654 at r1 (raw file):

}

func (q *StoreWorkQueue) AdmittedWorkDone(h StoreWorkHandle, ingestedIntoL0Bytes int64) error {

the linter will complain about the lack of commentary on the public methods.

---

pkg/util/admission/work_queue.go line 1726 at r1 (raw file):

) storeRequester {
	q := &StoreWorkQueue{
		q: *(makeWorkQueue(ambientCtx, KVWork, granter, settings, opts).(*WorkQueue)),

our linter, for better or for worse, doesn't like this because it copied a mutex. you'll need to rework the constructor a bit. Probably you'll need to add some sort of start method or function so you can separate the initialization from kicking off the goroutine.

---

pkg/util/admission/testdata/io_load_listener line 120 at r1 (raw file):

# don't limit the tokens.
#
# TODO: Remove this temp comment for reviewers. All of the earlier test cases

ack

---

pkg/util/admission/granter_test.go line 122 at r1 (raw file):

		buf.Reset()
		buf.Reset()
		buf.Reset()

extra reset calls?

Code quote:

		buf.Reset()
		buf.Reset()

[sumeerbhola]

TFTR!

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

---

-- commits line 6 at r1:

Previously, ajwerner wrote…

nit: precursor

Done

---

pkg/util/admission/work_queue.go line 1581 at r1 (raw file):

Previously, ajwerner wrote…

you'll need a comment here

Done

---

pkg/util/admission/work_queue.go line 1654 at r1 (raw file):

Previously, ajwerner wrote…

the linter will complain about the lack of commentary on the public methods.

Done

---

pkg/util/admission/work_queue.go line 1726 at r1 (raw file):

Previously, ajwerner wrote…

our linter, for better or for worse, doesn't like this because it copied a mutex. you'll need to rework the constructor a bit. Probably you'll need to add some sort of start method or function so you can separate the initialization from kicking off the goroutine.

Done, by doing all the work in initWorkQueue. I've kept the goroutine starting in the init method since there is no reason to delay it.

---

pkg/util/admission/testdata/io_load_listener line 120 at r1 (raw file):

Previously, ajwerner wrote…

ack

Removed

---

pkg/util/admission/granter_test.go line 122 at r1 (raw file):

Previously, ajwerner wrote…

extra reset calls?

ghost, methinks.
removed.

[ajwerner]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

[sumeerbhola]

bors r=ajwerner

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[a-robinson]

@sumeerbhola would there be any interest in backporting just this change from == 0 to <= 0 to v21.2?

The ioTokensExhaustedDurationMetric appears somewhat busted on v21.2 because every time the function containing this code (setAvailableIOTokensLocked in v21.2) is called when sg.availableIOTokens is less than 0, it re-adds now() - sg.exhaustedStart to the metric without resetting the value of sg.exhaustedStart.

On a node that's constantly overloaded, it ends up with a ioTokensExhaustedDurationMetric that increases roughly quadratically, such that taking its rate gives a linearly increasing graph over time, which doesn't seem to be the metric's intent? (especially given that this commit appears to have fixed it)

[a-robinson]

Sorry, I mean v22.1, not v21.2

[irfansharif]

yo @a-robinson, nice see you around! Few of us are out on PTO and others are swarming to shove stuff in for the impending release cut. Filed #86963 to make sure we fix it in 22.1, what you're saying looks right to me.

[a-robinson]

Thanks @irfansharif ! Clearly no rush since it's just a metric, just thought I'd mention it.