kvserver: fix up/downreplication status of non-voters in range report

[aayushshah15]

This commit updates the calcRangeCounter() method to consider non-voting
replicas when determining whether a range is over or under-replicated.
Without this change, ranges that have any non-voting replicas would show
up as under-replicated on the DB console range report page.

Release justification: necessary for observability into non-voting replicas

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

---

pkg/kv/kvserver/allocator.go, line 371 at r1 (raw file):

	}
	if need < 0 {
		return 0

minor nit: need = 0 to straighten out control flow and give this a comment like // Must be non-negative.

Or:

return max(
    min(
        zoneConfigNonVoterCount, 
        clusterNodes - numVoters, // cannot place on nodes with voters
    ), 
    0, // must be non-negative
)

---

pkg/kv/kvserver/replica_metrics.go, line 209 at r1 (raw file):

// replica is determined by checking its node in the provided liveness map.
func calcLiveNonVoterReplicas(desc *roachpb.RangeDescriptor, livenessMap liveness.IsLiveMap) int {
	var live int

Could we eliminate the duplication here with a helper:

func calcLiveVoterReplicas(desc *roachpb.RangeDescriptor, livenessMap liveness.IsLiveMap) int {
    return calcLiveReplicas(desc.Replicas().VoterDescriptors(), livenessMap)
}

func calcLiveNonVoterReplicas(desc *roachpb.RangeDescriptor, livenessMap liveness.IsLiveMap) int {
    ...
}

func calcLiveReplicas(replicas []roachpb.ReplicaDescriptor, livenessMap liveness.IsLiveMap) int {
	var live int
	for _, rd := range replicas {
		if livenessMap[rd.NodeID].IsLive {
			live++
		}
	}
	return live
}

---

pkg/kv/kvserver/replica_metrics_test.go, line 63 at r1 (raw file):

	{
		ctr, down, under, over := calcRangeCounter(11, desc, liveness.IsLiveMap{

Do we need tests where non-voting replicas result in underreplication and overreplication?

[aayushshah15]

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/allocator.go, line 371 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

minor nit: need = 0 to straighten out control flow and give this a comment like // Must be non-negative.

Or:

return max(
    min(
        zoneConfigNonVoterCount, 
        clusterNodes - numVoters, // cannot place on nodes with voters
    ), 
    0, // must be non-negative
)

done.

---

pkg/kv/kvserver/replica_metrics.go, line 209 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could we eliminate the duplication here with a helper:

func calcLiveVoterReplicas(desc *roachpb.RangeDescriptor, livenessMap liveness.IsLiveMap) int {
    return calcLiveReplicas(desc.Replicas().VoterDescriptors(), livenessMap)
}

func calcLiveNonVoterReplicas(desc *roachpb.RangeDescriptor, livenessMap liveness.IsLiveMap) int {
    ...
}

func calcLiveReplicas(replicas []roachpb.ReplicaDescriptor, livenessMap liveness.IsLiveMap) int {
	var live int
	for _, rd := range replicas {
		if livenessMap[rd.NodeID].IsLive {
			live++
		}
	}
	return live
}

done.

---

pkg/kv/kvserver/replica_metrics_test.go, line 63 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we need tests where non-voting replicas result in underreplication and overreplication?

done.

[craig]

Build failed:

• GitHub CI (Cockroach)

[aayushshah15]

It was missing a release justification.

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)