Skip to content
New issue

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

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

Already on GitHub? # to your account

Add dockerfile #175

Merged
merged 2 commits into from
Nov 5, 2021
Merged

Add dockerfile #175

merged 2 commits into from
Nov 5, 2021

Conversation

njgheorghita
Copy link
Collaborator

Add dockerfile. There's still some optimization that can be done here, particular with caching the layer for compiling dependencies. However, I wasn't able to figure out how to get this working with multiple workspaces, and after enough time sitting through docker build steps, I figured it's not a huge priority. A possible solution seems to be cargo-chef, but it's maybe overkill for the time being.

@njgheorghita njgheorghita requested a review from carver November 3, 2021 22:39
Dockerfile Outdated
@@ -0,0 +1,31 @@
# select build image
FROM rust as builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is neat, I hadn't seen multi-stage docker builds before (granted I haven't really used docker for maybe ~6 years).

Based on this link, and my intuition, the as would be clearest to be capitalized to highlight it as a Docker keyword.

Suggested change
FROM rust as builder
FROM rust AS builder

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a tag for the version of the rust image, it seems now it is downloading always the latest version?

Dockerfile Outdated
WORKDIR /trin

RUN apt-get update && apt-get install clang -y
RUN rustup component add rustfmt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: why do we need rustfmt in this docker builder?

Copy link
Collaborator Author

@njgheorghita njgheorghita Nov 5, 2021

Choose a reason for hiding this comment

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

Good catch, this was necessary in an earlier, fancier setup I was attempting when I was running cargo build in default mode rather than --release (which required rustfmt to compile librocks) - but it's no longer necessary so I've removed it

Comment on lines +11 to +18
# copy over manifests and source to build image
COPY ./Cargo.lock ./Cargo.lock
COPY ./Cargo.toml ./Cargo.toml
COPY ./src ./src
COPY ./trin-core ./trin-core
COPY ./trin-history ./trin-history
COPY ./trin-state ./trin-state
COPY ./ethportal-peertest ./ethportal-peertest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to bite us roughly every time a new crate is added... I'm wondering if there's any better way, like just copying the whole directory minus .git. This isn't a hard blocker, just something to consider.

Dockerfile Outdated
@@ -0,0 +1,31 @@
# select build image
FROM rust as builder
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a tag for the version of the rust image, it seems now it is downloading always the latest version?

FROM rust as builder

# create a new empty shell project
RUN USER=root cargo new --bin trin
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to use COPY . . here instead of creating an empty project and then copying all folders one by one? We can use also .dockerignore file if we want to exclude something.

@njgheorghita
Copy link
Collaborator Author

@ogenev @carver re: copying over the source folders individually. There are two reasons for the current setup...

  • It's a bit of a hangover from a more advanced setup I was trying for caching dependencies, where you have to copy over the manifests from each workspace individually. To be fair, we're not using that setup so it's no longer a consideration (though, I could see us using it in the future to optimize docker builds)
  • I have a lot of extra files in my trin folder, python scripts, rocks databases, md files, etc. This is the easiest way to explicitly copy over only the necessary files. But, I also understand that this isn't a great justification for the complexity

So, those are the reasons. Neither of which are convincing. But, also part of me likes the explicit copy. It's somewhat wordy, but avoids the scenario of accidentally building the image with a .env file or some other file type that you forgot wasn't accounted for in .dockerignore. The most convincing argument for .dockerignore is what Carver mentioned, but docker will yell at you immediately during the build while you're compiling that it's missing those files, and then it's simply adding another line.

But, if you agree / disagree, lmk and I can do the .dockerignore no problem

@carver
Copy link
Collaborator

carver commented Nov 5, 2021

Sure, it's a little messy, but I'm okay leaving the PR as-is.

It's compelling that the most likely failure mode would be: immediate build failure with somewhat clear message about missing a crate.

That's much better than the alternative: probably not notice that a bunch of extra stuff gets included in the build, and a bunch of unnecessary info gets pushed out to all the testnet nodes.

@njgheorghita njgheorghita merged commit 6413c05 into ethereum:master Nov 5, 2021
@njgheorghita njgheorghita deleted the new-dockerfile branch November 5, 2021 21:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants