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

Add Docker image #168

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Conversation

fracek
Copy link
Contributor

@fracek fracek commented Mar 3, 2022

I created a docker image so it's easier to run the node. It can also persist data between restarts.

@Mirko-von-Leipzig
Copy link
Contributor

Hmm okay so our CI secrets don't like external contributors. Will have to figure that out.

@Mirko-von-Leipzig
Copy link
Contributor

@fracek could you rebase on latest main? There is a small CI change that will hopefully let us run the tests fully for forks.

@fracek fracek force-pushed the feat/add-docker-container branch from eef8871 to fba85e3 Compare March 3, 2022 16:44
@fracek
Copy link
Contributor Author

fracek commented Mar 3, 2022

Ok I rebased it.

@fracek
Copy link
Contributor Author

fracek commented Mar 3, 2022

As mentioned on Discord there seems to be some issues with the image. The node is synchronizing but the RPC module is not working.

Copy link
Member

@CHr15F0x CHr15F0x left a comment

Choose a reason for hiding this comment

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

As I mentioned on Discord, I tried running the image and it turns out sync works but I cannot see rpc starting. I haven't investigated yet.

README.md Outdated Show resolved Hide resolved
@fracek fracek force-pushed the feat/add-docker-container branch 4 times, most recently from b1367f5 to 20f651f Compare March 5, 2022 17:10
@fracek
Copy link
Contributor Author

fracek commented Mar 5, 2022

I updated the image and now I can execute rpc_examples.sh using the docker image. The only outstanding issue is that the container cannot be gracefully exited with ctrl-c, but needs to be killed with docker kill.

Dockerfile Show resolved Hide resolved
@fracek fracek force-pushed the feat/add-docker-container branch from 20f651f to 50a06d4 Compare March 8, 2022 11:32
@fracek
Copy link
Contributor Author

fracek commented Mar 8, 2022

I pushed the user change. I also noticed that container takes a few seconds to stop with docker stop, but eventually it does.

Dockerfile Outdated Show resolved Hide resolved
@aphelionz
Copy link
Contributor

@fracek can we try this? I spent some time with alpine and I think its worthwhile. feel free to use this in your PR and still get credit 💪

########################################
# Stage 1: Build the pathfinder binary #
########################################
FROM rust:1.59-alpine AS rust-builder

RUN apk add --no-cache musl-dev gcc openssl-dev

WORKDIR /usr/src/pathfinder

# Build only the dependencies first. This utilizes
# container layer caching for Rust builds
RUN mkdir crates
RUN cargo new --lib crates/pedersen
# Correct: --lib. We'll handle the binary later.
RUN cargo new --lib crates/pathfinder
COPY Cargo.toml Cargo.toml

COPY crates/pathfinder/Cargo.toml crates/pathfinder/Cargo.toml
COPY crates/pedersen/Cargo.toml crates/pedersen/Cargo.toml
COPY crates/pedersen/benches crates/pedersen/benches

RUN RUSTFLAGS='-L/usr/lib -Ctarget-feature=-crt-static' cargo build --target=x86_64-unknown-linux-musl


# Compile the actual libraries and binary now
COPY . .

# Mark these for re-compilation 
RUN touch crates/pathfinder/src/lib.rs
RUN touch crates/pedersen/src/lib.rs

RUN RUSTFLAGS='-L/usr/lib -Ctarget-feature=-crt-static' cargo build --target=x86_64-unknown-linux-musl --bin pathfinder

#######################################
# Stage 2: Build the Python libraries #
#######################################
FROM python:3.8-alpine AS python-builder

RUN apk add --no-cache gcc musl-dev gmp-dev g++

WORKDIR /usr/share/pathfinder
COPY py py
RUN python3 -m pip install --upgrade pip
RUN python3 -m pip install -r py/requirements-dev.txt

# This reduces the size of the python libs by about 50%
ENV PY_PATH=/usr/local/lib/python3.8/
RUN find ${PY_PATH} -type d -a -name test -exec rm -rf '{}' +
RUN find ${PY_PATH} -type d -a -name tests  -exec rm -rf '{}' +
RUN find ${PY_PATH} -type f -a -name '*.pyc' -exec rm -rf '{}' +
RUN find ${PY_PATH} -type f -a -name '*.pyo' -exec rm -rf '{}' +

# Pathfinder expects a very specific directory structure for the
# python environment and scripts
# This docker image is not using virtual environments, so replicate
# the directory structure with a shim python script.
ENV VIRTUAL_ENV=/usr/share/pathfinder/py/venv
RUN mkdir -p ${VIRTUAL_ENV}/bin
RUN ln -s /usr/local/bin/python3 ${VIRTUAL_ENV}/bin/python 
RUN ${VIRTUAL_ENV}/bin/python --version

#######################
# Final Stage: Runner #
#######################
FROM python:3.8-alpine AS runner

RUN apk add --no-cache tini

COPY --from=rust-builder ["/usr/lib/libstdc++.so.6", "/usr/lib/libgcc_s.so.1", "/usr/lib/" ]
COPY --from=rust-builder /usr/src/pathfinder/target/x86_64-unknown-linux-musl/debug/pathfinder /usr/local/bin/pathfinder
COPY --from=python-builder /usr/local/lib/python3.8/ /usr/local/lib/python3.8/
COPY --from=python-builder /usr/share/pathfinder/py/venv /usr/share/pathfinder/py/venv

ENV VIRTUAL_ENV=/usr/share/pathfinder/py/venv
COPY py/src /usr/share/pathfinder/py/src

# Create directory and volume for persistent data
RUN mkdir -p /usr/share/pathfinder/data
RUN chown 1000:1000 /usr/share/pathfinder/data
VOLUME /usr/share/pathfinder/data

USER 1000:1000
EXPOSE 9545
WORKDIR /usr/share/pathfinder/data

ENTRYPOINT ["tini", "--"]
CMD ["/usr/local/bin/pathfinder"]

This:

  1. fixes the ctrl+c problem by using tini
  2. Keeps USER 1000
  3. Reduces the image size to 380MB from 688MB (and I'm sure we can find more optimizations too)

README.md Outdated Show resolved Hide resolved
@aphelionz
Copy link
Contributor

aphelionz commented Mar 9, 2022

@fracek heads up #188 was just merged which eliminates the need for the venv hack in the Dockerfile

At this point it might make sense for you to make a fresh PR with the alpine version sans the venv lines. What do you think?

@fracek
Copy link
Contributor Author

fracek commented Mar 9, 2022

@aphelionz I can rebase and amend this PR.

@aphelionz
Copy link
Contributor

Sounds good to me, thank you!

@fracek fracek force-pushed the feat/add-docker-container branch 2 times, most recently from 699b90c to 00ad25d Compare March 9, 2022 19:08
@fracek
Copy link
Contributor Author

fracek commented Mar 9, 2022

I updated the Dockerfile. I changed three things from what you posted:

  • Removed references to the virtual environment
  • Copy libgmp to the runner image
  • Add pathfinder to ENTRYPOINT so users can pass the arguments directly to docker run
  • Updated the readme to include --ethereum.password

Copy link
Contributor

@aphelionz aphelionz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @fracek, great work 👍

Copy link
Contributor

@aphelionz aphelionz left a comment

Choose a reason for hiding this comment

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

Whoops, actually. My bad. Let's use --release

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@fracek fracek force-pushed the feat/add-docker-container branch from 00ad25d to cc8ed10 Compare March 9, 2022 20:25
@fracek fracek requested a review from aphelionz March 9, 2022 20:26
@fracek
Copy link
Contributor Author

fracek commented Mar 9, 2022

I added the --release flag.

@aphelionz
Copy link
Contributor

Great work @fracek we'll look after merging once the tests pass

@aphelionz
Copy link
Contributor

@Mirko-von-Leipzig the test failure is the issue with GH secrets right? I don't think anything in .dockerignore / README.md / Dockerfile breaks this

@CHr15F0x
Copy link
Member

the test failure is the issue with GH secrets right?
Yes, it is.

@Mirko-von-Leipzig
Copy link
Contributor

Thanks @fracek!

@Mirko-von-Leipzig Mirko-von-Leipzig merged commit 8f573b8 into eqlabs:main Mar 10, 2022
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

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

LGTM other than the small nit-pick WRT data directory location.

COPY --from=python-builder /usr/local/lib/python3.8/ /usr/local/lib/python3.8/

# Create directory and volume for persistent data
RUN mkdir -p /usr/share/pathfinder/data
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, shouldn't this be something like /var/lib/pathfinder?

According to the Linux Filesystem Hierarchy Standard:

The /usr/share hierarchy is for all read-only architecture independent data files. [30]

And for /var/lib (FHS reference):

This hierarchy holds state information pertaining to an application or the system. State information is data that programs modify while they run, and that pertains to one specific host. Users must never need to modify files in /var/lib to configure a package's operation, and the specific file hierarchy used to store the data must not be exposed to regular users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, let's break this out into a new issue

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 hierarchy holds state information pertaining to an application or the system. State information is data that programs modify while they run, and that pertains to one specific host. Users must never need to modify files in /var/lib to configure a package's operation, and the specific file hierarchy used to store the data must not be exposed to regular users.

There should be 2 volumes: one for the configuration in /usr/share and one for application data in /var/lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fracek do you want to make a PR for those volumes? Also note I just created #190 if you want to give any feedback there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants