kvserver: improve comments for PriorityInversionRequeue

This commit improves the comments for PriorityInversionRequeue and clarifies the
contracts around action.Priority().

Author: wenyihu6

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

@@ -252,6 +252,20 @@ func (a AllocatorAction) SafeValue() {}
 // range. Within a given range, the ordering of the various checks inside
 // `Allocator.computeAction` determines which repair/rebalancing actions are
 // taken before the others.
+//
+// NB: Priorities should be non-negative and should be spaced in multiples of
+// 100 unless you believe they should belong to the same priority category.
+// AllocatorNoop should have the lowest priority. CheckPriorityInversion depends
+// on this contract. In most cases, the allocator returns a priority that
+// matches the definitions below. For AllocatorAddVoter,
+// AllocatorRemoveDeadVoter, and AllocatorRemoveVoter, the priority may be
+// adjusted (see ComputeAction for details), but the adjustment is expected to
+// be small (<49).
+//
+// Exceptions: AllocatorFinalizeAtomicReplicationChange, AllocatorRemoveLearner,
+// and AllocatorReplaceDeadVoter violates the spacing of 100. These cases
+// predate this comment, so we allow them as they belong to the same general
+// priority category.
 func (a AllocatorAction) Priority() float64 {
 	switch a {
 	case AllocatorFinalizeAtomicReplicationChange:
@@ -975,6 +989,49 @@ func (a *Allocator) ComputeAction(
 	return action, priority
 }
 
+// computeAction determines the action to take on a range along with its
+// priority.
+//
+// NB: The returned priority may include a small adjustment and therefore might
+// not exactly match action.Priority(). See AllocatorAddVoter,
+// AllocatorRemoveDeadVoter, AllocatorRemoveVoter below. The adjustment should
+// be <49 with two assumptions below. New uses on this contract should be
+// avoided since the assumptions are not strong guarantees (especially the
+// second one).
+//
+// The claim that the adjustment is < 49 has two assumptions:
+// 1. min(num_replicas,total_nodes) in zone configuration is < 98.
+// 2. when ranges are not under-replicated, the difference between
+// min(num_replicas,total_nodes)/2-1 and existing_replicas is < 49.
+//
+// neededVoters <= min(num_replicas,total_nodes)
+// desiredQuorum = neededVoters/2-1
+// quorum = haveVoters/2-1
+//
+// For AllocatorAddVoter, we know haveVoters < neededVoters
+// adjustment = desiredQuorum-haveVoters = neededVoters/2-1-haveVoters
+// To find the worst case (largest adjustment),
+//  1. haveVoters = neededVoters-1,
+//     adjustment = neededVoters/2-1-(neededVoters-1)
+//     = neededVoters/2-neededVoters = -neededVoters/2
+//  2. haveVoters = 0
+//     adjustement = neededVoters/2-1
+//
+// In order for adjustment to be <49, neededVoters/2<49 => neededVoters<98.
+// Hence the first assumption.
+//
+// For AllocatorRemoveDeadVoter, we know haveVoters >= neededVoters
+// adjustment = desiredQuorum-haveVoters = neededVoters/2-1-haveVoters
+// To find the worst case (largest adjustment),
+// 1. neededVoters/2-1 is much larger than haveVoters: given haveVoters >=
+// neededVoters, haveVoters/2-1 >= neededVoters/2-1. So this case is impossible.
+// 2. neededVoters/2-1 is much smaller than haveVoters: since ranges could be
+// over-replicated, theoretically speaking, there may be no upper bounds on
+// haveVoters. In order for adjustment to be < 49, we can only make an
+// assumption here that the difference between neededVoters/2-1 and haveVoters
+// cannot be >= 49 in this case.
+//
+// For AllocatorRemoveVoter, adjustment is haveVoters%2 = 0 or 1 < 49.
 func (a *Allocator) computeAction(
 	ctx context.Context,
 	storePool storepool.AllocatorStorePool,
@@ -3246,7 +3303,8 @@ func replDescsToStoreIDs(descs []roachpb.ReplicaDescriptor) []roachpb.StoreID {
 	return ret
 }
 
-// roundToNearestPriorityCategory rounds a priority to the nearest 100. n should be non-negative.
+// roundToNearestPriorityCategory rounds a priority to the nearest 100. n should
+// be non-negative.
 func roundToNearestPriorityCategory(n float64) float64 {
 	return math.Round(n/100.0) * 100
 }
@@ -3257,15 +3315,21 @@ func withinPriorityRange(priority float64) bool {
 	return AllocatorNoop.Priority() <= priority && priority <= AllocatorFinalizeAtomicReplicationChange.Priority()
 }
 
-// CheckPriorityInversion checks if the priority at enqueue time is higher than
-// the priority corresponding to the action computed at processing time. It
-// returns whether there was a priority inversion and whether the caller should
-// skip the processing of the range since the inversion is considered unfair.
-// Currently, we only consider the inversion as unfair if it has gone from a
-// repair action to lowest priority (AllocatorConsiderRebalance). We let
-// AllocatorRangeUnavailable, AllocatorNoop pass through since they are noop.
+// CheckPriorityInversion returns whether there was a priority inversion (and
+// the range should not be processed at this time, since doing so could starve
+// higher-priority items), and whether the caller should re-add the range to the
+// queue (presumably under its new priority). A priority inversion happens if
+// the priority at enqueue time is higher than the priority corresponding to the
+// action computed at processing time. Caller should re-add the range to the
+// queue if it has gone from a repair action to lowest priority
+// (AllocatorConsiderRebalance).
+//
+// Note: Changing from AllocatorRangeUnavailable/AllocatorNoop to
+// AllocatorConsiderRebalance is not treated as a priority inversion. Going from
+// a repair action to AllocatorRangeUnavailable/AllocatorNoop is considered a
+// priority inversion but shouldRequeue = false.
 //
-// NB: If shouldRequeue is true, isInversion must be true.
+// INVARIANT: shouldRequeue => isInversion
 func CheckPriorityInversion(
 	priorityAtEnqueue float64, actionAtProcessing AllocatorAction,
 ) (isInversion bool, shouldRequeue bool) {
@@ -3292,7 +3356,7 @@ func CheckPriorityInversion(
 	// action.Priority(). However, for AllocatorAddVoter,
 	// AllocatorRemoveDeadVoter, AllocatorRemoveVoter, the priority can be
 	// adjusted at enqueue time (See ComputeAction for more details). However, we
-	// expect the adjustment to be relatively small (<100). So we round the
+	// expect the adjustment to be relatively small (<49). So we round the
 	// priority to the nearest 100 to compare against
 	// actionAtProcessing.Priority(). Without this rounding, we might treat going
 	// from 10000 to 999 as an inversion, but it was just due to the adjustment.

pkg/kv/kvserver/replicate_queue.go

@@ -106,6 +106,7 @@ var EnqueueProblemRangeInReplicateQueueInterval = settings.RegisterDurationSetti
 // PriorityInversionRequeue is a setting that controls whether to requeue
 // replicas when their priority at enqueue time and processing time is inverted
 // too much (e.g. dropping from a repair action to AllocatorConsiderRebalance).
+// TODO(wenyihu6): flip default to true after landing 152596 to bake
 var PriorityInversionRequeue = settings.RegisterBoolSetting(
 	settings.SystemOnly,
 	"kv.priority_inversion_requeue_replicate_queue.enabled",
@@ -934,9 +935,9 @@ func (rq *replicateQueue) processOneChange(
 		}
 
 		if shouldRequeue && PriorityInversionRequeue.Get(&rq.store.cfg.Settings.SV) {
-			// Return true here to requeue the range. We can't return an error here
-			// because rq.process only requeue when error is nil. See
-			// replicateQueue.process for more details.
+			// Return true to requeue the range. Return the error to ensure it is
+			// logged and tracked in replicate queue bq.failures metrics. See
+			// replicateQueue.process for details.
 			return true /*requeue*/, maybeAnnotateDecommissionErr(
 				errors.Errorf("requing due to priority inversion: action=%s, priority=%v, enqueuePriority=%v",
 					change.Action, change.Action.Priority(), priorityAtEnqueue), change.Action)