Merge #158006

158006: mmaprototype: panic for no leaseholders r=wenyihu6 a=wenyihu6

Epic: CRDB-55052 
Release note: none

---
**mmaprototype: panic for no leaseholders**

This change commit ensureAnalyzedConstraints to panic when no leaseholder is
found instead of returning a bool that errors. This state of no leaseholder
should not occur — range messages are expected to include a leaseholder, and we
validate that pending changes to the range state should not result in a range
with no leaseholders.



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

Author: craig[bot]

pkg/kv/kvserver/allocator/mmaprototype/allocator_state.go

@@ -703,9 +703,9 @@ func sortTargetCandidateSetAndPick(
 	return cands.candidates[j].StoreID
 }
 
-func (cs *clusterState) ensureAnalyzedConstraints(rstate *rangeState) bool {
+func (cs *clusterState) ensureAnalyzedConstraints(rstate *rangeState) {
 	if rstate.constraints != nil {
-		return true
+		return
 	}
 	// Populate the constraints.
 	rac := newRangeAnalyzedConstraints()
@@ -721,13 +721,16 @@ func (cs *clusterState) ensureAnalyzedConstraints(rstate *rangeState) bool {
 	if leaseholder < 0 {
 		// Very dubious why the leaseholder (which must be a local store since there
 		// are no pending changes) is not known.
-		// TODO(sumeer): log an error.
 		releaseRangeAnalyzedConstraints(rac)
-		return false
+		// Possible that we are observing stale state where we've transferred the
+		// lease away but have not yet received a StoreLeaseholderMsg indicating
+		// that there is a new leaseholder (and thus should drop this range).
+		// However, even in this case, replica.IsLeaseholder should still be there
+		// based on to the stale state, so this should still be impossible to hit.
+		panic(errors.AssertionFailedf("no leaseholders found in %v", rstate.replicas))
 	}
 	rac.finishInit(rstate.conf, cs.constraintMatcher, leaseholder)
 	rstate.constraints = rac
-	return true
 }
 
 // Consider the core logic for a change, rebalancing or recovery.

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_rebalance_stores.go

@@ -285,10 +285,7 @@ func (re *rebalanceEnv) rebalanceReplicas(
 			log.KvDistribution.VInfof(ctx, 2, "skipping r%d: too soon after failed change", rangeID)
 			continue
 		}
-		if !re.ensureAnalyzedConstraints(rstate) {
-			log.KvDistribution.VInfof(ctx, 2, "skipping r%d: constraints analysis failed", rangeID)
-			continue
-		}
+		re.ensureAnalyzedConstraints(rstate)
 		isVoter, isNonVoter := rstate.constraints.replicaRole(store.StoreID)
 		if !isVoter && !isNonVoter {
 			// Due to REQUIREMENT(change-computation), the top-k is up to date, so
@@ -486,10 +483,7 @@ func (re *rebalanceEnv) rebalanceLeases(
 			log.KvDistribution.VInfof(ctx, 2, "skipping r%d: too soon after failed change", rangeID)
 			continue
 		}
-		if !re.ensureAnalyzedConstraints(rstate) {
-			log.KvDistribution.VInfof(ctx, 2, "skipping r%d: constraints analysis failed", rangeID)
-			continue
-		}
+		re.ensureAnalyzedConstraints(rstate)
 		if rstate.constraints.leaseholderID != store.StoreID {
 			// We should not panic here since the leaseQueue may have shed the
 			// lease and informed MMA, since the last time MMA computed the