diff --git a/pkg/kv/kvpb/errors_test.go b/pkg/kv/kvpb/errors_test.go index af7ceebc422b..e8114f3f7191 100644 --- a/pkg/kv/kvpb/errors_test.go +++ b/pkg/kv/kvpb/errors_test.go @@ -385,7 +385,7 @@ func TestNotLeaseholderError(t *testing.T) { err *NotLeaseHolderError }{ { - exp: `[NotLeaseHolderError] r1: replica not lease holder; current lease is repl=(n1,s1):1 seq=2 start=0.000000002,0 epo=1 pro=0.000000001,0`, + exp: `[NotLeaseHolderError] r1: replica not lease holder; current lease is repl=(n1,s1):1 seq=2 start=0.000000002,0 epo=1 min-exp=0.000000003,0 pro=0.000000001,0`, err: &NotLeaseHolderError{ RangeID: 1, Lease: &roachpb.Lease{ @@ -395,6 +395,7 @@ func TestNotLeaseholderError(t *testing.T) { Epoch: 1, Sequence: 2, AcquisitionType: roachpb.LeaseAcquisitionType_Transfer, + MinExpiration: hlc.Timestamp{WallTime: 3}, }, }, }, diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index 382565056192..a646d9900129 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -1856,7 +1856,7 @@ func (l Lease) SafeFormat(w redact.SafePrinter, _ rune) { if l.Type() == LeaseExpiration { w.Printf(" exp=%s", l.Expiration) } else { - w.Printf(" epo=%d", l.Epoch) + w.Printf(" epo=%d min-exp=%s", l.Epoch, l.MinExpiration) } w.Printf(" pro=%s", l.ProposedTS) } @@ -1902,8 +1902,10 @@ func (l Lease) Speculative() bool { return l.Sequence == 0 } -// Equivalent determines whether ol is considered the same lease -// for the purposes of matching leases when executing a command. +// Equivalent determines whether the old lease (l) is considered the same as +// the new lease (newL) for the purposes of matching leases when executing a +// command. +// // For expiration-based leases, extensions are allowed. // Ignore proposed timestamps for lease verification; for epoch- // based leases, the start time of the lease is sufficient to @@ -1921,12 +1923,32 @@ func (l Lease) Speculative() bool { // lease with the same replica and start time (representing a // promotion from expiration-based to epoch-based), but the // reverse is not true. +// +// One of the uses of Equivalent is in deciding what Sequence to assign to +// newL, so this method must not use the value of Sequence for equivalency. +// +// The Start time of the two leases is compared, and a necessary condition +// for equivalency is that they must be equal. So in the case where the +// caller is someone who is constructing a new lease proposal, it is the +// caller's responsibility to realize that the two leases *could* be +// equivalent, and adjust the start time to be the same. Even if the start +// times are the same, the leases could turn out to be non-equivalent -- in +// that case they will share a start time but not the sequence. +// +// NB: we do not allow transitions from epoch-based or leader leases (not +// yet implemented) to expiration-based leases to be equivalent. This was +// because both of the former lease types don't have an expiration in the +// lease, while the latter does. We can introduce safety violations by +// shortening the lease expiration if we allow this transition, since the +// new lease may not apply at the leaseholder until much after it applies at +// some other replica, so the leaseholder may continue acting as one based +// on an old lease, while the other replica has stepped up as leaseholder. func (l Lease) Equivalent(newL Lease, expToEpochEquiv bool) bool { // Ignore proposed timestamp & deprecated start stasis. l.ProposedTS, newL.ProposedTS = hlc.ClockTimestamp{}, hlc.ClockTimestamp{} l.DeprecatedStartStasis, newL.DeprecatedStartStasis = nil, nil - // Ignore sequence numbers, they are simply a reflection of - // the equivalency of other fields. + // Ignore sequence numbers, they are simply a reflection of the equivalency of + // other fields. Also, newL may not have an initialized sequence number. l.Sequence, newL.Sequence = 0, 0 // Ignore the acquisition type, as leases will always be extended via // RequestLease requests regardless of how a leaseholder first acquired its @@ -1949,6 +1971,12 @@ func (l Lease) Equivalent(newL Lease, expToEpochEquiv bool) bool { if l.Epoch == newL.Epoch { l.Epoch, newL.Epoch = 0, 0 } + + // For epoch-based leases, extensions to the minimum expiration are + // considered equivalent. + if l.MinExpiration.LessEq(newL.MinExpiration) { + l.MinExpiration, newL.MinExpiration = hlc.Timestamp{}, hlc.Timestamp{} + } case LeaseExpiration: switch newL.Type() { case LeaseEpoch: @@ -1963,11 +1991,12 @@ func (l Lease) Equivalent(newL Lease, expToEpochEquiv bool) bool { // case where Equivalent is not commutative, as the reverse transition // (from epoch-based to expiration-based) requires a sequence increment. // - // Ignore epoch and expiration. The remaining fields which are compared - // are Replica and Start. + // Ignore expiration, epoch, and min expiration. The remaining fields + // which are compared are Replica and Start. if expToEpochEquiv { - l.Epoch, newL.Epoch = 0, 0 - l.Expiration, newL.Expiration = nil, nil + l.Expiration = nil + newL.Epoch = 0 + newL.MinExpiration = hlc.Timestamp{} } case LeaseExpiration: @@ -2055,6 +2084,12 @@ func (l *Lease) Equal(that interface{}) bool { if l.Sequence != that1.Sequence { return false } + if l.AcquisitionType != that1.AcquisitionType { + return false + } + if !l.MinExpiration.Equal(&that1.MinExpiration) { + return false + } return true } diff --git a/pkg/roachpb/data.proto b/pkg/roachpb/data.proto index cbb536928d9c..718ab027f73d 100644 --- a/pkg/roachpb/data.proto +++ b/pkg/roachpb/data.proto @@ -675,6 +675,8 @@ message Lease { // The expiration is a timestamp at which the lease expires. This means that a // new lease can be granted for a later timestamp. This field is only set for // expiration-based leases. + // + // This is an exclusive value, i.e. the lease is valid in [start, expiration). util.hlc.Timestamp expiration = 2; // The address of the would-be lease holder. @@ -702,8 +704,9 @@ message Lease { (gogoproto.nullable) = false, (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/util/hlc.ClockTimestamp"]; - // The epoch of the lease holder's node liveness entry. If this value is - // non-zero, the expiration field is ignored. + // The epoch of the lease holder's node liveness entry. This field is only set + // for epoch-based leases. If this value is non-zero, the expiration field and + // the term field (not yet implemented) are ignored. int64 epoch = 6; // A zero-indexed sequence number which is incremented during the acquisition @@ -720,6 +723,16 @@ message Lease { // The type of acquisition event that result in this lease (transfer or // request). LeaseAcquisitionType acquisition_type = 8; + + // The minimum expiration at which the lease expires, independent of any other + // expiry condition. This field can be used to place a floor on the expiration + // for epoch-based leases and leader leases (not yet implemented) to prevent + // expiration regressions when upgrading from an expiration-based lease. It is + // not supported for expiration-based leases. + // + // Like expiration above, this is an exclusive value, i.e. the lease is valid + // in [start, max(min_expiration, )). + util.hlc.Timestamp min_expiration = 9 [(gogoproto.nullable) = false]; } // AbortSpanEntry contains information about a transaction which has diff --git a/pkg/roachpb/data_test.go b/pkg/roachpb/data_test.go index d627c438372a..ff594f6f1196 100644 --- a/pkg/roachpb/data_test.go +++ b/pkg/roachpb/data_test.go @@ -1099,7 +1099,7 @@ func TestLeaseStringAndSafeFormat(t *testing.T) { ReplicaID: 1, }, Start: makeClockTS(1, 1), - Expiration: makeClockTS(2, 1).ToTimestamp().Clone(), + Expiration: makeTS(2, 1).Clone(), ProposedTS: makeClockTS(1, 0), Sequence: 3, }, @@ -1113,12 +1113,13 @@ func TestLeaseStringAndSafeFormat(t *testing.T) { StoreID: 1, ReplicaID: 1, }, - Start: makeClockTS(1, 1), - ProposedTS: makeClockTS(1, 0), - Sequence: 3, - Epoch: 4, + Start: makeClockTS(1, 1), + ProposedTS: makeClockTS(1, 0), + Sequence: 3, + Epoch: 4, + MinExpiration: makeTS(2, 1), }, - exp: "repl=(n1,s1):1 seq=3 start=0.000000001,1 epo=4 pro=0.000000001,0", + exp: "repl=(n1,s1):1 seq=3 start=0.000000001,1 epo=4 min-exp=0.000000002,1 pro=0.000000001,0", }, } { t.Run(tc.name, func(t *testing.T) { @@ -1163,6 +1164,10 @@ func TestLeaseEquivalence(t *testing.T) { epoch1Voter := Lease{Replica: r1Voter, Start: ts1, Epoch: 1} epoch1Learner := Lease{Replica: r1Learner, Start: ts1, Epoch: 1} + epoch1MinExp2 := Lease{Replica: r1, Start: ts1, Epoch: 1, MinExpiration: ts2.ToTimestamp()} + epoch1MinExp3 := Lease{Replica: r1, Start: ts1, Epoch: 1, MinExpiration: ts3.ToTimestamp()} + epoch2MinExp2 := Lease{Replica: r1, Start: ts1, Epoch: 2, MinExpiration: ts2.ToTimestamp()} + testCases := []struct { l, ol Lease expSuccess bool @@ -1191,40 +1196,54 @@ func TestLeaseEquivalence(t *testing.T) { {epoch1, epoch1Voter, true}, // same epoch lease, different replica type {epoch1, epoch1Learner, true}, // same epoch lease, different replica type {epoch1Voter, epoch1Learner, true}, // same epoch lease, different replica type + // Test minimum expiration. + {epoch1, epoch1MinExp2, true}, // different epoch leases, newer min expiration + {epoch1, epoch1MinExp3, true}, // different epoch leases, newer min expiration + {epoch1MinExp2, epoch1, false}, // different epoch leases, older min expiration + {epoch1MinExp2, epoch1MinExp2, true}, // same epoch leases, same min expiration + {epoch1MinExp2, epoch1MinExp3, true}, // different epoch leases, newer min expiration + {epoch1MinExp3, epoch1, false}, // different epoch leases, older min expiration + {epoch1MinExp3, epoch1MinExp2, false}, // different epoch leases, older min expiration + {epoch1MinExp3, epoch1MinExp3, true}, // same epoch leases, same min expiration + {epoch1MinExp2, epoch2MinExp2, false}, // different epoch leases } for i, tc := range testCases { - // Test expToEpochEquiv = true. - require.Equal(t, tc.expSuccess, tc.l.Equivalent(tc.ol, true /* expToEpochEquiv */), "%d", i) - if tc.l == expire1 && tc.ol == epoch1 { - // The one case where expToEpochEquiv = false makes a difference. - require.Equal(t, !tc.expSuccess, tc.l.Equivalent(tc.ol, false /* expToEpochEquiv */), "%d", i) - } else { - require.Equal(t, tc.expSuccess, tc.l.Equivalent(tc.ol, false /* expToEpochEquiv */), "%d", i) - } + t.Run("", func(t *testing.T) { + // Test expToEpochEquiv = true. + require.Equal(t, tc.expSuccess, tc.l.Equivalent(tc.ol, true /* expToEpochEquiv */), "%d", i) + if tc.l == expire1 && tc.ol == epoch1 { + // The one case where expToEpochEquiv = false makes a difference. + require.Equal(t, !tc.expSuccess, tc.l.Equivalent(tc.ol, false /* expToEpochEquiv */), "%d", i) + } else { + require.Equal(t, tc.expSuccess, tc.l.Equivalent(tc.ol, false /* expToEpochEquiv */), "%d", i) + } + }) } - // #18689 changed the nullability of the DeprecatedStartStasis, ProposedTS, and Expiration - // field. It introduced a bug whose regression is caught below where a zero Expiration and a nil - // Expiration in an epoch-based lease led to mistakenly considering leases non-equivalent. - prePRLease := Lease{ - Start: hlc.ClockTimestamp{WallTime: 10}, - Epoch: 123, + t.Run("DeprecatedStartStasis", func(t *testing.T) { + // #18689 changed the nullability of the DeprecatedStartStasis, ProposedTS, and Expiration + // field. It introduced a bug whose regression is caught below where a zero Expiration and a nil + // Expiration in an epoch-based lease led to mistakenly considering leases non-equivalent. + prePRLease := Lease{ + Start: hlc.ClockTimestamp{WallTime: 10}, + Epoch: 123, - // The bug-trigger. - Expiration: new(hlc.Timestamp), + // The bug-trigger. + Expiration: new(hlc.Timestamp), - // Similar potential bug triggers, but these were actually handled correctly. - DeprecatedStartStasis: new(hlc.Timestamp), - ProposedTS: hlc.ClockTimestamp{WallTime: 10}, - } - postPRLease := prePRLease - postPRLease.DeprecatedStartStasis = nil - postPRLease.Expiration = nil + // Similar potential bug triggers, but these were actually handled correctly. + DeprecatedStartStasis: new(hlc.Timestamp), + ProposedTS: hlc.ClockTimestamp{WallTime: 10}, + } + postPRLease := prePRLease + postPRLease.DeprecatedStartStasis = nil + postPRLease.Expiration = nil - if !postPRLease.Equivalent(prePRLease, true) || !prePRLease.Equivalent(postPRLease, true) { - t.Fatalf("leases not equivalent but should be despite diff(pre,post) = %s", pretty.Diff(prePRLease, postPRLease)) - } + if !postPRLease.Equivalent(prePRLease, true) || !prePRLease.Equivalent(postPRLease, true) { + t.Fatalf("leases not equivalent but should be despite diff(pre,post) = %s", pretty.Diff(prePRLease, postPRLease)) + } + }) } func TestLeaseEqual(t *testing.T) { @@ -1237,6 +1256,7 @@ func TestLeaseEqual(t *testing.T) { Epoch int64 Sequence LeaseSequence AcquisitionType LeaseAcquisitionType + MinExpiration hlc.Timestamp } // Verify that the lease structure does not change unexpectedly. If a compile // error occurs on the following line of code, update the expectedLease @@ -1290,6 +1310,8 @@ func TestLeaseEqual(t *testing.T) { {ProposedTS: clockTS}, {Epoch: 1}, {Sequence: 1}, + {AcquisitionType: 1}, + {MinExpiration: ts}, } for _, c := range testCases { t.Run("", func(t *testing.T) { diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/range/rangeTable.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/range/rangeTable.tsx index 6d12c6aefa42..bf17c4cd3737 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/range/rangeTable.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/range/rangeTable.tsx @@ -86,6 +86,11 @@ const rangeTableDisplayList: RangeTableRow[] = [ display: "Lease Expiration", compareToLeader: true, }, + { + variable: "leaseMinExpiration", + display: "Lease Minimum Expiration", + compareToLeader: true, + }, { variable: "leaseAppliedIndex", display: "Lease Applied Index", @@ -750,6 +755,9 @@ export default class RangeTable extends React.Component { leaseExpiration: epoch ? rangeTableEmptyContent : this.contentTimestamp(lease.expiration, now), + leaseMinExpiration: epoch + ? this.contentTimestamp(lease.min_expiration, now) + : rangeTableEmptyContent, leaseAppliedIndex: this.createContent( FixLong(info.state.state.lease_applied_index), ),