Problem: testground infra is not easy to setup #1504
Conversation
Solution: - most of the features of testground are not really used by us yet, so for a simple benchmark to get a TPS result, it's possible to do an alternative setup. the alternative implementation setup all the node data files in advance, then run the nodes in containers in a stateless manner, they don't need any coordinations at runtime, so we don't need those infrastructures to support the coordinations, just deploy the testplan image to the cluster like a stateless service.
WalkthroughThe project enhancements encapsulate better organization and efficiency in the testing framework, particularly its blockchain simulation component. Key improvements include reorganized imports, streamlined paths, new and refined functions for node initialization and transaction generation, and the incorporation of stateless mode for simplified cluster setups. These changes significantly improve how nodes are initialized, transactions are generated, and clusters are set up and managed within the Docker environment. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Peer
participant SendTx
participant Stateless
participant Docker
User->>+Main: Initiate Test
Main->>+Docker: Start Docker Containers
Docker-->>-Main: Containers Running
Main->>+Peer: Bootstrap Nodes
Peer->>SendTx: Generate Transactions
SendTx->>Main: Load Generated
User->>+Stateless: Run Stateless Mode
Stateless->>+Peer: Initialize Nodes
Peer->>Stateless: Nodes Initialized
Stateless->>+Main: Nodes Active with config
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Co-authored-by: mmsqe <mavis@crypto.com> Signed-off-by: yihuang <huang@crypto.com>
quit wait for grpc service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
testground/benchmark/poetry.lockis excluded by!**/*.lock
Files selected for processing (7)
- testground/benchmark/benchmark/main.py (3 hunks)
- testground/benchmark/benchmark/peer.py (3 hunks)
- testground/benchmark/benchmark/sendtx.py (2 hunks)
- testground/benchmark/benchmark/stateless.py (1 hunks)
- testground/benchmark/benchmark/utils.py (1 hunks)
- testground/benchmark/compositions/docker-compose.jsonnet (1 hunks)
- testground/benchmark/pyproject.toml (2 hunks)
Files skipped from review due to trivial changes (1)
- testground/benchmark/compositions/docker-compose.jsonnet
Additional context used
Ruff
testground/benchmark/benchmark/main.py
30-30: Use context handler for opening files
(SIM115)
testground/benchmark/benchmark/stateless.py
5-5:
timeimported but unusedRemove unused import:
time(F401)
79-79: Use context handler for opening files
(SIM115)
95-98: Use
contextlib.suppress(subprocess.TimeoutExpired)instead oftry-except-passReplace with
contextlib.suppress(subprocess.TimeoutExpired)(SIM105)
GitHub Check: Lint python
testground/benchmark/benchmark/utils.py
[failure] 98-98:
./testground/benchmark/benchmark/utils.py:98:29: BLK100 Black would make changes.testground/benchmark/benchmark/stateless.py
[failure] 5-5:
./testground/benchmark/benchmark/stateless.py:5:1: F401 'time' imported but unused
[failure] 12-12:
./testground/benchmark/benchmark/stateless.py:12:20: BLK100 Black would make changes.
Additional comments not posted (17)
testground/benchmark/pyproject.toml (1)
17-17: LGTM!The additions of the
firedependency and thestateless-testcasescript are appropriate and align with the project's requirements.Also applies to: 28-28
testground/benchmark/benchmark/main.py (2)
9-10: LGTM!The import of
generate_loadfromsendtxis appropriate and aligns with the functionality needed in theentrypointfunction.
39-39: LGTM!The call to
generate_loadis correctly placed within theentrypointfunction.testground/benchmark/benchmark/sendtx.py (2)
2-2: LGTM!The import of
ThreadPoolExecutorandas_completedis appropriate for handling concurrent operations.
60-71: LGTM! Ensure proper error handling in threaded operations.The
generate_loadfunction is well-structured for generating load with multiple accounts and transactions. Ensure that proper error handling and resource management are in place within the threaded operations.testground/benchmark/benchmark/utils.py (1)
96-99: LGTM!The changes to
export_eth_accountenhance flexibility and usability by allowing additional keyword arguments and setting a default value forkeyring_backend.Tools
GitHub Check: Lint python
[failure] 98-98:
./testground/benchmark/benchmark/utils.py:98:29: BLK100 Black would make changes.testground/benchmark/benchmark/stateless.py (4)
101-112: LGTM!The function is correctly implemented.
114-126: LGTM!The function is correctly implemented.
128-138: LGTM!The function is correctly implemented.
141-146: LGTM!The function is correctly implemented.
testground/benchmark/benchmark/peer.py (7)
6-21: LGTM!The imports and constants are correctly defined.
24-51: LGTM!The function is correctly implemented.
54-94: LGTM!The function is correctly implemented.
97-115: LGTM!The function is correctly implemented.
Line range hint
119-139:
LGTM!The function is correctly implemented.
142-161: LGTM!The function is correctly implemented.
Line range hint
164-174:
LGTM!The function is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testground/benchmark/benchmark/stateless.py (1 hunks)
- testground/benchmark/benchmark/utils.py (1 hunks)
- testground/benchmark/pyproject.toml (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- testground/benchmark/benchmark/utils.py
- testground/benchmark/pyproject.toml
Additional context used
Ruff
testground/benchmark/benchmark/stateless.py
84-84: Use context handler for opening files
(SIM115)
100-103: Use
contextlib.suppress(subprocess.TimeoutExpired)instead oftry-except-passReplace with
contextlib.suppress(subprocess.TimeoutExpired)(SIM105)
Additional comments not posted (7)
testground/benchmark/benchmark/stateless.py (7)
41-41: Consider adding logging instead of print statements.Using a logging framework instead of print statements provides more control over the logging levels and outputs.
- print("init validator", i) + logging.info("init validator %d", i) ... - print("init fullnode", i) + logging.info("init fullnode %d", i) ... - print("prepare genesis") + logging.info("prepare genesis") ... - print("patch genesis") + logging.info("patch genesis")Also applies to: 44-44, 49-49, 53-53
81-85: Use context manager for opening files.Using a context manager ensures that the file is properly closed after its operations are done.
- logfile = home / "node.log" - proc = subprocess.Popen( - [cronosd, "start", "--home", str(home)], - stdout=open(logfile, "ab", buffering=0), - ) + logfile = home / "node.log" + with open(logfile, "ab", buffering=0) as log: + proc = subprocess.Popen( + [cronosd, "start", "--home", str(home)], + stdout=log, + )Tools
Ruff
84-84: Use context handler for opening files
(SIM115)
100-103: Usecontextlib.suppressfor handlingsubprocess.TimeoutExpired.Using
contextlib.suppressmakes the code cleaner and more readable.- try: - proc.wait() - except subprocess.TimeoutExpired: - pass + import contextlib + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait()Tools
Ruff
100-103: Use
contextlib.suppress(subprocess.TimeoutExpired)instead oftry-except-passReplace with
contextlib.suppress(subprocess.TimeoutExpired)(SIM105)
106-116: LGTM!The function
init_node_localis a straightforward wrapper aroundinit_node.
119-130: LGTM!The function
patch_configs_localwrites the genesis file and patches the configuration files.
133-143: LGTM!The function
node_indexdetermines the node index based on environment variables or the hostname.
146-147: LGTM!The function
maininitializes the CLI using the Fire library.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- flake.nix (1 hunks)
- testground/README.md (2 hunks)
- testground/benchmark/compositions/docker-compose.jsonnet (1 hunks)
- testground/benchmark/flake.nix (1 hunks)
Files skipped from review due to trivial changes (1)
- flake.nix
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/compositions/docker-compose.jsonnet
Additional context used
LanguageTool
testground/README.md
[uncategorized] ~76-~76: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...ess Mode To simplify cluster setup, we introduce a stateless mode. ## Generate Data Fil...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
Markdownlint
testground/README.md
72-72: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
73-73: Expected: 1; Actual: 3
Multiple consecutive blank lines(MD012, no-multiple-blanks)
83-83: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
89-89: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
74-74: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
Additional comments not posted (4)
testground/README.md (2)
80-84: LGTM!The instructions for generating data files locally are clear and accurate.
Tools
Markdownlint
83-83: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
88-90: LGTM!The instructions for running the setup in a local Docker environment are clear and accurate.
Tools
Markdownlint
89-89: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
testground/benchmark/flake.nix (2)
72-75: LGTM!The
stateless-testcaseapp configuration is correct and consistent.
Line range hint
1-71:
LGTM!The overall configuration is accurate and consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
testground/benchmark/benchmark/stateless.py (1)
32-61: Consider adding logging instead of print statements.Using a logging framework instead of print statements provides more control over the logging levels and outputs.
- print("init validator", i) + logging.info("init validator %d", i) ... - print("init fullnode", i) + logging.info("init fullnode %d", i) ... - print("prepare genesis") + logging.info("prepare genesis") ... - print("patch genesis") + logging.info("patch genesis")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testground/benchmark/benchmark/main.py (3 hunks)
- testground/benchmark/benchmark/sendtx.py (2 hunks)
- testground/benchmark/benchmark/stateless.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/benchmark/sendtx.py
Additional context used
Ruff
testground/benchmark/benchmark/main.py
28-28: Use context handler for opening files
(SIM115)
testground/benchmark/benchmark/stateless.py
84-84: Use context handler for opening files
(SIM115)
100-103: Use
contextlib.suppress(subprocess.TimeoutExpired)instead oftry-except-passReplace with
contextlib.suppress(subprocess.TimeoutExpired)(SIM105)
Additional comments not posted (10)
testground/benchmark/benchmark/main.py (5)
7-9: Imports look good.The updated imports are necessary for the new functionality.
19-19: Initialize ChainCommand instance correctly.The initialization of the
ChainCommandinstance withCONTAINER_CRONOSD_PATHlooks correct.
37-37: Generate load if not a validator.The logic to generate load if the context is not a validator looks correct.
44-44: Collect output if fullnode leader.The logic to collect output if the context is a fullnode leader looks correct.
Line range hint
60-60:
Main function looks good.The
mainfunction initializes the context and runs the appropriate test case correctly.testground/benchmark/benchmark/stateless.py (5)
1-22: Imports look good.The updated imports are necessary for the functionality in the file.
106-117: Function looks good.The
init_node_localfunction correctly initializes the node with the given parameters.
119-131: Function looks good.The
patch_configs_localfunction correctly patches the configuration files with the given parameters.
133-143: Function looks good.The
node_indexfunction correctly determines the node index based on the given logic.
146-147: Main function looks good.The
mainfunction correctly initializes theCLIclass withfire.Fire.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: yihuang <huang@crypto.com>
This reverts commit 8ecd342.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/README.md (2 hunks)
Additional context used
Markdownlint
testground/README.md
72-72: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
73-73: Expected: 1; Actual: 3
Multiple consecutive blank lines(MD012, no-multiple-blanks)
83-83: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
89-89: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
74-74: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
Additional comments not posted (1)
testground/README.md (1)
76-76: Fix: Correct verb tense.The verb "introduce" should be changed to fit the context better.
- To simplify cluster setup, we introduce a stateless mode. + To simplify cluster setup, we are introducing a stateless mode.Likely invalid or redundant comment.
yihuang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
testground/benchmark/benchmark/stateless.py (1)
32-61: Consider adding logging instead of print statements.Using a logging framework instead of print statements provides more control over the logging levels and outputs.
- print("init validator", i) + logging.info("init validator %d", i) ... - print("init fullnode", i) + logging.info("init fullnode %d", i) ... - print("prepare genesis") + logging.info("prepare genesis") ... - print("patch genesis") + logging.info("patch genesis")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testground/benchmark/benchmark/main.py (3 hunks)
- testground/benchmark/benchmark/sendtx.py (2 hunks)
- testground/benchmark/benchmark/stateless.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/benchmark/sendtx.py
Additional context used
Ruff
testground/benchmark/benchmark/main.py
30-30: Use context handler for opening files
(SIM115)
testground/benchmark/benchmark/stateless.py
84-84: Use context handler for opening files
(SIM115)
Additional comments not posted (9)
testground/benchmark/benchmark/main.py (5)
9-11: Approved: Necessary imports.The imports from
peer.pyandsendtx.pyare necessary for the new functionality.
21-21: Approved: Correct initialization.The
ChainCommandinitialization withCONTAINER_CRONOSD_PATHis correct and necessary for the new functionality.
39-39: Approved: Correct load generation.The
generate_loadfunction is correctly used to generate load with the specified number of accounts and transactions.
Line range hint
50-50:
Approved: Correct context initialization.The
Contextinitialization and test case execution are correct and necessary for the new functionality.
29-29: Use context handler for opening files.To ensure proper resource management and avoid potential file descriptor leaks, use a context handler for opening files.
- stdout=open(logfile, "ab", buffering=0), + with open(logfile, "ab", buffering=0) as log: + proc = subprocess.Popen( + [CONTAINER_CRONOSD_PATH, "start"], + stdout=log, + )Likely invalid or redundant comment.
testground/benchmark/benchmark/stateless.py (4)
1-22: Approved: Necessary imports.The added imports from various modules are necessary for the new functionality.
102-113: Approved: Correct node initialization.The
init_node_localfunction correctly initializes a node with the specified parameters.
115-127: Approved: Correct configuration patching.The
patch_configs_localfunction correctly writes the genesis file and patches the config files.
129-139: Approved: Correct node index retrieval.The
node_indexfunction correctly returns the node index based on the environment or hostname.
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Enhancements
Documentation