[CRDB-2743] sql: create builltin generator crdb_internal.probe_range

[Santamaura]

Previously there was difficulty in diagnosing kv layer health when
an incident occurs. This patch introduces the new virtual table
crdb_internal.probe_range which utilizes the kvprober to probe
each range to determine if the range can be reached or not.

resolves #61695

Release note: None

[cockroach-teamcity]

This change is 

[Santamaura]

One thing I am unsure of is in the case of a timeout whether the Prober.writePlanner will update the next step or not. If it doesn't and we are iterating through ranges we've already visited we may end up in an infinite loop.

[tbg]

Thanks Alex! I left some conceptual comments that I think will simplify things a bit, and will also address your question (by removing it).

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @joshimhoff, @koorosh, @Santamaura, and @zachlite)

---

pkg/sql/sem/builtins/generator_builtins.go, line 478 at r1 (raw file):

		makeGeneratorOverload(
			tree.ArgTypes{
				{Name: "timeout", Typ: types.Int},

shouldn't this be something like types.IntervalDurationType or types.Interval?

---

pkg/sql/sem/builtins/generator_builtins.go, line 1749 at r1 (raw file):

// rangeKeyIterator is a ValueGenerator that iterates over all
// SQL keys in a target range.
// REFER TO HERE

?

---

pkg/sql/sem/builtins/generator_builtins.go, line 2436 at r1 (raw file):

	if err != nil {
		return nil, pgerror.Newf(
			pgcode.InFailedSQLTransaction,

Is this really the right code?

---

pkg/sql/sem/builtins/generator_builtins.go, line 2465 at r1 (raw file):

		prober:          *kvProber,
		ranges:          make(map[int64]int),
		remainingRanges: int64(numRanges),

This doesn't seem right. The range count changes out from under you through splits and merges, so any code that stores it like this is likely fishy.

---

pkg/sql/sem/builtins/generator_builtins.go, line 2481 at r1 (raw file):

// Next implements the tree.ValueGenerator interface.
func (p *probeRangeGenerator) Next(ctx context.Context) (bool, error) {
	if p.remainingRanges == 0 {

Yeah this isn't going to be correct. If a range splits while the probe is underway, you'll end up stopping short. I'm not sure what will happen if a range merges but it doesn't really matter, we need to fix this.

I think the root problem is that this code wants to use the planner "as is". Instead, we should refactor the planner to make the interesting parts of it more reusable, and then reuse only those parts. We need only two things here: iterate through all range descriptors (i.e. discover all of the ranges in the system, with a consistent view) and then probe each of them.

Thinking this through a bit, there actually isn't a good reason to use the planner's ability to discover ranges, as it's too complex and prober-specific for what we need. The code we want is really this:

cockroach/pkg/sql/crdb_internal.go

Lines 3224 to 3227 in d4daa99

	ranges, err := kvclient.ScanMetaKVs(ctx, p.txn, roachpb.Span{
	Key: keys.MinKey,
	EndKey: keys.MaxKey,
	})

This isn't streaming which isn't ideal, but it's also what is used by crdb_internal.ranges_no_leases, so when we fix that we'll automatically fix it here. It is fine to use for now.

I'm also noticing that the internal_no_leases generator is much more compact and doesn't seem to need to define a new type, so maybe you can adopt this pattern here and forfeit a bunch of boilerplate? I might be missing something.

As for the probing itself, we "just" need to export proberOpsImpl:

cockroach/pkg/kv/kvprober/kvprober.go

Lines 160 to 186 in 18b95cb

	// proberOpsImpl is used to probe the kv layer.
	type proberOpsImpl struct {
	}
	// We attempt to commit a txn that reads some data at the key.
	func (p *proberOpsImpl) Read(key interface{}) func(context.Context, *kv.Txn) error {
	return func(ctx context.Context, txn *kv.Txn) error {
	_, err := txn.Get(ctx, key)
	return err
	}
	}
	// We attempt to commit a txn that puts some data at the key then deletes
	// it. The test of the write code paths is good: We get a raft command that
	// goes thru consensus and is written to the pebble log. Importantly, no
	// *live* data is left at the key, which simplifies the kvprober, as then
	// there is no need to clean up data at the key post range split / merge.
	// Note that MVCC tombstones may be left by the probe, but this is okay, as
	// GC will clean it up.
	func (p *proberOpsImpl) Write(key interface{}) func(context.Context, *kv.Txn) error {
	return func(ctx context.Context, txn *kv.Txn) error {
	if err := txn.Put(ctx, key, putValue); err != nil {
	return err
	}
	return txn.Del(ctx, key)
	}
	}

because that's what we want to invoke for the probe.

So in this PR, I would expect:

• get list of descs (just like ranges_no_leases)
• for each desc, invoke kvProbe{}.Write (or read depending on options), with much of the garnish that you have here already, and produce a row

[Santamaura]

The comments did help thanks 👍

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

---

pkg/sql/sem/builtins/generator_builtins.go, line 478 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

shouldn't this be something like types.IntervalDurationType or types.Interval?

Yeah makes more sense to be interval, changed.

---

pkg/sql/sem/builtins/generator_builtins.go, line 1749 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

?

A comment from when I was figuring out how builtin generators worked, removed.

---

pkg/sql/sem/builtins/generator_builtins.go, line 2436 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is this really the right code?

The error code is irrelevant now, removed.

---

pkg/sql/sem/builtins/generator_builtins.go, line 2465 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This doesn't seem right. The range count changes out from under you through splits and merges, so any code that stores it like this is likely fishy.

Right that makes sense.

---

pkg/sql/sem/builtins/generator_builtins.go, line 2481 at r1 (raw file):

I think the root problem is that this code wants to use the planner "as is". Instead, we should refactor the planner to make the interesting parts of it more reusable, and then reuse only those parts. We need only two things here: iterate through all range descriptors (i.e. discover all of the ranges in the system, with a consistent view) and then probe each of them.

Yeah I was iffy about using the current planner but wasn't sure if we should get the ranges how crdb_internal.ranges_no_leases does it. I've refactored the generator to use this way and then also just use Read or Write now.

I'm also noticing that the internal_no_leases generator is much more compact and doesn't seem to need to define a new type, so maybe you can adopt this pattern here and forfeit a bunch of boilerplate? I might be missing something.

When I was looking over those, there didn't seem to be a way of passing arguments into generators created in that way, if it turns out they can have arguments passed I can move it over.

[tbg]

Thanks! I pushed a commit with some fixes and a bunch of TODO(alex)es which are basically my review comments. Please integrate the code changes into your commit (if they make sense to you), no need to preserve my authorship.

[tbg]

Reviewed 5 of 5 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @joshimhoff, @koorosh, @tbg, and @zachlite)

[tbg]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @joshimhoff, @koorosh, and @zachlite)

[Santamaura]

I think I covered everything from the TODOs, let me know if there's any outstanding

[tbg]

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @joshimhoff, @koorosh, @Santamaura, and @zachlite)

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 74 at r5 (raw file):

				A write probe will effectively probe reads as well.
			`,
			// TODO(alex): add unit tests of this via a LogicTest. Make sure that the test doesn't verify anything that changes

Here's a TODO still. You have this test, though the "trace matches the string ..." part doesn't seem to be in it.

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 91 at r5 (raw file):

		EnumData: &types.EnumMetadata{
			LogicalRepresentations: enumMembers,
			PhysicalRepresentations: [][]byte{

I have no idea what's going on here, you may want to get a SQL person's review as well at least for these generator parts.

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 194 at r5 (raw file):

	}()

	ops := &kvprober.ProberOps{}

nit: you can make this a field on probeRangeGenerator. ProberOps is a struct{} so this line technically doesn't allocate but it saves future readers a double take.

---

pkg/sql/logictest/testdata/logic_test/generator_probe_ranges, line 17 at r5 (raw file):

query T
SELECT substring('$trace_str', 33, 34)

What's this doing? We want to check for evidence of a trace event that comes out of KV which I am sure this isn't doing.

[Santamaura]

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

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 74 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Here's a TODO still. You have this test, though the "trace matches the string ..." part doesn't seem to be in it.

😄 I didn't realize we wanted to actually match 'proposing command'. Fixed.

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 91 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I have no idea what's going on here, you may want to get a SQL person's review as well at least for these generator parts.

Good idea, if nobody reviews in some time I'll reach out to someone on SQL.

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 194 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: you can make this a field on probeRangeGenerator. ProberOps is a struct{} so this line technically doesn't allocate but it saves future readers a double take.

Done.

---

pkg/sql/logictest/testdata/logic_test/generator_probe_ranges, line 17 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

What's this doing? We want to check for evidence of a trace event that comes out of KV which I am sure this isn't doing.

Changed so that we confirm the verbose trace includes 'proposing command'

[tbg]

thank you! Still think this is worth a glance from SQL but this may even happen after the merge IMO.

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @joshimhoff, @koorosh, @Santamaura, and @zachlite)

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 74 at r5 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

😄 I didn't realize we wanted to actually match 'proposing command'. Fixed.

Wanna remove the TODO?

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 125 at r6 (raw file):

	// The below are updated during calls to Next() throughout the lifecycle of
	// probeRangeGenerator.
	ops    *kvprober.ProberOps

I was thinking ops kvprober.ProberOps and then in makeProbeRangeGenerator you can omit the field since the zero value is already good to go.

---

pkg/sql/logictest/testdata/logic_test/generator_probe_ranges, line 14 at r6 (raw file):

# test that the trace has a string matching `proposing command` to verify trace events
# from the kvserver write path are received

Nit:

# Test that .... are received.

[Santamaura]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @joshimhoff, @koorosh, @tbg, and @zachlite)

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 74 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Wanna remove the TODO?

Oops forgot. Removed.

---

pkg/sql/sem/builtins/generator_probe_ranges.go, line 125 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I was thinking ops kvprober.ProberOps and then in makeProbeRangeGenerator you can omit the field since the zero value is already good to go.

👍 done.

---

pkg/sql/logictest/testdata/logic_test/generator_probe_ranges, line 14 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Nit:

# Test that .... are received.

Done.

[Santamaura]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[joshimhoff]

I wonder if this tool would be available during a full cluster SQL outage such as the recent one involving a down system.lease? Maybe I'll run a test today!

[joshimhoff]

It worked fine:

root@:26257/?> select range_id, error, end_to_end_latency_ms from crdb_internal.probe_ranges(INTERVAL '1s', 'write');
  range_id | error | end_to_end_latency_ms
-----------+-------+------------------------
         1 |       |                    21
         2 |       |                    37
         3 |       |                    36
         4 |       |                    36
         5 |       |                    37
         6 |       |                    35
         7 | boom  |         9223372036854
         8 |       |                    34
         9 |       |                    36
        10 |       |                    37
        11 |       |                    35
        12 |       |                    36
        13 |       |                    35
        14 |       |                    36
        15 |       |                    18
        16 |       |                    36
        17 |       |                    37
        18 |       |                    36
        19 |       |                    35
        20 |       |                    37
        21 |       |                    35
        22 |       |                    35
        23 |       |                    37
        24 |       |                    35
        25 |       |                    36
        26 |       |                    36
        27 |       |                    36
        28 |       |                    38
        29 |       |                    35
        30 |       |                    35
        31 |       |                    36
        32 |       |                    35
        33 |       |                    35
        34 |       |                    36
        35 |       |                    35
        36 |       |                    35
        37 |       |                    37
        38 |       |                    35
        39 |       |                    35
        40 |       |                    36
        41 |       |                    35
        42 |       |                    36
        43 |       |                    35
        54 |       |                    36
        64 |       |                    36
(45 rows)

Here is the change I made locally to test:

~/g/s/g/c/cockroach [master] $ git diff
diff --git a/pkg/kv/kvserver/replica_send.go b/pkg/kv/kvserver/replica_send.go
index 3295886e91..78ccc46b7f 100644
--- a/pkg/kv/kvserver/replica_send.go
+++ b/pkg/kv/kvserver/replica_send.go
@@ -126,6 +126,10 @@ func (r *Replica) sendWithoutRangeID(
 ) (_ *roachpb.BatchResponse, rErr *roachpb.Error) {
        var br *roachpb.BatchResponse
 
+       if ba.RangeID == 7 {
+               return nil, roachpb.NewError(errors.New("boom"))
+       }
+
        if r.leaseholderStats != nil && ba.Header.GatewayNodeID != 0 {
                r.leaseholderStats.RecordCount(r.getBatchRequestQPS(ctx, ba), ba.Header.GatewayNodeID)
        }

I'm going to keep exploring this Q, but I'll hold off on posting anything, unless I learn something interesting.

[joshimhoff]

Maybe I'll write an integration test that makes all system ranges that we don't expect to be hard deps of crdb_internal.probe_range unavailable and sees what happens. Not sure if we should merge it, but it'll be fun at least.

[joshimhoff]

With all ranges with range IDs >= 2 unavailable, connecting to an insecure five node cluster + querying crdb_internal.probe_range is possible.

root@:26257/defaultdb> select range_id, error from crdb_internal.probe_ranges(INTERVAL '1s', 'write');
  range_id | error
-----------+--------
         1 |
         2 | boom
         3 | boom
         4 | boom
         5 | boom
         6 | boom
         7 | boom
         8 | boom
         9 | boom
        10 | boom
        11 | boom
        12 | boom
        13 | boom
        14 | boom
        15 | boom
        16 | boom
        17 | boom
        18 | boom
        19 | boom
        20 | boom
        21 | boom
        22 | boom
        23 | boom
        24 | boom
        25 | boom
        26 | boom
        27 | boom
        28 | boom
        29 | boom
        30 | boom
        31 | boom
        32 | boom
        33 | boom
        34 | boom
        35 | boom
        36 | boom
        37 | boom
        38 | boom
        39 | boom
        40 | boom
        41 | boom
        42 | boom
        43 | boom
        44 | boom
(44 rows)


Time: 68ms total (execution 68ms / network 0ms)

OTOH, if range with range ID = 1 is down also, we get an error. This makes sense, since probing requires an available meta2 range.

root@:26257/defaultdb> select range_id, error from crdb_internal.probe_ranges(INTERVAL '1s', 'write');
ERROR: boom

This is really awesome!

Some Qs:

1. Have we considered backporting this? I feel pretty confident that having this in CC will both improve time to fix and reduce bogus KV escalations.
2. Does anyone here have desired for me to write an integration test to make sure we maintain the nice availability property of crdb_internal.probe_range? The property is: crdb_internal.probe_range should be available unless meta2 is down.
3. If all ranges but meta2 are down, will connecting to a secure cluster work, assuming I use the root user plus a client credential? I hope so.

[joshimhoff]

Tiny follow up: #81107

[joshimhoff]

If all ranges but meta2 are down, will connecting to a secure cluster work, assuming I use the root user plus a client credential? I hope so.

I tested this. I could connect as root with root client credentials. Even with all ranges down including meta2. Yay! And this comment confirms that this behavior is a design goal:

// - if the user is root, the user is reported to exist immediately
// without querying system.users at all. The password retrieval
// is delayed until actually needed by the authentication method.
// This way, if the client presents a valid TLS certificate
// the password is not even needed at all. This is useful for e.g.
// cockroach node status.

[jreut]

@Santamaura Is it possible to backport this function to v22.1? We would love to use it sooner than later!

[Santamaura]

@Santamaura Is it possible to backport this function to v22.1? We would love to use it sooner than later!

Sure, backport PR has been opened.

[jreut]

@Santamaura Is it possible to backport this function to v22.1? We would love to use it sooner than later!

Sure, backport PR has been opened.

Out of curiosity, could I have done this myself? Are there docs I should have read first?

[Santamaura]

I made the backport via our backport tool. Doc on backports is here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/900005932/Backporting+a+change+to+a+release+branch

So for this particular PR possibly (since there were no complicated merge conflicts) for large PRs it's usually better for the author to do it since they have better context.

[joshimhoff]

Assuming not difficult, can we also backport to 21.2? Thanks for this great tool.

[Santamaura]

Sorry @joshimhoff was working on something and completely forgot about the 21.2 backport request, I've opened one now.

[joshimhoff]

Thanks! Appreciate it.