storage: check whether the acquiring replica can receive the lease

[ajwerner]

Before this PR, when evaluating lease transfers or requests we'd check whether
the evaluating replica could receive the lease rather than the acquiring
replica. This patch fixes that bug by checking the state of the correct
replica.

Fixes #38720.

Release note (bug fix): The system no longer erroneously transfers leases
to replicas which are in the process of being removed which can lead to
ranges being effectively unavailable due to an invalid lease.

[ajwerner]

This needs a test.

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)

---

pkg/storage/client_lease_test.go, line 352 at r1 (raw file):

	defer tc.Stopper().Stop(ctx)

	scratchStartKey := tc.ScratchRange(t)

I'll add some more commentary here about what's going on.

[nvanbenschoten]

thanks for getting this out so fast.

Do you mind writing up an issue about the other issue we hypothesized with RequestLease?

Reviewed 3 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, and @nvanbenschoten)

---

pkg/storage/client_lease_test.go, line 365 at r3 (raw file):

that at least

"that the test at least"?

---

pkg/storage/client_lease_test.go, line 395 at r3 (raw file):

					// The error generated prior to evaluation which we try to avoid
					// by sleeping a bit and running the go scheduler but still can
					// happen.

Do you mind explaining why we can get this in a little more detail? Is the problem that we occasionally haven't begun acquiring latches by the time we close(ch) and let the ChangeReplicas through?

---

pkg/storage/batcheval/cmd_lease.go, line 46 at r3 (raw file):

func checkCanReceiveLease(newLease *roachpb.Lease, rec EvalContext) error {
	repDesc, ok := rec.Desc().GetReplicaDescriptorByID(newLease.Replica.ReplicaID)

Is there a reason to switch from GetReplicaDescriptor to GetReplicaDescriptorByID?

[ajwerner]

Do you mind writing up an issue about the other issue we hypothesized with RequestLease?

Yep, doing that right now.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, and @nvanbenschoten)

[ajwerner]

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

---

pkg/storage/client_lease_test.go, line 395 at r3 (raw file):

Is the problem that we occasionally haven't begun acquiring latches by the time we close(ch) and let the ChangeReplicas through?

Yes, done.

---

pkg/storage/batcheval/cmd_lease.go, line 46 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a reason to switch from GetReplicaDescriptor to GetReplicaDescriptorByID?

Using the replica ID seems better. Imagine a case where n1/1 and now is n1/4. Imagine an attempt to transfer the lease to n1/1 The code here as well as evalNewLease only uses the store ID and I believe would permit that transfer which would be generally non-sensical.

It's not clear how such a request could come to exist but I don't see a reason to let a lease request for a replica which is not in the range but a store which is to succeed.

[nvanbenschoten]

Still LGTM

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

[ajwerner]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[ajwerner]

TeamCity builder ran out of space.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)