kvserver: fix bug that could lead to invalid non-voter rebalance attempts#73670
Conversation
|
This change is |
pkg/kv/kvserver/allocator_test.go
Outdated
| require.Falsef( | ||
| t, ok, "expected no action; got rebalance from s%d to s%d", remove.StoreID, add.StoreID, | ||
| ) | ||
| require.Regexp(t, "it already has a voter", finishAndGetRecording().String()) |
There was a problem hiding this comment.
nit: Maybe add a message "rebalancing failed for a unexpected reason" as we do with ok condition.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)
pkg/kv/kvserver/allocator_scorer.go, line 891 at r1 (raw file):
for _, replOnExemptedStore := range replicasOnExemptedStores { if store.StoreID == replOnExemptedStore.StoreID { log.VEventf(
Let's add a comment that indicates that replicasOnExemptedStores will be empty when considering voter targets, so this logic is only used when ranking candidates for the placement of a non-voting replica.
…mpts A bug was introduced in cockroachdb#61239 which made it such that non-voters could, in certain edge case scenarios, attempt to rebalance to stores that already had a voting replica for the same range. However, this bug wouldn't actually materialize into user observable symptoms because such rebalance decisions were rejected before the `replicateQueue` ever tried to execute them. See: https://github.com/cockroachdb/cockroach/blob/8bcd038ab6b76f6bf5894f2ffbbdbf10108b4c32/pkg/kv/kvserver/replicate_queue.go#L1257-L1271. This commit cleans this up and adds a test demonstrating one such edge case. Release note: None
b71a9cb to
6c829c2
Compare
aayushshah15
left a comment
There was a problem hiding this comment.
Thanks for taking a look.
bors r+
Reviewable status:
pkg/kv/kvserver/allocator_scorer.go, line 891 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Let's add a comment that indicates that
replicasOnExemptedStoreswill be empty when considering voter targets, so this logic is only used when ranking candidates for the placement of a non-voting replica.
done.
pkg/kv/kvserver/allocator_test.go, line 4146 at r1 (raw file):
Previously, aliher1911 (Oleg) wrote…
nit: Maybe add a message "rebalancing failed for a unexpected reason" as we do with ok condition.
done.
|
Build succeeded: |
A bug was introduced in #61239
which made it such that non-voters could, in certain edge case scenarios,
attempt to rebalance to stores that already had a voting replica for the same
range. However, this bug wouldn't actually materialize into user observable
symptoms because such rebalance decisions were rejected before the
replicateQueueever tried to execute them. See:cockroach/pkg/kv/kvserver/replicate_queue.go
Lines 1257 to 1271 in 8bcd038
This commit cleans this up and adds a test demonstrating one such edge case.
Release note: None