allocator: select a good enough store for decom/recovery #86267

lidorcarmel · 2022-08-16T22:09:05Z

Until now, when decommissioning a node, or when recovering from a dead
node, the allocator tries to pick one of the best possible stores as
the target for the recovery.

Because of that, we sometimes see multiple stores recover replicas
to the same store, for example, when decommissioning a node and
at the same time adding a new node.

This PR changes the way we select a destination store by choosing
a random store out of all the stores that are "good enough" for
the replica. The risk diversity is still enforced, but we may
recover a replica to a store that is considered "over full", for
example.

Note that during upreplication the allocator will still try to use
one of the "best" stores as targets.

Fixes: #86265

Release note: None

Release justification: a relatively small change, and it can be
reverted by setting kv.allocator.recovery_store_selector=best.

cockroach-teamcity · 2022-08-16T22:09:13Z

This change is

kvoli

Curious if you have run any benchmarks for the change - I think we should validate in in roachprod but otherwise LGTM.

Reviewed 3 of 3 files at r1, 6 of 7 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @AlexTalks, @kvoli, and @lidorcarmel)

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer_test.go line 230 at r2 (raw file):

	allocRand := makeAllocatorRand(rand.NewSource(0))
	for ii, tc := range testCases {
		t.Logf("ii=%d", ii)

What thist.LogF is for?

lidorcarmel

yep, see the issue #86265
thanks!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @AlexTalks, and @kvoli)

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer_test.go line 230 at r2 (raw file):

Previously, kvoli (Austen) wrote…

What thist.LogF is for?

oops, leftovers.
removed.

kvoli · 2022-08-18T02:38:53Z

Nice results!

AlexTalks

Had some comments about the setting name but this looks overall really good to me!

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15, @AlexTalks, @kvoli, and @lidorcarmel)

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 98 at r3 (raw file):

var enableRecoverToGoodEnoughStores = settings.RegisterBoolSetting(
	settings.SystemOnly,
	"kv.allocator.recover_to_good_enough_stores.enabled",

nit: does this setting (and name) make sense? perhaps we should have a setting that is something like:
kv.allocator.recovery_store_selector where the options are BEST, GOOD, ANY or something? I worry about the "good enough" terminology a bit, but if this makes sense to all I won't block on this of course.

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 897 at r3 (raw file):

// selectWorst randomly chooses one of the worst candidate stores from a sorted
// (by score reversed) candidate list using the provided random generator.
func (cl candidateList) selectWorst(randGen allocatorRand) *candidate {

FYI - just curious, what is this used for?

aayushshah15

Apologies for my delays.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @AlexTalks, @kvoli, and @lidorcarmel)

pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 714 at r3 (raw file):

				nil,
				nil,
				Dead, // Dead and Decommissioning should behave the same here

nit: can we randomly assign Dead or Decommissioning here to assert that they do indeed behave the same (just to prevent future regressions here).

In preparation for adding a new selection function for a good enough candidate, rename the existing "good" to "best". Release note: None

Until now, when decommissioning a node, or when recovering from a dead node, the allocator tries to pick one of the best possible stores as the target for the recovery. Because of that, we sometimes see multiple stores recover replicas to the same store, for example, when decommissioning a node and at the same time adding a new node. This PR changes the way we select a destination store by choosing a random store out of all the stores that are "good enough" for the replica. The risk diversity is still enforced, but we may recover a replica to a store that is considered "over full", for example. Note that during upreplication the allocator will still try to use one of the "best" stores as targets. Fixes: cockroachdb#86265 Release note: None Release justification: a relatively small change, and it can be reverted by setting kv.allocator.recovery_store_selector=best.

lidorcarmel

np at all, thanks both! PTAL.

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

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 98 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

nit: does this setting (and name) make sense? perhaps we should have a setting that is something like:
kv.allocator.recovery_store_selector where the options are BEST, GOOD, ANY or something? I worry about the "good enough" terminology a bit, but if this makes sense to all I won't block on this of course.

Done.
I'm not worried about naming here because this setting should not be used (unless we really broke something), and I'm not sure we will have other strategies for selecting a destination store so I thought a bool is good enough.. but either way works (or, both names are good enough 😄 )

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 897 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

FYI - just curious, what is this used for?

when removing a replica we want to remove the worst.

pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 714 at r3 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

nit: can we randomly assign Dead or Decommissioning here to assert that they do indeed behave the same (just to prevent future regressions here).

Done (alternate the 2 instead of random).

aayushshah15

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

AlexTalks

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

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 98 at r3 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

Done.
I'm not worried about naming here because this setting should not be used (unless we really broke something), and I'm not sure we will have other strategies for selecting a destination store so I thought a bool is good enough.. but either way works (or, both names are good enough 😄 )

LGTM

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 897 at r3 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

when removing a replica we want to remove the worst.

OK, got it!

lidorcarmel

Thanks all for the review!
bors r+

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

craig · 2022-08-25T20:42:30Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-25T21:44:18Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-08-26T03:50:56Z

Build succeeded:

Bazel Essential CI (Cockroach)

lidorcarmel force-pushed the lidor_allocation_to_good_enough branch from 2ccbb7f to 3c30e03 Compare August 16, 2022 22:21

lidorcarmel requested review from kvoli, aayushshah15 and AlexTalks August 16, 2022 22:33

lidorcarmel mentioned this pull request Aug 16, 2022

Recover replicas to a "good enough" store instead of the "best" store #86265

Closed

lidorcarmel marked this pull request as ready for review August 16, 2022 23:23

lidorcarmel requested a review from a team as a code owner August 16, 2022 23:23

kvoli approved these changes Aug 17, 2022

View reviewed changes

lidorcarmel force-pushed the lidor_allocation_to_good_enough branch from 3c30e03 to 16d4269 Compare August 17, 2022 21:41

lidorcarmel commented Aug 17, 2022

View reviewed changes

AlexTalks reviewed Aug 19, 2022

View reviewed changes

aayushshah15 approved these changes Aug 23, 2022

View reviewed changes

kvoli added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Aug 24, 2022

lidorcarmel added 2 commits August 24, 2022 14:10

allocator: rename candidate selection function

091c13b

In preparation for adding a new selection function for a good enough candidate, rename the existing "good" to "best". Release note: None

lidorcarmel force-pushed the lidor_allocation_to_good_enough branch from 16d4269 to 3ce0fd5 Compare August 24, 2022 22:07

lidorcarmel commented Aug 24, 2022

View reviewed changes

aayushshah15 approved these changes Aug 24, 2022

View reviewed changes

AlexTalks approved these changes Aug 25, 2022

View reviewed changes

lidorcarmel commented Aug 25, 2022

View reviewed changes

craig bot merged commit 3688055 into cockroachdb:master Aug 26, 2022

lidorcarmel deleted the lidor_allocation_to_good_enough branch August 26, 2022 03:52

lidorcarmel mentioned this pull request Sep 14, 2022

kvserver: don't transfer leases to new nodes until upreplicated #87847

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allocator: select a good enough store for decom/recovery #86267

allocator: select a good enough store for decom/recovery #86267

lidorcarmel commented Aug 16, 2022 •

edited

Loading

cockroach-teamcity commented Aug 16, 2022

kvoli left a comment

lidorcarmel left a comment

kvoli commented Aug 18, 2022

AlexTalks left a comment •

edited

Loading

aayushshah15 left a comment

lidorcarmel left a comment

aayushshah15 left a comment

AlexTalks left a comment

lidorcarmel left a comment

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 26, 2022

allocator: select a good enough store for decom/recovery #86267

allocator: select a good enough store for decom/recovery #86267

Conversation

lidorcarmel commented Aug 16, 2022 • edited Loading

cockroach-teamcity commented Aug 16, 2022

kvoli left a comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

kvoli commented Aug 18, 2022

AlexTalks left a comment • edited Loading

Choose a reason for hiding this comment

aayushshah15 left a comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

aayushshah15 left a comment

Choose a reason for hiding this comment

AlexTalks left a comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

craig bot commented Aug 25, 2022

craig bot commented Aug 25, 2022

craig bot commented Aug 26, 2022

lidorcarmel commented Aug 16, 2022 •

edited

Loading

AlexTalks left a comment •

edited

Loading