• This PR lacks any new tests for the admission package. I'll add those once the KV folks are comfortable with the plumbing changes made in kvserver.
• The first commit is from #85059
• The follower write accounting uses a suggestion from @nvanbenschoten in https://cockroachlabs.slack.com/archives/C038JEXC5AT/p1658763170402819?thread_ts=1657630075.576439&cid=C038JEXC5AT. See that thread for discussion on how it is hard to understand queueing behavior if the token estimation and token consumption does not account for these.
• The snapshot ingestion accounting, where we ignore those bytes, is a simplification of what was prototyped in #80607. What is done here is hopefully a reasonable intermediate step.

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

Those results look sound. Most of my comments are for my own understanding as usual, so .

It would be helpful to spell out in the commit message the reason behind ignoring snapshot-driven-l0-bytes-ingested in some of these components, if we didn't we'd see a large increase in token estimations for regular writes and it would take time for those estimations to get back to an accurate value for just regular writes. During that time we're likely to be under-admitting work because of the over-estimates. (Is this the right way to think about it?) How much of this over-estimation matters given regular writes are able to return unused tokens back in post-work? I can still understand why we'd want to avoid over-estimating at all using the scheme in this PR, I just want to make sure I understand the effects of not having this PR.

I've also now read #80914, so have a coarse understanding of what we're building towards. I haven't read #85722 yet so don't fully see how these tokens map to underlying bandwidth use and how such bandwidth is proportioned in the face of low-volume snapshots and a large volume of snapshots with the scheme outlined in #80914.

I'm not the most familiar with the command application codepaths, but reading through that slack thread and taking a cursory look, I'm wondering if this is the best plumbing point. In this current form, if the command batch fails partway through and we don't end up applying everything, our admission stats+subsequent estimates will be off. I also don't yet know if the overhead of iterating through the commands once at the outset has a measurable enough overhead (mentioned in comment below). Here's my guess at a more appropriate integration point without knowing what code changes are necessary.

1. Over here we should be able to get an accurate view of follower {write,ingested} bytes + # of commands that only considers commands applied successfully (see the error handling that comes after):

2. To collect these stats, could we do them at the same place we're currently maintaining these stats (under batch.Stage):

Probably the current layout is fine, especially if it keeps the code simpler.

Reviewed 2 of 17 files at r1, 19 of 20 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten, @sumeerbhola, and @tbg)

-- commits line 108 at r3:
[nit] Mention that we're doing so without seeking admission/permission (even if obvious since we're consuming tokens after having done the work).

pkg/kv/kvserver/replica_raft.go line 860 at r3 (raw file):

		return stats, getNonDeterministicFailureExplanation(err), err
	}
	followerWriteBytes := appTask.GetFollowerStoreWriteBytesForCommittedEntries(ctx)

[nit] Pull this into the if block below to avoid doing it if disabled.

pkg/kv/kvserver/replica_raft.go line 860 at r3 (raw file):

		return stats, getNonDeterministicFailureExplanation(err), err
	}
	followerWriteBytes := appTask.GetFollowerStoreWriteBytesForCommittedEntries(ctx)

Do we have microbenchmarks in the command application path that can tell us what the overhead of this is? It looks cheap enough, but there are two lock acquisitions underneath FollowerStoreWriteBytes to maintain the stats correctly.

pkg/kv/kvserver/replica_raftstorage.go line 945 at r3 (raw file):

		}
	}
	var ingestStats pebble.IngestOperationStats

Ditto, fold this into the nil check below. I was looking through non-test callers of db.IngestExternalFiles and found these two:

Do we need to eventually plumb statistics from those ingestion points into AC? (I've not closely read #80914 so my question is likely naive. EDIT: Just read that PR, still have the question). Leave TODOs if so. Ideally we'd get rid of IngestExternalFiles altogether and replace it with just the one variant that returns statistics, ignoring results when unnecessary.

pkg/kv/kvserver/store.go line 3926 at r3 (raw file):

}

// SnapshotIngested implements the KVAdmissionController itnerface.

Typo: itnerface.

pkg/util/admission/store_token_estimation.go line 310 at r1 (raw file):

//   accurate. Additionally, we will scale down the bytes for
//   at-admission-tokens by the fraction
//   bytes-that-sought-permission/(bytes-that-sought-permission +

I'm not seeing this scale-down math for work that sought admission vs. bypassed it. Can you point me to it? If it's missing and we still think it's a good idea, should we add a TODO somewhere?

pkg/util/admission/testdata/format_adjust_tokens_stats.txt line 6 at r3 (raw file):

compaction score 0.000 (0 ssts, 0 sub-levels), L0 growth 0 B (write 0 B ingest 0 B ignored 0 B): requests 0 (0 bypassed) with 0 B acc-write (0 B bypassed) + 0 B acc-ingest (0 B bypassed) + write-model 0.00x+0 B (smoothed 0.00x+0 B) + ingested-model 0.00x+0 B (smoothed 0.00x+0 B) + at-admission-tokens 0 B, compacted 0 B [≈0 B], flushed 0 B [≈0 B]; admitting all
real-numbers:
compaction score 2.700[L0-overload] (195 ssts, 27 sub-levels), L0 growth 577 MiB (write 0 B ingest 0 B ignored 0 B): requests 0 (0 bypassed) with 0 B acc-write (0 B bypassed) + 0 B acc-ingest (0 B bypassed) + write-model 0.00x+0 B (smoothed 0.00x+0 B) + ingested-model 0.00x+0 B (smoothed 0.00x+0 B) + at-admission-tokens 0 B, compacted 77 MiB [≈62 MiB], flushed 0 B [≈0 B]; admitting 116 MiB (rate 7.7 MiB/s) due to L0 growth (used 0 B)

Update test to include non-zero values for bypassed writes.

TFTR!

It would be helpful to spell out in the commit message the reason behind ignoring snapshot-driven-l0-bytes-ingested in some of these components, if we didn't we'd see a large increase in token estimations for regular writes and it would take time for those estimations to get back to an accurate value for just regular writes. During that time we're likely to be under-admitting work because of the over-estimates. (Is this the right way to think about it?)

Yes, that's the right way to think about it. Updated the commit message.

How much of this over-estimation matters given regular writes are able to return unused tokens back in post-work?

It will matter since the locally proposed writes and ingests token consumption model will be wrong, so they will overconsume to compensate. And the model will be wrong in the sense of having a large constant, which means even a mix of locally proposed workloads of different sizes will not be well modeled (which we should be otherwise able to, with a constant factor close to 1, and most of the bytes being accounted for via the multiplier).

I've also now read #80914, so have a coarse understanding of what we're building towards

We may or may not build towards that, but this PR is roughly a step in the right direction.

I haven't read #85722 yet so don't fully see how these tokens map to underlying bandwidth use and how such bandwidth is proportioned in the face of low-volume snapshots and a large volume of snapshots with the scheme outlined in #80914.

We are going to use these same bypassed writes and ingests when fitting the model for the full incoming bytes into the LSM. So from a modeling of token consumption perspective, this PR gives us a clean answer for disk bandwidth too. However, we won't be consuming any elastic tokens for these follower writes, since they are not something we can actually stop -- the elastic tokens are only for locally proposed work.

I'm not the most familiar with the command application codepaths, but reading through that slack thread and taking a cursory look, I'm wondering if this is the best plumbing point.

I'm not familiar with it either, but I took a look at your suggestion and it is likely better. I had looked for a way to not introduce additional methods in the apply package, but had missed that we have a specific implementation of apply.StateMachine that we have created and given to apply.Task, and that we are already tracking stats in there. I'll give it a try and ping this PR when ready.

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

-- commits line 108 at r3:

Done

pkg/kv/kvserver/replica_raft.go line 860 at r3 (raw file):

Done.
btw, we still have these non-nil gated logic for admission control, which is a leftover from the initial integration. There shouldn't be any production code where r.store.cfg.KVAdmissionController is nil.

pkg/kv/kvserver/replica_raft.go line 860 at r3 (raw file):

There aren't microbenchmarks for admission control code. Those are probably worth adding in the future. In the past I have looked at contention profiles in roachtests and they have not looked significant.

pkg/kv/kvserver/replica_raftstorage.go line 945 at r3 (raw file):

Ditto, fold this into the nil check below.

we need to call IngestExtternalFiles* anyway.

Do we need to eventually plumb statistics from those ingestion points into AC?

We could, but the current plan is not to bother unless we really have a need since that is below raft code, and hopefully our linear model does a decent job of telling us what fraction will be ingested into L0. If we can't model it well, we could look into it.

pkg/kv/kvserver/store.go line 3926 at r3 (raw file):

Done

pkg/util/admission/store_token_estimation.go line 310 at r1 (raw file):

I had decided not to do this, since we are compensating for things at done time anyway. But thinking again about this now, it may have been muddled thinking, and I should indeed do this.
I'll make the change and ping.

pkg/util/admission/testdata/format_adjust_tokens_stats.txt line 6 at r3 (raw file):

This will be subsumed by the TODO I added based on your comment on the previous PR. I prefer to do it later.

Great to see this come together! I didn't scrutinize the test cases in detail but this looks good.

I am curious if we're perhaps regressing, in some way, on snapshot overload with this change. Let's say we have a node where the I/O overload is driven 50% by foreground traffic and 50% by snapshots (which all go to L0 in this example for reasons).
Before this PR, the L0 growth in snapshots would penalize the foreground traffic. The snapshots would continue, but foreground traffic would basically stop - the I/O load drops to 50% of what it was before admission control kicked in.
After this PR, we're basically "ignoring" the snapshots. Admission control pretends the L0 growth caused by them isn't actually happening. Say workload and snaps are contributing 100mb/s into L0, each. We're still overloaded, so it is doing something to slow down foreground traffic. We notice that L0 is growing at ~100mb/s (it's really 200mb/s but we're ignoring half). We're compacting out of L0 at 100mb/s as well. So we're targeting 50mb/s (half of L0->LX compaction rate), in effect slowing the foreground workload down to 50mb/s. But the aggregate 150mb/s will probably still overwhelm the disk, at least if it's one of the 125mb/s AWS gp3 volumes.

Of course this is all a bit contrived, since on a 125mb/s volume you better not set your snapshot rate limits to >= 100mb/s! And we could always lower it. Additionally, the allocator would not place new replicas on an overloaded store. And, additionally, other leaseholders would pause this follower, i.e. not send raft snapshots either (this is assuming the cluster setting for this is on). So there are multiple levels of defense here already.

And if they all fail - can still lower the snap rates. I assume that seeing I/O overload, we would check the aggregate disk throughput, as well as the snapshot rcvd bytes rate. We'd see the latter reflect a substantial fraction of the aggregate throughput and go to lower the snapshot rate (or to temporarily stop snapshots by recommissioning, etc). That seems reasonable enough. The IO overload log message would also indicate a large byte count for the "ignored" fields, which is a more direct (though only to experts) clue.

If it isn't too much work, could you share whatever you use to run your overload experiments? I am currently working on a few follow-ups to #83851 and during stability plan to revisit the corresponding roachtest #81516, and hope to add more powerful coverage of various overload scenarios including some that you have concocted, to additionally bolster our confidence in the protections we've been adding.

Reviewed 6 of 17 files at r1, 17 of 20 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @nvanbenschoten, and @sumeerbhola)

pkg/kv/kvserver/replica_raft.go line 862 at r4 (raw file):

	if r.store.cfg.KVAdmissionController != nil {
		followerWriteBytes := appTask.GetFollowerStoreWriteBytesForCommittedEntries(ctx)
		r.store.cfg.KVAdmissionController.FollowerStoreWriteBytes(

Could you add a comment about the placement of this call? At this point, we have the entries in the log & know they are committed, but we have not applied them yet. We're realistically about to do so momentarily so it doesn't really matter, but conceptually, do we want to do this after or before application? Or even after the append, before we even know the entry is committed? In other words, do we have any intuition about the ideal world semantics we want here.

pkg/util/admission/testdata/io_load_listener line 284 at r4 (raw file):

set-state l0-bytes=1000 l0-added-write=191000 l0-added-ingested=30000 l0-files=21 l0-sublevels=21 print-only-first-tick=true
----
compaction score 1.050[L0-overload] (21 ssts, 21 sub-levels), L0 growth 20 KiB (write 20 KiB ingest 0 B ignored 0 B): requests 10 (0 bypassed) with 20 KiB acc-write (0 B bypassed) + 0 B acc-ingest (0 B bypassed) + write-model 1.00x+1 B (smoothed 1.26x+1 B) + ingested-model 0.00x+0 B (smoothed 1.12x+1 B) + at-admission-tokens 5.9 KiB, compacted 20 KiB [≈59 KiB], flushed 0 B [≈0 B]; admitting 27 KiB (rate 1.8 KiB/s) due to L0 growth (used 0 B)

drive-by comment, would it make sense to highlight, in that message, a constant term >> 1 in the model, since it indicates that the fit isn't working well?

Something like write-model 2.99x+1025[!] whenever b > 10 or something like that?

TFTRs!

@irfansharif I have made the plumbing change to collect the stats in replicaAppBatch.Stage and to transfer them to replicaStateMachine in recordStatsOnCommit.

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

pkg/kv/kvserver/replica_raft.go line 862 at r4 (raw file):

I think ideally we want them after committing. I have changed this based on @irfansharif 's earlier suggestion, so it happens after commit.

pkg/util/admission/testdata/io_load_listener line 284 at r4 (raw file):

This log entry requires enough expertise to understand as it is. I somewhat doubt that adding an exclamation mark will help.

Also rebased on master since the first commit from the other PR has merged.

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

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

pkg/util/admission/store_token_estimation.go line 310 at r1 (raw file):

Sorry, I was not thinking straight last night. This is handled in that we are taking the total bytes added to L0 and dividing it by the total work count (including those that bypassed). It is just that the logic is not the same as what was written in the TODO.

I haven't read the updates to util/admission and am getting pulled away for meetings, but the plumbing changes to kvserver to pass the write bytes from follower entry application and snapshot ingestion in to StoreWorkQueue all look good.

Reviewed 3 of 20 files at r2, 15 of 18 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @sumeerbhola, and @tbg)

pkg/kv/kvserver/apply/cmd.go line 44 at r5 (raw file):

	// once per Command.
	AckErrAndFinish(context.Context, error) error
	// GetStoreWriteByteSizes returns the size of the writes to the store:

Does this still need to be on the interface? We no longer use it from this package.

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

pkg/util/admission/store_token_estimation.go line 310 at r1 (raw file):

That approach makes sense, thanks.

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

pkg/kv/kvserver/apply/cmd.go line 44 at r5 (raw file):

Good point. Done.