-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Adding caching to Dockerfiles #5570
Conversation
License: MIT Signed-off-by: Rob Deutsch <rdeutschob@gmail.com>
License: MIT Signed-off-by: Rob Deutsch <rdeutschob@gmail.com>
NOTE: At this point, is it worth removing |
@@ -9,15 +9,16 @@ MAINTAINER Lars Gierth <lgierth@ipfs.io> | |||
ENV GX_IPFS "" | |||
ENV SRC_DIR /go/src/github.com/ipfs/go-ipfs | |||
|
|||
COPY . $SRC_DIR |
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 took a similar but different approach in making the ipfs-cluster docker build to be faster. Instead of caching the gx dependencies in the docker cache, which still requires you to load the gx dependencies via the docker network stack (hits ipfs.io/ipfs/... for each dependency, even though 99% they are already in your local gopath) at least once but also each time they are updated, I run gx install --local
before building the docker container, which brings all the deps into the vendor/
directory and I added docker specific Make install target which uses gx install --local
. This way the when docker copies the SRC_DIR into the build context it also brings the gx packages with it and then when building inside the container gx is able to use the already 'local' dependencies.
The advantage of this is that you get the benefit of the docker cache as well as never having to use the docker network stack to go out and retrieve dependencies again.
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.
Hi @lanzafame , thanks for the info.
Unfortunately that is not a great solution for me, because I use a remote Docker host and I don't want to upload hundreds of megabytes worth of build context.
There are many reasons to use a remote build host. I use one because I'm on macOS.
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.
@rob-deutsch Would that maybe be case for a third Dockerfile, i.e. Dockerfile.remote? I have had enough fights with docker daemon networking issues that I would rather not see anymore network reliant requests be added to the Dockerfile.
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.
To start with, I think this PR should be merged, because (I believe) it's a strict improvement over what's currently there.
Longer term, I don't mind whether there's a separate Dockerfile that sends all the dependancies in the build context.
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.
@rob-deutsch Have you considered "Docker for Mac" (free as in beer) which is actually just docker running in a headless linux VM on your local machine? It would still copy hundreds of megabytes of build context, but it would do so very quickly since nothing is transmitted over the network. Just a thought, anyway.
@mgoelzer I'm completely lost when it comes to Docker. Do you happen to know the ramifications of this change? |
So, I'm going to see if I can get this merged (even though I'm pretty much completely lost here). Looking back through history (see 81c8cff), it looks like we first split the dockerfile when we introduced that caching. As far as I can tell, that was the only difference. So, the question is: should we have just introduced caching into the original dockerfile instead of splitting it? As far as I can tell, yes(?) Note: We now use a smaller base image in the main dockerfile so, regardless, the split isn't useless (even if it was at the beginning). |
RUN cd $SRC_DIR \ | ||
&& mkdir .git/objects \ | ||
RUN set -x \ | ||
&& go get github.com/whyrusleeping/gx \ |
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 would prefer if gx
and gx-go
were copied over from the bin/
directory instead of getting the latest version every time so that way you have one less possible variation between a local build env and the docker daemon build env.
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.
Actually, I'm pretty sure we neither want nor need gx in the docker build. We only need to do this here because we haven't run make build
yet (which installs gx). Instead of this, I believe we should be running make deps
(Dockerfile.fast should be doing the same thing).
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.
make deps
just install gx,gx-go
if they aren't present which will be the case since the whole directory isn't being copied in. The added issue is that make deps
uses dist_get
to install gx,gx-go
and dist_get
also is missing due to the bin/
directory not being copied into the docker build context. Though using make deps
solves the which version of gx,gx-go
is being grabbed issue. We still need to copy in bin/dist_get
at the least though to make it work.
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 see. Yes, we should be copying bin (I guess? I haven't read the docker documentation)
Unfortunately, at this point, I'm not sure if anyone who understands the design decisions behind our current Dockerfiles has the bandwidth to maintain them. In light of that, I think we'll apply the old adage "If it ain't broke, don't fix it." and leave good enough alone (for now). (context: #6040) |
Thanks for the update @Stebalien, I'm afraid I'm also lacking bandwidth at the moment. |
Dockerfile.fast
is neat because it caches the gx dependancies in an intermediary image before building go-ipfs. It will cache these dependancies untilpackage.json
changes.I can't see any reason why
Dockerfile
shouldn't do the same caching.I've also rejigged the order of both Dockerfiles so that su-exec and tini are cached.