release-23.1: allocator: promote non-voter in 1-voter rebalance

[kvoli]

Backport 1/1 commits from #106202.

/cc @cockroachdb/release

---

When there is only 1 voter and 1 non-voter, it was not possible to rebalance the voter replica onto the store which had the non-voter. When attempting to rebalance with only 1 voter, we fall back to adding the new replica first. Adding this new replica fails if the store already has a non-voting replica.

This patch updates the 1 voter case in ReplicationChangesForRebalance to also consider whether the add target store already has a non-voting replica. If a non-voting replica is found, a promotion is now issued rather than just adding the voter.

Fixes: #104066
Epic: none

Release note: None

---

Release justification: Fixes corner case bug when rebalancing voter to existing non-voter in 1 voter case.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Patches should only be created for serious issues or test-only changes.
• Patches should not break backwards-compatibility.
• Patches should change as little code as possible.
• Patches should not change on-disk formats or node communication protocols.
• Patches should not add new functionality.
• Patches must not add, edit, or otherwise modify cluster versions; or add version gates.

If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

• What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
• Will this work in a cluster of mixed patch versions? Did we test that?
• If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[andrewbaptist]

See the comment above, if it is possible to hit this then it would be a problem.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @kvoli)

---

pkg/kv/kvserver/replicate_queue.go line 1864 at r1 (raw file):

		// types in the range. Check if the add target already has a replica, if so
		// it must be a non-voter or the rebalance is invalid.
		if found && rdesc.Type == roachpb.NON_VOTER {

I didn't review this on master, and this might be the same issue here. There are other cases where found == true but the type is not NON_VOTER. For instance if it was in a learner state. The example of getting here is

1. Hit this code path and try adding a learner.
2. Crash before the snapshot is sent
3. Restart and get into this same code path. In this case the Type will be LEARNER and found will be true.
4. Incorrectly hit the assertion below (and stay stuck in this situation forever).

I'm not sure if there is some other reason why this won't happen, but I think this is possible. One reason might be that we would go down the path of either promoting or removing the learner before ever getting here, but I'm not sure...

---

pkg/kv/kvserver/replicate_queue.go line 1935 at r1 (raw file):

	}
	return chgs, performingSwap, nil

nit: Don't add the extra blank line.

[kvoli]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @erikgrinaker)

---

pkg/kv/kvserver/replicate_queue.go line 1864 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

I didn't review this on master, and this might be the same issue here. There are other cases where found == true but the type is not NON_VOTER. For instance if it was in a learner state. The example of getting here is

1. Hit this code path and try adding a learner.
2. Crash before the snapshot is sent
3. Restart and get into this same code path. In this case the Type will be LEARNER and found will be true.
4. Incorrectly hit the assertion below (and stay stuck in this situation forever).

I'm not sure if there is some other reason why this won't happen, but I think this is possible. One reason might be that we would go down the path of either promoting or removing the learner before ever getting here, but I'm not sure...

We could never hit this case more than once (unless we are unaware of the learner). The reason is because the replicate queue would call ComputeAction and see the learner and return AllocatorRemoveLearner:

cockroach/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

Line 883 in 3cae059

if learners := desc.Replicas().LearnerDescriptors(); len(learners) > 0 {

I don't think this is a risk to worry about.

---

pkg/kv/kvserver/replicate_queue.go line 1935 at r1 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

nit: Don't add the extra blank line.

Removed.

[andrewbaptist]

[kvoli]

TYFTR

Github CI [DEPRECATED] failing during lint, appears safe to ignore here as it shows up on most other 23.1 backports I checked.