Merge pull request #11657 from petermattis/pmattis/test-remove-placeholder-race

storage: fix a bug in placeholder locking

Author: petermattis

pkg/storage/client_raft_test.go

@@ -1847,6 +1847,42 @@ func TestRaftRemoveRace(t *testing.T) {
 	}
 }
 
+// TestRemovePlaceholderRace adds and removes a replica repeatedly (similar to
+// TestRaftRemoveRace) in an attempt to stress the locking around replica
+// placeholders.
+func TestRemovePlaceholderRace(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	mtc := startMultiTestContext(t, 3)
+	defer mtc.Stop()
+
+	const rangeID = roachpb.RangeID(1)
+	mtc.replicateRange(rangeID, 1, 2)
+
+	repl, err := mtc.stores[0].GetReplica(rangeID)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	ident := mtc.idents[1]
+	ctx := repl.AnnotateCtx(context.Background())
+
+	for i := 0; i < 100; i++ {
+		for _, action := range []roachpb.ReplicaChangeType{roachpb.REMOVE_REPLICA, roachpb.ADD_REPLICA} {
+			if err := repl.ChangeReplicas(
+				ctx,
+				action,
+				roachpb.ReplicaDescriptor{
+					NodeID:  ident.NodeID,
+					StoreID: ident.StoreID,
+				},
+				repl.Desc(),
+			); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+}
+
 // TestStoreRangeRemoveDead verifies that if a store becomes dead, the
 // ReplicateQueue will notice and remove any replicas on it.
 func TestStoreRangeRemoveDead(t *testing.T) {

pkg/storage/replica.go

@@ -2375,8 +2375,6 @@ func (r *Replica) handleRaftReadyRaftMuLocked(
 			return stats, err
 		}
 
-		// handleRaftReady is called under the processRaftMu lock, so it is
-		// safe to lock the store here.
 		if err := func() error {
 			r.store.mu.Lock()
 			defer r.store.mu.Unlock()

pkg/storage/store.go

@@ -1914,6 +1914,8 @@ func (s *Store) addReplicaInternalLocked(repl *Replica) error {
 	return nil
 }
 
+// addPlaceholderLocked adds the specified placeholder. Requires that Store.mu
+// and Replica.raftMu are held.
 func (s *Store) addPlaceholderLocked(placeholder *ReplicaPlaceholder) error {
 	rangeID := placeholder.Desc().RangeID
 	if exRng := s.mu.replicasByKey.ReplaceOrInsert(placeholder); exRng != nil {
@@ -1928,13 +1930,15 @@ func (s *Store) addPlaceholderLocked(placeholder *ReplicaPlaceholder) error {
 
 // removePlaceholder removes a placeholder for the specified range if it
 // exists, returning true if a placeholder was present and removed and false
-// otherwise.
+// otherwise. Requires that Replica.raftMu is held.
 func (s *Store) removePlaceholder(rngID roachpb.RangeID) bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	return s.removePlaceholderLocked(rngID)
 }
 
+// addPlaceholderLocked adds the specified placeholder. Requires that Store.mu
+// and Replica.raftMu are held.
 func (s *Store) removePlaceholderLocked(rngID roachpb.RangeID) bool {
 	placeholder, ok := s.mu.replicaPlaceholders[rngID]
 	if !ok {
@@ -3242,9 +3246,14 @@ func (s *Store) processReady(rangeID roachpb.RangeID) {
 			// replicasByKey map. While the replica will usually consume the
 			// placeholder itself, that isn't guaranteed and so this invocation
 			// here is crucial (i.e. don't remove it).
+			//
+			// We need to hold raftMu here to prevent removing a placeholder that is
+			// actively being used by Store.processRaftRequest.
+			r.raftMu.Lock()
 			if s.removePlaceholder(r.RangeID) {
 				atomic.AddInt32(&s.counts.droppedPlaceholders, 1)
 			}
+			r.raftMu.Unlock()
 		}
 	}
 }