kvserver: teach allocator to prescribe non-voter addition/removal #59389

aayushshah15 · 2021-01-25T18:36:29Z

This PR adds basic logic in the allocator to spit out actions that
prescribe upreplication or downreplication of non-voting replicas based
on zone configs.

Note that this commit does not enable the replicateQueue or the
StoreRebalancer to actually act on these newly added
AllocatorActions, not does it make the allocator smart enough to
prescribe rebalancing non-voting replicas. This commit also does not
teach the allocator to rank non-voting replicas for addition or removal.

Release note: None

cockroach-teamcity · 2021-01-25T18:36:50Z

This change is

nvanbenschoten

Reviewed 3 of 3 files at r2, 6 of 6 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @andreimatei)

pkg/kv/kvserver/allocator.go, line 52 at r3 (raw file):

	// Priorities for various repair operations.
	// TODO DURING REVIEW: Do these priorities look sane? Note that these

This comment was helpful. I never knew that this was the only use of these priorities. Could you add to the comment to indicate this.

Now that I know that, I'm left wondering whether the magnitude of these priorities matter, or whether only their relative order matters. For instance, is it meaningful that addDecommissioningReplacementVoterPriority has 5x the priority of removeDeadVoterPriority? Would addDecommissioningReplacementVoterPriority float64 = 1001 have the exact same behavior?

pkg/kv/kvserver/allocator.go, line 52 at r3 (raw file):

	// Priorities for various repair operations.
	// TODO DURING REVIEW: Do these priorities look sane? Note that these

Yes, these priorities make sense to me.

pkg/kv/kvserver/allocator.go, line 159 at r3 (raw file):

type allocatorError struct {
	constraints []zonepb.ConstraintsConjunction
	// TODO(aayush): !!! This might need to be augmented.

What did you decide here?

pkg/kv/kvserver/allocator.go, line 317 at r3 (raw file):

// available for up-replication.
//
// If `num_voters` aren't explicitly configured in the zone config, all replicas

"all num_replicas replicas"

pkg/kv/kvserver/allocator.go, line 323 at r3 (raw file):

// this method should only be consulted after voting replicas have been
// upreplicated / rebalanced off of dead/decommissioning nodes.
func GetNeededNonVoters(numVoters, zoneConfigNumNonVoters, clusterNodes int) int {

nit: we have zoneConfigVoterCount above and zoneConfigNumNonVoters here. Let's be uniform.

pkg/kv/kvserver/allocator.go, line 411 at r3 (raw file):

	quorum := computeQuorum(haveVoters)

	// TODO(aayush): When haveVoters < neededVoters but we don't have quorum to

Any idea why we don't already do this? The code structure makes me wonder if it's intentional.

pkg/kv/kvserver/allocator.go, line 425 at r3 (raw file):

	}

	liveVoterReplicas, deadVoterReplicas := a.storePool.liveAndDeadReplicas(voterReplicas)

nit: should we rename this to liveVoters and deadVoters to fit the rest of the variable naming in this function?

pkg/kv/kvserver/allocator.go, line 460 at r3 (raw file):

	// Non-voting replica addition.
	//liveNonVoters, deadNonVoters := a.storePool.liveAndDeadReplicas(nonVoterReplicas)

Do we need to handle the if haveNonVoters == neededNonVoters && len(deadNonVoters) > 0 { case? In fact, do we need a AllocatorReplaceDeadNonVoter action? Does that fall into your future change to rebalance non-voting replicas?

pkg/kv/kvserver/allocator.go, line 465 at r3 (raw file):

	if haveNonVoters < neededNonVoters {
		priority := addMissingNonVoterPriority
		action := AllocatorAddNonVoter

Isn't this supposed to be a lower priority than AllocatorRemoveDeadVoter? Will that work if we have this check before the if len(deadVoterReplicas) > 0 { check?

This TODO was added in 2016 and the surrounding structure of the method has entirely changed in the intervening period. It doesn't make sense anymore as we do indeed handle heterogenous sets of replica constraints now. Release note: None

This commit renames the existing `AllocatorAction`s and priorities to indicate that they pertain only to voting replicas. This is intended to pave the way for new non-voter specific allocator actions and priorities that follow in the subsequent commits in this PR. Release note: None

aayushshah15

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @andreimatei, and @nvanbenschoten)

pkg/kv/kvserver/allocator.go, line 52 at r3 (raw file):

I'm left wondering whether the magnitude of these priorities matter

The difference between addMissingVoterPriority and addDecommissioningReplacementVoterPriority makes sense to me because we modulate the priority of adding missing voters based on how far we are from our expected voter count. But for the rest I don't think the magnitude matters.

pkg/kv/kvserver/allocator.go, line 159 at r3 (raw file):