Consensus node Dockerfile for test framework (BFT-398) #57
Conversation
node/Dockerfile
Outdated
| @@ -0,0 +1,23 @@ | |||
| # Build Stage | |||
| FROM rust:1.72.1 as build | |||
| COPY . /node/ | |||
There was a problem hiding this comment.
this is a bad idea, because node/target contains the local build artifacts which can be enormous.
There was a problem hiding this comment.
it would be nice to have it rebuilt only if the code changes, so perhaps all the docker-related configuration (and makefiles) would be better off outside of node/ dir?
There was a problem hiding this comment.
I'm not entirely sure if I grasp this correctly. I completely agree that copying the entire node crate into Docker could lead to problems. Would moving everything outside the node directory help alleviate this? Currently, the entire crate is built within a Docker image, and the final image only includes the generated binaries. I attempted to compile everything outside Docker and only copy the target directory, but that requires cross-compilation to x86_64 architecture if run on a Mac or another ARM machine. It's technically feasible, but I'm uncertain if we intend to pursue this, considering potential additional dependencies. Let me know if my understanding is accurate or if there's something I might be overlooking.
There was a problem hiding this comment.
I would recommend copying everything EXCEPT the target directory to avoid pulling in local artifacts. Alternatively you can try to pull in only files tracked by git (git ls-tree) but I don't know if that's an easy thing to do.
There was a problem hiding this comment.
moving your docker-related configs outside of the node directory will prevent rebuilding the image when you change the config. Ideally it would be great to have the "cargo build" command split into building dependencies and the local code, in a way that the image layer with dependencies' artifacts is not rebuilt every time you update the local code (I don't know how to do that though without changing the build system, so this is just a thought for the future).
There was a problem hiding this comment.
Okay got it. I added the node/target dir to the .dockerignore file so it should not be cloned inside the containers. Also I moved every docker related file and the makefile to the root of the project and adapted every script and command to work with the new paths. Let me know if I missed something
There was a problem hiding this comment.
Regarding incorporating dependencies and local code in different Docker layers to cache the initial ones, it seems feasible. However, as you mentioned, this would entail altering the structure of the Cargo.toml. My initial thought on achieving this is to include only external dependencies in the Cargo.toml of the node crate. Then, within the Cargo.toml of each distinct crate, build every member of the workspace. This way, we could initially build all the external dependencies to cache them and subsequently build the actual node code. I plan to explore this in a separate pull request to avoid making unnecessary changes for this one.
There was a problem hiding this comment.
fyi node/ is not a crate, but a workspace. Mind that we shouldn't downgrade the quality of our Cargo.toml files when improving the Dockerfiles, as regular rust development experience is more important here. Other than that feel free to experiment if you have some time.
|
|
||
| 1. Generate a file named `addresses.txt` in the root directory of the tools crate, containing node addresses in the format `IP:PORT`, with each address on a separate line. | ||
| 2. Run `make node-configs`. This command will establish a directory named `node-configs` and create a folder for each address listed in the `.txt` file, providing the necessary configuration files for the respective node. |
There was a problem hiding this comment.
It is not totally clear that the Makefile is located in a parent directory, and running make commands from this location would fail.
There was a problem hiding this comment.
You're right, with new changes in the directories and overall structure I forgot to update the README. Now it should be updated including the last changes added from this comments. Let me know if it's more clear now.
node/tools/README.md
Outdated
| # Running a test node | ||
| # Running a test consensus node | ||
|
|
||
| ## Local | ||
|
|
||
| 1. Generate a file named `addresses.txt` in the root directory of the tools crate, containing node addresses in the format `IP:PORT`, with each address on a separate line. |
There was a problem hiding this comment.
I think it would be beneficial to start with a template file which can be seen and just modified.
node/tools/README.md
Outdated
|
|
||
| 1. Generate a file named `addresses.txt` in the root directory of the tools crate, containing node addresses in the format `IP:PORT`, with each address on a separate line. | ||
| 2. Run `make node-configs`. This command will establish a directory named `node-configs` and create a folder for each address listed in the `.txt` file, providing the necessary configuration files for the respective node. | ||
| 3. Execute `make run-node IP=<NODE_IP>`. The default value for this command would be `127.0.0.1:3054`. Note that running this command will take control of the terminal. | ||
| 2. Run `make node_configs`. This command will establish a directory named `node-configs` and create a folder for each address listed in the `.txt` file, providing the necessary configuration files for the respective node. |
There was a problem hiding this comment.
I found this a bit confusing given the header "Running a test consensus node", while both this step and the previous one are intended for running multiple nodes. Perhaps add some clarification about that.
|
|
||
| # Docker commands | ||
|
|
||
| docker_node_configs: |
There was a problem hiding this comment.
This command doesn't work from outside the container, as well as being undocumented (also in the /node/tools/ README file).
There was a problem hiding this comment.
This command shouldn't be important for the user. It is used by Docker in the initial phase of the image building process to generate the configuration. However, I added a comment above the command with this disclaimer.
Makefile
Outdated
| node: | ||
| export RUST_LOG=INFO && cd ${EXECUTABLE_NODE_DIR}/node-configs/${IP} && cargo run -- --database ../../database/${IP} | ||
|
|
||
| node_configs: |
There was a problem hiding this comment.
Maybe nodes_config instead? To clarify that this is not associated with a single node.
There was a problem hiding this comment.
Sounds good, I also changed the generated directory where the configs will be.
| ipv4_address: 172.12.0.10 | ||
| node-2: | ||
| image: consensus-node | ||
| container_name: consensus-node-2 |
There was a problem hiding this comment.
Did you check the scale feature for docker-compose services, for supporting a dynamic set of nodes?
There was a problem hiding this comment.
I checked it, and it was my initial choice. The thing about that feature is that it duplicates the exact same container, and I needed to manually assign IPs to the containers to run the example in a single command where I can write the IPs in the config file, knowing them before running the container. Perhaps I overlooked something, and it's still feasible to use it.
Dockerfile
Outdated
| @@ -0,0 +1,22 @@ | |||
| # Build Stage | |||
| FROM rust:1.72.1 as build | |||
|
|
||
| cd $(hostname -i):3054 | ||
| export RUST_LOG=INFO | ||
| ../executor |
There was a problem hiding this comment.
Seeing this "../" path is a bit confusing, considering it's located at the top-level folder. Perhaps add some comments that this is to be used within the docker container, or put it within the Dockerfile altogether.
There was a problem hiding this comment.
I renamed the file to docker-entrypoint and be more clear about what is its purpose. I added a comment anyways to explain how it is used
| rm -rf ${EXECUTABLE_NODE_DIR}/node-configs | ||
| rm -rf ${EXECUTABLE_NODE_DIR}/database | ||
|
|
||
| clean_docker: |
There was a problem hiding this comment.
What about a command for stopping the container(s), without a full cleanup (which currently also removes the image build)?
| COPY Makefile . | ||
| WORKDIR /node | ||
| RUN apt-get update && apt-get install -y libclang-dev | ||
| RUN cargo build --release |
There was a problem hiding this comment.
Following the discussion you had about caching:
The image build runs line 6 and 7 on every code change, which takes very long time.
I think it's possible to better use layer caching so that at least line 6 won't be repeated (I'm not sure why it does).
As for line 7, as you already mentioned, it should be possible to avoid the reinstalling of dependencies, by only copying Cargo.toml and Cargo.lock files, then create a dummy main.rs (RUN echo "fn main() {println!(\"dummy\")}" > src/main.rs) and do cargo build to cache the dependencies. Then, copy the rest of the source code and build again.
This is not critical for now, but might worth trying.
There was a problem hiding this comment.
Cargo.toml is a workspace manifest, so it would need to be converted to a crate manifest.
There was a problem hiding this comment.
Exactly, that was my initial approach for caching external dependencies but since it works as a workspace manifest all the local code would be necessary to build along them.
brunoffranca
changed the title
What ❔
Add Dockerfile to create a container with a consensus node running in it.
Why ❔
This introduces a Dockerfile for the test framework proposal, enabling the execution of a node in isolation for potential use in future consensus among N nodes in the network. Additionally, a compose file is included for testing purposes, facilitating the simultaneous launch of two nodes that can initiate consensus, each running in a separate container. This file might be replaced in the future with a script that initializes N nodes, allowing for easier management, potentially with Kubernetes or a similar tool.