generalize Node.run for invocation from both real node and from ThreadNet tests and refine interface used by real node #2720
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nfrisby
force-pushed
the
nfrisby/generalize-node-run
branch
from
8b3dc4a
to
4f85f26
Compare
|
I integrated the simplifications we discussed. |
nfrisby
force-pushed
the
nfrisby/generalize-node-run
branch
2 times, most recently
from
dee7a16
to
7f1976e
Compare
nfrisby
force-pushed
the
nfrisby/generalize-node-run
branch
from
7f1976e
to
2d2e158
Compare
nfrisby
changed the title
nfrisby
force-pushed
the
nfrisby/generalize-node-run
branch
from
2d2e158
to
5ce77c8
Compare
|
I've updated the PR description and pushed a commit that satisfies everything we discussed. I'm marking this as ready to review. |
Adds the following parameters to the DiIffusionApplications record type, so
that it can be used both for the real diffusion layer and for a mock layer in
the ThreadNet tests.
* addresses: real uses RemoteAddress and LocalAddress, ThreadNet uses
CoreNodeId
* version data: real uses NodeToNodeVersionData and NodeToClientVersionData,
ThreadNet uses UnversionedProtocolData
* monad: real uses IO, ThreadNet uses IOLike m => m
The ThreadNet tests will run a mock diffusion layer. It's simpler than the real diffusion layer, and it's connection/disconnection timeline can be scripted according to each generated " test plan " (eg enabling shrinking to an explicitly static topology).
The ThreadNet tests use a mock filesystem, so .run must not directly use Ouroboros.Consensus.Storage.FS.IO.ioHasFS to initialize the ChainDB. Moreover, the tests use fully separate mock FSs for each internal database, for example to better localize reporting of unclosed file handles. So .run should not even directly organize the internal databases as sibling folders.
Moreover, the ThreadNet tests limit the ChainSync delays by construction instead of dynamically.
The ThreadNet tests uses CoreNodeId whereas the real node uses RemoteAddress (ie SockAddr) and LocalAddress (ie FilePath).
The cardano-node invocation currently uses these functions. We therefore add sufficient corresponding fields to StdRunNodeArgs so that the cardano-node invocation can be simplified: it just passes the data without having to first interpret it as these customisation functions.
It is not an option the real node likely has to override, better to not require them to provide it.
mrBliss
force-pushed
the
nfrisby/generalize-node-run
branch
from
5ce77c8
to
e8c011c
Compare
mrBliss
reviewed
Nov 12, 2020
mrBliss
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 12, 2020
In IntersectMBO/ouroboros-network#2720 we reorganised the arguments to `Node.run` so that the consensus ThreadNet tests can use the same `Node.run` function as the real node while using a custom network layer. In this commit we propagate that change. For people that want to override some arguments that now seem to have disappeared: they have been moved to the `LowLevelRunNodeArgs` record in consensus. See the `stdLowLevelRunNodeArgsIO` and `Node.runWith` functions for constructing and running with custom `LowLevelRunNodeArgs`.
|
I have opened a draft PR in |
mrBliss
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 12, 2020
In IntersectMBO/ouroboros-network#2720 we reorganised the arguments to `Node.run` so that the consensus ThreadNet tests can use the same `Node.run` function as the real node while using a custom network layer. In this commit we propagate that change. For people that want to override some arguments that now seem to have disappeared: they have been moved to the `LowLevelRunNodeArgs` record in consensus. See the `stdLowLevelRunNodeArgsIO` and `Node.runWith` functions for constructing and running with custom `LowLevelRunNodeArgs`.
For consistency.
Left-over from alignment in the distant past.
In `cardano-node`, the default values for node-to-node and node-to-client versions are used. The only reason for having them in `RunNodeArgs` was to make it possible for the network team to test the node with different versions. They can still do this by using `.runWith` and `stdLowLevelRunNodeArgsIO`, and overriding the fields manually.
`RunNodeArgs` contains the `ProtocolInfo`, which already contains the `NetworkMagic`. So don't require it in `StdRunNodeArgs` but derive it from the `RunNodeArgs`. This means we have to pass `RunNodeArgs` to `stdLowLevelRunNodeArgsIO`.
mrBliss
force-pushed
the
nfrisby/generalize-node-run
branch
from
e8c011c
to
df460d0
Compare
|
bors merge |
mrBliss
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 12, 2020
In IntersectMBO/ouroboros-network#2720 we reorganised the arguments to `Node.run` so that the consensus ThreadNet tests can use the same `Node.run` function as the real node while using a custom network layer. In this commit we propagate that change. For people that want to override some arguments that now seem to have disappeared: they have been moved to the `LowLevelRunNodeArgs` record in consensus. See the `stdLowLevelRunNodeArgsIO` and `Node.runWith` functions for constructing and running with custom `LowLevelRunNodeArgs`.
|
Build succeeded: |
iohk-bors bot
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Nov 12, 2020
2080: Adapt to the reorganised Node.run arguments in consensus r=intricate a=mrBliss In IntersectMBO/ouroboros-network#2720 we reorganised the arguments to `Node.run` so that the consensus ThreadNet tests can use the same `Node.run` function as the real node while using a custom network layer. In this commit we propagate that change. For people that want to override some arguments that now seem to have disappeared: they have been moved to the `LowLevelRunNodeArgs` record in consensus. See the `stdLowLevelRunNodeArgsIO` and `Node.runWith` functions for constructing and running with custom `LowLevelRunNodeArgs`. Co-authored-by: Thomas Winant <thomas@well-typed.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes three changes to the
RunNodeArgsinterface.It splits off some arguments of
RunNodeArgsinto a separateLowLevelRunNodeArgsrecord.RunNodeArgsare necessary arguments torun.LowLeveRunNodeArgsare the kinds of arguments that a "power user" such as the tests would use.It introduces a new record
StdRunNodeArgsthat captures the exact fields that the real invocation fromcardano-nodewould like to provide when invokingrun. We also provide a function that mapsStdRunNodeArgstoLowLevelRunNodeArgsby providing defaults, setting typical file paths, etc. This is how we hideLowLevelRunNodeArgsfrom the real invocation incardano-node.It renames the internal
NodeArgstoNodeKernelArgs.