Have you considered the alternative way of implementing this that does not rely on the system.tenants table to be available on all nodes? I think you would only have had to modify shouldSplitRange with code that looks at the start and end key, and if the range is nonempty (as mvccstats can tell cheaply) we split at the first tenant boundary for which we see a key in the range. This would also handle the merge queue (which has a similar check), though we would want to avoid having to read data in the case in which the merge won't be allowed. That unfortunately seems hard, since the decision is made on the left but we would want to not merge if the RHS is empty. Hmm. Maybe we would merge only if the LHS is empty? Then we would end up with permanent "buffer ranges" between tenants, so at most double the number of empty ranges as tenants in the cluster. Actually the doubling goes away if we do allow a merge of an empty LHS with a nonempty RHS (in which case that tenant's range will "reach out to the empty left", which may be fine).

Anyway, just rambling. I think it's worthwhile to document the alternative and the decision in the commit message and perhaps even in the system config code.

Reviewed 23 of 23 files at r1, 8 of 8 files at r2, 4 of 4 files at r3, 4 of 4 files at r4, 2 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @pbardea)

pkg/config/system.go, line 209 at r3 (raw file):
While you're here mind adding a comment to {low,high}Index?

lowIndex (in s.Values) is the first and highIndex one past the last KV pair in the descriptor table.

pkg/config/system.go, line 608 at r3 (raw file):

			return nil
		}
		// MakeTenantPrefix(lowTenIDExcl) is either the start key of this key

This comment would be helpful where lowTenIDExcl is defined. I stared at that a bit to understand the Excl and it also helps understand the special casing of MaxTenantID.

pkg/config/system.go, line 624 at r3 (raw file):

		}
		if keys.MakeTenantPrefix(highTenIDExcl).Equal(searchSpan.EndKey) {
			// MakeTenantPrefix(highTenIDExcl) is the end key of this key range.

Comment on why there isn't an exception for MinTenantID - are we splitting at /Tenant/1? That should not be necessary, right? /Tenant/1/* is empty due to the system tenant's encoding, so /Tenant/2 should be the first reasonable split point. I'm fine still splitting at /Tenant/1 to make sure that tenantID 2 looks "exactly" like any other tenant, though, which is perhaps what you were thinking in the first place.

pkg/config/system.go, line 643 at r3 (raw file):

	lowBound := keys.SystemSQLCodec.TenantMetadataKey(lowTenID)
	lowIndex := s.getIndexBound(lowBound)
	// NOTE: we Next() the tenant key because highTenID is inclusive but the

Can't you do this in a simpler way? You just care about lowIndex and whether it points at a key with a tenant ID smaller than highTenID:

lowBound := keys.SystemSQLCodec.TenantMetadataKey(lowTenID)
lowIndex := s.getIndexBound(lowBound)
if lowIndex == len(s.Values) {
   // No keys within range found.
   return nil
}
// Choose the first key in this range. Extract its tenant ID.
splitKey := s.Values[lowIndex].Key
splitTenID, err := keys.SystemSQLCodec.DecodeTenantMetadataID(splitKey)
if err != nil {
  log.Errorf(ctx, "unable to decode tenant ID from system config: %s", err)
  return nil
}
if splitTenID > highBound {
  // No keys within range found.
  return nil
}
return roachpb.RKey(keys.MakeTenantPrefix(splitTenID))

I'm fine keeping as-is as well, I just thought you'd maybe want to sidestep the Uint64 complications.

TFTR!

Have you considered the alternative way of implementing this that does not rely on the system.tenants table to be available on all nodes?

Briefly, but only before I refreshed myself on how we enforce splits between table boundaries. After realizing that these table splits points are a pure function of the SystemConfig alone, I decided that it would be a mistake to do things differently and add brand new complexity for tenant keyspace boundaries. The two concepts are too closely analogous, especially given how we store tenant metadata, for the alternative to seem like the right approach. And if it was, then shouldn't we be doing the same thing with table boundaries?

That admittedly would not be a particularly strong argument if we were building this from scratch, but given that this needs to play nicely with the existing structure and there aren't any blatant downsides to the approach here, I think the consistency argument wins out for me.

Past all that, I also think there's some complication with such a proposal when performing the first tenant split. A descriptor will look like {/Table/50, /Max}. How do we decide when to add the first tenant split? Is the last range in the system scanning over all of its keys whenever it calls shouldSplitRange?

I think it's worthwhile to document the alternative and the decision in the commit message

Agreed. Added to the commit message.

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

pkg/config/system.go, line 209 at r3 (raw file):

Done.

pkg/config/system.go, line 608 at r3 (raw file):

Done.

pkg/config/system.go, line 624 at r3 (raw file):

Done. It's actually a bit subtle why we don't need to special-case DecodeTenantPrefix(searchSpan.EndKey) == MinTenantID, so it's nice to point that out explicitly.

I think you're aware, but just so we're on the same page though, MinTenantID is 2. So we

pkg/config/system.go, line 643 at r3 (raw file):

I like that much better! It also avoids a key encoding. Done.