storage: smarten GC of orphaned replicas of subsumed ranges

When a range is subsumed, there are two paths by which its replicas can
be cleaned up. The first path is that the subsuming replica, when it
applies the merge trigger, removes the subsumed replica. This is the
common case, as all replicas are collocated when the merge transaction
starts.

The second path is that the subumed replica is later cleaned up by the
replica GC queue. This occurs when the subsuming range is rebalanced
away shortly after the merge and so never applies the merge trigger,
"orphaning" the subsuming replica.

The replica GC queue must be careful to never to GC a replica that could
be subsumed. If it discovers that a merge occurred, it needs to "prove"
that the replica is actually orphaned. It does so by checking whether
the left neighbor's local descriptor matches the meta2 descriptor; if it
does not, the left neighbor is out of date and could possibly still
apply a merge trigger, so the replica cannot be GC'd.

Unfortunately, the replica GC queue tried to be too clever: it assumed
such a proof was not necessary if the store was still a member of the
subsuming range. Concretely, suppose adjacent ranges A and B merge, and
store 2's replica of B is orphaned. When the replica GC queue looks up
B's descriptor in meta2, it will get the descriptor for the combined
range AB instead and correctly infer that a merge occurred. It also
assumed that, because AB is listed as having a replica on store2, that
the merge must be applying soon.

This assumption was wrong. Suppose the merged range AB immediately
splits back into A and B. The replica GC queue, considering store 2's
replica of the new B, will, again, correctly infer that a merge took
place (even though the descriptor it fetches from meta2 will have the
same start and end key as its local descriptor, it will have a new range
ID), but now its assumption that a replica of A must exist on the store
is incorrect! A may have been rebalanced away, in which case we *must*
GC the old copy of B, or the store will never be able to accept a
snapshot for the new copy of B.

This scenario was observed in several real clusters, and easily
reproduces when restoring TPC-C.

The fix is simple: teach the replica GC queue to always perform the
proof when a range has been merged away. Attempting to be clever just to
save one meta2 lookup was a bad idea.

Touches #31409.

Release note: None

pkg/storage/client_merge_test.go

@@ -1668,7 +1668,7 @@ func TestStoreRangeMergeAddReplicaRace(t *testing.T) {
 	}
 }
 
-func TestStoreRangeMergeSlowUnabandonedFollower(t *testing.T) {
+func TestStoreRangeMergeSlowUnabandonedFollower_NoSplit(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	ctx := context.Background()
@@ -1726,6 +1726,79 @@ func TestStoreRangeMergeSlowUnabandonedFollower(t *testing.T) {
 	mtc.transferLease(ctx, lhsDesc.RangeID, 0, 2)
 }
 
+func TestStoreRangeMergeSlowUnabandonedFollower_WithSplit(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	ctx := context.Background()
+	storeCfg := storage.TestStoreConfig(nil)
+	storeCfg.TestingKnobs.DisableReplicateQueue = true
+	mtc := &multiTestContext{storeConfig: &storeCfg}
+	mtc.Start(t, 3)
+	defer mtc.Stop()
+	store0, store2 := mtc.Store(0), mtc.Store(2)
+
+	mtc.replicateRange(roachpb.RangeID(1), 1, 2)
+	lhsDesc, rhsDesc, err := createSplitRanges(ctx, store0)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Wait for store2 to hear about the split.
+	testutils.SucceedsSoon(t, func() error {
+		_, err := store2.GetReplica(rhsDesc.RangeID)
+		return err
+	})
+
+	// Start dropping all Raft traffic to the LHS on store2 so that it won't be
+	// aware that there is a merge in progress.
+	mtc.transport.Listen(store2.Ident.StoreID, &unreliableRaftHandler{
+		rangeID:            lhsDesc.RangeID,
+		RaftMessageHandler: store2,
+	})
+
+	args := adminMergeArgs(lhsDesc.StartKey.AsRawKey())
+	_, pErr := client.SendWrapped(ctx, store0.TestSender(), args)
+	if pErr != nil {
+		t.Fatal(pErr)
+	}
+
+	// Now split the newly merged range splits back out at exactly the same key.
+	// When the replica GC queue looks in meta2 it will find the new RHS range, of
+	// which store2 is a member. Note that store2 does not yet have an initialized
+	// replica for this range, since it would intersect with the old RHS replica.
+	_, newRHSDesc, err := createSplitRanges(ctx, store0)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Remove and GC the LHS replica on store2. This ensures that the RHS replica
+	// on store2 will never be subsumed, because the merge trigger will never be
+	// applied by the LHS.
+	mtc.unreplicateRange(lhsDesc.RangeID, 2)
+	lhsRepl2 := store2.LookupReplica(lhsDesc.StartKey)
+	if err := store2.ManualReplicaGC(lhsRepl2); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := store2.GetReplica(lhsDesc.RangeID); err == nil {
+		t.Fatal("lhs replica not destroyed on store2")
+	}
+
+	// Transfer the lease on the new RHS to store2 and wait for it to apply. This
+	// will force its replica to of the new RHS to become up to date, which
+	// indirectly tests that the replica GC queue cleans up the old RHS replica.
+	mtc.transferLease(ctx, newRHSDesc.RangeID, 0, 2)
+	testutils.SucceedsSoon(t, func() error {
+		rhsRepl, err := store2.GetReplica(newRHSDesc.RangeID)
+		if err != nil {
+			return err
+		}
+		if !rhsRepl.OwnsValidLease(mtc.clock.Now()) {
+			return errors.New("rhs store does not own valid lease for rhs range")
+		}
+		return nil
+	})
+}
+
 func TestStoreRangeMergeSlowAbandonedFollower(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 

pkg/storage/replica_gc_queue.go

@@ -237,28 +237,18 @@ func (rgcq *replicaGCQueue) process(
 		}); err != nil {
 			return err
 		}
-	} else if currentMember {
-		// This store is a current member of a different range that overlaps with
-		// this one. This situation can only happen when we are a current member
-		// of the range that subsumed this one, but our replica of the subsuming
-		// range has not yet applied the merge trigger. This replica must be
-		// preserved so it can be subsumed.
-		if log.V(1) {
-			log.Infof(ctx, "range merged away; allowing merge trigger on LHS to subsume it")
-		}
 	} else {
-		// This case is tricky. This range has been merged away, and this store is
-		// not a member of the current range. It is likely that we can GC this
-		// replica, but we need to be careful. If this store has a replica of the
-		// subsuming range that has not yet applied the merge trigger, we must not
-		// GC this replica.
+		// This case is tricky. This range has been merged away, so it is likely
+		// that we can GC this replica, but we need to be careful. If this store has
+		// a replica of the subsuming range that has not yet applied the merge
+		// trigger, we must not GC this replica.
 		//
 		// We can't just ask our local left neighbor whether it has an unapplied
 		// merge, as if it's a slow follower it might not have learned about the
-		// merge yet! What we can do, though, is check whether the generation of
-		// our local left neighbor matches the generation of its meta2 descriptor.
-		// If it is generationally up-to-date, it has applied all splits and
-		// merges, and it is thus safe to remove this replica.
+		// merge yet! What we can do, though, is check whether the generation of our
+		// local left neighbor matches the generation of its meta2 descriptor. If it
+		// is generationally up-to-date, it has applied all splits and merges, and
+		// it is thus safe to remove this replica.
 		leftRepl := repl.store.lookupPrecedingReplica(desc.StartKey)
 		if leftRepl != nil {
 			leftDesc := leftRepl.Desc()
@@ -272,7 +262,8 @@ func (rgcq *replicaGCQueue) process(
 			}
 			if leftReplyDesc := rs[0]; !leftDesc.Equal(leftReplyDesc) {
 				if log.V(1) {
-					log.Infof(ctx, "left neighbor not up-to-date; cannot safely GC range yet")
+					log.Infof(ctx, "left neighbor %s not up-to-date with meta descriptor %s; cannot safely GC range yet",
+						leftDesc, leftReplyDesc)
 				}
 				return nil
 			}