Exciting to see this come together! Haven't had a very thorough look, but publishing comments so far. A lot of these are questions for me that we can discuss when we meet later today.

Reviewed 8 of 8 files at r1, 7 of 26 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @benbardin)

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 69 at r2 (raw file):

  // GcTtlSeconds bounds the configuration of gc.ttl_seconds.
  Int32Range gc_ttl_seconds = 1;

nit: Should we call this GCTTLSeconds to (pardon the pun) conform with the {Span,Zone}Config messages?

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 102 at r2 (raw file):

  //
  // The fallback constraints should conform to the allowed constraints; if
  // they do not, then a clamped configuration will not conform. This is the

nit: I don't follow what the last sentence is saying here.

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 111 at r2 (raw file):

  message ConstraintBounds {
    option (gogoproto.equal) = true;
    Int32Range num_replicas = 1;

What's the motivation behind putting num_replicas' and num_votersin this message? Instead of nesting it here, could we put it onSpanConfigBoundslikeRangeMinBytes` etc.?

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 126 at r2 (raw file):

    // clamping a configuration which is out of conformance.
    //
    // If the existing constraint had just one entry with no numeric bound,

(For me) let's talk about this part when we meet.

pkg/spanconfig/spanconfigbounds/bounds.go line 27 at r2 (raw file):

	redact.SafeFormatter

	// conforms checks whether the value conforms. It does not modify the value.

You have the "conforms" and "clamp" comments switched around.

pkg/spanconfig/spanconfigbounds/fields.go line 43 at r2 (raw file):

// Note that the order of fields in this slice matters. When clamping, fields
// will be clamp4ed in order. This matters for fields like constraints and

nit: "clamp4ed"

RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @benbardin)

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 69 at r2 (raw file):

Done.

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 102 at r2 (raw file):

I lightly touched it up.

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 111 at r2 (raw file):

Done.

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 126 at r2 (raw file):

Done.

pkg/spanconfig/spanconfigbounds/bounds.go line 27 at r2 (raw file):

Done.

pkg/spanconfig/spanconfigbounds/fields.go line 43 at r2 (raw file):

Done.

Reviewed 30 of 30 files at r3, 32 of 32 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @benbardin)

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 116 at r4 (raw file):

  // ConstraintBounds represents bounds on voter_constraints, constraints,
  // lease_preferences, num_voters, and num_replicas.

nit: just voter_constraints, constraints, and lease_preferences now?

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 118 at r4 (raw file):

  // lease_preferences, num_voters, and num_replicas.
  //
  // The behavior of ConstraintBounds in the case of non-conformance may be

Is this the right place to talk about the behavior of ConstraintBounds as it applies to the three fields? If not, we should direct the reader to inline comments in the clamp function.

Alternatively, do you think it's worth adding a high level overview about "how clamping works" for all fields in the SpanConfigBounds proto in a doc.go as a single point of reference for future readers instead?

pkg/multitenant/tenantcapabilities/tenantcapabilitiespb/capabilities.proto line 151 at r4 (raw file):

    // distribution of replicas, to the extent possible, but as applied to
    // the fallback constraints.
    repeated cockroach.roachpb.Constraint allowed = 3 [(gogoproto.nullable) = false];

nit: here, and below, 1 and 2 instead of 3 and 4?

pkg/spanconfig/spanconfigbounds/bool_field.go line 44 at r4 (raw file):

	case globalReads:
		return &c.GlobalReads
		// TODO(ajwerner, XXX): Decide what to do about these fields.

Should we just add an assertion failed here, given we never expect this to be called for either of these two fields? We could just uncomment what you have here, but maybe adding a panic here would nudge whoever ends up here to think more about the structure here (given this is tied to SpanConfigBounds, and we don't have these fields on the proto).

pkg/spanconfig/spanconfigbounds/constraints_field.go line 174 at r4 (raw file):

	switch f {
	case voterConstraints:
		// In the common settings, voter constraints either attempt to state

Instead of being general with the "common settings" phrasing, could we phrase this (and some of the commentary below, which isn't already) explicitly in the context of MR primitives?

pkg/spanconfig/spanconfigbounds/lease_preferences_field.go line 88 at r4 (raw file):

}

func (c *leasePreferencesBound) clamp(t *roachpb.SpanConfig, f Field) (changed bool) {

The clamping behaviour here deserves a comment.

pkg/spanconfig/spanconfigbounds/testdata/regions line 102 at r4 (raw file):

# Note that this case is interesting because we won't touch the out-of-bounds
# num_replicas in the voter_constraints. It's invalid but it's okay because we

Is it even possible to end up with such a invalid zone/span config in the first place?

pkg/spanconfig/spanconfigbounds/testdata/truncated_lease_prefs line 19 at r4 (raw file):

# preferences are going to be clamped. What's weird about it is that
# we're going to end up with a conflicting lease preference which conforms
# but does not have any voters in it.

Could we end up with something unsatisfiable here? Whatever the construction, we're guaranteed to have a replica where the lease is being pinned. We're also guaranteed that voters won't be fully constrained to a different region -- so the replica in the region where the lease is pinned can be a voter without violating any constraints. Are we smart enough to recognize it needs to be a voter though?

pkg/spanconfig/spanconfigbounds/testdata/truncated_lease_prefs line 66 at r4 (raw file):

   constraints: <
     key: "region"
@@ -21,21 +21,15 @@

nit: the default value for the diff context makes this case slightly hard to read -- you could consider making it configurable and increasing it for this test case in particular. FWIW, I found the error messages above much easier to follow anyway, so not doing so isn't a big deal.

pkg/spanconfig/spanconfigbounds/testdata/truncated_lease_prefs line 97 at r4 (raw file):

 >

# Exercise the logic to clamp the lease preferences in the other direction.

nit: Not sure what "other direction" means in this context.

pkg/spanconfig/spanconfigbounds/testdata/truncated_lease_prefs line 143 at r4 (raw file):

-    key: "region"
     value: "us-central1"
   >

Could we also add a test for when none of the constraints in lease_preferences match what is allowed in the bound? For example, bound_to_us with something like:

lease_preferences: <constraints: <key: "region" value: "europe-west1">>
lease_preferences: <constraints: <key: "region" value: "europe-west2">>

pkg/sql/logictest/testdata/logic_test/zone_config_system_tenant line 185 at r4 (raw file):

90001
1
1

revert?

Approved for backupccl