kv: consolidate range lease validation

[nvanbenschoten]

Closes #123498.
Informs #118435.

This commit extracts lease request and lease transfer validation into the kv/kvserver/leases package which was introduced in #124682. This avoids the scattered leases validity checks which were previously spread between replica_range_lease.go, replica_proposal_buf.go cmd_lease_request.go, cmd_lease_transfer.go.

This makes the logic easier to understand and easier to test. It will also make the logic easier to adjust when we make the following two changes in service of #118435:

• default to rejecting lease requests on unknown leaders
• perform lease request validation above raft, like we do for lease transfers

This second change also ties in with Leader leases (#123847). Until we make that change, we will occasionally need to propose expiration-based leases (e.g. when we're not the leader) just for them to be rejected by the proposal buffer.

This is a fairly large PR, and I'm willing to split it up if that makes sense.

Release note: None

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[arulajmani]

Sorry for letting this sit for a bit.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/leases/verify.go line 86 at r1 (raw file):

func verifyAcquisition(ctx context.Context, st Settings, i VerifyInput) error {
	// Handle an edge case about lease acquisitions: we don't want to forward

nit: a few of these sentences are above 80 chars, this might need a re-wrap.

(throughout this function)

---

pkg/kv/kvserver/leases/verify.go line 231 at r1 (raw file):

	// log such that it will be able to apply the lease transfer through log entry
	// application then this unavailability window will be acceptable. This may be
	// a faulty assumption in cases with severe replication lag, but we must

Unrelated to this patch, and not really actionable here, but I'm wondering if replication AC v2 might exacerbate this in the future.

---

pkg/kv/kvserver/leases/build_test.go line 679 at r1 (raw file):

	}
	verifyInput := noZeroBuildInput.toVerifyInput()
	require.NoError(t, zerofields.NoZeroField(verifyInput),

TIL this is a thing. Nice test.

[nvanbenschoten]

TFTR!

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

---

pkg/kv/kvserver/leases/verify.go line 86 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: a few of these sentences are above 80 chars, this might need a re-wrap.

(throughout this function)

Hm, Goland with the Wrap to Column plugin set to 80 chars is showing these comments as all being properly wrapped. Maybe it's counting tabs differently than you were?

---

pkg/kv/kvserver/leases/verify.go line 231 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Unrelated to this patch, and not really actionable here, but I'm wondering if replication AC v2 might exacerbate this in the future.

It's very possible. AC v2 could cause us to delay log replication to a follower, which may expand the unavailability window of a poorly timed lease transfer. At some point, I imagine that we will tighten the protections around lease transfers along the lines of #96304 or #116231 to avoid transferring a lease to a follower replica that is being paced.

I'll ask around about whether there's anything that we can do with priority levels in the meantime.

[nvanbenschoten]

bors r=arulajmani

[craig]

Build succeeded:

• macos_arm64_build
• check_generated_code
• local_roachtest_fips
• lint
• acceptance
• macos_amd64_build
• local_roachtest
• linux_arm64_build
• docker_image_amd64
• windows_build
• examples_orms
• unit_tests
• linux_amd64_build
• linux_amd64_fips_build