Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e: fill schema with many namespaces to span ranges #349

Merged
merged 2 commits into from Dec 30, 2021

Conversation

ecordell
Copy link
Contributor

@ecordell ecordell commented Dec 22, 2021

This PR restores the e2e test suite after #332 broke them. Now that namespaces are read at specific revisions on tuple read/write, it's more difficult to reason about which cockroach nodes will be used for a given operation. This PR accounts for the additional namespace reads in the test.

What the test used to do:

  • write a schema
  • fill with tuples so that tuples span across ranges
  • slow time on a node
  • create tuples until we see the second write hit the slow node, which causes the check to return an incorrect value
  • (enable the mitigations and confirm it works)

What the test does now:

  • writes many schemas so that schemas span ranges
  • find a schema that has a leader on the slow node. this ensures that when the namespaces are read when writing to the slow node that the slow node doesn't need to communicate to other nodes (which would sync their timestamps)
  • fill just the schema we identified with tuples so that the tuples for that schema span multiple ranges
  • the rest of the test is mostly the same, though the network delay was removed in favor of a sleep after the first write, which seems to be more reliable

@ecordell ecordell marked this pull request as draft December 22, 2021 14:59
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Dec 22, 2021
@ecordell ecordell force-pushed the enable-e2e branch 28 times, most recently from 6a4e5a4 to 4e1a7c4 Compare December 23, 2021 21:35
e2e/newenemy/newenemy_test.go Outdated Show resolved Hide resolved
e2e/spice/spicedb.go Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@ type Manager interface {
// ReadNamespaceAndTypes reads a namespace definition, version, and type system and returns it if found.
ReadNamespaceAndTypes(ctx context.Context, nsName string, revision decimal.Decimal) (*v0.NamespaceDefinition, *NamespaceTypeSystem, error)

// Closes the namespace manager, disposing of any resources.
// Close closes the namespace manager, disposing of any resources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to enforce linting rules on the e2e code? Because it's a separate project almost all the tooling doesn't run on this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually in the main module, not e2e

but yeah we should probably run the linters over this too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually in the main module, not e2e

Yep, the fact that this is a lint fix just reminded me that the other files are not being checked.

e2e/newenemy/newenemy_test.go Outdated Show resolved Hide resolved
@ecordell ecordell force-pushed the enable-e2e branch 17 times, most recently from f200428 to c4cc7d9 Compare December 29, 2021 20:58
in order for the newenemy test to work, we need to ensure that the
additional reads for namespaces that have been added for namespace
versioning do not re-sync timestamps across cockroach nodes.

by first filling the datastore with a large number of namespaces, we
can pick one that has a range leader on the slow node, so that future writes to the slow node don't involve reads from other nodes

the core of the test is otherwise the same, though extra machinery for
reasoning about range leaders was added
@ecordell ecordell merged commit 6f66209 into authzed:main Dec 30, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants