test(network): graceful cleanup for more consistent CI runs #1960
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThis update improves the cleanup process for validator nodes in the Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/common/testutil/cli/network.go (2 hunks)
Additional comments not posted (4)
x/common/testutil/cli/network.go (4)
632-645: Approve the use of wait group and concurrent goroutines for cleanup.The use of a wait group and concurrent goroutines ensures that all services are stopped before cleanup, improving coordination and reliability.
654-665: Verify the retry mechanism for stopping validators.The retry mechanism with a maximum of 5 retries and 100ms sleep intervals is a good approach. However, ensure that this is sufficient and doesn't introduce any unexpected issues in different environments.
718-725: LGTM!The
ValidatorsStoppedfunction is simple and effective.
727-738: LGTM!The
IsStoppedmethod is straightforward and correctly implemented.
x/common/testutil/cli/network.go
Outdated
| // stopValidatorNode shuts down all services associated with a validator node. | ||
| // | ||
| // It gracefully stops the Tendermint node, API, gRPC, gRPC-Web, and JSON-RPC | ||
| // services. This function is designed to be run concurrently for multiple | ||
| // validators during network cleanup. | ||
| // | ||
| // The function uses graceful shutdown methods where available to allow ongoing | ||
| // operations to complete before terminating. This approach helps prevent | ||
| // resource leaks and ensures a clean shutdown of all components. | ||
| // | ||
| // Parameters: | ||
| // - v: Pointer to the Validator struct containing service references. | ||
| // | ||
| // Note: Errors during shutdown are currently ignored to ensure all services | ||
| // attempt to stop, even if one fails. Consider adding error logging for | ||
| // debugging in production environments. | ||
| func stopValidatorNode(v *Validator) { | ||
| if v.tmNode != nil && v.tmNode.IsRunning() { | ||
| _ = v.tmNode.Stop() | ||
| v.tmNode.Wait() // Wait for the service to fully stop | ||
| } | ||
|
|
||
| if v.api != nil { | ||
| // Close the API server. | ||
| // Any blocked "Accept" operations will be unblocked and return errors. | ||
| _ = v.api.Close() | ||
| } | ||
|
|
||
| if v.grpc != nil { | ||
| // GracefulStop stops the gRPC server gracefully. It stops the server from | ||
| // accepting new connections and RPCs and blocks until all the pending RPCs are | ||
| // finished. | ||
| v.grpc.GracefulStop() | ||
| if v.grpcWeb != nil { | ||
| _ = v.grpcWeb.Close() | ||
| } | ||
| } | ||
|
|
||
| if v.jsonrpc != nil { | ||
| _ = v.jsonrpc.Close() | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Consider adding error logging for debugging.
The stopValidatorNode function currently ignores errors during shutdown. Adding error logging could help with debugging in production environments.
if v.tmNode != nil && v.tmNode.IsRunning() {
if err := v.tmNode.Stop(); err != nil {
log.Errorf("Error stopping tmNode: %v", err)
}
v.tmNode.Wait() // Wait for the service to fully stop
}
Unique-Divine
force-pushed
the
ud/test-ci-graceful
branch
from
cbe89c6
to
486bd34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- x/common/testutil/cli/network.go (2 hunks)
- x/common/testutil/cli/util.go (1 hunks)
- x/evm/keeper/grpc_query_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/common/testutil/cli/network.go
Additional comments not posted (5)
x/common/testutil/cli/util.go (1)
80-80: LGTM! The logger assignment is correctly added.This ensures that the Tendermint node's logger is set, which is crucial for logging and debugging.
x/evm/keeper/grpc_query_test.go (3)
8-8: LGTM! Thestringspackage import is appropriate.The
stringspackage will be useful for the string manipulations in the test functions.
825-831: Verify the root cause of gas value discrepancy.The change to replace "35062" with "35050" in the expected response ensures the test passes. However, it would be beneficial to investigate why the gas values differ to address the root cause.
904-910: Verify the root cause of gas value discrepancy.The change to replace "35062" with "35050" in the expected response ensures the test passes. However, it would be beneficial to investigate why the gas values differ to address the root cause.
CHANGELOG.md (1)
83-83: Entry for PR #1960 looks good.The new entry is clear and concise, providing the necessary context about the change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
==========================================
- Coverage 65.66% 65.65% -0.02%
==========================================
Files 256 257 +1
Lines 16392 16433 +41
==========================================
+ Hits 10764 10789 +25
- Misses 4829 4840 +11
- Partials 799 804 +5
|
Purpose / Abstract
Summary by CodeRabbit
Refactor
testutilclitotestnetworkacross multiple files for better organization and clarity.Cleanupmethod to ensure services stop gracefully before cleanup.New Features
Bug Fixes
Tests