Skip to content

Commit

Permalink
kvserver: allow voter demoting to get a lease, in case there's an inc…
Browse files Browse the repository at this point in the history 
…oming voter

It is possible that we've entered a joint config where the leaseholder is being
replaced with an incoming voter. We try to transfer the lease away, but this fails.
In this case, we need the demoted voter to re-aquire the lease, as it might be an epoch
based lease, that doesn't expire. Otherwise we might be stuck without anyone else getting
the lease.

TODO: test and backport this

Release note: None
  • Loading branch information
@shralex
shralex committed Jul 1, 2022
1 parent 82923fc commit 723a8a8
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 12 deletions.
44 changes: 35 additions & 9 deletions pkg/kv/kvserver/batcheval/cmd_lease_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,36 @@ func TestCheckCanReceiveLease(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

const none = roachpb.ReplicaType(-1)

for _, tc := range []struct {
leaseholderType roachpb.ReplicaType
eligible bool
leaseholderType roachpb.ReplicaType
anotherReplicaType roachpb.ReplicaType
eligible bool
}{
{leaseholderType: roachpb.VOTER_FULL, eligible: true},
{leaseholderType: roachpb.VOTER_INCOMING, eligible: true},
{leaseholderType: roachpb.VOTER_OUTGOING, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, eligible: false},
{leaseholderType: roachpb.LEARNER, eligible: false},
{leaseholderType: roachpb.NON_VOTER, eligible: false},
{leaseholderType: roachpb.VOTER_FULL, anotherReplicaType: none, eligible: true},
{leaseholderType: roachpb.VOTER_INCOMING, anotherReplicaType: none, eligible: true},

// A VOTER_OUTGOING should only be able to get the lease if there's a VOTER_INCOMING
{leaseholderType: roachpb.VOTER_OUTGOING, anotherReplicaType: none, eligible: false},
{leaseholderType: roachpb.VOTER_OUTGOING, anotherReplicaType: roachpb.VOTER_INCOMING, eligible: true},
{leaseholderType: roachpb.VOTER_OUTGOING, anotherReplicaType: roachpb.VOTER_OUTGOING, eligible: false},
{leaseholderType: roachpb.VOTER_OUTGOING, anotherReplicaType: roachpb.VOTER_FULL, eligible: false},

// A VOTER_DEMOTING_LEARNER should only be able to get the lease if there's a VOTER_INCOMING
{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, anotherReplicaType: none, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, anotherReplicaType: roachpb.VOTER_INCOMING, eligible: true},
{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, anotherReplicaType: roachpb.VOTER_FULL, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_LEARNER, anotherReplicaType: roachpb.VOTER_OUTGOING, eligible: false},

// A VOTER_DEMOTING_NON_VOTER should only be able to get the lease if there's a VOTER_INCOMING
{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, anotherReplicaType: none, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, anotherReplicaType: roachpb.VOTER_INCOMING, eligible: true},
{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, anotherReplicaType: roachpb.VOTER_FULL, eligible: false},
{leaseholderType: roachpb.VOTER_DEMOTING_NON_VOTER, anotherReplicaType: roachpb.VOTER_OUTGOING, eligible: false},

{leaseholderType: roachpb.LEARNER, anotherReplicaType: none, eligible: false},
{leaseholderType: roachpb.NON_VOTER, anotherReplicaType: none, eligible: false},
} {
t.Run(tc.leaseholderType.String(), func(t *testing.T) {
repDesc := roachpb.ReplicaDescriptor{
Expand All @@ -285,6 +304,13 @@ func TestCheckCanReceiveLease(t *testing.T) {
rngDesc := roachpb.RangeDescriptor{
InternalReplicas: []roachpb.ReplicaDescriptor{repDesc},
}
if tc.anotherReplicaType != none {
anotherDesc := roachpb.ReplicaDescriptor{
ReplicaID: 2,
Type: &tc.anotherReplicaType,
}
rngDesc.InternalReplicas = append(rngDesc.InternalReplicas, anotherDesc)
}
err := roachpb.CheckCanReceiveLease(rngDesc.InternalReplicas[0], rngDesc.Replicas())
require.Equal(t, tc.eligible, err == nil, "err: %v", err)
})
Expand Down
106 changes: 104 additions & 2 deletions pkg/kv/kvserver/client_lease_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ func TestGossipNodeLivenessOnLeaseChange(t *testing.T) {
}

// TestCannotTransferLeaseToVoterOutgoing ensures that the evaluation of lease
// requests for nodes which are already in the VOTER_OUTGOING state will fail.
// requests for nodes which are already in the VOTER_OUTGOING state will fail
// (in this test, there is no VOTER_INCOMING node).
func TestCannotTransferLeaseToVoterOutgoing(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down Expand Up @@ -229,7 +230,6 @@ func TestCannotTransferLeaseToVoterOutgoing(t *testing.T) {
_, err = tc.Server(0).DB().AdminChangeReplicas(ctx,
scratchStartKey, desc, []roachpb.ReplicationChange{
{ChangeType: roachpb.REMOVE_VOTER, Target: tc.Target(2)},
{ChangeType: roachpb.ADD_VOTER, Target: tc.Target(3)},
})
require.NoError(t, err)
}()
Expand Down Expand Up @@ -262,7 +262,109 @@ func TestCannotTransferLeaseToVoterOutgoing(t *testing.T) {
close(ch)
wg.Wait()
})
}

// TestTransferLeaseToVoterOutgoingWithIncoming ensures that the evaluation of lease
// requests for nodes which are already in the VOTER_OUTGOING state succeeds
// when there is a VOTER_INCOMING node.
func TestTransferLeaseToVoterOutgoingWithIncoming(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
ctx := context.Background()

knobs, ltk := makeReplicationTestKnobs()
// Add a testing knob to allow us to block the change replicas command
// while it is being proposed. When we detect that the change replicas
// command to move n3 to VOTER_OUTGOING has been evaluated, we'll send
// the request to transfer the lease to n3. The hope is that it will
// get past the sanity above latch acquisition prior to change replicas
// command committing.
var scratchRangeID atomic.Value
scratchRangeID.Store(roachpb.RangeID(0))
changeReplicasChan := make(chan chan struct{}, 1)
shouldBlock := func(args kvserverbase.ProposalFilterArgs) bool {
// Block if a ChangeReplicas command is removing a node from our range.
return args.Req.RangeID == scratchRangeID.Load().(roachpb.RangeID) &&
args.Cmd.ReplicatedEvalResult.ChangeReplicas != nil &&
len(args.Cmd.ReplicatedEvalResult.ChangeReplicas.Removed()) > 0
}
blockIfShould := func(args kvserverbase.ProposalFilterArgs) {
if shouldBlock(args) {
ch := make(chan struct{})
changeReplicasChan <- ch
<-ch
}
}
knobs.Store.(*kvserver.StoreTestingKnobs).TestingProposalFilter = func(args kvserverbase.ProposalFilterArgs) *roachpb.Error {
blockIfShould(args)
return nil
}
tc := testcluster.StartTestCluster(t, 4, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{Knobs: knobs},
ReplicationMode: base.ReplicationManual,
})
defer tc.Stopper().Stop(ctx)

scratchStartKey := tc.ScratchRange(t)
desc := tc.AddVotersOrFatal(t, scratchStartKey, tc.Targets(1, 2)...)
scratchRangeID.Store(desc.RangeID)
// Make sure n1 has the lease to start with.
err := tc.Server(0).DB().AdminTransferLease(context.Background(),
scratchStartKey, tc.Target(0).StoreID)
require.NoError(t, err)

// The test proceeds as follows:
//
// - Send an AdminChangeReplicasRequest to remove n3 and add n4
// - Block the step that moves n3 to VOTER_OUTGOING on changeReplicasChan
// - Send an AdminLeaseTransfer to make n3 the leaseholder
// - Try really hard to make sure that the lease transfer at least gets to
// latch acquisition before unblocking the ChangeReplicas.
// - Unblock the ChangeReplicas.
// - Make sure the lease transfer succeeds.

ltk.withStopAfterJointConfig(func() {
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
_, err = tc.Server(0).DB().AdminChangeReplicas(ctx,
scratchStartKey, desc, []roachpb.ReplicationChange{
{ChangeType: roachpb.REMOVE_VOTER, Target: tc.Target(2)},
{ChangeType: roachpb.ADD_VOTER, Target: tc.Target(3)},
})
require.NoError(t, err)
}()
ch := <-changeReplicasChan
wg.Add(1)
go func() {
defer wg.Done()
// Make sure the lease is currently on n1.
desc, err := tc.LookupRange(scratchStartKey)
require.NoError(t, err)
leaseHolder, err := tc.FindRangeLeaseHolder(desc, nil)
require.NoError(t, err)
require.Equal(t, tc.Target(0), leaseHolder,
errors.Errorf("Leaseholder supposed to be on n1."))
// Move the lease to n3, the VOTER_DEMOTING_LEARNER.
err = tc.Server(0).DB().AdminTransferLease(context.Background(),
scratchStartKey, tc.Target(2).StoreID)
require.NoError(t, err)
// Make sure the lease moved to n3.
leaseHolder, err = tc.FindRangeLeaseHolder(desc, nil)
require.NoError(t, err)
require.Equal(t, tc.Target(2), leaseHolder,
errors.Errorf("Leaseholder supposed to be on n3."))
}()
// Try really hard to make sure that our request makes it past the
// sanity check error to the evaluation error.
for i := 0; i < 100; i++ {
runtime.Gosched()
time.Sleep(time.Microsecond)
}
close(ch)
wg.Wait()
})
}

// TestStoreLeaseTransferTimestampCacheRead verifies that the timestamp cache on
Expand Down
19 changes: 18 additions & 1 deletion pkg/roachpb/metadata_replicas.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@ func predVoterFullOrIncoming(rDesc ReplicaDescriptor) bool {
return false
}

func predVoterIncoming(rDesc ReplicaDescriptor) bool {
switch rDesc.GetType() {
case VOTER_INCOMING:
return true
default:
}
return false
}

func predLearner(rDesc ReplicaDescriptor) bool {
return rDesc.GetType() == LEARNER
}
Expand Down Expand Up @@ -152,6 +161,10 @@ func (d ReplicaSet) VoterDescriptors() []ReplicaDescriptor {
return d.FilterToDescriptors(predVoterFullOrIncoming)
}

func (d ReplicaSet) containsVoterIncoming() bool {
return len(d.FilterToDescriptors(predVoterIncoming)) > 0
}

// LearnerDescriptors returns a slice of ReplicaDescriptors corresponding to
// learner replicas in `d`. This may allocate, but it also may return the
// underlying slice as a performance optimization, so it's not safe to modify
Expand Down Expand Up @@ -532,7 +545,11 @@ func CheckCanReceiveLease(wouldbeLeaseholder ReplicaDescriptor, replDescs Replic
repDesc, ok := replDescs.GetReplicaDescriptorByID(wouldbeLeaseholder.ReplicaID)
if !ok {
return errReplicaNotFound
} else if !repDesc.IsVoterNewConfig() {
} else if !(repDesc.IsVoterNewConfig() || (repDesc.IsAnyVoter() && replDescs.
containsVoterIncoming())) {
// We allow an outgoing / demoting voter to receive the lease if there's an
// incoming voter. In this case, when exiting the joint config, we will
// transfer the lease to the incoming voter.
return errReplicaCannotHoldLease
}
return nil
Expand Down

0 comments on commit 723a8a8

Please sign in to comment.