storage: check leaseholder status in AdminSplit retry loop

[nvanbenschoten]

Fixes #23673.

This change adds a call to redirectOnOrAcquireLease to the AdminSplit
retry loop. This ensures that each attempt to perform a split is done
by a leaseholder. Without this check, it was possible that a stale
follower or a replica that had already been removed from the range
could end up in an infinite retry loop. This was possible because
these replicas could have stale knowledge about the state of the
range descriptor. The effect of this was that the replicas' updates
to the local range descriptor key could indefinitely hit
ConditionFailedErrors, which are retried.

The change also adds a regression test that creates this
"replica removed while splitting" scenario. Without the leaseholder
check introduced here, the test would fail.

Release note: None

[cockroach-teamcity]

This change is 

[benesch]

Now that the root cause of this infinite retry loop is fixed, we should consider reverting #23310. This loop should not retry indefinitely. If it does, exiting will just mask a more serious issue.

Well, the intent was to return the persistent error up into the log file or jobs table where it would be visible to the user. Granted, I botched that 🙈, but I'm still nervous about retry loops that never bail out. Is this concern misplaced? This edge case went unnoticed for a while; I'm worried there might be another.

[bdarnell]

I think the limit on the retry loop should stay.

---

Reviewed 3 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/replica_command.go, line 716 at r2 (raw file):

	for retryable := retry.StartWithCtx(ctx, retryOpts); retryable.Next(); {
		// Admin commands always require the range lease to begin (see
		// executeAdminBatch), but we may have lost it while in this retry loop.

There are also entry points to AdminSplit that don't go through executeAdminBatch, IIRC.

---

pkg/storage/store.go, line 669 at r2 (raw file):

	EvalKnobs batcheval.TestingKnobs

	// TestingRequestFilter is called before evaluating each command.

Be more specific, since there's already EvalKnobs.TestingEvalFilter which runs at evaluation time. (the comment on ReplicaRequestFilter about when this runs belongs here, not on the function typedef)

---

Comments from Reviewable

[nvanbenschoten]

TFTRs! Added a new commit (now the second) to address #23310 (comment).

---

Review status: 2 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/storage/replica_command.go, line 716 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

There are also entry points to AdminSplit that don't go through executeAdminBatch, IIRC.

It doesn't look like there are anymore. Everything else goes through the Replica by sending an AdminSplitRequest.

You might be thinking about the splitQueue, which calls adminSplitWithDescriptor directly. Because this bypasses AdminSplit, it is not subject to the retry loop. This looks like it was an intentional decision made in #10728 (see #10728 (comment)).

---

pkg/storage/store.go, line 669 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Be more specific, since there's already EvalKnobs.TestingEvalFilter which runs at evaluation time. (the comment on ReplicaRequestFilter about when this runs belongs here, not on the function typedef)

Expanded this comment here but left the other because all of these function typedefs seem to have similar descriptions.

---

Comments from Reviewable