kv: make TestFollowerReadsWithStaleDescriptor work with multi-tenancy

This previously didn't work for a slew of reasons, commented in-line.

References #142800
Release note: None

Author: arulajmani

pkg/ccl/kvccl/kvfollowerreadsccl/BUILD.bazel

@@ -60,6 +60,7 @@ go_test(
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/kvserverbase",
         "//pkg/kv/kvtestutils",
+        "//pkg/multitenant/tenantcapabilitiespb",
         "//pkg/roachpb",
         "//pkg/rpc",
         "//pkg/rpc/rpcbase",

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvtestutils"
+	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilitiespb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/rpc/rpcbase"
@@ -804,9 +805,8 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 		base.TestClusterArgs{
 			ReplicationMode: base.ReplicationManual,
 			ServerArgs: base.TestServerArgs{
-				Settings:          settings,
-				DefaultTestTenant: base.TODOTestTenantDisabled,
-				UseDatabase:       "t",
+				Settings:    settings,
+				UseDatabase: "t",
 			},
 			// n4 pretends to have low latency to n2 and n3, so that it tries to use
 			// them for follower reads.
@@ -847,15 +847,24 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 	// load based rebalancing to make sure it doesn't move.
 	kvserverbase.LoadBasedRebalancingMode.Override(ctx, &settings.SV, kvserverbase.LBRebalancingOff)
 
+	// NB: Tenants need capabilities to be able to relocate ranges.
+	if !tc.Server(0).DeploymentMode().IsSingleTenant() {
+		require.NoError(t, tc.Server(0).GrantTenantCapabilities(
+			context.Background(), serverutils.TestTenantID(),
+			map[tenantcapabilitiespb.ID]string{
+				tenantcapabilitiespb.CanAdminRelocateRange: "true",
+			}))
+	}
+
 	n1 := sqlutils.MakeSQLRunner(tc.Conns[0])
 	n1.Exec(t, `CREATE DATABASE t`)
 	n1.Exec(t, `CREATE TABLE test (k INT PRIMARY KEY)`)
 	n1.Exec(t, `ALTER TABLE test EXPERIMENTAL_RELOCATE VOTERS VALUES (ARRAY[1,2], 1)`)
 	// Speed up closing of timestamps, in order to sleep less below before we can
 	// use follower_read_timestamp(). follower_read_timestamp() uses the sum of
 	// the following settings.
-	n1.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.target_duration = '0.1s'`)
-	n1.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.side_transport_interval = '0.1s'`)
+	closedts.TargetDuration.Override(ctx, &settings.SV, 100*time.Millisecond)
+	closedts.SideTransportCloseInterval.Override(ctx, &settings.SV, 100*time.Millisecond)
 
 	// Sleep so that we can perform follower reads. The read timestamp needs to be
 	// above the timestamp when the table was created.
@@ -865,11 +874,16 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 
 	// Run a query on n4 to populate its cache.
 	n4 := sqlutils.MakeSQLRunner(tc.Conns[3])
-	n4.Exec(t, "SELECT * from test WHERE k=1")
 	// Check that the cache was indeed populated.
 	var tableID uint32
 	n1.QueryRow(t, `SELECT id from system.namespace WHERE name='test'`).Scan(&tableID)
-	tablePrefix := keys.MustAddr(keys.SystemSQLCodec.TablePrefix(tableID))
+	tablePrefix := keys.MustAddr(tc.Server(0).Codec().TablePrefix(tableID))
+	// NB: Splitting a range helps prevent tenants from getting an out of band
+	// RangeDescriptor update to the RangeCache. Unclear about the exact mechanics
+	// at play here, but they don't matter for our test.
+	_, _, err := tc.SplitRange(roachpb.Key(tablePrefix))
+	require.NoError(t, err)
+	n4.Exec(t, "SELECT * from test WHERE k=1")
 	n4Cache := tc.Server(3).DistSenderI().(*kvcoord.DistSender).RangeDescriptorCache()
 	entry, err := n4Cache.TestingGetCached(ctx, tablePrefix, false, roachpb.LAG_BY_CLUSTER_SETTING)
 	require.NoError(t, err)
@@ -964,8 +978,18 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 
 	// Sanity check that the plan was distributed.
 	require.True(t, strings.Contains(rec.String(), "creating DistSQL plan with isLocal=false"))
-	// Look at the trace and check that we've served a follower read.
-	require.True(t, kvtestutils.OnlyFollowerReads(rec), "query was not served through follower reads: %s", rec)
+	// NB: We're distributing the plan here, and it (the DistSender) is running on
+	// n3. Note that we've only injected latencies on n4, so any replica is fair
+	// game for n3. When run in normal or shared-process multi-tenancy
+	// deployments, n3 will route to its local replica (which is a follower), as
+	// that guy always sorts first. However, for external process multi-tenancy,
+	// there's no such concept of a local replica. Requests can therefore be
+	// routed to either n1 or n3, so we can't make any assertions about whether
+	// there'll be a follower read or not.
+	if !tc.Server(0).DeploymentMode().IsExternal() {
+		// Look at the trace and check that we've served a follower read.
+		require.True(t, kvtestutils.OnlyFollowerReads(rec), "query was not served through follower reads: %s", rec)
+	}
 	// Verify that we didn't produce the "misplanned ranges" metadata that would
 	// purge the non-stale entries from the range cache on n4.
 	require.False(t, strings.Contains(rec.String(), "clearing entries overlapping"))

pkg/testutils/serverutils/api.go

@@ -148,6 +148,11 @@ func (d DeploymentMode) IsExternal() bool {
 	return d == ExternalProcess
 }
 
+// IsSingleTenant returns whether the deployment mode is single tenant or not.
+func (d DeploymentMode) IsSingleTenant() bool {
+	return d == SingleTenant
+}
+
 // RPCConn defines a common interface for creating RPC clients. It hides the
 // underlying RPC connection (gRPC or DRPC), making it easy to swap
 // them without changing the caller code.