Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Update Dockerfile #1359

Merged
merged 4 commits into from
Dec 12, 2019
Merged

Update Dockerfile #1359

merged 4 commits into from
Dec 12, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Dec 5, 2019

What was wrong?

  1. eth2 dockerfile is broken.
  2. Try to make multiple dockerfiles more organized.

How was it fixed?

  1. Move Dockerfiles under ./docker folder
  2. Rename Dockerfile.eth to beacon.Dockerfile. Reasons:
    • IMO it's better to make different software (eth1 trinity, beacon-node, valdiator-client) in different Dockerfiles, and use docker-compose to set them up together.
    • Use .Dockerfile file name for GitHub/editor highlight. ✨
  3. Fix beacon.Dockerfile and add CI test.

The following command works for me to:

docker build -t="ethereum/trinity-beacon" -f ./docker/beacon.Dockerfile ./docker/
docker run ethereum/trinity-beacon -l=DEBUG interop --validators=0,1,2 --start-delay=10 --wipedb

p.s. I believe that a single beacon node can't finalize block because it doesn't import the attestaton to the pool. The fix is in #1311.

To-Do

  • Clean up commit history

Cute Animal Picture

mammoth-3599754_640

docker/beacon.Dockerfile Show resolved Hide resolved
RUN mkdir -p /usr/src/app
WORKDIR /usr/src/app

# Install deps
RUN apt-get update
RUN apt-get -y install libsnappy-dev gcc g++ cmake

ARG GITREF=interop

RUN git clone https://github.com/ethereum/trinity.git .
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this will build the master branch Trinity, instead of PR branch of Trinity. Not sure if this is what we want.

If we want to build the PR branch, we should use something like COPY . trinity

Copy link
Contributor Author

@hwwhww hwwhww Dec 5, 2019

Choose a reason for hiding this comment

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

I read some docker best practice articles earlier today. An argument is that git clone gives us some flexibility at the development stage, and COPY is more common in production.

I think at the development stage, using git clone makes it a more isolated environment, which is good in some way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to build the PR branch, we should use something

That's an interesting point though. The Eth1 Dockerfile uses COPY and I actually wanted to change it to use git clone but your point makes me re-think that. We had a few times where the docker build exposed a general problem with Trinity (usually dependency issues) and as you say, if you use git clone then the CI run ends up always using the source from master which sounds like a weakness.

Another possible idea would be to pre-process the Dockerfile to replace the $GITREF like we do with the DappNode build.

trinity/Makefile

Lines 89 to 92 in 0e3e67d

create-dappnode-image: clean
sed -i -e 's/ARG GITREF=\w*/ARG GITREF=$(trinity_version)/g' ./dappnode/build/Dockerfile
cd ./dappnode && dappnodesdk increase $(dappnode_bump)
cd ./dappnode && dappnodesdk build

EXPOSE 30303 30303/udp
# Trinity shutdowns aren't yet solid enough to avoid the fix-unclean-shutdown
ENTRYPOINT trinity $EXTRA_OPTS fix-unclean-shutdown && trinity-beacon $EXTRA_OPTS
Copy link
Contributor

Choose a reason for hiding this comment

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

Just pointing out that this removes the fix-unclean-shutdown but keeps the comment above that explains it.

Copy link
Contributor

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

I don't have a strong preference on how the Eth2 Dockerfile should be organized (e.g. git clone vs COPY) and this doesn't touch the Eth1 Dockerfile apart from moving it so 👍 from me.

Copy link
Contributor

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Looks good now. Only a nitpick.

docker/beacon.Dockerfile Outdated Show resolved Hide resolved
@hwwhww hwwhww merged commit 913215f into ethereum:master Dec 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants