Skip to content

Commit

Permalink
Merge #126202
Browse files Browse the repository at this point in the history 
126202: roachpb: add MinExpiration field to Lease r=arulajmani a=nvanbenschoten

First half of #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

Co-authored-by: Nathan VanBenschoten <[email protected]>
  • Loading branch information
@nvanbenschoten
craig[bot] and nvanbenschoten committed Jul 1, 2024
2 parents 053bc17 + 96cf596 commit ccb6f96
Show file tree
Hide file tree
Showing 5 changed files with 123 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
53 changes: 44 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 @@ -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
}

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
86 changes: 54 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 @@ -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) {
Expand Down
8 changes: 8 additions & 0 deletions pkg/ui/workspaces/db-console/src/views/reports/containers/range/rangeTable.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -750,6 +755,9 @@ export default class RangeTable extends React.Component<RangeTableProps, {}> {
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),
),
Expand Down

0 comments on commit ccb6f96

Please sign in to comment.