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

[tmpnet] Enable single node networks #3003

Merged
merged 5 commits into from
May 21, 2024
Merged

Conversation

marun
Copy link
Contributor

@marun marun commented May 8, 2024

Why this should be merged

Single-node networks can be useful for development.

How this works

Allow the number of nodes to be configured with --node-count to the e2e suite and tmpnetctl and ensure --sybil-protection-enabled=false is configured when a network is started with a single node.

How this was tested

CI

@marun marun self-assigned this May 8, 2024
@marun marun requested review from abi87 and gyuho as code owners May 8, 2024 01:16
@marun marun added the testing This primarily focuses on testing label May 8, 2024
tests/fixture/tmpnet/node.go Show resolved Hide resolved
Comment on lines -41 to -42
nodes, err := tmpnet.NewNodes(tmpnet.DefaultNodeCount)
require.NoError(ginkgo.GinkgoT(), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why panicking instead of using the require infra? No action required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this were only for e2e it makes sense, but I was looking to support antithesis testing as well as e2e and we're not using assertions because failing fast isn't the pattern. The process running the tests needs to run continuously for an arbitrary length of time and error logging is the signal that something is wrong.

Copy link
Contributor Author

@marun marun May 14, 2024

Choose a reason for hiding this comment

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

Hmm, I think I'm confusing things with that mention of 'failing fast'. That's the reason to not use require, but it doesn't explain why I'm using panic. I wanted to enable a simpler declarative form for network definition e.g.

network := tmpnet.Network{
    ...
    Nodes: tmpnet.NewNodesOrPanic(tmpnet.DefaultNodeCount),
} 

The counterpoint is that any network with subnets requires associating nodes to subnets so this simple declarative form isn't sufficient.

So maybe this approach is best described as an experiment to see if simpler panic-based test setup helpers are a good idea or not. My thinking is that this approach is compatible so long as we're either going to use these functions at the start of an antithesis or load test, where failure to perform test setup should exit the process, or with a test runner like ginkgo where a panic at any point during test execution will fail the test but not halt execution of subsequent tests.

@StephenButtolph StephenButtolph added this to the v1.11.6 milestone May 21, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue May 21, 2024
Merged via the queue into master with commit 34e7b2f May 21, 2024
19 of 20 checks passed
@StephenButtolph StephenButtolph deleted the tmpnet-single-node branch May 21, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants