Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build single-binary networks #510

Closed
Geod24 opened this issue Jan 31, 2020 · 5 comments
Closed

Build single-binary networks #510

Geod24 opened this issue Jan 31, 2020 · 5 comments

Comments

@Geod24
Copy link
Member

@Geod24 Geod24 commented Jan 31, 2020

This is an idea I want to leave here for discussion.

Currently, we have 2 kinds of tests:

  • Unittests, which use localrest to simulate a network.
  • Full fledged network tests, using docker-compose (see tests/system).

However, full-fledged integration tests are quite expensive (require to spawn a container, no sharing of data, etc...). One possibility to test network would be to be able to generate a single binary which exposes a network via the usual Node API, but internally uses LocalRest for networking.

With a sensible CLI, it could also possibly allow to test multiple network configurations (e.g. introducing bad nodes) without recompilation.

@Geod24 Geod24 added this to the 2. Validator milestone Feb 25, 2020
@Geod24

This comment has been minimized.

Copy link
Member Author

@Geod24 Geod24 commented Feb 25, 2020

So currently we have system integration tests (https://github.com/bpfkorea/agora/tree/v0.x.x/tests/system) which spawns 3 docker images of the node, with 3 different configuration files (https://github.com/bpfkorea/agora/tree/v0.x.x/tests/system/node under {0,1,2}/config.yaml), and a runs a test defined here. The full pipeline is run via ci/system_integration_test.d

However, spawning 3 docker images is expensive and won't scale very well. One thing we'd like to do is to have an intermediate between the LocalRest tests we have in source/agora/test and the full-fledge (complete) system integration tests described here.

This can be done by having a special main that creates nodes based on its arguments. All it would need is the ability to read multiple config files, and instantiate multiple Node as a result. A good starting point is to look at this code.

@Geod24

This comment has been minimized.

Copy link
Member Author

@Geod24 Geod24 commented Feb 25, 2020

Also it is probably best to make it another "target", or configuration. This is what we are currently doing with agora-cli:

agora/dub.json

Lines 41 to 71 in c7314c2

"configurations": [
{
"name": "server",
"targetName": "agora",
"mainSourceFile": "source/agora/node/main.d",
"excludedSourceFiles": [ "source/agora/cli/*" ]
},
{
"name": "cli",
"targetName": "agora-cli",
"mainSourceFile": "source/agora/cli/main.d",
"excludedSourceFiles": [
"source/agora/api/Validator.d",
"source/agora/common/Config.d",
"source/agora/network/*",
"source/agora/node/*",
"source/scpp/*",
"source/scpd/*",
"source/agora/consensus/protocol/*"
]
},
{
"name": "unittest",
"targetName": "agora-unittests",
"excludedSourceFiles": [ "source/agora/cli/main.d" ],
"sourceFiles": [
"source/scpp/build/DSizeChecks.o",
"source/scpp/build/DLayoutChecks.o"
]
}
],

@bpalaggi bpalaggi added this to To do in Sprint #12 (2020-02-25 to 2020-03-9) via automation Feb 25, 2020
@MukeunKim MukeunKim moved this from To do to In progress (Max 5) in Sprint #12 (2020-02-25 to 2020-03-9) Feb 25, 2020
@MukeunKim MukeunKim linked a pull request that will close this issue Feb 25, 2020
@MukeunKim

This comment has been minimized.

Copy link
Member

@MukeunKim MukeunKim commented Feb 26, 2020

@Geod24 I am going to use LocalRest to create nodes according to the config file.
Is it correct to use LocalRest?

@Geod24

This comment has been minimized.

Copy link
Member Author

@Geod24 Geod24 commented Feb 26, 2020

When we discussed the topic in our sprint planning, I thought so.
However, using LocalRest would prevent the client talking to any node, so all things considered I don't think it should use it.

@MukeunKim MukeunKim moved this from In progress (Max 5) to Review/Testing (Max 2) in Sprint #12 (2020-02-25 to 2020-03-9) Mar 2, 2020
@Geod24 Geod24 linked a pull request that will close this issue Mar 2, 2020
@MukeunKim MukeunKim removed a link to a pull request Mar 4, 2020
@Geod24 Geod24 closed this Mar 5, 2020
Sprint #12 (2020-02-25 to 2020-03-9) automation moved this from Review/Testing (Max 2) to Done Mar 5, 2020
@Geod24

This comment has been minimized.

Copy link
Member Author

@Geod24 Geod24 commented Mar 5, 2020

Done in #592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.