asim: add zone config satisfiability check #110968

wenyihu6 · 2023-09-20T14:25:08Z

asim: correct a typo in ComplexConfig

This patch corrects a typo where "US_East_3" was duplicated. It corrects it to
"US_East_4".

Release Note: none
Epic: none

asim: add zone config satisfiability check

Now that we have added the option to generate random span configurations in
#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: #110967
Part of: #106192
Release Note: none
Epic: none

blathers-crl · 2023-09-20T14:25:13Z

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:

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.

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:25:19Z

This change is

wenyihu6 · 2023-09-20T14:26:05Z

Please only review the last two commits. The first commit is from #110967.

blathers-crl · 2023-09-20T14:32:33Z

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.

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:33:57Z

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.

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:00:05Z

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.

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:00:51Z

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.

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-21T07:45:28Z

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.

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-21T10:04:06Z

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.

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-21T11:03:54Z

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.

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-21T11:09:15Z

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.

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-21T11:10:21Z

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.

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-21T11:15: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:

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.

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-21T12:31:37Z

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-25T23:54:55Z

I expect that the second commit will require a few iterations of review. Let me know if you would like me to pull the first commit into its own PR to get the trivial part in first

blathers-crl · 2023-09-26T09:58:10Z

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.}

blathers-crl · 2023-09-26T10:27:59Z

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.}

blathers-crl · 2023-09-26T10:31:26Z

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.}

blathers-crl · 2023-09-26T10:53:56Z

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.}

blathers-crl · 2023-09-26T10:54:44Z

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.}

blathers-crl · 2023-09-26T11:03:21Z

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.}

blathers-crl · 2023-09-26T11:05:02Z

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

I'm still reviewing the config_validator.go changes. Flushing some comments.

Reviewed 5 of 5 files at r1, 9 of 14 files at r2, 3 of 4 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

pkg/kv/kvserver/asim/validator/config_validator.go line 156 at r3 (raw file):

// Otherwise, it returns error (not `nil`).
func (m *mockAllocator) processConstraintsHelper(
	constraintsConjunction []roachpb.ConstraintsConjunction,

nit: naming this constraintConjunctions would be clearer, because this is a bag of conjunctions.

pkg/kv/kvserver/asim/validator/config_validator.go line 384 at r3 (raw file):

// non-voters are then assigned as needed. If any zones or regions lack
// available nodes for assignment, the constraint is considered as
// unsatisfiable.

This is a great high level summary of the algorithm. Could you expand this further, and explain why this algorithm will work given the limitations (assumptions)?

IIUC the algorithm works because of the structure of region-zone hierarchy. Where there intersection of any two region-region or zone-zone combos is empty, and node's only have a region and zone locality.

Could you think of an example where this wouldn't work, if we allow for intersecting constraint conjunctions? No need to solve this here, but it would be good to write out.

pkg/kv/kvserver/asim/validator/validator_test.go line 172 at r3 (raw file):

			description: "insufficient replicas for region constraint",
			constraint: "num_replicas=28 num_voters=28 " +
				"constraints={'+region=US_East':17,'+region=US_West':2,'+region=EU':10}",

nit: wouldn't this also fail due to sum(constraints) not matching the num_replicas (28 vs 29)? It might help to reduce EU:9 to make the test case focus on just US_East having a constrained num_replicas greater than the stores in number of stores in US_East.

pkg/kv/kvserver/asim/validator/validator_test.go line 233 at r3 (raw file):

		},
	}
	for _, tc := range testCases {

Nice tests!

wenyihu6 · 2023-09-26T15:30:24Z

pkg/kv/kvserver/asim/validator/config_validator.go line 384 at r3 (raw file):

Previously, kvoli (Austen) wrote…

This is a great high level summary of the algorithm. Could you expand this further, and explain why this algorithm will work given the limitations (assumptions)?

IIUC the algorithm works because of the structure of region-zone hierarchy. Where there intersection of any two region-region or zone-zone combos is empty, and node's only have a region and zone locality.

Could you think of an example where this wouldn't work, if we allow for intersecting constraint conjunctions? No need to solve this here, but it would be good to write out.

I'm not sure if I'm understanding this correctly. Could you give me an example on what non-empty region-region combo means and when node has more than one region or zone locality? My understanding was that regions or zones are non-overlapping. And each node have unique region and zone locality. And clusterInfo.Regions would specify regions, zones, and nodes under it.

wenyihu6 · 2023-09-26T15:43:01Z

pkg/kv/kvserver/asim/validator/config_validator.go line 384 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

I'm not sure if I'm understanding this correctly. Could you give me an example on what non-empty region-region combo means and when node has more than one region or zone locality? My understanding was that regions or zones are non-overlapping. And each node have unique region and zone locality. And clusterInfo.Regions would specify regions, zones, and nodes under it.

or do you mean there exists duplicate constraints for the same region value (something like constraints={'+region=US_East':1,'+region=US_East':2})?

I will address the comments once I've finished procrastinating and https://en.wikipedia.org/wiki/BoJack_Horseman

kvoli

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

pkg/kv/kvserver/asim/validator/config_validator.go line 384 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

or do you mean there exists duplicate constraints for the same region value (something like constraints={'+region=US_East':1,'+region=US_East':2})?

Using attributes, which constraints can match on, it is possible, and something we support (begrudgingly at this stage).

e.g. locality=region=a,zone=a1 attributes=disk

This patch corrects a typo where "US_East_3" was duplicated. It corrects it to "US_East_4". Release Note: none Epic: none

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 · 2024-04-20T03:45:44Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

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 seprandverify branch from 19e3e9e to 086b773 Compare September 20, 2023 14:32

wenyihu6 changed the title ~~asim: check zone config satisfiability~~ asim: add zone config satisfiability check Sep 20, 2023

wenyihu6 force-pushed the seprandverify branch from 086b773 to 81801f8 Compare September 20, 2023 14:33

wenyihu6 force-pushed the seprandverify branch from 81801f8 to e0401dc Compare September 20, 2023 14:59

wenyihu6 force-pushed the seprandverify branch from e0401dc to 129b919 Compare September 20, 2023 15:00

wenyihu6 force-pushed the seprandverify branch from 129b919 to 3bb1ece Compare September 21, 2023 07:45

wenyihu6 force-pushed the seprandverify branch from 3bb1ece to e371177 Compare September 21, 2023 10:04

wenyihu6 force-pushed the seprandverify branch from e371177 to 1bcb99b Compare September 21, 2023 11:03

wenyihu6 force-pushed the seprandverify branch from 1bcb99b to cc18873 Compare September 21, 2023 11:09

wenyihu6 force-pushed the seprandverify branch from cc18873 to 5cd6735 Compare September 21, 2023 11:10

wenyihu6 force-pushed the seprandverify branch from 5cd6735 to 6336704 Compare September 21, 2023 11:15

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 seprandverify branch from 6336704 to 30ce904 Compare September 21, 2023 12:31

wenyihu6 force-pushed the seprandverify branch from 28a99fc to 6d71d49 Compare September 26, 2023 09:58

wenyihu6 force-pushed the seprandverify branch from 6d71d49 to b985879 Compare September 26, 2023 10:27

wenyihu6 force-pushed the seprandverify branch from b985879 to 1486ffa Compare September 26, 2023 10:31

wenyihu6 force-pushed the seprandverify branch from 1486ffa to 1910e7d Compare September 26, 2023 10:53

wenyihu6 force-pushed the seprandverify branch from 1910e7d to fb4ee49 Compare September 26, 2023 10:54

wenyihu6 force-pushed the seprandverify branch from fb4ee49 to b91baf8 Compare September 26, 2023 11:03

wenyihu6 force-pushed the seprandverify branch from b91baf8 to 10a5973 Compare September 26, 2023 11:04

kvoli reviewed Sep 26, 2023

View reviewed changes

wenyihu6 and others added 4 commits April 19, 2024 21:11

asim: correct a typo in ComplexConfig

cd0d377

This patch corrects a typo where "US_East_3" was duplicated. It corrects it to "US_East_4". Release Note: none Epic: none

a

60de973

a

d765730

wenyihu6 force-pushed the seprandverify branch from 10a5973 to d765730 Compare April 20, 2024 03:45

wenyihu6 added 3 commits April 20, 2024 00:41

a

67f6a97

a

bb44ef7

a

1afa1de

wenyihu6 force-pushed the seprandverify branch from 8ede669 to 1afa1de Compare April 20, 2024 05:36

kvoli removed their assignment Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asim: add zone config satisfiability check #110968

asim: add zone config satisfiability check #110968

wenyihu6 commented Sep 20, 2023 •

edited

Loading

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

wenyihu6 commented Sep 20, 2023 •

edited

Loading

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 20, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

wenyihu6 commented Sep 25, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

kvoli left a comment

wenyihu6 commented Sep 26, 2023

wenyihu6 commented Sep 26, 2023 •

edited

Loading

kvoli left a comment •

edited

Loading

blathers-crl bot commented Apr 20, 2024

asim: add zone config satisfiability check #110968

Are you sure you want to change the base?

asim: add zone config satisfiability check #110968

Conversation

wenyihu6 commented Sep 20, 2023 • edited Loading

blathers-crl bot commented Sep 20, 2023

cockroach-teamcity commented Sep 20, 2023

wenyihu6 commented Sep 20, 2023 • edited Loading

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 20, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

blathers-crl bot commented Sep 21, 2023

wenyihu6 commented Sep 25, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

blathers-crl bot commented Sep 26, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Sep 26, 2023

wenyihu6 commented Sep 26, 2023 • edited Loading

kvoli left a comment • edited Loading

Choose a reason for hiding this comment

blathers-crl bot commented Apr 20, 2024

wenyihu6 commented Sep 20, 2023 •

edited

Loading

wenyihu6 commented Sep 20, 2023 •

edited

Loading

wenyihu6 commented Sep 26, 2023 •

edited

Loading

kvoli left a comment •

edited

Loading