kv: promote expiration-based lease to epoch without sequence number change

[nvanbenschoten]

Fixes #117630.
Fixes #90656.
Fixes #98553.

Informs #61986.
Informs #115191.

This commit updates the post-lease transfer promotion of expiration-based leases to epoch-based leases to not change the sequence number of the lease. This avoids invalidating all requests proposed under the original expiration-based lease, which can lead to RETRY_ASYNC_WRITE_FAILURE errors.

The change accomplishes this by updating the Lease.Equivalent method to consider an expiration-based lease to be equivalent to an epoch-based lease that is held by the same replica and has the same start time. Doing so requires some care, because lease equivalency is checked below Raft and needs to remain deterministic across binary versions.

This change requires a cluster version check, so it cannot be backported.

Release note (bug fix): Improved an interaction during range lease transfers which could previously cause RETRY_ASYNC_WRITE_FAILURE errors to be returned to clients.

[cockroach-teamcity]

This change is 

[erikgrinaker]

This change requires a cluster version check, so it cannot be backported.

That's too bad. We should document for support purposes that these can also be avoided by setting kv.transfer_expiration_leases_first.enabled = false, with caveats about the unavailability risk.

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

---

pkg/kv/kvserver/kvserverpb/proposer_kv.proto line 140 at r3 (raw file):

  // - in v24.2, we can set this field to true unconditionally for lease request
  //   proposals. It must still be consulted during lease application, because
  //   the raft proposal may have been performed by an older node.

It's annoying that we need 4 releases to migrate this, but it seems prudent. I was thinking of ways to get it down to 3, but it's riskier and there's no real rush I guess.

---

pkg/roachpb/data.go line 1885 at r3 (raw file):

// NB: Lease.Equivalent is NOT symmetric. For expiration-based
// leases, a lease is equivalent to another with an equal or
// later expiration, but not an earlier expiration.

nit: should we spell out the expiration->epoch case here, which is asymmetric as well?

---

pkg/roachpb/data.go line 1939 at r3 (raw file):

			// For expiration-based leases, extensions are considered equivalent.
			// This is one case where Equivalent is not commutative and, as such,

nit: ditto this comment.

[nvanbenschoten]

This change requires a cluster version check, so it cannot be backported.

That's too bad. We should document for support purposes that these can also be avoided by setting kv.transfer_expiration_leases_first.enabled = false, with caveats about the unavailability risk.

We put #116092 in place in earlier releases to eliminate the risk of these RETRY_ASYNC_WRITE_FAILURE retries for COPY. Beyond that specific use case, I'm not sure how much more prescriptive we can be towards support. kv.transfer_expiration_leases_first.enabled is an important protection and we don't want users switching that to false indefinitely, so I don't think we want to document that in a runbook as the "solution" to RETRY_ASYNC_WRITE_FAILURE errors.

Still, I think there are cases where this setting can temporarily help and it's better to keep support in the loop, so I wrote up https://cockroachlabs.atlassian.net/wiki/spaces/CKB/pages/3332505662/RETRY+ASYNC+WRITE+FAILURE+during+lease+transfers about this.

TFTR!

bors r=erikgrinaker

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

---

pkg/roachpb/data.go line 1885 at r3 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: should we spell out the expiration->epoch case here, which is asymmetric as well?

Done.

---

pkg/roachpb/data.go line 1939 at r3 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: ditto this comment.

Done.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)