Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @andreimatei)

pkg/kv/kvserver/allocator.go, line 114 at r1 (raw file):

// These are the possible allocator actions.
const (
	_ AllocatorAction = iota

Do you have a sense for why we keep actions and priorities separate? It seems like a recipe for bugs. There's an N to 1 mapping between the two concepts, right? So then would a (AllocatorAction).Priority method be appropriate to avoid bugs in ComputeAction?

(no need to hold up this PR on the discussion)

pkg/kv/kvserver/allocator.go, line 572 at r1 (raw file):

	}
	liveNonVoters, deadNonVoters := a.storePool.liveAndDeadReplicas(nonVoterReplicas)
	if haveNonVoters == neededNonVoters && len(deadNonVoters) > 0 {

Let's add comments like we have above that explain in prose what each of these cases means.

pkg/kv/kvserver/replicate_queue.go, line 367 at r2 (raw file):

		return false, nil

	// Add replicas

nit: punctuation on each of these.

pkg/kv/kvserver/replicate_queue.go, line 468 at r2 (raw file):

func getRemoveIdx(repls, deadOrDecommissioningRepls []roachpb.ReplicaDescriptor) (removeIdx int) {
	removeIdx = -1
	if len(deadOrDecommissioningRepls) == 0 {

Aren't we already checking this outside of the function? If so, consider making this a precondition of the method and panicking if it is not upheld.

Or even better, just take a single deadOrDecommissioningRepl roachpb.ReplicaDescriptor arg so that callers aren't tricked into thinking that anyone other than the first deadOrDecommissioned replica is considered.

pkg/kv/kvserver/replicate_queue.go, line 496 at r2 (raw file):

	dryRun bool,
) (requeue bool, _ error) {
	desc, zone := repl.DescAndZone()

It's not entirely new logic, but the repeated calls to repl.DescAndZone() scare me. It seems error prone to call allocator.ComputeAction with one zone and descriptor and then carry out the action with a separate zone and descriptor. Does that concern you at all?

pkg/kv/kvserver/replicate_queue.go, line 928 at r2 (raw file):

	decommissioningReplica := decommissioningReplicas[0]

	if targetType == voterTarget {

We have logic like this in a few places, but I wonder whether we want it. Is there any reason to bake in this assumption that non-voting replicas will never hold the lease? That will probably never change, but is there a downside to gating calls to maybeTransferLeaseAway? Isn't it a quick no-op if the target isn't the leaseholder?

pkg/kv/kvserver/replicate_queue_test.go, line 398 at r2 (raw file):

		require.Eventually(t, func() bool {
			return check(tc, scratchRange)
		}, 15*time.Second, time.Second)

Is a 1 second check a little slow? I'd imagine we'd want to run it every 10 ms or something much faster.

pkg/kv/kvserver/replicate_queue_test.go, line 398 at r2 (raw file):

		require.Eventually(t, func() bool {
			return check(tc, scratchRange)
		}, 15*time.Second, time.Second)

s/15*time.Second/testutils.DefaultSucceedsSoonDuration/

pkg/kv/kvserver/replicate_queue_test.go, line 409 at r2 (raw file):

	}

	t.Run("replace", func(t *testing.T) {

Could you give these two subtests comments explaining what they're testing and why they're different.

I've got nothing beyond what Nathan said.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @nvanbenschoten)

pkg/kv/kvserver/allocator.go, line 114 at r1 (raw file):

+1 and maybe we can get rid of the numerical values of priorities with this occasion, assuming they're meaningless

pkg/kv/kvserver/allocator.go, line 127 at r2 (raw file):

	AllocatorReplaceDecommissioningNonVoter
	AllocatorRemoveDecommissioningVoter
	AllocatorRemoveDecommissioningNonVoter

is there a good reason why there's distinct actions RemoveDead* versus RemoveDecomissioning* ? It would seem to me that a removal is a removal. Perhaps the different priorities are the only reason?

pkg/kv/kvserver/replicate_queue.go, line 928 at r2 (raw file):

+1
The "maybe" in maybeTransferLeaseAway sounds like it could handle any pre-conditions.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

pkg/kv/kvserver/allocator.go, line 114 at r1 (raw file):
The numerical values of the priorities (or something like it) I think do need to exist because of this sort of logic: https://github.com/cockroachdb/cockroach/blob/819d931c6c768f9252cf89b8bc46db72fc1a3175/pkg/kv/kvserver/allocator.go#L483

This logic makes the replicateQueue prioritize ranges that are missing, say 3 voters, over ranges that are missing 1 voter.

then would a (AllocatorAction).Priority method be appropriate to avoid bugs in ComputeAction?

Done. LMK if you dislike what I've done.

pkg/kv/kvserver/allocator.go, line 572 at r1 (raw file):

Done.

pkg/kv/kvserver/allocator.go, line 127 at r2 (raw file):

We could've had different priorities but the same AllocatorAction, so that's not it. The differences are encapsulated in removeDead and removeDecommissioning. They're fairly minor: slightly different logging, different metric being updated, one tries to maybeTransferLeaseAway while the other can assume that the replica being removed doesn't have the lease.

pkg/kv/kvserver/replicate_queue.go, line 367 at r2 (raw file):

Fixed.

pkg/kv/kvserver/replicate_queue.go, line 468 at r2 (raw file):

Done.

pkg/kv/kvserver/replicate_queue.go, line 496 at r2 (raw file):

So the hazard here is just that we might have decided to add / remove / rebalance based on one version of the range descriptor but then execute upon that decision based on another version? This can only happen if we raced with the mergeQueue or the StoreRebalancer, which is definitely possible. However, I don't think anything bad can really happen in such a circumstance since:

1. Those callers won't add / remove replicas (except for short-lived learners, which should be copacetic), they will just rebalance a range.
2. The exact replication change that is executed is still governed by the {Allocate, Rebalance, Remove}{Voter, NonVoter} set of methods, so we know that we're not making a decision that could, for instance, drive the range into unavailability or something.
3. the replicateQueue requeues ranges back to be processed again after it executes upon an allocator action. If we raced with something and somehow ended up making a suboptimal rebalancing decision, we will correct it on the next attempt.

Does that make sense? Do you think this reasoning should end up in the comments?

pkg/kv/kvserver/replicate_queue.go, line 928 at r2 (raw file):

Yeah we make this check right at the beginning of maybeTransferLeaseAway anyway. Removed.

pkg/kv/kvserver/replicate_queue_test.go, line 398 at r2 (raw file):

I was hoping for the infrequent checks to make it less likely to flake under race. I bumped it to 100ms.

pkg/kv/kvserver/replicate_queue_test.go, line 398 at r2 (raw file):

Done.

pkg/kv/kvserver/replicate_queue_test.go, line 409 at r2 (raw file):

Done. LMK if they seem lacking.

Reviewed 1 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15 and @andreimatei)

pkg/kv/kvserver/allocator.go, line 395 at r1 (raw file):

	if a.storePool == nil {
		// Do nothing if storePool is nil for some unittests.
		return AllocatorNoop, 0

I'm very positive on most of this change! But I'm not sure it improved this particular function. The elimination of short-circuiting makes this more difficult to follow. Now every case needs to ask the question: "what happens after this?" I'd encourage us to revert back to the old style and just have a bit of return action, action.Priority() repetition.

pkg/kv/kvserver/replicate_queue.go, line 496 at r2 (raw file):

Well, I think the point stands that it took a fairly long message to convince me that it isn't subtly broken 😃 And the next reader is going to probably think the same thing, at which point they may be right because things may have changed in very distant parts of the system. This kind of logic is such a red flag because its correctness can't be reasoned about locally. It requires the caller to ask - "what could have happened between the first time I looked and this and the next time I looked at it? Could I have raced with a concurrent modification? If so, in which ways can this state be different now from what it looked like when I decided to take some action?".

Do we gain anything from this structure? Would the fix be as straightforward as plumbing the existing desc and zone down to each of these functions?

Also, no need to make any changes for this here.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @nvanbenschoten)

pkg/kv/kvserver/allocator.go, line 395 at r1 (raw file):

Done.

pkg/kv/kvserver/replicate_queue.go, line 496 at r2 (raw file):

I think the point stands that it took a fairly long message to convince me that it isn't subtly broken 😃

Yaa very fair.

Would the fix be as straightforward as plumbing the existing desc and zone down to each of these functions?

I think so, yes. With the plumbing, if we used a stale descriptor, it would get rejected when AdminChangeReplicas checks it against the version in KV.

Also, no need to make any changes for this here.

Added a TODO for now.

I'll update the PR with a good test for that case, but it should be ready for a look aside from that.

I've added this test. PTAL.

I recognize that this bad boy is blocking the beta, I'll try and get the PR through CI and backport as soon as I get LGTM on the test.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei and @nvanbenschoten)

Reviewed 5 of 5 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)