asim: enable random zone config event generation #110967

wenyihu6 · 2023-09-20T14:22:06Z

Previously, zone config event generation used hardcoded span configurations.
This limits our ability to test the allocator more thoroughly.

To improve this, this patch enables random span configs to be generated and
applied as part of the simulation. These configurations are generated by
randomly selecting the primary region, region V.S. zone survival goal, and
leaseholder preference.

The following command is now supported:
"rand_events" [cycle_via_random_survival_goals]

Part of: #106192
Release Note: none
Epic: none

blathers-crl · 2023-09-20T14:22:12Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

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

cockroach-teamcity · 2023-09-20T14:22:16Z

This change is

blathers-crl · 2023-09-20T14:23:03Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

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

blathers-crl · 2023-09-20T14:30:20Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

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

blathers-crl · 2023-09-20T15:01:55Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

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

blathers-crl · 2023-09-21T13:06:16Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

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

kvoli

Reviewed 9 of 9 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

pkg/kv/kvserver/asim/tests/rand_gen.go line 344 at r1 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

Is this better?

Yup, just a minor nit: "... two hardcoded zone configurations." plural.

pkg/sql/region_util.go line 132 at r1 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

I couldn't think of an easy way to do this since zoneConfigForMultiRegionDatabase and DefaultZoneConfig both creates a new struct. I could think of these options: 1. Create a method to combine two structs 2. Create an extension method similar to zoneConfigForMultiRegionDatabase that would extend from DefaultZoneConfig rather than creating a new one 3. Create a new method that takes in a ZoneConfig and returns the hydrated version of it after filling in the required fields. Option 3 seems most suitable. But if making a zone config hydrated is only useful here, the current approach seems fine

Perhaps a middle ground between 1 (combining the structs), and the current approach could work:

zc, err := zoneConfigForMultiRegionDatabase(regionConfig)
defaultZoneConfig := zonepb.DefaultZoneConfig()
zc.RangeMinBytes = defaultZoneConfig.RangeMinBytes
zc.RangeMaxBytes = defaultZoneConfig.RangeMaxBytes
zc.GC = defaultZoneConfig.GC
zc.NullVoterConstraintsIsEmpty = true
return zc, err

My concern is with too many magic numbers floating around makes it harder for others quickly dropping by this code to find the "source of truth", and by extension easier to introduce related bugs in the future.

pkg/kv/kvserver/asim/tests/testdata/rand/rand_event line 88 at r1 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

Yup

Nice, thanks for clarifying.

pkg/kv/kvserver/asim/tests/testdata/rand/rand_event line 134 at r1 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

These lines are because failed assertions output include an extra empty line (in PrintSpanConfigConformanceList). And empty lines emits "----" in datadriven. I opened up a separate PR to get rid of this. #111204

Nice, thanks!

blathers-crl · 2023-09-25T15:57:34Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

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

wenyihu6 · 2023-09-25T15:58:18Z

pkg/sql/region_util.go line 132 at r1 (raw file):

Previously, kvoli (Austen) wrote…

Perhaps a middle ground between 1 (combining the structs), and the current approach could work:
zc, err := zoneConfigForMultiRegionDatabase(regionConfig)
defaultZoneConfig := zonepb.DefaultZoneConfig()
zc.RangeMinBytes = defaultZoneConfig.RangeMinBytes
zc.RangeMaxBytes = defaultZoneConfig.RangeMaxBytes
zc.GC = defaultZoneConfig.GC
zc.NullVoterConstraintsIsEmpty = true
return zc, err
My concern is with too many magic numbers floating around makes it harder for others quickly dropping by this code to find the "source of truth", and by extension easier to introduce related bugs in the future.

I see. That is much neater. Done.

blathers-crl · 2023-09-25T16:40:36Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

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

kvoli

Reviewed 7 of 7 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

Previously, zone config event generation used hardcoded span configurations. This limits our ability to test the allocator more thoroughly. To improve this, this patch enables random span configs to be generated and applied as part of the simulation. These configurations are generated by randomly selecting the primary region, region V.S. zone survival goal, and leaseholder preference. ``` The following command is now supported: "rand_events" [cycle_via_random_survival_goals] ``` Part of: cockroachdb#106192 Release Note: none Epic: none

kvoli · 2023-09-25T19:23:02Z

Rebased on master to pickup #111204.

kvoli · 2023-09-25T21:19:50Z

bors r=kvoli

craig · 2023-09-25T23:16:48Z

Build succeeded:

Bazel Essential CI (Cockroach)

wenyihu6 · 2023-09-25T23:39:38Z

Thanks for the review and helping me merge it ❤️

Now that we have added the option to generate random span configurations in cockroachdb#110967, we want to have a way to check whether these configurations are satisfiable with the cluster setting. This patch adds the validation check. Please note that the validation process can be expensive with a time complexity of O(max(node count in the cluster, number of replica constraints, number of voter constraints)). To perform this validation and see which span config could lead to failure, please use following command: ``` "eval" [verbose=validate] ``` See also: cockroachdb#110967 Part of: cockroachdb#106192 Release Note: none Epic: none

blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Sep 20, 2023

wenyihu6 force-pushed the seprand branch from 753d49f to 809950c Compare September 20, 2023 14:22

wenyihu6 mentioned this pull request Sep 20, 2023

asim: add zone config satisfiability check #110968

Open

wenyihu6 force-pushed the seprand branch from 809950c to 48e6a5c Compare September 20, 2023 14:30

wenyihu6 force-pushed the seprand branch from 48e6a5c to 26728e7 Compare September 20, 2023 15:01

wenyihu6 marked this pull request as ready for review September 21, 2023 12:28

wenyihu6 requested a review from a team as a code owner September 21, 2023 12:28

wenyihu6 force-pushed the seprand branch from 26728e7 to acccbc5 Compare September 21, 2023 13:06

kvoli reviewed Sep 25, 2023

View reviewed changes

wenyihu6 force-pushed the seprand branch from 2d59706 to 114b051 Compare September 25, 2023 15:57

wenyihu6 requested a review from kvoli September 25, 2023 16:01

wenyihu6 force-pushed the seprand branch from 114b051 to 43ed4b7 Compare September 25, 2023 16:40

yuzefovich removed the X-blathers-untriaged blathers was unable to find an owner label Sep 25, 2023

kvoli approved these changes Sep 25, 2023

View reviewed changes

kvoli force-pushed the seprand branch from 43ed4b7 to 91adebd Compare September 25, 2023 19:22

craig bot merged commit 46cb4e3 into cockroachdb:master Sep 25, 2023
8 checks passed

wenyihu6 deleted the seprand branch September 25, 2023 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asim: enable random zone config event generation #110967

asim: enable random zone config event generation #110967

wenyihu6 commented Sep 20, 2023 •

edited

Loading

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 21, 2023

kvoli left a comment

blathers-crl bot commented Sep 25, 2023

wenyihu6 commented Sep 25, 2023

blathers-crl bot commented Sep 25, 2023

kvoli left a comment

kvoli commented Sep 25, 2023

kvoli commented Sep 25, 2023

craig bot commented Sep 25, 2023

wenyihu6 commented Sep 25, 2023

asim: enable random zone config event generation #110967

asim: enable random zone config event generation #110967

Conversation

wenyihu6 commented Sep 20, 2023 • edited Loading

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 20, 2023

blathers-crl bot commented Sep 21, 2023

kvoli left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Sep 25, 2023

wenyihu6 commented Sep 25, 2023

blathers-crl bot commented Sep 25, 2023

kvoli left a comment

Choose a reason for hiding this comment

kvoli commented Sep 25, 2023

kvoli commented Sep 25, 2023

craig bot commented Sep 25, 2023

wenyihu6 commented Sep 25, 2023

wenyihu6 commented Sep 20, 2023 •

edited

Loading