storage/reports: change node liveness considerations

pkg/storage/node_liveness.go

@@ -700,13 +700,15 @@ func (nl *NodeLiveness) GetLiveness(nodeID roachpb.NodeID) (storagepb.Liveness,
 	return nl.getLivenessLocked(nodeID)
 }
 
-// GetLivenessStatusMap generates map from NodeID to LivenessStatus.
-// This includes only node known to gossip. To include all nodes,
-// Callers should consider calling (statusServer).NodesWithLiveness()
-// instead where possible.
+// GetLivenessStatusMap generates map from NodeID to LivenessStatus for all
+// nodes known to gossip. Nodes that haven't pinged their liveness record for
+// more than server.time_until_store_dead are considered dead.
 //
-// GetLivenessStatusMap() includes removed nodes (dead +
-// decommissioned).
+// To include all nodes (including ones not in the gossip network), callers
+// should consider calling (statusServer).NodesWithLiveness() instead where
+// possible.
+//
+// GetLivenessStatusMap() includes removed nodes (dead + decommissioned).
 func (nl *NodeLiveness) GetLivenessStatusMap() map[roachpb.NodeID]storagepb.NodeLivenessStatus {
 	now := nl.clock.PhysicalTime()
 	livenesses := nl.GetLivenesses()

pkg/storage/reports/constraint_stats_report_test.go

@@ -443,7 +443,8 @@ type node struct {
 	locality string
 	stores   []store
 	// dead, if set, indicates that the node is to be considered dead for the
-	// purposes of reports generation.
+	// purposes of reports generation. In production, this corresponds to a node
+	// with an expired liveness record.
 	dead bool
 }
 
@@ -818,12 +819,18 @@ func processSplits(
 	keyScanner keysutil.PrettyScanner, splits []split,
 ) ([]roachpb.RangeDescriptor, error) {
 	ranges := make([]roachpb.RangeDescriptor, len(splits))
+	var lastKey roachpb.Key
 	for i, split := range splits {
 		prettyKey := splits[i].key
 		startKey, err := keyScanner.Scan(split.key)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to parse key: %s", prettyKey)
 		}
+		if lastKey.Compare(startKey) != -1 {
+			return nil, errors.WithStack(errors.Newf(
+				"unexpected lower or duplicate key: %s (prev key: %s)", prettyKey, lastKey))
+		}
+		lastKey = startKey
 		var endKey roachpb.Key
 		if i < len(splits)-1 {
 			prettyKey := splits[i+1].key

pkg/storage/reports/replication_stats_report.go

@@ -395,15 +395,26 @@ func (v *replicationStatsVisitor) countRange(
 	key ZoneKey, replicationFactor int, r *roachpb.RangeDescriptor,
 ) {
 	voters := len(r.Replicas().Voters())
-	underReplicated := replicationFactor > voters
-	overReplicated := replicationFactor < voters
-	var liveNodeCount int
+	var liveVoters int
 	for _, rep := range r.Replicas().Voters() {
 		if v.nodeChecker(rep.NodeID) {
-			liveNodeCount++
+			liveVoters++
 		}
 	}
-	unavailable := liveNodeCount < (len(r.Replicas().Voters())/2 + 1)
+
+	// TODO(andrei): This unavailability determination is naive. We need to take
+	// into account two different quorums when the range is in the joint-consensus
+	// state. See #43836.
+	unavailable := liveVoters < (voters/2 + 1)
+	// TODO(andrei): In the joint-consensus state, this under-replication also
+	// needs to consider the number of live replicas in each quorum. For example,
+	// with 2 VoterFulls, 1 VoterOutgoing, 1 VoterIncoming, if the outgoing voter
+	// is on a dead node, the range should be considered under-replicated.
+	underReplicated := replicationFactor > liveVoters
+	overReplicated := replicationFactor < voters
+	// Note that a range can be under-replicated and over-replicated at the same
+	// time if it has many replicas, but sufficiently many of them are on dead
+	// nodes.
 
 	v.report.AddZoneRangeStatus(key, unavailable, underReplicated, overReplicated)
 }

pkg/storage/reports/replication_stats_report_test.go

@@ -283,29 +283,37 @@ func TestReplicationStatsReport(t *testing.T) {
 					},
 				},
 				splits: []split{
+					// No problem.
 					{key: "/Table/t1/pk/100", stores: []int{1, 2, 3}},
 					// Under-replicated.
 					{key: "/Table/t1/pk/101", stores: []int{1}},
+					// Under-replicated.
+					{key: "/Table/t1/pk/102", stores: []int{1, 2}},
+					// Under-replicated because 4 is dead.
+					{key: "/Table/t1/pk/103", stores: []int{1, 2, 4}},
 					// Under-replicated and unavailable.
-					{key: "/Table/t1/pk/101", stores: []int{3}},
+					{key: "/Table/t1/pk/104", stores: []int{3}},
 					// Over-replicated.
-					{key: "/Table/t1/pk/101", stores: []int{1, 2, 3, 4}},
+					{key: "/Table/t1/pk/105", stores: []int{1, 2, 3, 4}},
+					// Under-replicated and over-replicated.
+					{key: "/Table/t1/pk/106", stores: []int{1, 2, 4, 5}},
 				},
 				nodes: []node{
 					{id: 1, stores: []store{{id: 1}}},
 					{id: 2, stores: []store{{id: 2}}},
-					{id: 3, stores: []store{{id: 3}}, dead: true},
-					{id: 4, stores: []store{{id: 4}}},
+					{id: 3, stores: []store{{id: 3}}},
+					{id: 4, stores: []store{{id: 4}}, dead: true},
+					{id: 5, stores: []store{{id: 3}}, dead: true},
 				},
 			},
 			exp: []replicationStatsEntry{
 				{
 					object: "t1.p1",
 					zoneRangeStatus: zoneRangeStatus{
-						numRanges:       4,
+						numRanges:       7,
 						unavailable:     1,
-						underReplicated: 2,
-						overReplicated:  1,
+						underReplicated: 5,
+						overReplicated:  2,
 					},
 				},
 			},

pkg/storage/reports/reporter.go

@@ -27,7 +27,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -57,8 +56,6 @@ type Reporter struct {
 	meta1LeaseHolder *storage.Store
 	// Latest zone config
 	latestConfig *config.SystemConfig
-	// Node liveness by store id
-	nodeLiveStatus map[roachpb.NodeID]storagepb.NodeLivenessStatus
 
 	db        *client.DB
 	liveness  *storage.NodeLiveness
@@ -177,8 +174,6 @@ func (stats *Reporter) update(
 		return nil
 	}
 
-	stats.updateNodeLiveness()
-
 	if err := stats.updateLocalityConstraints(); err != nil {
 		log.Errorf(ctx, "unable to update the locality constraints: %s", err)
 	}
@@ -199,14 +194,19 @@ func (stats *Reporter) update(
 		return storeDescs
 	}
 
+	isLiveMap := stats.liveness.GetIsLiveMap()
+	isNodeLive := func(nodeID roachpb.NodeID) bool {
+		return isLiveMap[nodeID].IsLive
+	}
+
 	// Create the visitors that we're going to pass to visitRanges() below.
 	constraintConfVisitor := makeConstraintConformanceVisitor(
 		ctx, stats.latestConfig, getStoresFromGossip, constraintsSaver)
 	localityStatsVisitor := makeLocalityStatsVisitor(
 		ctx, stats.localityConstraints, stats.latestConfig,
-		getStoresFromGossip, stats.isNodeLive, locSaver)
+		getStoresFromGossip, isNodeLive, locSaver)
 	statusVisitor := makeReplicationStatsVisitor(
-		ctx, stats.latestConfig, stats.isNodeLive, replStatsSaver)
+		ctx, stats.latestConfig, isNodeLive, replStatsSaver)
 
 	// Iterate through all the ranges.
 	const descriptorReadBatchSize = 10000
@@ -290,27 +290,9 @@ func (stats *Reporter) updateLocalityConstraints() error {
 	return nil
 }
 
-func (stats *Reporter) updateNodeLiveness() {
-	stats.nodeLiveStatus = stats.liveness.GetLivenessStatusMap()
-}
-
 // nodeChecker checks whether a node is to be considered alive or not.
 type nodeChecker func(nodeID roachpb.NodeID) bool
 
-func (stats *Reporter) isNodeLive(nodeID roachpb.NodeID) bool {
-	l, ok := stats.nodeLiveStatus[nodeID]
-	if !ok {
-		return false
-	}
-	switch l {
-	// Decommissioning nodes are considered live nodes.
-	case storagepb.NodeLivenessStatus_LIVE, storagepb.NodeLivenessStatus_DECOMMISSIONING:
-		return true
-	default:
-		return false
-	}
-}
-
 // zoneResolver resolves ranges to their zone configs. It is optimized for the
 // case where a range falls in the same range as a the previously-resolved range
 // (which is the common case when asked to resolve ranges in key order).