Maru testing cluster #455
Maru testing cluster #455
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
============================================
- Coverage 83.28% 83.09% -0.20%
- Complexity 987 1084 +97
============================================
Files 212 225 +13
Lines 7258 7920 +662
Branches 516 591 +75
============================================
+ Hits 6045 6581 +536
- Misses 921 1004 +83
- Partials 292 335 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Filter94
left a comment
Filter94
left a comment
There was a problem hiding this comment.
I didn't review all the tests yet
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/MaruCluster.kt
Outdated
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/MaruCluster.kt
Outdated
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/MaruCluster.kt
Outdated
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/genesis/MaruGenesisFactory.kt
Outdated
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/genesis/MaruGenesisFactory.kt
Outdated
Show resolved
Hide resolved
| * SPDX-License-Identifier: MIT OR Apache-2.0 | ||
| */ | ||
| package maru.p2p.testutils | ||
| package maru.test.util |
There was a problem hiding this comment.
This move means that P2P tests have to pull a lot of stuff from Besu, for example org.hyperledger.besu.internal:besu-acceptance-tests-dsl, which they don't need. I think findFreePorts belongs to P2P test fixtures
There was a problem hiding this comment.
It was duplicated in the App tests as well, not just inside P2P. What approach do you propose to avoid duplication?
This move means that P2P tests have to pull a lot of stuff from Besu, for example org.hyperledger.besu.internal:besu-acceptance-tests-dsl
IMO, the P2PTest package would benefit from using the maru cluster directly with a single/fake EL sequencer node that creates fake blocks....
There was a problem hiding this comment.
IMO, the P2PTest package would benefit from using the maru cluster directly
I think this way we're testing a subset of functionality (P2P) with a whole application, which is a more expensive way to test P2P than using P2P directly.
What approach do you propose to avoid duplication?
Since findFreePorts was defined in test fixtures, it should have been importable by implementation(testFixtures(project(":p2p")))
There was a problem hiding this comment.
I think this way we're testing a subset of functionality (P2P) with a whole application, which is a more expensive way to test P2P than using P2P directly.
Define "expensive"
I care about: 1) readability, 2) maintenance, and 3) test runtime
Using this cluster with fake EL IMO will favour 1 and 2, with a negligible hint on 3.
Are you keen on this revert? The reason I am asking is that I foresee the findPorts being useful on something else that does not need the P2P module, and honestly, I think we have other things with much higher priority and tech debt than squeezing perfect modules when we don't reach unanimity;
There was a problem hiding this comment.
Using this cluster with fake EL IMO will favour 1 and 2, with a negligible hint on 3
If you want to use MaruCluster to test P2P, you can as well remove P2P tests from the p2p module and define them tests as integrationTest-s in app module.
If we instantiate the whole Maru stack to test P2P, it negatively impacts readability and maintenance. If it made sense, testing pyramid wouldn't have to be a pyramid, we'd cover everything as integrationTest-s in the app.
First undesirable effect of your suggestion, coming to my mind is that if something in the root of Maru App is broken, which prevents instantiation of Maru, it would fail the current app integrationTests + the tests that are currently inside P2P, for which you suggest to use MaruCluster. I think that's harmful for the P2P module in the long run.
I foresee the findPorts being useful on something else that does not need the P2P module
I will oppose every new usage of findPorts by default, because this pattern feels wrong. In real world, ports are either set to 0 and assigned by the OS or a specific port number reserved for a certain instance ahead of time. And findPorts is trying to do the work that is supposed to be done by the OS.
Are you keen on this revert?
I am, because at the moment I disagree with the design implications it has, which we discussed in this thread
There was a problem hiding this comment.
reverted. Please take a look
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/MaruConfigHelper.kt
Outdated
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/BesuCluster.kt
Outdated
Show resolved
Hide resolved
…sisFactory.kt Co-authored-by: Roman Vaseev <[email protected]> Signed-off-by: Fluent Crafter <[email protected]>
jvm-libs/test-utils/src/test/kotlin/maru/test/genesis/BeaconGenesisFactoryTest.kt
Show resolved
Hide resolved
jvm-libs/test-utils/src/main/kotlin/maru/test/genesis/BesuGenesisFactory.kt
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Directory creation missing for RocksDB data path
The ensureDirectoryExists call for config.persistence.dataPath was removed before database creation. While the function was moved and applied to the private key's parent directory, the persistence data directory is no longer explicitly created. This can cause RocksDB database initialization to fail if the data directory doesn't exist.
app/src/main/kotlin/maru/app/MaruAppFactory.kt#L125-L135
maru/app/src/main/kotlin/maru/app/MaruAppFactory.kt
Lines 125 to 135 in 6fd5e1e
| return when { | ||
| label.contains("sequencer", ignoreCase = true) -> NodeRole.Sequencer | ||
| label.contains("validator", ignoreCase = true) -> NodeRole.Sequencer | ||
| label.contains("bootnode", ignoreCase = true) -> NodeRole.Bootnode |
There was a problem hiding this comment.
I'm more leaning on the previous approach of using label.startsWith because the given label could be in sth like non-sequencer, non-validator, sequencer-bootnode, etc.., as there is no restriction on the label, at least checking the prefix could eliminate some mis-naming, wdyt?
| error = it | ||
| } | ||
| } | ||
| sockets.forEach { runCatching { it.close() } } |
There was a problem hiding this comment.
It should fail if it fails to close the socket, since you do not set reuseaddress=true, if the socket failed to close that port is unusable.
There was a problem hiding this comment.
It was a deliberate choice. My rationale was :
- This is for ephemeral environments, we don't care much if we waste a few ports
- throwing when it fails to close can leave even more ports hanging
reuseaddress=truefrom what read on Java Doc is to allow ServerSocket to bind to a port that is in CLOSING state. I don't see how this influences the close behaviour.
| return forksInfo | ||
| .indexOfFirst { currentTimestamp >= it.forkSpec.timestampSeconds } | ||
| .let { | ||
| // if no matching fork found, it means we are before the first fork, so return the last index |
There was a problem hiding this comment.
Can you add a comment as to why this is a valid condition and why we should not raise exception/
There was a problem hiding this comment.
Maru atm does not officially support that the 1st fork is after current clock time. However, from the forkIdManager pov that's not relevant because peering can happen. Improved the comment.
jvm-libs/test-utils/src/main/kotlin/maru/test/genesis/MaruGenesisFactory.kt
Outdated
Show resolved
Hide resolved
gauravahuja
left a comment
gauravahuja
left a comment
There was a problem hiding this comment.
lgtm, just a few comments
There was a problem hiding this comment.
Bug: Directory creation missing for RocksDB data path
The removal of ensureDirectoryExists(config.persistence.dataPath) before creating the RocksDB database will cause a failure if the parent directory doesn't exist. The change moved directory creation to only happen for the privateKeyPath.parent (line 407), but the dataPath itself still needs its directory structure to exist before RocksDB attempts to create the database. RocksDB typically does not create parent directories automatically, so this will fail when config.persistence.dataPath points to a non-existent directory.
app/src/main/kotlin/maru/app/MaruAppFactory.kt#L126-L135
maru/app/src/main/kotlin/maru/app/MaruAppFactory.kt
Lines 126 to 135 in a2689e7
jvm-libs/test-utils/src/main/kotlin/maru/test/cluster/MaruCluster.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| @Order(1) |
There was a problem hiding this comment.
@fluentcrafter Why the tests need to be run sequentially? Any limitations on running them in parallel? Btw I did try to run them in parallel locally, and they passed
There was a problem hiding this comment.
It's because lack of resources and flakiness on the CI. In parallel, it would be too many Besu's at the same time and they suffer from starvation...
| } | ||
|
|
||
| ensureDirectoryExists(privateKeyPath.parent) | ||
|
|
There was a problem hiding this comment.
Bug: DataPath Directory Not Ensured During Key Setup
The ensureDirectoryExists call has been moved outside the getOrGeneratePrivateKey function and placed inside it. However, the original code at line 127 called config.persistence.dataPath.createDirectories() which ensured the data path directory was created. This line was removed, but the new code only ensures the parent directory of the private key path exists. If config.persistence.dataPath and config.persistence.privateKeyPath.parent are different directories, then config.persistence.dataPath may not be created, which could cause issues when creating the database at line 127-133.
| followerELNodeEngineApiWeb3JClients.forEach { (_, web3jClient) -> web3jClient.eth1Web3j.shutdown() } | ||
| p2pNetwork.close() | ||
| vertx.close() | ||
| protocolStarter.close() |
There was a problem hiding this comment.
Bug: Shutdown order mix-up: protocolStarter after vertx breakage
The order of closing resources has been changed incorrectly. Previously, protocolStarter.close() was called before beaconChain.close(), which is correct because the protocol starter may need to access the beacon chain during shutdown. The new code closes protocolStarter after closing vertx, which could cause issues if the protocol starter needs vertx resources during shutdown. Additionally, the comment "close db last, otherwise other components may fail trying to save data" suggests that beaconChain.close() should indeed be last, but closing protocolStarter after vertx may violate proper shutdown ordering if the protocol starter depends on vertx resources.
Note
Introduce reusable Maru/Besu test cluster libs and genesis builders, add comprehensive tests, and fix P2P discovery port and fork selection with small API/cleanup tweaks.
jvm-libs/test-utils):BesuClusterandMaruClusterfor programmatic node orchestration (start/stop, bootnodes, dynamic add/remove, discovery/static peering).MaruConfigHelper,GenesisFactory,MaruGenesisFactory,BesuGenesisFactory) and bundledbesu-genesis-template.json.NodeBuilder,TestMaruAppFactory.createMaru,BesuFactoryenhancements (naming, discovery, sync config).BesuNode,MaruApp,MaruCluster;NetworkUtilfor free ports.BesuClusterTest,MaruClusterTest, and genesis factory tests.0; exposenodeAddressesvia getter; improve fork selection inForkPeeringManagerwhen first fork is in the future.TestUtils.findFreePortwith newNetworkUtil; parameterize ports.BeaconChain: addgetLatestBeaconBlock()convenience.MaruApp.close(): closeprotocolStarterbefore DB; minor directory/private key handling refactor inMaruAppFactory.kotlin.time.ExperimentalTime; update test-utils dependencies; enable parallel test forks.Written by Cursor Bugbot for commit 47ffb93. This will update automatically on new commits. Configure here.