From f3b5c6202bdd7ed9685ffa14b3523376effd3f51 Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Wed, 13 Dec 2017 16:50:51 -0500 Subject: [PATCH] storage: Fix simulation of rebalance removals to actually remove targets If the first target attempted was rejected due to the simulation claiming that it would be immediately removed, we would reuse the modified `rangeInfo.Desc.Replicas` that had the target added to it, messing with future iterations of the loop. Also, we weren't properly modifying the `candidates` slice, meaning that we could end up trying the same replica multiple times. I have a test for this, but it doesn't pass yet because the code in #18364 actually isn't quite sufficient for fixing cases like #20241. I'll send that out tomorrow once I have a fix done. Release note: None --- pkg/storage/allocator.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/storage/allocator.go b/pkg/storage/allocator.go index f5f9e1842f3a..7ec40770560c 100644 --- a/pkg/storage/allocator.go +++ b/pkg/storage/allocator.go @@ -556,12 +556,14 @@ func (a Allocator) RebalanceTarget( if removeReplica.StoreID != target.store.StoreID { break } - newTargets := candidates.removeCandidate(*target) - newTarget := newTargets.selectGood(a.randGen) - if newTarget == nil { + // Remove the considered target from our modified RangeDescriptor and from + // the candidates list, then try again if there are any other candidates. + rangeInfo.Desc.Replicas = rangeInfo.Desc.Replicas[:len(rangeInfo.Desc.Replicas)-1] + candidates = candidates.removeCandidate(*target) + target = candidates.selectGood(a.randGen) + if target == nil { return nil, "" } - target = newTarget } details, err := json.Marshal(decisionDetails{ Target: target.String(),