Merge #111918

111918: mma: more normalization of constraints r=kvoli a=sumeerbhola

We now also do some normalization of constraints (in addition to voter constraints). This helps with a better choice of where to add a non-voter if the voter constraints are temporarily unsatisfiable. This was motivated by looking at the config in #106559 (though this is unrelated to the bug there).

Informs #103320

Epic: CRDB-25222

Release note: None

Co-authored-by: sumeerbhola <[email protected]>

Author: craig[bot]

pkg/kv/kvserver/allocator/mma/constraint.go

@@ -127,8 +127,6 @@ func (ic internedConstraint) less(b internedConstraint) bool {
 // constraints are in increasing order using internedConstraint.less.
 type constraintsConj []internedConstraint
 
-type conjunctionRelationship int
-
 func (nconf *normalizedSpanConfig) uninternedConfig() roachpb.SpanConfig {
 	var conf roachpb.SpanConfig
 	conf.NumReplicas = nconf.numReplicas
@@ -158,6 +156,8 @@ func (nconf *normalizedSpanConfig) uninternedConfig() roachpb.SpanConfig {
 	return conf
 }
 
+type conjunctionRelationship int
+
 // Relationship between conjunctions used for structural normalization. This
 // relationship is solely defined based on the conjunctions, and not based on
 // what stores actually match. It simply assumes that if two conjuncts are not
@@ -211,13 +211,13 @@ func (cc constraintsConj) relationship(b constraintsConj) conjunctionRelationshi
 	if extraInB > 0 && extraInCC == 0 {
 		return conjStrictSuperset
 	}
-	// (extraInCC == 0 || extraInBB > 0) && (extraInB == 0 || extraInCC > 0)
+	// (extraInCC == 0 || extraInB > 0) && (extraInB == 0 || extraInCC > 0)
 	// =>
-	// (extraInCC == 0 && extraInB == 0) || (extraInBB > 0 && extraInCC > 0)
+	// (extraInCC == 0 && extraInB == 0) || (extraInB > 0 && extraInCC > 0)
 	if extraInCC == 0 && extraInB == 0 {
 		return conjEqualSet
 	}
-	// (extraInBB > 0 && extraInCC > 0)
+	// (extraInB > 0 && extraInCC > 0)
 	if inBoth > 0 {
 		return conjIntersecting
 	}
@@ -576,6 +576,118 @@ func doStructuralNormalization(conf *normalizedSpanConfig) (*normalizedSpanConfi
 		}
 	}
 	conf.voterConstraints = vc
+
+	// We are done with normalizing voter constraints. We also do some basic
+	// normalization for constraints: we have seen examples where the
+	// constraints are under-specified and give freedom in the choice of
+	// constraintsForAddingNonVoter() that isn't actually there. Note that when
+	// all the voter constraints are satisfied (which we try to do before
+	// satisfying non-voter constraints), then the under-specification does not
+	// hurt the choice made in constraintsForAddingNonVoter(). However, we could
+	// have situations where an outage of some kind is preventing all the voter
+	// constraints from being satisfied, and now we need to place a non-voter --
+	// placing that non-voter correctly will avoid the need to move it later
+	// when the voter constraint does get satisfied.
+	//
+	// For example, see the config from #106559 in testdata/normalize_config.
+	// The constraints for us-west-1 and us-east-1 are under-specified in
+	// needing only 1 replica, while voter constraints specify we need 2
+	// replicas in each. Consider if it were left under-specified, and we had
+	// only 3 voters, 2 in us-west-1 and 1 in us-east-1 and we were temporarily
+	// unable to add a voter in us-east-1. Say we lose the non-voter too, and
+	// need to add one. With the under-specified constraint we could add the
+	// non-voter anywhere, since we think we are allowed 2 replicas with the
+	// empty constraint conjunction. This is technically true, but once we have
+	// the required second voter in us-east-1, we will need to move that
+	// non-voter to us-central-1, which is wasteful.
+	if emptyConstraintIndex >= 0 {
+		// Recompute the relationship since voterConstraints have changed.
+		emptyVoterConstraintIndex = -1
+		rels = rels[:0]
+		for i := range conf.voterConstraints {
+			if len(conf.voterConstraints[i].constraints) == 0 {
+				// We don't actually use emptyVoterConstraintIndex later, but it is
+				// harmless to recompute, and will avoid subtle bugs if we change the
+				// logic below to start using it.
+				emptyVoterConstraintIndex = i
+			}
+			for j := range conf.constraints {
+				rels = append(rels, relationshipVoterAndAll{
+					voterIndex: i,
+					allIndex:   j,
+					voterAndAllRel: conf.voterConstraints[i].constraints.relationship(
+						conf.constraints[j].constraints),
+				})
+			}
+		}
+		// Sort these relationships in the order we want to examine them.
+		sort.Slice(rels, func(i, j int) bool {
+			return rels[i].voterAndAllRel < rels[j].voterAndAllRel
+		})
+		// Ignore conjIntersecting.
+		index = 0
+		for rels[index].voterAndAllRel == conjIntersecting {
+			index++
+		}
+		voterConstraintHasEqualityWithConstraint := make([]bool, len(conf.voterConstraints))
+		// For conjEqualSet, if we can grab from the emptyConstraintIndex, do so.
+		for ; index < len(rels) && rels[index].voterAndAllRel <= conjEqualSet; index++ {
+			rel := rels[index]
+			voterConstraintHasEqualityWithConstraint[rel.voterIndex] = true
+			if rel.allIndex == emptyConstraintIndex {
+				// rel.voterIndex must be emptyVoterConstraintIndex.
+				continue
+			}
+			if conf.constraints[rel.allIndex].numReplicas < conf.voterConstraints[rel.voterIndex].numReplicas {
+				toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas -
+					conf.constraints[rel.allIndex].numReplicas
+				availableCount := conf.constraints[emptyConstraintIndex].numReplicas
+				if availableCount < toAddCount {
+					toAddCount = availableCount
+				}
+				conf.constraints[emptyConstraintIndex].numReplicas -= toAddCount
+				conf.constraints[rel.allIndex].numReplicas += toAddCount
+			}
+		}
+		// For conjStrictSubset, if the subset relationship is with
+		// emptyConstraintIndex, grab from there.
+		for ; index < len(rels) && rels[index].voterAndAllRel <= conjStrictSubset; index++ {
+			rel := rels[index]
+			if rel.allIndex != emptyConstraintIndex {
+				continue
+			}
+			if voterConstraintHasEqualityWithConstraint[rel.voterIndex] {
+				// Already has a corresponding constraint, that we have considered in
+				// the previous loop.
+				continue
+			}
+			availableCount := conf.constraints[emptyConstraintIndex].numReplicas
+			if availableCount > 0 {
+				toAddCount := conf.voterConstraints[rel.voterIndex].numReplicas
+				if toAddCount > availableCount {
+					toAddCount = availableCount
+				}
+				conf.constraints[emptyConstraintIndex].numReplicas -= toAddCount
+				conf.constraints = append(conf.constraints, internedConstraintsConjunction{
+					numReplicas: toAddCount,
+					constraints: conf.voterConstraints[rel.voterIndex].constraints,
+				})
+			}
+		}
+		// We may have appended to conf.constraints in the previous loop. We like
+		// to keep the empty constraint as the last one, so do a swap if needed.
+		n := len(conf.constraints) - 1
+		if n != emptyConstraintIndex {
+			conf.constraints[n], conf.constraints[emptyConstraintIndex] =
+				conf.constraints[emptyConstraintIndex], conf.constraints[n]
+		}
+		// If the empty constraint does not have any replicas due to the
+		// normalization, discard it.
+		if conf.constraints[n].numReplicas == 0 {
+			conf.constraints = conf.constraints[:n]
+		}
+	}
+
 	return conf, err
 }
 

pkg/kv/kvserver/allocator/mma/testdata/normalize_config

@@ -20,7 +20,8 @@ input:
 output:
  num-replicas=5 num-voters=3
  constraints:
-   :5
+   +region=a:3
+   :2
  voter-constraints:
    +region=a:3
 
@@ -114,7 +115,9 @@ output:
  constraints:
    +region=b:1
    +region=c:1
-   :3
+   +region=a,-zone=a1:1
+   +region=a,+zone=a1:1
+   :1
  voter-constraints:
    +region=a,+zone=a1:1
    +region=a,-zone=a1:1
@@ -144,7 +147,8 @@ output:
    +region=b:1
    +region=c:1
    +region=d:1
-   :2
+   +region=a,-zone=a1:1
+   +region=a,+zone=a1:1
  voter-constraints:
    +region=a,+zone=a1:1
    +region=a,-zone=a1:1
@@ -221,7 +225,8 @@ output:
    +region=c:1
    +region=d:1
    +region=e:1
-   :4
+   +region=f:2
+   :2
  voter-constraints:
    +region=f:2
    +region=e:1
@@ -263,7 +268,8 @@ output:
    +region=c:1
    +region=d:1
    +region=e:1
-   :4
+   +region=f:2
+   :2
  voter-constraints:
    +region=f:2
    +region=c:1
@@ -293,7 +299,9 @@ output:
  constraints:
    +region=b,+zone=b2:1
    +region=c,+zone=c2:1
-   :3
+   +region=a,-zone=a1:1
+   +region=b,+zone=b1:1
+   :1
  voter-constraints:
    +region=b,+zone=b1:1
    +region=a,-zone=a1:1
@@ -356,7 +364,7 @@ output:
    +region=b,+zone=b2:1
    +region=c,+zone=c2:1
    +region=d,+zone=d2:1
-   :1
+   +region=e:1
  voter-constraints:
    +region=e:1
    +region=c,+zone=c2:1
@@ -387,3 +395,62 @@ output:
    +region=a,+zone=a1:2
    +region=a,+zone=a2:2
    +region=a,+zone=a3:1
+
+# Config from #106559.
+normalize num-replicas=6 num-voters=5
+constraint num-replicas=1 +region=eu-west-1
+constraint num-replicas=1 +region=us-central-1
+constraint num-replicas=1 +region=us-east-1
+constraint num-replicas=1 +region=us-west-1
+voter-constraint num-replicas=2 +region=us-west-1
+voter-constraint num-replicas=2 +region=us-east-1
+----
+input:
+ num-replicas=6 num-voters=5
+ constraints:
+   +region=eu-west-1:1
+   +region=us-central-1:1
+   +region=us-east-1:1
+   +region=us-west-1:1
+ voter-constraints:
+   +region=us-west-1:2
+   +region=us-east-1:2
+output:
+ num-replicas=6 num-voters=5
+ constraints:
+   +region=eu-west-1:1
+   +region=us-central-1:1
+   +region=us-east-1:2
+   +region=us-west-1:2
+ voter-constraints:
+   +region=us-west-1:2
+   +region=us-east-1:2
+   +region=eu-west-1:1
+
+# Config from #122292. Note that the normalization over-specifies by adding a
+# voter constraint for +region=a:1. Based on the original configuration,
+# either +region=a:1 or +region=b:1 could be added, but we cannot express a
+# disjunction.
+normalize num-replicas=4 num-voters=3
+constraint num-replicas=1 +region=a
+constraint num-replicas=1 +region=b
+constraint num-replicas=1 +region=c
+voter-constraint num-replicas=2 +region=c
+----
+input:
+ num-replicas=4 num-voters=3
+ constraints:
+   +region=a:1
+   +region=b:1
+   +region=c:1
+ voter-constraints:
+   +region=c:2
+output:
+ num-replicas=4 num-voters=3
+ constraints:
+   +region=a:1
+   +region=b:1
+   +region=c:2
+ voter-constraints:
+   +region=c:2
+   +region=a:1