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

Consensus node Dockerfile for test framework (BFT-398) #57

Merged
merged 31 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6bd6170
Add dockerfile for executable node
IAvecilla Jan 10, 2024
b2fb9ab
Add compose file for testing purposes
IAvecilla Jan 10, 2024
e768bc6
Add entrypoint for node consensus dockerfile
IAvecilla Jan 12, 2024
e6b6487
Add some comments and improve compose test file
IAvecilla Jan 12, 2024
4ca7fdc
Add new makefile commands to run dockerized consensus node
IAvecilla Jan 12, 2024
28df58b
Update readme with docs to run consensus node in docker
IAvecilla Jan 12, 2024
3e4b881
Delete unnecesary building dependency in compose file
IAvecilla Jan 15, 2024
febcafc
Rename docker image
IAvecilla Jan 15, 2024
b2e7e9c
Set container names manually in compose file
IAvecilla Jan 15, 2024
5e27e6e
Separate config directory for nodes running in docker
IAvecilla Jan 15, 2024
6e2236a
Fix node configuration for docker consensus example
IAvecilla Jan 15, 2024
810a4b5
Improve command to run a node in a container
IAvecilla Jan 15, 2024
4088c0a
Generate the node configs in release mode
IAvecilla Jan 15, 2024
4475489
Fix docker cleanup to force deletion
IAvecilla Jan 15, 2024
689b6d5
Remove unnecesary copies to container
IAvecilla Jan 15, 2024
99dc691
Add target dir to docker ignore
IAvecilla Jan 16, 2024
a3b270d
Move every docker related config file to the project root
IAvecilla Jan 16, 2024
f93890f
Fix typo in README
IAvecilla Jan 17, 2024
f3893cf
Fix typo in makefile command
IAvecilla Jan 17, 2024
1c721c8
Make the path to makefile be the same for local and docker
IAvecilla Jan 17, 2024
8cac863
disabled clipply lint
pompon0 Jan 17, 2024
d24c8bc
fixed lint, updated deps
pompon0 Jan 17, 2024
9520c4c
Change file name for the docker entrypoint and add comment to the script
IAvecilla Jan 18, 2024
54c72ce
Move version to latest for rust image
IAvecilla Jan 18, 2024
13e61e1
Change name of command and dir generation for node config
IAvecilla Jan 18, 2024
3b8d299
Add command to stop dockerized nodes
IAvecilla Jan 18, 2024
2d09c1d
Add example file with local address for node configuration
IAvecilla Jan 18, 2024
c287c56
Update README with new updates
IAvecilla Jan 18, 2024
e66cec9
Write a general overview on the README
IAvecilla Jan 18, 2024
987add2
Merge branch 'main' into standalone_node_dockerfile
brunoffranca Jan 22, 2024
222fca2
Update dependencies.
brunoffranca Jan 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/node/target
22 changes: 22 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Build Stage
FROM rust:1.72.1 as build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rust:latest?

COPY /node/ /node/
COPY Makefile .
WORKDIR /node
RUN apt-get update && apt-get install -y libclang-dev
RUN cargo build --release
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo.toml is a workspace manifest, so it would need to be converted to a crate manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RUN cd .. && make docker_node_configs

# Runtime Stage
FROM debian:stable-slim as runtime

COPY --from=build /node/target/release/executor /node/
COPY --from=build /node/tools/docker-config/node-configs /node/
COPY entrypoint.sh /node/

WORKDIR /node
RUN chmod +x entrypoint.sh

ENTRYPOINT ["./entrypoint.sh"]

EXPOSE 3054
41 changes: 41 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
.PHONY: node node_configs docker_node_configs node_docker consensus_docker_example clean clean_docker
IP?=127.0.0.1:3054
DOCKER_IP=172.12.0.10
EXECUTABLE_NODE_DIR=node/tools

# Locally run commands

node:
export RUST_LOG=INFO && cd ${EXECUTABLE_NODE_DIR}/node-configs/${IP} && cargo run -- --database ../../database/${IP}

node_configs:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe nodes_config instead? To clarify that this is not associated with a single node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I also changed the generated directory where the configs will be.

cd ${EXECUTABLE_NODE_DIR} && cargo run --bin localnet_config -- --input-addrs addresses.txt --output-dir node-configs

# Docker commands

docker_node_configs:
Copy link
Contributor

@moshababo moshababo Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command doesn't work from outside the container, as well as being undocumented (also in the /node/tools/ README file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cd ${EXECUTABLE_NODE_DIR} && cargo run --release --bin localnet_config -- --input-addrs docker-config/addresses.txt --output-dir docker-config/node-configs

node_docker:
mkdir -p ${EXECUTABLE_NODE_DIR}/docker-config
cd ${EXECUTABLE_NODE_DIR}/docker-config && rm -rf addresses.txt && echo ${DOCKER_IP}:3054 >> addresses.txt
docker-compose up -d node-1

consensus_docker_example:
mkdir -p ${EXECUTABLE_NODE_DIR}/docker-config
cd ${EXECUTABLE_NODE_DIR}/docker-config && rm -rf addresses.txt && touch addresses.txt && echo 172.12.0.10:3054 >> addresses.txt && echo 172.12.0.11:3054 >> addresses.txt
docker-compose up -d

# Clean commands

clean: clean_docker
rm -rf ${EXECUTABLE_NODE_DIR}/node-configs
rm -rf ${EXECUTABLE_NODE_DIR}/database

clean_docker:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a command for stopping the container(s), without a full cleanup (which currently also removes the image build)?

rm -rf ${EXECUTABLE_NODE_DIR}/docker-config
docker rm -f consensus-node
docker rm -f consensus-node-1
docker rm -f consensus-node-2
docker network rm -f node-net
docker image rm -f consensus-node
28 changes: 28 additions & 0 deletions compose.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
version: "3.9"

services:
node-1:
build: .
image: consensus-node
container_name: consensus-node-1
networks:
node_net:
# This allow us to know the ip of the node-1 container to fill the address in the config file
# Only for test purposes, may be removed in the future
ipv4_address: 172.12.0.10
node-2:
image: consensus-node
container_name: consensus-node-2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check the scale feature for docker-compose services, for supporting a dynamic set of nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

networks:
node_net:
# This allow us to know the ip of the node-2 container to fill the address in the config file
# Only for test purposes, may be removed in the future
ipv4_address: 172.12.0.11

networks:
node_net:
name: node-net
ipam:
config:
- subnet: "172.12.0.0/24"
gateway: "172.12.0.1"
5 changes: 5 additions & 0 deletions entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/bash

cd $(hostname -i):3054
export RUST_LOG=INFO
../executor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading
Loading