admission,kvserver: allow specification of a min-rate for snapshot disk writes

[sumeerbhola]

This eliminates the risk of starvation for incoming snapshots. When a
snapshot write proceeds despite lack of tokens in the requester's token
bucket, the write bytes are accounted for, which affects other elastic
work that requests bytes in the future (as desired). The minRate is
calculated to ensure that the snapshots will be consumed in half the
configured timeout allotted by the sender.
A new AdmittedSnapshotBytes counter metric is added as well.

Epic: none

Release note (ops change): A new cluster setting
kvadmission.store.snapshot_ingest_bandwidth_control.min_rate.enabled
is introduced. When true and disk bandwidth-based admission control is
active, snapshot ingestion will be admitted at a minimum rate. This
prevents snapshot ingestion from being starved by other elastic work.

[cockroach-teamcity]

This change is 

[sumeerbhola]

Only the last commit is relevant.

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

[sumeerbhola]

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

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 151 at r3 (raw file):

			// to set a deadline. All we want here is to prevent the sender from
			// timing out.
			minRate = rebalanceSnapshotRate.Get(&s.cfg.Settings.SV)

@tbg the minRate is simply the send rate, but we could potentially relax this -- this is where your input is needed.
Also, currently we look at the cluster setting once per incoming snapshot. That should be easy to change if you think we should be more up-to-date when the cluster setting is changed.

[sumeerbhola]

I didn't look at the tests but the rest

@sumeerbhola reviewed 7 files and made 5 comments.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @angeladietz and @tbg).

---

-- commits line 11 at r6:
nit: reminder about this todo

---

pkg/util/admission/snapshot_queue.go line 187 at r6 (raw file):

	defer func() {
		if err == nil {
			// todo(angela): if count is negative, is it correct to be decrementing the counter?

I think not. There is some kind of test build that asserts that the counter only increases.

We could ignore the negative values for now since they will only happen at the end when there was a small over-estimation. Most calls here are happening when SnapshotBurstSize is exceeded, and so the intermediate over-estimation will be swallowed inside SnapshotPacer. In other parts of the AC code we sometimes have two counters and one has to subtract one from the other, but that seems excessive in this case.

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 148 at r6 (raw file):

		minRate := int64(0)
		if admission.DiskBandwidthForSnapshotIngestMinRateEnabled.Get(&s.cfg.Settings.SV) {
			maxFractionOfTimeoutSpentApplyingSnapshot := 1 - snapshotReservationQueueTimeoutFraction.Get(&s.cfg.Settings.SV)

isn't this minFraction... given it is 1-x?
What we want is a lower-bound on the slowdown factor that will be acceptable to the sender. Which is what we get when multiplying two lower-bound terms below. Is my understanding correct?

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

			// Cap minRate at 10% of the store's provisioned bandwidth to
			// prevent oversaturating the disk when rebalanceSnapshotRate is large.
			storeBW := s.cfg.KVAdmissionController.GetProvisionedBandwidth(s.StoreID())

@tbg and you can make the final decision on this, but I am not sure we need to do anything with provisioned bandwidth here, since if we slow down a lot we risk a timeout. Perhaps we should simply notice that the provisioned bandwidth is insufficient and log a warning, instead of changing minRate.

[angeladietz]

@angeladietz made 3 comments and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola and @tbg).

---

-- commits line 11 at r6:

Previously, sumeerbhola wrote…

nit: reminder about this todo

done, and squashed my commits into this one. Would appreciate a brief look at the release note since this is one of the first release notes i've written.

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 148 at r6 (raw file):

Previously, sumeerbhola wrote…

isn't this minFraction... given it is 1-x?
What we want is a lower-bound on the slowdown factor that will be acceptable to the sender. Which is what we get when multiplying two lower-bound terms below. Is my understanding correct?

yes your understanding is correct, I updated the variable name to be more accurate.

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

Previously, sumeerbhola wrote…

@tbg and you can make the final decision on this, but I am not sure we need to do anything with provisioned bandwidth here, since if we slow down a lot we risk a timeout. Perhaps we should simply notice that the provisioned bandwidth is insufficient and log a warning, instead of changing minRate.

ack, i'll wait for tobi to chime in. My reasoning behind this is that if the customer sets rebalanceSnapshotRate to be super big, then the minRate will also be super big, so the snapshot ingestion could interfere with foreground traffic. I'm okay to just log a warning instead if that's preferred.

[tbg]

after comments 🚀

@tbg reviewed 11 files and all commit messages, made 9 comments, resolved 3 discussions, and dismissed @sumeerbhola from a discussion.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @angeladietz and @sumeerbhola).

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 151 at r3 (raw file):

Previously, sumeerbhola wrote…

@tbg the minRate is simply the send rate, but we could potentially relax this -- this is where your input is needed.
Also, currently we look at the cluster setting once per incoming snapshot. That should be easy to change if you think we should be more up-to-date when the cluster setting is changed.

Looking at this only once per snapshot seems fine to me. Using the send rate for now seems fine to me too, though it negates some of the benefits. We can do better once we have #160106.

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

Previously, angeladietz (Angela Dietz) wrote…

ack, i'll wait for tobi to chime in. My reasoning behind this is that if the customer sets rebalanceSnapshotRate to be super big, then the minRate will also be super big, so the snapshot ingestion could interfere with foreground traffic. I'm okay to just log a warning instead if that's preferred.

A good outcome of this work would be if we could at least experimentally tell people that they can set the snapshot rate sky high (with disk bandwidth AC configured) and "let it rip". If the sender-side timeout becomes a problem, users can increase queueGuaranteedProcessingTimeBudget (and ultimately we'll have to provide a solution that works better out of the box). queueGuaranteedProcessingTimeBudget defaults to 60s, so the effective transfer rate for a 512mb range would have to fall below 8mb/s to get a timeout, so hopefully not too frequent in practice. Ultimately, that sender-side mechanism needs to be reworked. I filed #160106. In fact I made an epic: https://cockroachlabs.atlassian.net/browse/CRDB-58158

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 166 at r8 (raw file):

			if storeBW > 0 {
				maxSnapshotMinRate := int64(float64(storeBW) * 0.10)
				if minRate > maxSnapshotMinRate {

if storeBW > 0 {
  minRate = min(minRate, int64(float64(storeBW) * 0.1)
}

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 173 at r8 (raw file):

		timer := &timeutil.Timer{}
		pacer = admission.NewSnapshotPacer(snapshotQ, minRate, timer.AsTimerI())

I assume this will take a non-zero &Timer in tests, right? Otherwise, no point to pass this in.

---

pkg/util/admission/snapshot_queue.go line 69 at r8 (raw file):

	settings.SystemOnly,
	"kvadmission.store.snapshot_ingest_bandwidth_control.min_rate.enabled",
	"if set to true, snapshot ingests will be admitted at a minimum rate",

Mention here that this setting only has an effect when provisioned disk bandwidth is enabled, and that disabling this setting in that situation can lead to snapshots being starved out by foreground traffic.

---

pkg/util/admission/snapshot_queue.go line 95 at r8 (raw file):

		AdmittedSnapshotBytes: *metric.NewCounter(metric.Metadata{
			Name:        "admission.admitted_snapshot_bytes",
			Help:        "Number of bytes admitted for snapshot ingests",

Clarify whether this will count bytes that were admitted because throttling was disabled (i.e. when provisioned bandwidth is not configured, there is no throttling, but will this counter increase?)

---

pkg/util/admission/snapshot_queue.go line 279 at r8 (raw file):

				tokensToSubtract = 0
			}
			// TODO(aaditya): Ideally, we also remove the item from the actual queue.

was this copy-pasted from someplace else? Aaditya has been gone a while, we should at least not give him new TODOs 😄
Maybe there is a way to cross-reference the multiple places that have this issue in another way.

---

pkg/kv/kvserver/queue.go line 83 at r8 (raw file):

// snapshotIngestSlowdown is the factor used to determine the minimum rate of
// snapshot ingestion. This is half of the permittedRangeScanSlowdown factor,

nit: "ingest" is a pebble concept (ingest an SST into the LSM). Looking at this code, I think it might also be better to just inline this at the usage site (reference permittedRangeScanSlowdown directly).

[sumeerbhola]

@sumeerbhola made 1 comment.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @angeladietz and @tbg).

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

A good outcome of this work would be if we could at least experimentally tell people that they can set the snapshot rate sky high (with disk bandwidth AC configured) and "let it rip". If the sender-side timeout becomes a problem, users can increase queueGuaranteedProcessingTimeBudget ... Ultimately, that sender-side mechanism needs to be reworked

You all should make the final decision, but the part about letting it rip before fixing the sender-side mechanism makes me very nervous. We could be wasting a lot of bandwidth by timing out after sending part of the snapshot, which can be both a resource cost and a real cost (in public cloud), and additionally degrade the cluster if no snapshot transfers are succeeding (folks can run under-replicated for a long time before noticing if they haven't setup their alerting correctly).

[tbg]

@tbg made 1 comment.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @angeladietz and @sumeerbhola).

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

Previously, sumeerbhola wrote…

A good outcome of this work would be if we could at least experimentally tell people that they can set the snapshot rate sky high (with disk bandwidth AC configured) and "let it rip". If the sender-side timeout becomes a problem, users can increase queueGuaranteedProcessingTimeBudget ... Ultimately, that sender-side mechanism needs to be reworked

You all should make the final decision, but the part about letting it rip before fixing the sender-side mechanism makes me very nervous. We could be wasting a lot of bandwidth by timing out after sending part of the snapshot, which can be both a resource cost and a real cost (in public cloud), and additionally degrade the cluster if no snapshot transfers are succeeding (folks can run under-replicated for a long time before noticing if they haven't setup their alerting correctly).

I didn't mean to suggest doing this in production. Rather, it would be a good outcome of this work if in controlled experiments (where snapshots don't run into the timeout) can succeed at much higher speeds, without impacting foreground traffic. For production, we'd need the timeout mechanism to be more robust ("if it does do work, don't cancel it"), which is tracked in the epic.

[angeladietz]

@angeladietz made 3 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @sumeerbhola and @tbg).

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 163 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I didn't mean to suggest doing this in production. Rather, it would be a good outcome of this work if in controlled experiments (where snapshots don't run into the timeout) can succeed at much higher speeds, without impacting foreground traffic. For production, we'd need the timeout mechanism to be more robust ("if it does do work, don't cancel it"), which is tracked in the epic.

thanks for the input and creating that new epic! I updated the code to log a warning instead of lowering the minrate according to the provisioned bandwidth.

---

pkg/kv/kvserver/kv_snapshot_strategy.go line 173 at r8 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I assume this will take a non-zero &Timer in tests, right? Otherwise, no point to pass this in.

yep, manual timers are passed in in tests.

---

pkg/util/admission/snapshot_queue.go line 279 at r8 (raw file):

Previously, tbg (Tobias Grieger) wrote…

was this copy-pasted from someplace else? Aaditya has been gone a while, we should at least not give him new TODOs 😄
Maybe there is a way to cross-reference the multiple places that have this issue in another way.

yeah, looks like it was copied from ~40 lines up. fixed.

[angeladietz]

bors r+

[craig]

Build succeeded:

• local_roachtest
• unit_tests
• local_roachtest_fips
• check_generated_code
• lint
• examples_orms
• docker_image_amd64
• linux_amd64_fips_build
• acceptance
• linux_amd64_build