Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Multistage build docker #115

Merged

Conversation

mattwr18
Copy link
Member

@mattwr18 mattwr18 commented Jan 3, 2019

fixes #93

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
.travis.yml Outdated
- docker build --build-arg BUILD_COMMIT=$TRAVIS_COMMIT -t humanconnection/nitro-web .
- docker-compose -f docker-compose.yml up -d
- docker build --build-arg BUILD_COMMIT=$TRAVIS_COMMIT --target production -t humanconnection/nitro-web .
- docker-compose -f docker-compose.travis.yml up -d
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- docker-compose -f docker-compose.travis.yml up -d
- docker-compose -f docker-compose.yml up -f docker-compose.travis.yml up -d

@mattwr18 I suspect this is the reason for the failing build. Without the default docker-compose.yml frontend and backend are not joining the same network and cannot reach each other. Also we wait-on http://localhost:3000 later in .travis.yml and if we don't expose port 3000 we will wait forever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, I'll give this a go! I am exposing 3000 on line 34... although I believe it is waiting cause it is not spinning up properly, although, it is failing silently

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? I believe with docker-compose you have to expose all the ports in the .yml file explicitly. (Although I never looked it up)


services:
backend:
image: humanconnection/nitro-web:builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
image: humanconnection/nitro-web:builder

I think it's in the base docker-compose.yml so not needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know the reason for having yet another docker-compose.yml just for travis? Because of shared folders. Travis struggles to delete them after running the tests, thus silently failing our deployments. The reason why Travis cannot delete the files is that the docker user on Travis creates the files, not the user who is later trying to delete them.

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
.travis.yml Outdated
@@ -19,7 +19,7 @@ before_install:

install:
- docker build --build-arg BUILD_COMMIT=$TRAVIS_COMMIT --target production -t humanconnection/nitro-web .
- docker-compose -f docker-compose.yml -f docker-compose.travis.yml up -d
- docker-compose -f docker-compose.yml up -f docker-compose.travis.yml up -d
Copy link
Contributor

Choose a reason for hiding this comment

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

no no, the up is there already 😉 @mattwr18

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh, right... I thought I had missed it yesterday from your review above... it's still failing the build though

@roschaefer
Copy link
Contributor

@mattwr18 great work, can you tell me how large the docker image is on your machine after this PR?

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

@mattwr18 great work, can you tell me how large the docker image is on your machine after this PR?

REPOSITORY                      TAG                 IMAGE ID            CREATED             SIZE
<none>                          <none>              c94475d09e1e        6 seconds ago       318MB

A little bigger than a third of the size it was!!
Do you understand why the Repository and Tag are ? when I first built the image from the Dockerfile in the master branch it looked like humanconnection/nitro-web latest 4a714bb66e4c 21 hours ago 902MB

@roschaefer
Copy link
Contributor

@mattwr18 yes, you can add a repository and a tag. See our build server and the deploy script:

docker build [...] -t humanconnection/nitro-web:latest .

adds the repository and

docker push humanconnection/nitro-web:latest

does the same for docker push.

@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

Output from build, command docker-compose -f docker-compose.yml -f docker-compose.travis.yml up -d

...
Creating nitro-web_webapp_1
...

but unfortunately still getting this output

docker-compose exec webapp yarn run lint
No container found for webapp_1
The command "docker-compose exec webapp yarn run lint" exited with 1.

@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

I've run the exact same commands as from the build locally and they have passed...

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
@mattwr18 ouch, that was a hard to spot error. It struck my eye when I
was switching back and forth between `docker-compose.override.yml` and
`docker-compose.travis.yml`.
@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

ahhh duh!! sorry 😛

roschaefer and others added 3 commits January 4, 2019 19:52

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
This should fix our build server. Probably the container immediately
exited on Travis which is using the `builder` stage for testing.

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

Size of the production image now

humanconnection/nitro-web       latest              aa9218a28cfd        5 minutes ago       319MB

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
Needed to run tests

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
@appinteractive
Copy link
Member

Why not just copy all into the build image and trusting the dockerignore file? @mattwr18

@mattwr18
Copy link
Member Author

mattwr18 commented Jan 4, 2019

that's a good idea @appinteractive, right now we were just concerned with the minimum to get it passing the build, but we can definitely explore that route.

Currently, it's unexpectedly failing the build because of many linting errors, which we are not able to reproduce locally.
https://travis-ci.com/Human-Connection/Nitro-Web/builds/96354499#L1006

@appinteractive
Copy link
Member

appinteractive commented Jan 4, 2019

Yeah that’s because of failing config files for the Linter. Please try to copy the whole root dir and trust the dockerignore. After that you just copy the needed files to the production image.

That way the build will be more solid to change imho.

Dockerfile Outdated
FROM base as production
ENV NODE_ENV=production
COPY --from=build-and-test ./nitro-web/node_modules ./node_modules
COPY --from=build-and-test ./nitro-web/plugins ./plugins
Copy link
Member

Choose a reason for hiding this comment

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

Why do you copy the plugins and static files over when you already copied everything in the base image?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, carry over from before your above suggestion.

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
…3-minimize-docker-image-size-with-multi-stage-build

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden

Verified

This commit was signed with the committer’s verified signature.
epdenouden Ewout Pieter den Ouden
…3-minimize-docker-image-size-with-multi-stage-build
@mattwr18
Copy link
Member Author

mattwr18 commented Jan 9, 2019

@appinteractive @roschaefer I have gotten this build to pass reliably again and cleaned it up even more in the process, taking into account your suggestions. It is not an ideal situation, however.

The reason it was failing the build before @roschaefer is because for some reason the commands docker-compose exec webapp yarn run lint and yarn run test are being run from the production image, which after having removed the copy node_modules from build and test, does not include the devDependencies jest or eslint necessary to run the commands.

This is also the reason I needed to add the -e NODE_ENV=test... I was having a look at how the docker-compose -f docker-compse.yml -f docker-compose.travis.yml up -d command works and it seems like the Docker docs encourage to have a separate yml for production. See https://docs.docker.com/compose/extends/#different-environments and https://docs.docker.com/compose/production/ @roschaefer

@roschaefer roschaefer force-pushed the 93-minimize-docker-image-size-with-multi-stage-build branch from a96221f to 6b4601e Compare January 10, 2019 23:54
roschaefer and others added 3 commits January 11, 2019 00:54
I give up now after numerous attempts to reduce the docker image size.
The only thing left is that I've put the NODE_ENV=.. configuration in
the docker-compose.X.yml files, I think that's slightly cleaner
This reverts commit 6b4601e.

@mattwr18 for reasons I don't understand, our build server hangs
https://travis-ci.com/Human-Connection/Nitro-Web/builds/96979338

Honestly, I start to think this is a bug in `docker-compose`
@roschaefer
Copy link
Contributor

For the record:
screenshot 21

@roschaefer
Copy link
Contributor

roschaefer commented Jan 11, 2019

@mattwr18 I'm done, I believe this is a bug with docker-compose.yml. My latest reverted change is an equivalent refactoring. I don't understand why the build was failing. Nevertheless, I think this PR is an improvement. I'll merge

@roschaefer roschaefer merged commit 9825b41 into master Jan 11, 2019
@roschaefer roschaefer deleted the 93-minimize-docker-image-size-with-multi-stage-build branch January 11, 2019 11:31
@roschaefer
Copy link
Contributor

@mattwr18 🎉
screenshot 22

@mattwr18
Copy link
Member Author

Wow!! what happened!? I thought it was around 315MB, still an big improvement, but nothing the 114MB!!!
This is great!! I want to get more experience with this by multi-staging all the Dockerfiles I helped create!! Thanks for your help @appinteractive @roschaefer @ksinkar :)

@roschaefer
Copy link
Contributor

@mattwr18 I assume it comes from the node_modules and the built files in styleguide that are not included. I still believe it can be compressed even more by not including the development dependencies. But after spending hours of try/error and being confused by the behaviour of docker-compose I leave that for some point in the future.

@mattwr18
Copy link
Member Author

Did you have a look at the links I shared with you here about the use of docker-compose in production?

roschaefer pushed a commit that referenced this pull request Mar 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants