cardano-testnet: Add --nodes flag for per-node binary configuration#6559
cardano-testnet: Add --nodes flag for per-node binary configuration#6559palas wants to merge 13 commits into
--nodes flag for per-node binary configuration#6559Conversation
f0bcf8d to
88a49cf
Compare
--nodes flag for per-node binary configuration
9ca8af4 to
77fa7d8
Compare
88a49cf to
450f24b
Compare
carbolymer
left a comment
There was a problem hiding this comment.
Exception handling is most important here
def6fd7 to
393ad15
Compare
450f24b to
6530ec1
Compare
393ad15 to
eb5f40e
Compare
6530ec1 to
8bb24b5
Compare
744a4ed to
68aa58b
Compare
Co-authored-by: Mateusz Galazyn <228866+carbolymer@users.noreply.github.com>
ebdfb5a to
0bbd001
Compare
There was a problem hiding this comment.
Pull request overview
Adds per-node configuration support to cardano-testnet via a new --nodes CLI flag, allowing each node to be designated as SPO/relay and optionally pinned to a specific cardano-node binary (recorded in the generated env and reused when starting from --node-env).
Changes:
- Introduces
--nodes SPEC[,SPEC...]parsing (with role ordering validation and optionalnode-bin=path) and updates CLI help golden files. - Extends testnet node configuration types to carry an optional per-node binary path, and persists that choice into
node-data/nodeN/env. - Updates node startup to optionally use a custom binary via a new
procCustomhelper and astartNodesignature change.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Node/Shutdown.hs | Updates test to new node options type (NodeWithOptions). |
| cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Gov/ProposeNewConstitutionSPO.hs | Updates test to new node options type (NodeWithOptions). |
| cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/LeadershipSchedule.hs | Updates test node creation + startNode call to pass optional custom binary. |
| cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/KesPeriodInfo.hs | Updates startNode call to pass optional custom binary. |
| cardano-testnet/test/cardano-testnet-golden/files/golden/help/create-env.cli | Updates golden help output to document --nodes. |
| cardano-testnet/test/cardano-testnet-golden/files/golden/help/cardano.cli | Updates golden help output to document --nodes. |
| cardano-testnet/test/cardano-testnet-golden/files/golden/help.cli | Updates top-level golden help usage to show --nodes alternative. |
| cardano-testnet/src/Testnet/Start/Types.hs | Replaces exported node option types with NodeWithOptions/TestnetNodesWithOptions and adds nodeBin. |
| cardano-testnet/src/Testnet/Start/Cardano.hs | Writes/reads per-node env YAML for custom binaries; threads nodeBin into node startup. |
| cardano-testnet/src/Testnet/Runtime.hs | Extends startNode to accept an optional custom node binary. |
| cardano-testnet/src/Testnet/Process/RunIO.hs | Adds procCustom helper for running an explicitly provided binary path. |
| cardano-testnet/src/Parsers/Run.hs | Uses new readNodesWithOptionsFromEnv reader. |
| cardano-testnet/src/Parsers/Cardano.hs | Adds --nodes flag and Parsec-based parseNodeSpecs implementation. |
| cardano-testnet/src/Cardano/Testnet.hs | Updates re-exports to the renamed node options API. |
| cardano-testnet/changelog.d/20260506_035740_palas_testnet_specify_node_bin_per_node.md | Adds changelog entry for --nodes. |
| cardano-testnet/cardano-testnet.cabal | Adds parsec dependency for --nodes parsing. |
| cardano-node/src/Cardano/Node/Testnet/Paths.hs | Adds defaultNodeEnvFile path helper. |
| cardano-node-chairman/test/Spec/Chairman/Cardano.hs | Updates chairman test to use the new default node options value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| , UpdateTimestamps(..) | ||
| , TestnetOnChainParams(..) | ||
| , mainnetParamsRequest | ||
| , TestnetNodeOptions(..) | ||
| , NodeOptions(..) | ||
| , cardanoDefaultTestnetNodeOptions | ||
| , TestnetNodesWithOptions(..) | ||
| , NodeWithOptions(..) | ||
| , cardanoDefaultTestnetNodesWithOptions |
| pNodes = OA.option readNodeSpecs | ||
| ( OA.long "nodes" | ||
| <> OA.help "Comma-separated node specifications. SPO nodes must come before relay nodes. \ | ||
| \Each spec is a role (spo or relay) optionally followed by :node-bin=<path>. \ |
There was a problem hiding this comment.
This needs to exist in documentation somewhere it can't only be in the help text.
| specs <- nodeSpec `sepBy1` char ',' | ||
| let (spos, relays) = span (\(role, _) -> role == Spo) specs | ||
| unless (all (\(role, _) -> role == Relay) relays) $ | ||
| fail "SPO nodes must come before relay nodes. Example: --nodes spo,spo,relay,relay" |
There was a problem hiding this comment.
Why? Does the order actually matter or is it a matter of the presence of SPOs? Can you also separate the parsing and validation logic here?
There was a problem hiding this comment.
We have several options, but I thought this was the least problematic one. For example, we could:
- Allow random orderings: but that would affect a lot of code that assumes SPOs go before relays, this wasn't a problem before because you couldn't specify, also we would need to store a mapping between pools and nodes, which currently are just folders numbered with consecutive numbers starting in 1.
- Sort them before generating the config: but I think that would be misleading. For example, if someone writes
relay,relay,spo,relay, we could silently donode1=spo, node2=relay, node3=relay, node4=relay, but I think that would break the users expectations. - Complain when spos are not the first nodes: this forces the user to reorder and avoids the false expectation. Also, it allows us to not have to modify the rest of the code. And it has another very good advantage, we can easily enforce that there is at least one spo by just using a pair (spo=
NEL NodeWithOptions, relay=[NodeWithOptions]). If we don't ensure that SPO nodes go first, this becomes tricker.
So I thought the last one (complaining) was the best approach.
About parsing versus validation: I can do that, but I think it is a bad idea, because:
- As it is, it is impossible to "forget" to validate, and that is very good. In fact, I already forgot validating in one place last week and copilot caught it.
- The types already enforce what is being validated, so we would need to introduce a new intermediate type that doesn't enforce it for it to be returned by the parser.
Also it goes against the idea of "parse, don't validate".
71406e5 to
375d6a9
Compare
Description
Adds a
--nodesCLI flag tocardano-testnetthat allows specifying the role (SPO or relay) and optionally a customcardano-nodebinary for each node in the testnet. This enables running testnets with mixed node versions, which is needed for testing compatibility across node releases.The new flag coexists with the existing
--num-pool-nodesvia<|>in the parser, so either can be used:At creation time, custom binaries are validated by running
--versionand the result is recorded in a YAML env file (node-data/nodeN/env). At runtime, the env file is read to resolve the binary, falling back to the default resolution when no env file exists.Base PR: #6563 (enforces SPOs come first and splits node list into SPO and relay)
Checklist
See Running tests for more details
CHANGELOG.mdfor affected package.cabalfiles are updatedhlint. See.github/workflows/check-hlint.ymlto get thehlintversionstylish-haskell. See.github/workflows/stylish-haskell.ymlto get thestylish-haskellversionghc-9.6andghc-9.12