Skip to content

Commit 58d0fc3

Browse files
craig[bot]tbg
craig[bot]
and
tbg
committed
Merge #40329
40329: storage: return LeaseRejectedError when learner requests lease r=danhhz a=tbg We would previously return an unstructured error which would leak to the client. The LeaseRejectedError gets turned into a NotLeaseholderError instead, and DistSender will try another replica. Fixes #40323. Note that DistSender doesn't send to learners in the first place, so it's unclear why we asked a learner to request a lease, but it's likely that we upgraded the learner to a voter while it was down (the test below restarts nodes) and it received a request while it was already a voter but not aware of it. Release note: None Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
2 parents c55d392 + 9376c8a commit 58d0fc3

4 files changed

Lines changed: 25 additions & 12 deletions

File tree

‎pkg/storage/batcheval/cmd_lease.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,15 @@ func checkCanReceiveLease(rec EvalContext) error {
5858
//
5959
// Since the leaseholder can't remove itself and is a VOTER_FULL, we
6060
// also know that in any configuration there's at least one VOTER_FULL.
61-
return errors.Errorf(`cannot transfer lease to replica of type %s`, t)
61+
//
62+
// TODO(tbg): if this code path is hit during a lease transfer (we check
63+
// upstream of raft, but this check has false negatives) then we are in
64+
// a situation where the leaseholder is a node that has set its
65+
// minProposedTS and won't be using its lease any more. Either the setting
66+
// of minProposedTS needs to be "reversible" (tricky) or we make the
67+
// lease evaluation succeed, though with a lease that's "invalid" so that
68+
// a new lease can be requested right after.
69+
return errors.Errorf(`replica of type %s cannot hold lease`, t)
6270
}
6371
return nil
6472
}

‎pkg/storage/batcheval/cmd_lease_request.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@ func RequestLease(
3636
// newFailedLeaseTrigger() to satisfy stats.
3737
args := cArgs.Args.(*roachpb.RequestLeaseRequest)
3838

39+
prevLease, _ := cArgs.EvalCtx.GetLease()
40+
rErr := &roachpb.LeaseRejectedError{
41+
Existing: prevLease,
42+
Requested: args.Lease,
43+
}
44+
3945
// For now, don't allow replicas of type LEARNER to be leaseholders. There's
4046
// no reason this wouldn't work in principle, but it seems inadvisable. In
4147
// particular, learners can't become raft leaders, so we wouldn't be able to
@@ -48,14 +54,8 @@ func RequestLease(
4854
// If this check is removed at some point, the filtering of learners on the
4955
// sending side would have to be removed as well.
5056
if err := checkCanReceiveLease(cArgs.EvalCtx); err != nil {
51-
return newFailedLeaseTrigger(false /* isTransfer */), err
52-
}
53-
54-
prevLease, _ := cArgs.EvalCtx.GetLease()
55-
56-
rErr := &roachpb.LeaseRejectedError{
57-
Existing: prevLease,
58-
Requested: args.Lease,
57+
rErr.Message = err.Error()
58+
return newFailedLeaseTrigger(false /* isTransfer */), rErr
5959
}
6060

6161
// MIGRATION(tschottdorf): needed to apply Raft commands which got proposed

‎pkg/storage/batcheval/cmd_lease_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,13 @@ func TestLeaseCommandLearnerReplica(t *testing.T) {
129129
// Learners are not allowed to become leaseholders for now, see the comments
130130
// in TransferLease and RequestLease.
131131
_, err := TransferLease(ctx, nil, cArgs, nil)
132-
require.EqualError(t, err, `cannot transfer lease to replica of type LEARNER`)
132+
require.EqualError(t, err, `replica of type LEARNER cannot hold lease`)
133133

134134
cArgs.Args = &roachpb.RequestLeaseRequest{}
135135
_, err = RequestLease(ctx, nil, cArgs, nil)
136-
require.EqualError(t, err, `cannot transfer lease to replica of type LEARNER`)
136+
137+
const exp = `cannot replace lease repl=(n0,s0):? seq=0 start=0.000000000,0 exp=<nil> ` +
138+
`with repl=(n0,s0):? seq=0 start=0.000000000,0 exp=<nil>: ` +
139+
`replica of type LEARNER cannot hold lease`
140+
require.EqualError(t, err, exp)
137141
}

‎pkg/storage/batcheval/cmd_resolve_intent_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ type mockEvalCtx struct {
4242
gcThreshold hlc.Timestamp
4343
term, firstIndex uint64
4444
canCreateTxnFn func() (bool, hlc.Timestamp, roachpb.TransactionAbortedReason)
45+
lease roachpb.Lease
4546
}
4647

4748
func (m *mockEvalCtx) String() string {
@@ -119,7 +120,7 @@ func (m *mockEvalCtx) GetLastReplicaGCTimestamp(context.Context) (hlc.Timestamp,
119120
panic("unimplemented")
120121
}
121122
func (m *mockEvalCtx) GetLease() (roachpb.Lease, roachpb.Lease) {
122-
panic("unimplemented")
123+
return m.lease, roachpb.Lease{}
123124
}
124125

125126
func TestDeclareKeysResolveIntent(t *testing.T) {

0 commit comments

Comments
 (0)