Skip to content

Commit

Permalink
roachpb: add MinExpiration field to Lease
Browse files Browse the repository at this point in the history 
Informs cockroachdb#125235.

This commit adds a new MinExpiration field to the Lease struct. This
field defines 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.

The field is not yet used.

Release note: None
  • Loading branch information
@nvanbenschoten
nvanbenschoten committed Jul 1, 2024
1 parent 0292c45 commit 7781cfa
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 44 deletions.
3 changes: 2 additions & 1 deletion pkg/kv/kvpb/errors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -395,6 +395,7 @@ func TestNotLeaseholderError(t *testing.T) {
Epoch: 1,
Sequence: 2,
AcquisitionType: roachpb.LeaseAcquisitionType_Transfer,
MinExpiration: hlc.Timestamp{WallTime: 3},
},
},
},
Expand Down
50 changes: 41 additions & 9 deletions pkg/roachpb/data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -2058,6 +2087,9 @@ func (l *Lease) Equal(that interface{}) bool {
if l.AcquisitionType != that1.AcquisitionType {
return false
}
if !l.MinExpiration.Equal(&that1.MinExpiration) {
return false
}
return true
}

Expand Down
17 changes: 15 additions & 2 deletions pkg/roachpb/data.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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, <expiration from epoch or term>)).
util.hlc.Timestamp min_expiration = 9 [(gogoproto.nullable) = false];
}

// AbortSpanEntry contains information about a transaction which has
Expand Down
85 changes: 53 additions & 32 deletions pkg/roachpb/data_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -1291,6 +1311,7 @@ func TestLeaseEqual(t *testing.T) {
{Epoch: 1},
{Sequence: 1},
{AcquisitionType: 1},
{MinExpiration: ts},
}
for _, c := range testCases {
t.Run("", func(t *testing.T) {
Expand Down

0 comments on commit 7781cfa

Please sign in to comment.