Merge cockroachdb#73756

73756: kvserver/loqrecovery: check full key coverage in quorum recovery r=erikgrinaker a=aliher1911

Previously when doing unsafe replica recovery, if some ranges are
missing or represented by stale replicas that were split or merged,
recovery will change cluster to inconsistent state with gaps or
overlaps in keyspace.
This change adds checks for range completeness as well as adds a
preference for replicas with higher range applied index.

Release note: None

Fixes cockroachdb#73662

Co-authored-by: Oleg Afanasyev <[email protected]>

pkg/cli/debug_recover_loss_of_quorum.go

@@ -280,8 +280,9 @@ Discarded live replicas: %d
 `, report.TotalReplicas, len(report.PlannedUpdates), report.DiscardedNonSurvivors)
 	for _, r := range report.PlannedUpdates {
 		_, _ = fmt.Fprintf(stderr, "Recovering range r%d:%s updating replica %s to %s. "+
-			"Discarding replicas: %s\n",
-			r.RangeID, r.StartKey, r.OldReplica, r.Replica, r.DiscardedReplicas)
+			"Discarding available replicas: [%s], discarding dead replicas: [%s].\n",
+			r.RangeID, r.StartKey, r.OldReplica, r.Replica,
+			r.DiscardedAvailableReplicas, r.DiscardedDeadReplicas)
 	}
 
 	deadStoreMsg := fmt.Sprintf("\nDiscovered dead stores from provided files: %s",

pkg/kv/kvserver/loqrecovery/BUILD.bazel

@@ -46,6 +46,7 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/uuid",
         "@com_github_cockroachdb_datadriven//:datadriven",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",
         "@in_gopkg_yaml_v2//:yaml_v2",
         "@io_etcd_go_etcd_raft_v3//raftpb",

pkg/kv/kvserver/loqrecovery/apply.go

@@ -102,7 +102,7 @@ func PrepareUpdateReplicas(
 	}
 
 	if len(missing) > 0 {
-		report.MissingStores = storeListFromSet(missing)
+		report.MissingStores = storeSliceFromSet(missing)
 	}
 	return report, nil
 }
@@ -113,7 +113,7 @@ func applyReplicaUpdate(
 	clock := hlc.NewClock(hlc.UnixNano, 0)
 	report := PrepareReplicaReport{
 		RangeID:  update.RangeID,
-		Replica:  *update.NewReplica,
+		Replica:  update.NewReplica,
 		StartKey: update.StartKey.AsRKey(),
 	}
 

pkg/kv/kvserver/loqrecovery/loqrecoverypb/recovery.go

@@ -60,3 +60,15 @@ func (m ReplicaUpdate) NodeID() roachpb.NodeID {
 func (m ReplicaUpdate) StoreID() roachpb.StoreID {
 	return m.NewReplica.StoreID
 }
+
+// Replica gets replica for the store where this info and range
+// descriptor were collected. Returns err if it can't find replica
+// descriptor for the store it originated from.
+func (m *ReplicaInfo) Replica() (roachpb.ReplicaDescriptor, error) {
+	if d, ok := m.Desc.GetReplicaDescriptor(m.StoreID); ok {
+		return d, nil
+	}
+	return roachpb.ReplicaDescriptor{}, errors.Errorf(
+		"invalid replica info: its own store s%d is not present in descriptor replicas %s",
+		m.StoreID, m.Desc)
+}

pkg/kv/kvserver/loqrecovery/loqrecoverypb/recovery.proto

@@ -50,7 +50,8 @@ message ReplicaUpdate {
   int32 old_replica_id = 3 [(gogoproto.customname) = "OldReplicaID",
     (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.ReplicaID",
     (gogoproto.moretags) = "yaml:\"OldReplicaID\""];
-  roachpb.ReplicaDescriptor new_replica = 4 [(gogoproto.moretags) = "yaml:\"NewReplica\""];
+  roachpb.ReplicaDescriptor new_replica = 4 [(gogoproto.nullable) = false,
+    (gogoproto.moretags) = "yaml:\"NewReplica\""];
   int32 next_replica_id = 5 [(gogoproto.customname) = "NextReplicaID",
     (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.ReplicaID",
     (gogoproto.moretags) = "yaml:\"NextReplicaID\""];

pkg/kv/kvserver/loqrecovery/recovery_env_test.go

@@ -12,8 +12,10 @@ package loqrecovery
 
 import (
 	"context"
+	"fmt"
 	"sort"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -29,12 +31,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/keysutil"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/datadriven"
+	"github.com/cockroachdb/errors"
 	"go.etcd.io/etcd/raft/v3/raftpb"
 	"gopkg.in/yaml.v2"
 )
 
-// Range info used for test data to avoid providing unnecessary fields that are not used in
-// replica removal.
+// Range info used for test data to avoid providing unnecessary fields that are
+// not used in replica removal.
 type testReplicaInfo struct {
 	// Replica location.
 	NodeID  roachpb.NodeID  `yaml:"NodeID"`
@@ -129,7 +132,14 @@ func (e *quorumRecoveryEnv) Handle(t *testing.T, d datadriven.TestData) string {
 		t.Fatalf("%s: unknown command %s", d.Pos, d.Cmd)
 	}
 	if err != nil {
-		return err.Error()
+		// This is a special case of error. Coverage errors provide properly
+		// formatted report as a separate function to better separate processing
+		// from presentation.
+		details := errors.GetAllDetails(err)
+		if len(details) > 0 {
+			return fmt.Sprintf("ERROR: %s", strings.Join(details, "\n"))
+		}
+		return fmt.Sprintf("ERROR: %s", err.Error())
 	}
 	if len(out) > 0 {
 		return out
@@ -160,7 +170,8 @@ func (e *quorumRecoveryEnv) handleReplicationData(t *testing.T, d datadriven.Tes
 		key, desc, replicaState, hardState := buildReplicaDescriptorFromTestData(t, replica)
 
 		eng := e.getOrCreateStore(ctx, t, replica.StoreID, replica.NodeID)
-		if err = storage.MVCCPutProto(ctx, eng, nil, key, clock.Now(), nil /* txn */, &desc); err != nil {
+		if err = storage.MVCCPutProto(ctx, eng, nil, key, clock.Now(), nil, /* txn */
+			&desc); err != nil {
 			t.Fatalf("failed to write range descriptor into store: %v", err)
 		}
 
@@ -209,14 +220,10 @@ func buildReplicaDescriptorFromTestData(
 		DeprecatedGenerationComparable: nil,
 		StickyBit:                      nil,
 	}
-	localReplica, ok := desc.GetReplicaDescriptor(replica.StoreID)
-	if !ok {
-		t.Fatalf("invalid test data descriptor on replica doesn't contain itself")
-	}
 	lease := roachpb.Lease{
 		Start:           clock.Now().Add(5*time.Minute.Nanoseconds(), 0).UnsafeToClockTimestamp(),
 		Expiration:      nil,
-		Replica:         localReplica,
+		Replica:         desc.InternalReplicas[0],
 		ProposedTS:      nil,
 		Epoch:           0,
 		Sequence:        0,
@@ -284,7 +291,8 @@ func (e *quorumRecoveryEnv) getOrCreateStore(
 			StoreID:   storeID,
 		}
 		if err = storage.MVCCPutProto(
-			context.Background(), eng, nil, keys.StoreIdentKey(), hlc.Timestamp{}, nil, &sIdent); err != nil {
+			context.Background(), eng, nil, keys.StoreIdentKey(), hlc.Timestamp{}, nil,
+			&sIdent); err != nil {
 			t.Fatalf("failed to populate test store ident: %v", err)
 		}
 		wrapped.engine = eng
@@ -316,24 +324,26 @@ func (e *quorumRecoveryEnv) groupStoresByNode(
 	t *testing.T, storeIDs []roachpb.StoreID,
 ) map[roachpb.NodeID][]storage.Engine {
 	nodes := make(map[roachpb.NodeID][]storage.Engine)
-	iterateSelectedStores(t, storeIDs, e.stores, func(store storage.Engine, nodeID roachpb.NodeID, storeID roachpb.StoreID) {
-		nodes[nodeID] = append(nodes[nodeID], store)
-	})
+	iterateSelectedStores(t, storeIDs, e.stores,
+		func(store storage.Engine, nodeID roachpb.NodeID, storeID roachpb.StoreID) {
+			nodes[nodeID] = append(nodes[nodeID], store)
+		})
 	return nodes
 }
 
 func (e *quorumRecoveryEnv) groupStoresByNodeStore(
 	t *testing.T, storeIDs []roachpb.StoreID,
 ) map[roachpb.NodeID]map[roachpb.StoreID]storage.Batch {
 	nodes := make(map[roachpb.NodeID]map[roachpb.StoreID]storage.Batch)
-	iterateSelectedStores(t, storeIDs, e.stores, func(store storage.Engine, nodeID roachpb.NodeID, storeID roachpb.StoreID) {
-		nodeStores, ok := nodes[nodeID]
-		if !ok {
-			nodeStores = make(map[roachpb.StoreID]storage.Batch)
-			nodes[nodeID] = nodeStores
-		}
-		nodeStores[storeID] = store.NewBatch()
-	})
+	iterateSelectedStores(t, storeIDs, e.stores,
+		func(store storage.Engine, nodeID roachpb.NodeID, storeID roachpb.StoreID) {
+			nodeStores, ok := nodes[nodeID]
+			if !ok {
+				nodeStores = make(map[roachpb.StoreID]storage.Batch)
+				nodes[nodeID] = nodeStores
+			}
+			nodeStores[storeID] = store.NewBatch()
+		})
 	return nodes
 }
 
@@ -390,10 +400,11 @@ func (e *quorumRecoveryEnv) handleDumpStore(t *testing.T, d datadriven.TestData)
 	for _, storeID := range stores {
 		var descriptorViews []storeDescriptorView
 		store := e.stores[storeID]
-		err := kvserver.IterateRangeDescriptorsFromDisk(ctx, store.engine, func(desc roachpb.RangeDescriptor) error {
-			descriptorViews = append(descriptorViews, descriptorView(desc))
-			return nil
-		})
+		err := kvserver.IterateRangeDescriptorsFromDisk(ctx, store.engine,
+			func(desc roachpb.RangeDescriptor) error {
+				descriptorViews = append(descriptorViews, descriptorView(desc))
+				return nil
+			})
 		if err != nil {
 			t.Fatalf("failed to make a dump of store replica data: %v", err)
 		}

pkg/kv/kvserver/loqrecovery/testdata/invalid_input

@@ -0,0 +1,23 @@
+# Test verifies that if we have replica with incorrect descriptor that doesn't contain its own store replica,
+# we detect that and don't produce bad results or crash.
+replication-data
+- StoreID: 1
+  RangeID: 1
+  StartKey: /Min
+  EndKey: /Max
+  Replicas:  # this replica is bad, it doesn't contain itself in the replicas set
+  - { NodeID: 2, StoreID: 2, ReplicaID: 2}
+  - { NodeID: 3, StoreID: 3, ReplicaID: 3}
+  - { NodeID: 4, StoreID: 4, ReplicaID: 1}
+  RangeAppliedIndex: 10
+  RaftCommittedIndex: 13
+----
+ok
+
+collect-replica-info stores=(1)
+----
+ok
+
+make-plan
+----
+ERROR: invalid replica info: its own store s1 is not present in descriptor replicas r1:/M{in-ax} [(n2,s2):2, (n3,s3):3, (n4,s4):1, next=4, gen=3]