kvserver: modernize AdminRelocateRange

[aayushshah15]

Previously, AdminRelocateRange's contract was to transfer the lease away to
the first voting replica in the input slice of targets. However, this was
mostly a result of the fact that leaseholder replicas weren't allowed to remove
themselves. Thus, the caller had to specifically tell AdminRelocateRange where
it wanted the lease to be, and the AdminRelocateRange logic would ensure to
transfer the lease away to this replica either before it removed the
leaseholder or after it was done performing all the replica relocations.

There are two internal callers of AdminRelocateRange, and this property of
the AdminRelocateRange API is unfortunate for both of them.

1. The merge queue takes extra effort to avoid a lease transfer when calling
   into AdminRelocateRange.
   

   cockroach/pkg/kv/kvserver/merge_queue.go

   Lines 338 to 348 in 52c2df4

   	// AdminRelocateRange moves the lease to the first target in the list, so
   	// sort the existing leaseholder there to leave it unchanged.
   	lease, _ := lhsRepl.GetLease()
   	for i := range voterTargets {
   	if t := voterTargets[i]; t.NodeID == lease.Replica.NodeID && t.StoreID == lease.Replica.StoreID {
   	if i > 0 {
   	voterTargets[0], voterTargets[i] = voterTargets[i], voterTargets[0]
   	}
   	break
   	}
   	}

2. The store rebalancer performs a lease transfer based on very naive logic
   that is not optimal.
   

   cockroach/pkg/kv/kvserver/store_rebalancer.go

   Lines 601 to 605 in c2f7bd6

   	// Pick the voter with the least QPS to be leaseholder;
   	// RelocateRange transfers the lease to the first provided target.
   	//
   	// TODO(aayush): Does this logic need to exist? This logic does not take
   	// lease preferences into account. So it is already broken in a way.

With #74077, leaseholder replicas will be able to remove themselves by
transferring the lease away to a suitable replica after they enter the joint
configuration of the replication change. This patch makes it such that, going
forward, AdminRelocateRange will only perform an explicit lease transfer if
the caller specifically asks for it. The patch also contains a migration path
for this functionality.

Once #74077 lands, a future patch will change the store rebalancer and the
merge queue behavior to leverage this new functionality.

Release note: None

[cockroach-teamcity]

This change is 

[aayushshah15]

@shralex and @nvanbenschoten: This patch does what I've individually spoken to both of you about. #74077 depends on this since without it, AdminRelocateRange is going to lead to redundant lease transfers (since in that patch, we're potentially doing lease transfers inside AdminChangeReplicas). Once that patch lands, I will change the merge queue and the store rebalancer behavior to set this newly added TransferLeaseToFirstVoter to false, which will be a good simplification since we can then address the 2 points in the PR description.

[nvanbenschoten]

Reviewed 9 of 11 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

[aayushshah15]

@nvanbenschoten, do you mind giving this another quick look?

The previous revision tried to redundantly transfer the lease to voterTargets[0] even if it had already done so before while relocating some replicas.

[nvanbenschoten]

I don't understand this new change. My understanding of relocateOne is that it should only return a lease target if either len(ops) == 0 or we are removing the current leaseholder. So I don't see how this code change makes a difference. Are you referring to code redundancy or operational redundancy?

I think this code would be easier to write/read if we either pulled the len(ops) == 0 handling entirely out of relocateOne, or pushed the transferLeaseToFirstVoter handling entirely into relocateOne.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

[aayushshah15]

My understanding of relocateOne is that it should only return a lease target if either len(ops) == 0 or we are removing the current leaseholder

Yep that's correct. The previous revision was attempting a lease transfer at the end (if transferLeaseToFirstVoter was true) even if leaseTarget was nil. This meant that in the latter scenario (i.e. if we'd already removed the current leaseholder before), we were attempting the lease transfer redundantly.

pushed the transferLeaseToFirstVoter handling entirely into relocateOne.

Done. Much better.

[nvanbenschoten]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

[aayushshah15]

TFTR

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)