Skip to content

improved Dockerfile using multistage to reduce image size #5187

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

Closed
wants to merge 9 commits into from

Conversation

barakbd
Copy link

@barakbd barakbd commented Nov 20, 2018

This is an improved Dockerfile which reduces the final image size.
I suggest to leave the comments and links in for future reference and clarity.
I do not see in the travis.yaml that it is being built and pushed to Dockerhub.

@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

Merging #5187 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5187      +/-   ##
==========================================
- Coverage   93.91%   93.87%   -0.04%     
==========================================
  Files         123      123              
  Lines        8939     8950      +11     
==========================================
+ Hits         8395     8402       +7     
- Misses        544      548       +4
Impacted Files Coverage Δ
src/Routers/PushRouter.js 92.85% <0%> (-3.58%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 91.48% <0%> (-0.73%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.11% <0%> (-0.06%) ⬇️
src/RestWrite.js 93.24% <0%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c419ec...a21c2ec. Read the comment docs.

@flovilmart
Copy link
Contributor

The build is automated in dockerhub so there is no need to use travis to push.

Thanks for the PR. While this seems as always over complicated, this doesn't really solve either the onboarding situation with docker as well as installing 3rd party packages, passing a custom config etc..

@barakbd
Copy link
Author

barakbd commented Nov 27, 2018

This is a great improvement to reduce the image size. Multistage Docker builds are standard for some time now (and not over complicated)
As for tutorials on how to get a Dockerized Parse Server running, there are multiple examples on the web already. Here are a few:

  1. https://medium.com/@levioza/setup-a-parse-server-in-a-few-minutes-using-docker-compose-c81825c27a4
  2. https://github.com/bitnami/bitnami-docker-parse

There are even Helm Charts ready to install:

  1. https://github.com/helm/charts/tree/master/stable/parse
  2. https://bitnami.com/stack/parse/helm

The purpose of this repo should be to optimize the docker image which everyone is pulling.

marksharaabi
marksharaabi previously approved these changes Dec 7, 2018
@barakbd
Copy link
Author

barakbd commented Dec 11, 2018

@flovilmart - what else is needed for this PR to merge?
It is not blocking anything. Who is an approving reviewer?

@flovilmart
Copy link
Contributor

I am, I need to have a look.

@barakbd
Copy link
Author

barakbd commented Dec 11, 2018

Dockerfile Outdated
# RUN ls -al

# capture git_commit in label
ARG GIT_COMMIT
Copy link
Contributor

Choose a reason for hiding this comment

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

is this argument passed by anyone?

Copy link
Author

Choose a reason for hiding this comment

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

Dockerfile Outdated
# list all dir/files - for debugging purposes
# RUN ls -al

# ---- UNIT TESTS AND FLOW LINT in stage 3------
Copy link
Contributor

Choose a reason for hiding this comment

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

why keep that if it is not run? We should probably either remove or run the tests

Copy link
Author

Choose a reason for hiding this comment

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

It seems that the tests depend on a db, so we cannot run them inside Dockerfile.
Also, the tests are already setup to run in Travis CI.
I am just leaving the comments here for future reference, in case someone wants to change this.

Copy link
Contributor

Choose a reason for hiding this comment

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

So please remove it!

RUN npm install
# copy all context into WORKDIR (/parse-server) excluding items in .dockerignore
COPY . .
# Need to run build explicitly as npm will not auto run scripts as ROOT
Copy link
Contributor

Choose a reason for hiding this comment

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

why run as ROOT then? If there's no need, please run unpriviledged

# base image for release stage with only prod dependencies
FROM base AS dependencies
# set npm configs
RUN npm set progress=false && npm config set depth 0
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of set depth 0?

There is no affected command: https://docs.npmjs.com/misc/config#depth

# ------- Stage 1 - Base ---------
FROM node:carbon-alpine as base
# apk - https://www.cyberciti.biz/faq/10-alpine-linux-apk-command-examples/
# RUN apk update && apk upgrade && \
Copy link
Contributor

Choose a reason for hiding this comment

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

why keep those??

@@ -1,21 +1,86 @@
FROM node:carbon
# https://blog.hasura.io/an-exhaustive-guide-to-writing-dockerfiles-for-node-js-web-apps-bbee6bd2f3c4
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to copy paste the sources of the code you're writing. please remove


RUN mkdir -p /parse-server/config
VOLUME /parse-server/config
# ------- Stage 1 - Base ---------
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove useless comments



# ------- Stage 4 - Release ---------
FROM dependencies AS release
Copy link
Contributor

Choose a reason for hiding this comment

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

all in all each layer uses the previous one completely, what's the point then?

@flovilmart
Copy link
Contributor

@barakbd what size are you at currently? I have a very simple Dockerfile that ends up with a 160Mb size:

FROM node:carbon-alpine as build 

RUN apk update; \
  apk add git;
RUN npm install -g npm@latest;
WORKDIR /tmp

COPY package*.json ./
RUN npm ci

COPY . .
RUN npm run build

FROM node:carbon-alpine as release

WORKDIR /parse-server

COPY package.json .
RUN npm install --production 

COPY bin bin
COPY public_html public_html
COPY views views
COPY --from=build /tmp/lib lib

ENV PORT=1337

USER node

EXPOSE $PORT

ENTRYPOINT ["node", "./bin/parse-server"]

@barakbd
Copy link
Author

barakbd commented Dec 17, 2018

I get almost same size (176mb).
Does the image work?
Do npm prepare && postinstall scripts run automatically? I think I had to run them explicitly since npm won't run auto scripts in root user:
https://docs.npmjs.com/misc/scripts#user
npm/npm#17346

This looks fine. The only improvement my file has is shorter build time, since it only installs each npm package once, but I doubt that's a big concern.

I am still testing building with labels that contain the git commit. Dockerhub build environment has a few variables, one of which is SOURCE_COMMIT. Feel free to use your Dockerfile, and I will make another pull request regarding the build hook.

@flovilmart
Copy link
Contributor

Not sure there’s a real need for the source commit as it is recommended not to use ‘latest’ in any case.

Yes the image ‘works’. Try it out. I may have missed the VOLUMES directive.

The npm install is quite fast, and it’s always built by docker hub itself, so speed is not that important

@barakbd
Copy link
Author

barakbd commented Dec 18, 2018

It is not recommended to use latest for tags, not for labels.
Since you can move tags between git commits, you may build a new docker image with the same tag (assuming you're docker tag is based on your git tag), but the source code is different. The most direct way to connect docker image and source code is label it with the commit hash.

@flovilmart
Copy link
Contributor

in any case, closing in favor of #5248

@flovilmart flovilmart closed this Dec 18, 2018
@flovilmart
Copy link
Contributor

It is not recommended to use latest for tags, not for labels.

What do you mean?

Since you can move tags between git commits, you may build a new docker image with the same tag (assuming you're docker tag is based on your git tag), but the source code is different.

Yes and the respository will equally point to different source code. BUT you need to force push to git as well as the docker repository, so ¯_(ツ)_/¯

The most direct way to connect docker image and source code is label it with the commit hash.

Which is not provided by default by docker hub as an environment variable, so I guess this is not 'that' important.

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.

3 participants