storage: issue swaps on AllocatorConsiderRebalance

[tbg]

Change the rebalancing code so that it not only looks up a new replica to
add, but also picks one to remove. Both actions are then given to a
ChangeReplicas invocation which will carry it out atomically as long as
that feature is enabled.

Release note (bug fix): Replicas can now be moved between stores without
entering an intermediate configuration that violates the zone constraints.
Violations may still occur during zone config changes, decommissioning, and
in the presence of dead nodes (NB: the remainder be addressed in a future
change, so merge the corresponding release note)

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 4 of 4 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/storage/allocator.go, line 402 at r2 (raw file):

		// The dead replica(s) will be down-replicated later.
		priority := addDeadReplacementPriority
		log.VEventf(ctx, 3, "AllocatorAdd - replacement for %d dead replicas priority=%.2f",

AllocatorReplaceDead

---

pkg/storage/allocator.go, line 410 at r2 (raw file):

		// Range has decommissioning replica(s), which should be replaced.
		priority := addDecommissioningReplacementPriority
		log.VEventf(ctx, 3, "AllocatorAdd - replacement for %d decommissioning replicas priority=%.2f",

AllocatorReplaceDecommissioning

---

pkg/storage/allocator_test.go, line 5023 at r2 (raw file):

				action, _ := a.ComputeAction(ctx, zone, &desc)
				require.Equal(t, c.expectedAction.String(), action.String())

Why do we need the .String()?

---

pkg/storage/replicate_queue.go, line 344 at r2 (raw file):

		existingReplicas := liveVoterReplicas
		// WIP(tbg): pass a slice of replicas that can be replaced in.
		// In ReplaceDead, the it's the dead replicas, in ReplaceDecommissioning

s/the it's/it's/

---

pkg/storage/replicate_queue.go, line 423 at r5 (raw file):

		ctx,
		repl,
		[]roachpb.ReplicationChange{{Target: newReplica, ChangeType: roachpb.ADD_REPLICA}},

Can we not use roachpb.MakeReplicationChanges here and below?

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

not taking into account the replica we're going to remove

I must be missing how we're ensuring this. findRemoveTarget doesn't mutate existingReplicas, does it?

---

pkg/storage/replicate_queue_test.go, line 140 at r6 (raw file):

	// Query the range log to see if anything unexpected happened. Concretely,
	// we'll make sure that our tracked ranges never had >=3 replicas.

> 3 replicas

[tbg]

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

---

pkg/storage/allocator_test.go, line 5023 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why do we need the .String()?

It was printed as %d for some reason.

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

not taking into account the replica we're going to remove

I must be missing how we're ensuring this. findRemoveTarget doesn't mutate existingReplicas, does it?

We're removing one from existingReplicas and are adding one that is not in existingReplicas, hence they're not the same, so I think the comment is true-- but looking into this I saw that I'm reinventing the wheel:

cockroach/pkg/storage/allocator.go

Lines 708 to 715 in 8d8e8ff

	removeReplica, removeDetails, err := a.simulateRemoveTarget(
	ctx,
	target.store.StoreID,
	zone,
	replicaCandidates,
	replicaCandidates,
	rangeUsageInfo,
	)

I should return both the removal and addition target from `RebalanceTarget.

[nvanbenschoten]

Reviewed 5 of 5 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 4 of 4 files at r11.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

We're removing one from existingReplicas and are adding one that is not in existingReplicas

Yeah, but where are we doing that?

Oh, unless the point is that existingReplicas still contains removeReplica when passed to RebalanceTarget, in which case the "not taking into account the replica we're going to remove" is easy to misinterpret as the opposite of what it's intending to say.

I should return both the removal and addition target from `RebalanceTarget.

You're doing that now?

[tbg]

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

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

You're doing that now?

Yep, done

[nvanbenschoten]

Reviewed 1 of 3 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

pkg/storage/allocator_test.go, line 609 at r12 (raw file):

		{
			var rangeUsageInfo RangeUsageInfo
			target, details, _, ok := a.RebalanceTarget(

Is details in the wrong position?

---

pkg/storage/allocator_test.go, line 855 at r12 (raw file):

	sg.GossipStores(stores, t)
	for i := 0; i < 10; i++ {
		target, details, _, ok := a.RebalanceTarget(

Same question.

---

pkg/storage/allocator_test.go, line 873 at r12 (raw file):

	sg.GossipStores(stores, t)
	for i := 0; i < 10; i++ {
		target, details, _, ok := a.RebalanceTarget(

Mind checking all of these?

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

You're doing that now?

Yep, done

We don't need to transfer the lease away from removeTarget?

---

pkg/storage/replicate_queue.go, line 697 at r12 (raw file):

	// rebalance. Attempt to find a rebalancing target.
	if !rq.store.TestingKnobs().DisableReplicaRebalancing {
		// Then, not taking into account the replica we're going to remove, see

This comment is strange now, since this is the only step.

[tbg]

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

---

pkg/storage/replicate_queue.go, line 717 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We don't need to transfer the lease away from removeTarget?

Good catch, we do. Added.

[nvanbenschoten]

Reviewed 2 of 2 files at r13.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/storage/replicate_queue.go, line 705 at r13 (raw file):

		} else if done, err := rq.maybeTransferLeaseAway(ctx, repl, removeTarget.StoreID, dryRun); err != nil {
			log.VEventf(ctx, 1, "want to remove self, but failed to transfer lease away: %s", err)
		} else if !done {

nit: I'd do this like:

if !ok {
    ...
} else if done, err := rq.maybeTransferLeaseAway(ctx, repl, removeTarget.StoreID, dryRun); err != nil {
    ...
} else if done {
    // Lease is now elsewhere, so we're not in charge any more.
    return false, nil
} else {
    ...
}

This gives you a place to add a comment.

[tbg]

TFTR!

bors r=nvanbenschoten

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

[craig]

Build succeeded

• GitHub CI (Cockroach)