kvcoord: secondary tenants do not take network latency into account when routing batch requests

[arulajmani]

Describe the problem

The DistSender sends batch requests that interact with a range to its replicas. In general, it does so by ordering replicas based on the latency between the requesting node and the node on which the replica lives. If the request must be sent to the leaseholder (and the leaseholder is known) the leaseholder is moved to the front of the queue. See:

cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go

Lines 1951 to 1975 in 4d1f40b

	switch ba.RoutingPolicy {
	case roachpb.RoutingPolicy_LEASEHOLDER:
	// First order by latency, then move the leaseholder to the front of the
	// list, if it is known.
	if !ds.dontReorderReplicas {
	replicas.OptimizeReplicaOrder(ds.getNodeDescriptor(), ds.latencyFunc)
	}
	idx := -1
	if leaseholder != nil {
	idx = replicas.Find(leaseholder.ReplicaID)
	}
	if idx != -1 {
	replicas.MoveToFront(idx)
	leaseholderFirst = true
	} else {
	// The leaseholder node's info must have been missing from gossip when we
	// created replicas.
	log.VEvent(ctx, 2, "routing to nearest replica; leaseholder not known")
	}
	case roachpb.RoutingPolicy_NEAREST:
	// Order by latency.
	log.VEvent(ctx, 2, "routing to nearest replica; leaseholder not required")
	replicas.OptimizeReplicaOrder(ds.getNodeDescriptor(), ds.latencyFunc)

For follower read requests, this has the effect that they are always routed to the nearest replica. Unfortunately, this doesn't work quite as intended for secondary tenants. Instead, because a secondary tenant's DistSender is unaware of which node it is running on, it ends up randomly ordering the replica slice because of:

cockroach/pkg/kv/kvclient/kvcoord/replica_slice.go

Lines 205 to 208 in 4d1f40b

	if nodeDesc == nil {
	shuffle.Shuffle(rs)
	return
	}

To see why a secondary tenant's dist sender is unaware of which node it is running on, it's because this cast fails:

cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go

Lines 564 to 568 in cc155a1

	// TODO(nvanbenschoten): open an issue about the effect of this.
	g, ok := ds.nodeDescs.(*gossip.Gossip)
	if !ok {
	return nil
	}

@nvanbenschoten was this the hazard you had in mind in the TODO above, or is there more here?

To Reproduce

I discovered this when trying to convert our follower_reads roachtest to run as secondary tenants. TODO(arul): link a WIP PR.

Expected behavior

Secondary tenants should take network latency into account when routing requests. Consequently, follower reads issued by secondary tenants should be served from the nearest replica (instead of a random one).

Additional context

We'd want to solve this to harness many of the benefits listed in #72593 for secondary tenants.

cc @cockroachdb/kv

Jira issue: CRDB-15402

Epic: CRDB-14202

[knz]

A request when investigating this: be careful to highlight the problem exists with "secondary tenant running as separate SQL-only servers".

For dedicated/SH we will have secondary tenants running in-process with KV, with a 1:1 relationship with the KV node. In that case, we will be able to use the base algorithm.

[nvanbenschoten]

For dedicated/SH we will have secondary tenants running in-process with KV, with a 1:1 relationship with the KV node. In that case, we will be able to use the base algorithm.

By this do you mean that these secondary tenants will be aware of a local NodeDescriptor?

[knz]

By this do you mean that these secondary tenants will be aware of a local NodeDescriptor?

We don't need the entire node descriptor though? just the ID and locality attributes? We're already planning to include those in the sql_livness table.

[andy-kimball]

We'll definitely need this fixed before we can make MR Serverless a reality. It's a "ship-stopper" issue.

[nvanbenschoten]

This looks straightforward. The fix will involve a bit of plumbing and a generalization of ReplicaSlice.OptimizeReplicaOrder. My suggestion is to change the signature of that method from:

func (rs ReplicaSlice) OptimizeReplicaOrder(nodeDesc *roachpb.NodeDescriptor, latencyFn LatencyFunc)

to

// nodeID can be 0, in which case it is ignored
// latencyFn can be nil, in which case it will not be used
func (rs ReplicaSlice) OptimizeReplicaOrder(nodeID roachpb.NodeID, latencyFn LatencyFunc, locality roachpb.Locality)

Once we have that,

1. use Gossip if available to grab the client's NodeID (DistSender.getNodeID)
2. get rid of DistSender.getNodeDescriptor
3. finally, plumb in the local process's Locality into the DistSender (cfg.Locality in server.NewServer and server.makeTenantSQLServerArgs)