-
Notifications
You must be signed in to change notification settings - Fork 30
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
chore: add docker #114
chore: add docker #114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth moving all the docker-related stuff to a folder called docker
so we can also add docs on how to run it.
Dockerfile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tweaked this a bit based on https://github.com/talaia-labs/rust-teos/blob/master/docker/Dockerfile:
# Use the rust image as the base image for the build stage
FROM rust:latest AS builder
# Copy the rust-teos source code
COPY . /tmp/sim-ln
# Install the dependencies required for building sim-ln
RUN apt-get update \
&& apt-get -y --no-install-recommends install protobuf-compiler musl-tools
RUN cd /tmp/sim-ln \
&& rustup target add x86_64-unknown-linux-musl \
# Rustfmt is needed to format the grpc stubs generated by tonic
&& rustup component add rustfmt \
# Cross compile with musl as the target, so teosd can run on alpine
&& RUSTFLAGS='-C target-feature=+crt-static' cargo build --locked --release --target x86_64-unknown-linux-musl
# Use a new stage with a smaller base image to reduce image size
FROM alpine:latest
RUN apk update && apk upgrade
# UID and GID for the teosd user
ENV SIMLN_UID=1001 SIMLN_GID=1001
# Copy the sim-cli binaries from the build stage to the new stage
COPY --from=builder /tmp/sim-ln/target/x86_64-unknown-linux-musl/release/sim-cli /usr/local/bin/
CMD ["sim-cli", "--config", "config.json"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice this won't work atm given there's still an issue with how the config file is passed to the container. Hoewever, you can test this properly builds and executes the binary if yu change the cmd command to be sim-cli -help
for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it may be worth adding an entrypoint
script to the execution of the cli can be split to add different modifiers, such as the debug log or run time. I'd suggest you check: https://github.com/talaia-labs/rust-teos/tree/master/docker
docker-compose.yml
Outdated
@@ -0,0 +1,9 @@ | |||
version: '3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can live without docker compose? Since the use case that we primarily forsee is simln dropping into other people's existing setups (they'll add it to their docker compose.
For including a volume we can just add instructions to include a volume with docker run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I've reworked the pr and removed compose. Finally got docker to work. turns out the arch of your pc affects the type of musl tool you download to use with protobuf. I am working on an entryscript to pass in the config and other commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be way trickier that I thought to do in a user-friendly way.
Specifically because we have a config because the paths/IPs need to be relative to code executing in the container.
I'd suggest that we add some docs around this outlining:
- That you need to mount a volume when you
docker run
which includes all the "artifacts" you need. - That the paths in
config.json
should be expressed relative to the file structure in the volume.
- That the IP addresses need to be docker IP addresses (not localhost).
Makefile
Outdated
start: | ||
docker run -it -e ${ARGS} sim-ln:tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I'm missing here is the ability to include a volume that contains the certs/macaroons etc that we'll need to read to talk to the node.
If our config provides local filesystem paths then sim-cli tries to read them from within the docker file it won't find them. I think that a simple approach could be to require that the user mounts a single volume that contains the macaroons etc and the config.json and then we look for everything in there.
Pretty brutal to be dealing with these IPs/paths in the docker context.
Makefile
Outdated
@@ -0,0 +1,5 @@ | |||
start-deps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also nit that this should probably be build-docker
and run-docker
below.
Rough ideas: we could have a
Update: @sr-gi, I just saw your #130 changes related to how we pass sim file to the cli. WDYT of this proposal? |
I think it makes sense if you're running a certain scenario more than once, but I'm not sure how that applies to running different ones. How would this work if I want to run with |
User would need to add all |
Hmm.... I'm not sure about this flow. Its a bit tacky to me except If I don't fully grasp it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK.
This is ✨ the nicest ✨ docker setup and docs I have ever encountered. The automated setup of config makes it an absolute joy to work with. Really, really great stuff.
I have a few nits on naming and fixing up documentation which I'd like to get in for the release - specifically making a note that folks can bring their own volume if they don't want to use our script (which will be useful in the case where we don't have everything we need locally, so we can't just copy it into a volume).
Makefile
Outdated
@echo "HELP Set to true to print the help message (default: false) e.g. <make run HELP=true>" | ||
@echo "PRINT_BATCH_SIZE Set the batch size for printing the results (default: 1000) e.g. <make run PRINT_BATCH_SIZE=100>" | ||
|
||
run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: make these docker-run
and docker-stop
, since we may want to use run
for cargo run
.
Makefile
Outdated
@echo "PRINT_BATCH_SIZE Set the batch size for printing the results (default: 1000) e.g. <make run PRINT_BATCH_SIZE=100>" | ||
|
||
run: | ||
docker run --rm --name sim-ln --init -v simln-config:/data -e CONFIG_PATH=/data/config.json -e LOG_LEVEL=$(LOG_LEVEL) -e HELP=${HELP} -e PRINT_BATCH_SIZE=${PRINT_BATCH_SIZE} -e TOTAL_TIME=${TOTAL_TIME} sim-ln |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker run -d
so that this runs fully in the background?
docker/docker.md
Outdated
|
||
## Mounting the Volume | ||
|
||
To ensure that the Docker container can access the configuration and other necessary data, we use Docker volumes. Before you can run the container, you need to set up the volume with the necessary data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"other necessary data" -> "authentication credentials"
docker/docker.md
Outdated
|
||
To ensure that the Docker container can access the configuration and other necessary data, we use Docker volumes. Before you can run the container, you need to set up the volume with the necessary data. | ||
|
||
To do this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it would be helpful to note that we've provided an easy way to do this, but you can also do it yourself (eg in the case where the macaroons etc are on another machine).
Something along the lines of:
- If you're running nodes locally -> you can use this script, it'll copy everything and create a config suitable for the container.
- If you're running nodes remotely, you'll need to provide your own volume and config that points to the files location in the volume.
Can also add a note about IP addresses here.
docker/docker.md
Outdated
- `PRINT_BATCH_SIZE`: Set the batch size for printing the results. | ||
- `TOTAL_TIME`: Adjusts the total time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits on the variable docs:
PRINT_BATCH_SIZE
: determines the number of payment results that will be written to disk at a time.TOTAL_TIME
: the total runtime for the simulation expressed in seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd call this SIM_TIME
.
PS: I just realized this is also called total_time
in the CLI. Should we take the opportunity and rename it?
docker/docker.md
Outdated
## Notes for Developers: | ||
|
||
- When you use the `mount-volume` command, it sets up a Docker volume named `simln-config`. This volume contains your `config.json` file and other necessary files, ensuring that the data is accessible inside the Docker container. | ||
- The paths to certificates, macaroons, etc., in your `config.json` are automatically adjusted to point to their corresponding paths inside the Docker volume. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-emphasize that IP address is not converted while we're here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I had to go look for the host.docker.internal
thing to make it run with polar
docker/entrypoint.sh
Outdated
# Check for a custom configuration path | ||
if [[ ! -z ${CONFIG_PATH} ]]; then | ||
START_COMMAND="$START_COMMAND --config $CONFIG_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase, we removed --config
recently so now it's just sim-cli sim.json
(since it's always required).
Also decided to refer to that file as sim.json
since we may one day want to have a config file for the actual simulation (containing things like log level and print batch etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. I have a few comments, check inline.
Something I'm also wondering: should both .dockerignore
and the Makefile
be moved to the docker
folder? They only make sense within docker, and you may be reading the docker readme from within that folder already, so I may expect to run the commands there too.
Lastly, this needs rebase (aliases are not available atm), and commits should be squashed IMO, given they don't make sense standalone
@echo "Variables:" | ||
@echo "CONFIG_PATH Path to the config.json file." | ||
@echo "LOG_LEVEL Set the logging level (default: info) e.g. <make run LOG_LEVEL=debug>" | ||
@echo "HELP Set to true to print the help message (default: false) e.g. <make run HELP=true>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a flag (and make it lower-case because why not)? i.e. if passed, it is true, otherwise, it is false.
make run help
docker/docker.md
Outdated
- `PRINT_BATCH_SIZE`: Set the batch size for printing the results. | ||
- `TOTAL_TIME`: Adjusts the total time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd call this SIM_TIME
.
PS: I just realized this is also called total_time
in the CLI. Should we take the opportunity and rename it?
Makefile
Outdated
@echo "stop Stops the Docker container." | ||
@echo "" | ||
@echo "Variables:" | ||
@echo "CONFIG_PATH Path to the config.json file." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: SIMFILE_PATH
?
docker/docker.md
Outdated
|
||
Other configurable variables include: | ||
|
||
- `HELP`: Set to `true` to print the help message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this help the makefile help, or sim-cli
help?
If it is the latter, I left a comment regarding making it a flag and lowercase, disregard the lowercase part
docker/docker.md
Outdated
To do this: | ||
|
||
```bash | ||
make mount-volume CONFIG_PATH=/path/to/your/config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a required parameter we shouldn't need to specify the ENV variable. i.e:
make mount-volume /path/to/your/config.json
docker/docker.md
Outdated
## Notes for Developers: | ||
|
||
- When you use the `mount-volume` command, it sets up a Docker volume named `simln-config`. This volume contains your `config.json` file and other necessary files, ensuring that the data is accessible inside the Docker container. | ||
- The paths to certificates, macaroons, etc., in your `config.json` are automatically adjusted to point to their corresponding paths inside the Docker volume. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I had to go look for the host.docker.internal
thing to make it run with polar
I think that it would be nice to have a
No strong feelings about where this one lives. |
4bf762b
to
3bcfe8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 🚀
Thanks for the lightning-fast turnaround on this one 🙏 🙇
Only last ask if to have run-docker
updated to be a background deployment (-d
) if you've got time for one last push. Otherwise let's shipppp it 🐑
Makefile
Outdated
run-docker: | ||
docker run --rm --name sim-ln --init -v simln-data:/data -e SIMFILE_PATH=/data/sim.json -e LOG_LEVEL=$(LOG_LEVEL) -e HELP=${HELP} -e PRINT_BATCH_SIZE=${PRINT_BATCH_SIZE} -e TOTAL_TIME=${TOTAL_TIME} sim-ln |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: docker run -d
so that this is in the background?
3bcfe8b
to
d8576dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There are some pending thigs but I'm happy to ship it with he current fixes and improve it down the line
@echo "stop Stops the Docker container." | ||
@echo "" | ||
@echo "Variables:" | ||
@echo "SIMFILE_PATH Path to the sim.json file." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra leading space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK @ d8576dc
To run: