storage: add new CheckSSTConflicts randomized test

[jbowens]

enginepb: add MVCCStats.Formatted

Move the storage test formatStats function onto the MVCCStats type itself in
preparation for using it in additional places.

storage: change CheckSSTConflicts start and end types

Callers of CheckSSTConflicts pass in start and end boundaries for the sstable.
Previously, these were passed as MVCCKeys, although the end key's timestamp was
ignored. This resulted in confusing semantics whereby the end boundary was
interpreted as exclusive end bound of end.Key.

storage: add new CheckSSTConflicts randomized test

Add a new randomized test exercising CheckSSTConflicts. Additionally, sketch
out a meta package to aid in writing randomized tests like this one. In the
future, this package may be extracted to the cockroachdb/metamorphic
repository.

Epic: None
Release note: None
Informs #94141.
Informs cockroachdb/pebble#2086.

[cockroach-teamcity]

This change is 

[jbowens]

This test is currently failing. I'm hoping it's failing due to #94141 and the test can be used to produce a simpler reproduction of that issue.

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

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola)

---

pkg/storage/meta/run.go line 58 at r2 (raw file):

// each item. It's intended to be used with a function returned by Weighted,
// whhere the item itself is func(rng *rand.Rand).
func Generate[I any](rng *rand.Rand, n int, fn func(*rand.Rand) func(*rand.Rand) I) []I {

I don't get the generic-ness of ItemWeight if we're only going to use them with this function which assumes that the items are a func(*rand.Rand) I.

Can't we have a GenWithWeight[I] struct { Gen func(*rand.Rand) I; Weight int } and pass a ...GenWithWeight[I] to Generate ? Are we envisioning any other kind of usage?

[jbowens]

@itsbilal — looks like this is still failing after rebasing over master, including #98426.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @sumeerbhola)

---

pkg/storage/meta/run.go line 58 at r2 (raw file):

Are we envisioning any other kind of usage?

Yes. Picking among weighted items is expected to be useful during generation of the individual options themselves as well. For example, the Pebble metamorphic tests pick among IterOptions.KeyTypes enum values.

[jbowens]

Looks like this one is still failing, even if kvnemesis isn't 🤔

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @sumeerbhola)

[itsbilal]

@jbowens I realized that the randomized test produces sst range keys that are of different timestamps. While the function supports that, it's unlikely (impossible I believe?) for that to happen with practical uses of AddSSTable; the entirety of the SST should be of the same timestamp. Maybe someone in KV can confirm, but at least this sort of a failure doesn't point to something imminently serious.

[jbowens]

I'm forgetting, do we support RESTORE-ing with history, or only backing up with history and restoring the most recent values? If we RESTORE with history, that must use AddSSTable without setting the sstTimestamp parameter.

I suppose the disaggregated storage RESTORE will need to restore with history or perform read-time timestamp rewriting, but that'll be for 23.2 in any case.

[jbowens]

@itsbilal I asked in #disaster-recovery and it sounds like both cluster-to-cluster streaming and tenant restores both use AddSSTable with varied timestamps.

[jbowens]

I'd sometimes see this error in this test, which would cause a panic and fail the test:

I fixed this—there was a sequence of during generation that could leave rangeStart unset, generating a key with a NUL key start bound.

With

./dev test -f TestCheckSSTConflictsRandomized pkg/storage --stress

it looks like we still have at least one more stats calculation issue :(

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

[itsbilal]

This came up in the storage weekly, but the stats failures I'm seeing are involving the same key being present in the sst at different timestamps, of which one has a conflict but we don't error on the conflict (and I believe this function has never errored on this type of a conflict). Here's one example:

          SST:
            "ni"/0.000001035,0→/BYTES/jjub
            "ni"/0.000000970,0→/<empty>
          Before:
            "nd"/0.000000992,0→/BYTES/ljxrdz
            "ni"/0.000000970,0→/BYTES/xscxnv
          After:
            "nd"/0.000000992,0→/BYTES/ljxrdz
            "ni"/0.000001035,0→/BYTES/jjub
            "ni"/0.000000970,0→/<empty>

We should have errored on the ni@970 conflict but we didn't, instead our stats are incorrect as it just assumed ni@1035 is the only inbound key with the prefix "ni".

Asking disaster recovery on whether this is possible in the first place.

[itsbilal]

In addition, I noticed that there's a subtle bug in how we set start and end when calling CheckSSTConflicts in the randomized test. All callers of CheckSSTConflicts pass meta keys i.e. timestamp 0 as start/end bounds of the sstable, except for the randomized test. This change fixes this:

diff --git a/pkg/storage/sst_test.go b/pkg/storage/sst_test.go
index 5400e12f114..95f4fc8e546 100644
--- a/pkg/storage/sst_test.go
+++ b/pkg/storage/sst_test.go
@@ -476,8 +476,8 @@ func (o *checkSSTConflictsOp) Run(l *meta.Logger, s *sstTestState) {
                ctx,
                f.Bytes(),
                s.engine,
-               s.bufferedOps[0].sortKey(),
-               s.bufferedOps[len(s.bufferedOps)-1].sortKey(),
+               MVCCKey{Key: s.bufferedOps[0].sortKey().Key},
+               MVCCKey{Key: s.bufferedOps[len(s.bufferedOps)-1].sortKey().Key},
                o.start,
                o.end,
                o.disallowShadowing != nil,
@@ -585,7 +585,7 @@ func (o *addSSTableOp) Run(l *meta.Logger, s *sstTestState) {
                ctx,
                f.Bytes(),
                s.engine,
-               s.bufferedOps[0].sortKey(),
+               MVCCKey{Key: s.bufferedOps[0].sortKey().Key},
                MVCCKey{Key: o.end},
                o.start,
                o.end,

[itsbilal]

Took a look at all uses of AddSSTable that I could find, and I couldn't find a single one where we add multiple MVCC keys per roachpb.Key, and also do conflict checking. The c2c case, where we could add multiple keys, sets none of DisallowConflicts, DisallowShadowing or DisallowShadowingBelow so CheckSSTConflicts won't run there to begin with. It's why the assumption that this function has made for years (incl. pre-range keys) is safe to make.

Knowing this, I think we should update the randomized test to only output one TS per user key for point keys.

[jbowens]

In addition, I noticed that there's a subtle bug in how we set start and end when calling CheckSSTConflicts in the randomized test. All callers of CheckSSTConflicts pass meta keys i.e. timestamp 0 as start/end bounds of the sstable, except for the randomized test. This change fixes this:

There's a lot of subtlety there in the CheckSSTConflicts API. The end boundary doesn't make sense as a MVCCKey if we require end.Key to be an exclusive bound (not even end itself).

[jbowens]

4417 runs so far, 0 failures, over 21m55s

😎

Thanks for your help here @itsbilal!

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

[jbowens]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[yuzefovich]

On CI run:

Failed
=== RUN   TestCheckSSTConflictsRandomized
    test_log_scope.go:161: test logs captured to: /artifacts/tmp/_tmp/c5eb8fc8b8e683c19f3c3e4238f64094/logTestCheckSSTConflictsRandomized3898596176
    test_log_scope.go:79: use -show-logs to present logs inline
    sst_test.go:183: Using seed -4844134980158906090; to re-run set COCKROACH_RANDOM_SEED=-4844134980158906090
    run.go:148: PutMVCC timestamp is empty
    run.go:155: History:
        
        op      0: MVCCDelete("diez"@0) = ok
        op      1: CheckSSTConflicts(disallowShadowing=<nil>, usePrefix=true) = error: PutMVCC timestamp is empty
    panic.go:522: -- test log scope end --

ERROR: a panic has occurred!
Details cannot be printed yet because we are still unwinding.
Hopefully the test harness prints the panic below, otherwise check the test logs.

test logs left over in: /artifacts/tmp/_tmp/c5eb8fc8b8e683c19f3c3e4238f64094/logTestCheckSSTConflictsRandomized3898596176
--- FAIL: TestCheckSSTConflictsRandomized (0.02s)

bors r-

[craig]

Canceled.

[itsbilal]

LGTM

Does the failing seed pass with this change?

[jbowens]

Yes, although stressing locally again uncovered another failure:

sst_test.go:183: Using seed -4293931100192978035; to re-run set COCKROACH_RANDOM_SEED=-4293931100192978035
    run.go:148: range "u": stats calculated from CheckSSTConflicts differ:
        SST:
          key_count=+1 key_bytes=+17 val_count=+1 val_bytes=+12 range_key_count=+1 range_key_bytes=+17 range_val_count=+1 range_val_bytes=+7
        Conflict:
          range_key_count=+1 range_key_bytes=+20 range_val_count=+1 range_val_bytes=+7
        Before:
          range_key_count=1 range_key_bytes=17 range_val_count=1 range_val_bytes=7
        Before+SST+Conflict:
          key_count=1 key_bytes=17 val_count=1 val_bytes=12 range_key_count=3 range_key_bytes=54 range_val_count=3 range_val_bytes=21
        ComputeStats:
          key_count=1 key_bytes=17 val_count=1 val_bytes=12 range_key_count=1 range_key_bytes=14 range_val_count=1 range_val_bytes=7
        Diff:
          range_key_count=-2 range_key_bytes=-40 range_val_count=-2 range_val_bytes=-14

This might be a consequence of the SST defining utsp both within the bounds of a MVCC Delete Range and as a point?

          SST:
            u{fvx-u}/0.000000156,0→/<empty>
            "utsp"/0.000000104,0→/BYTES/
          Before:
            u{-jbst}/0.000000156,0→/<empty>
          After:
            u{-u}/0.000000156,0→/<empty>
            "utsp"/0.000000104,0→/BYTES/
          error: range "u": stats calculated from CheckSSTConflicts differ:

[itsbilal]

@jbowens At first glance that looks like a genuine failure i.e we should be returning a WriteTooOld error (I think) if a point is sliding under an existing range key. But maybe we aren't because it's also under an existing sst range key, which is an idempotent write with the existing ext range key (i.e. same timestamp). Idempotent writes cause all sorts of special cases and are a headache to handle in CheckSSTConflicts, and this is a partial idempotent write which makes it especially annoying.

I imagine the stats failure would occur even if the point didn't exist, as it looks like it's about the handling of partially-idempotent range keys merging into existing range keys. The point seems to be correctly accounted for (even if maybe it should be a conflict).