kv: acquire clock reading for lease request under lock #135042
Conversation
This commit adds a pair of assertions near evaluation-time lease manipulation which demonstrate why the changes to the new lease are safe. Epic: None Release note: None
Informs cockroachdb#134171. This commit updates the handling of prevLeaseManipulation during lease construction to not revoke the previous lease if it has already expired when switching lease types. This was causing unnecessary bumps to the MinLeaseProposedTS in tests like TestRequestsOnLaggingReplica (cockroachdb#134171). Release note: None
Fixes cockroachdb#134171. This commit adds validation that lease construction that the min lease proposed timestamp is never in advance of the current time. The commit then fixes a case where this could occur by acquiring a clock reading under the replica mutex in redirectOnOrAcquireLeaseForRequest. While I was never able to reproduce the issue in TestRequestsOnLaggingReplica, I did independently and reliably hit the same issue when adding lease type changes to kvnemesis (cockroachdb#125260, PR to follow soon) and confirmed that this change fixes the issue. Epic: None
nvanbenschoten
added
the
backport-24.3.x
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)
pkg/kv/kvserver/leases/build.go line 607 at r2 (raw file):
prevType := i.PrevLease.Type() indirectExp := prevType != roachpb.LeaseExpiration switchingType := prevType != nextLease.Type()
Were you able to figure out why switchingType was true? Were we acquiring an expiration based lease for some reason?
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/leases/build.go line 607 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Were you able to figure out why
switchingTypewas true? Were we acquiring an expiration based lease for some reason?
What I think I saw happen was that a leader lease expired when a raft leadership term ended. The replica then attempted to acquire the lease again, and hit this path because it was a follower:
cockroach/pkg/kv/kvserver/leases/build.go
Line 387 in 2520c51
So once the leader lease expired, it tried to acquire an expiration based lease. I think we will allow this until we get kv.lease.reject_on_leader_unknown.enabled defaulted to true (#118435).
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/leases/build.go line 607 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
What I think I saw happen was that a leader lease expired when a raft leadership term ended. The replica then attempted to acquire the lease again, and hit this path because it was a follower:
cockroach/pkg/kv/kvserver/leases/build.go
Line 387 in 2520c51
So once the leader lease expired, it tried to acquire an expiration based lease. I think we will allow this until we get
kv.lease.reject_on_leader_unknown.enableddefaulted to true (#118435).
Nice!
|
TFTR! bors r+ |
|
blathers backport 24.3.0-rc |
Fixes #134171.
This commit adds validation that lease construction that the min lease proposed timestamp is never in advance of the current time.
The commit then fixes a case where this could occur by acquiring a clock reading under the replica mutex in redirectOnOrAcquireLeaseForRequest.
While I was never able to reproduce the issue in
TestRequestsOnLaggingReplica, I did independently and reliably hit the same issue when adding lease type changes tokvnemesis(#125260, PR to follow soon) and confirmed that this change fixes the issue.Epic: None