Skip to content
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

Use smaller base image #46

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

sthuber90
Copy link

To use smaller base image, alpine is used as a new base. To keep only required files multi-stage builds are used. In addition, unnecessary files are cleaned up from node_modules folder.

This PR is intended to fix #44

The overall size reduction on linux/amd64 (Mac) can be seen here:
Bildschirmfoto 2020-07-06 um 17 31 38

As you can see MagicMirror still works:
Bildschirmfoto 2020-07-06 um 17 32 26

- multi stage build
- sh instead of bash
@bastilimbach
Copy link
Owner

Wow the size difference is huge! Thank you for your contribution 😊

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
config
modules
build.sh
test-build.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the .git directly, should it also be ignored?

Copy link
Author

Choose a reason for hiding this comment

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

Please see my comment #46 (comment)

Dockerfile Outdated
Comment on lines 9 to 11
RUN apk update && apk add --no-cache git \
&& git clone --depth 1 -c advice.detachedHead=false -b ${branch} https://github.com/MichMich/MagicMirror.git . \
&& apk del git

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

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

True. I will remove apk del git in the next commit. Nevertheless, I would prefer to stick with the same base image and definitely not switch between alpine and ubuntu or debian. Using the same base image in all the stages makes the build actually faster because docker does not have to pull different base images to start the build.

Build times on my machine

Stage 0 Stage 1 time
node:12  node:12-alpine 225,53 s
node:12-alpine node:12-alpine 139,66 s

Between the builds I did run docker system prune --allto start from scratch. The time was recorderd with time docker build -t mm:latest .

Dockerfile Outdated
Comment on lines 25 to 36
&& cp -R \
config \
css \
fonts \
index.html \
js \
node_modules \
package.json \
package-lock.json \
serveronly \
translations \
vendor /opt/magic_mirror/dist
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to copy over all files and just delete the test directory and some dot files, but excluding the small text files are only some KB size. The most size comes from the base image and node_modules directory.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what the benefit of this would be. Adding specifically the files and folders required is what docker recommends and I think it makes clearer what is used and required for MagicMirror to run and what can be left aside. Also, if the structure of MagicMirror changes or other folders are added we would have to update the Dockerfile to remove them as well, whereas now we have to add them if they are required for it to work, otherwise there's no change needed.

In short, even though it's quite the copying taking place it is much more tranparent what is added to the docker image.

@sthuber90
Copy link
Author

sthuber90 commented Jul 8, 2020

Thank you for your input. I reduced the COPYing between the stages. package.json and package-lock.json are needed for MagicMirror to run, otherwise it errors with a message mentioning those files are missing.

As git is not part of the alpine based image the following error occures.

[2020-07-08 07:04:20.600] [LOG]    Ready to go! Please point your browser to: http://0.0.0.0:8080
[2020-07-08 07:05:12.323] [LOG]    Create new calendar fetcher for url: http://www.calendarlabs.com/ical-calendar/ics/76/US_Holidays.ics - Interval: 300000
[2020-07-08 07:05:12.435] [LOG]    Create new news fetcher for url: http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml - Interval: 300000
[2020-07-08 07:05:12.474] [ERROR]  Error: spawn git ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:267:19)
    at onErrorNT (internal/child_process.js:469:16)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
[2020-07-08 07:05:12.969] [INFO]   Newsfeed-Fetcher: Broadcasting 57 items.

The error doesn't seem to affect MagicMirror. Everything seems to work as usual.

Adding gitadds 18 MB to the image size and another error appears, while the previously mentioned disappears:

[2020-07-08 07:18:18.900] [LOG]    Ready to go! Please point your browser to: http://0.0.0.0:8080
[2020-07-08 07:19:25.987] [LOG]    Create new calendar fetcher for url: http://www.calendarlabs.com/ical-calendar/ics/76/US_Holidays.ics - Interval: 300000
[2020-07-08 07:19:26.005] [LOG]    Create new news fetcher for url: http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml - Interval: 300000
[2020-07-08 07:19:26.038] [ERROR]  fatal: not a git repository (or any of the parent directories): .git

[2020-07-08 07:19:26.494] [INFO]   Newsfeed-Fetcher: Broadcasting 57 items.

Adding the .git folder from MagicMirror to the docker image as well resolves the errors, but it feels weird to have git in a production image.

The new image size with git and .git would then be 330 MB on linux/amd64

What are your thoughts on this?

@khassel
Copy link
Contributor

khassel commented Jul 8, 2020

My thoughts:

  • is git only a build dependency? The default module updatenotification needs git, other foreign modules too, e.g. MMM-RemoteControl. Installing new modules will not work from within the container.
  • ensubst is used in the entrypoint script, but AFAIS gettext is installed and deinstalled in the Dockerfile
  • switching from debian to alpine will break all images which are using this image as base image.

@sthuber90
Copy link
Author

  • I think gitis and should be treated as a development resource / build dependency but I also get that it should be part of the image as MagicMirror is not in any way packaged
  • Thank you for noticing, I will fix this in one of the next commits so that envsubst is working
  • Maybe true but changes are sometimes harsh. Leaving the jokes aside, what do you think about having debian and alpine based images in parallel? I'm thinking of something similar like the official docker images i.e. https://hub.docker.com/_/node?tab=tags. That way people depending on debian can continue to do so, but at the same time you can get a more efficient, smaller base image with alpine

It is clear that space is not as huge of a cost factor as it used to be. Nevertheless, I don't see the point of using up almost 2 GB of my raspberry's space when also ~330 MB will do. I still strongly believe that having a smaller image is desireable

@Legion2
Copy link
Contributor

Legion2 commented Jul 9, 2020

I like the idea of having the normal debian based image as default and the alpine image with a -alpine tag

@khassel
Copy link
Contributor

khassel commented Jul 9, 2020

I think @bastilimbach have to decide if he wants to maintain both images.

A long time ago I discussed with him the idea not only providing images for server only mode. He didn't want this at that time so I decided to make my own docker setup. And here I decided already to provide both image types (alpine and debian).

I think we should bring these things together again, so may take a look at this setup for deciding what of the stuff should also implemented here.

Beside the size (the debian image is currently 544MB) there are may other things to discuss as running as non-root and copying default modules in entrypoint.

@sthuber90
Copy link
Author

On a side note the 544 MB you mentioned are only shown like this on Docker hub. If you pull the image the size is above 1 GB
Bildschirmfoto 2020-07-10 um 10 50 32

The images tagged with 2.12.0 are the ones I created during my work in progress. The one with the latest tag was pulled just now from Docker Hub docker pull bastilimbach/docker-magicmirror:latest.

Like the idea of having one repository for MagicMirror in docker. Honestly, started with my own Docker Image as well but came to my senses and thought that in the end everybody benefits if it's all maintainable in one repository.

In the end, it's still your decision @bastilimbach

In the meantime, maybe I will have some time to try to setup a draft of how we can build both debian and alpine images

@khassel
Copy link
Contributor

khassel commented Jul 10, 2020

The 544 MB are the result building with this Dockerfile using slim base image.

The build setup could be similiar to mine, building debian first and copying the mm-folder to the alpine image.

@sthuber90
Copy link
Author

Alright, I was able to put a little something together. Didn't have the time to have a throughout look into your repo @khassel
Bildschirmfoto 2020-07-13 um 18 35 40
As you can see, Travis now builds MagicMirror version 2.12.0 on NodeJS 12 and 14 for Debian and Alpine with different tags. I mean this as a kinda proposal of what could be done. Currently, only buster based images get tagged with "latest" but not alpine to avoid any conflicts with solutions built on top of the current Debian based image.

To Do:

  • consistently tag 12-buster or 14-buster as "latest"
  • build and tag an image using MagicMirror develop branch
  • decide on next steps

Looking forward to your feedback and input

@Legion2
Copy link
Contributor

Legion2 commented Jul 14, 2020

@bastilimbach can we merge #45 first?

@Legion2
Copy link
Contributor

Legion2 commented Jul 14, 2020

we can also create a GitHub action which checks for new releases of Magic Mirror and creates a pull request here with the generated dockerfiles for the new release.

@khassel
Copy link
Contributor

khassel commented Jul 14, 2020

Looking forward to your feedback and input

I feel not very comfortable only commenting things here ...

  • may I missed something but I don't understand the need of templating. Why not using ARG in the Dockerfile to provide the TAG?
    ARG TAG
    FROM node:${TAG}
    
    And you can use other files as Dockerfile using the -f flag, e.g. docker buildx -f Dockerfile-${template} ...
  • if templating is needed, I think the results should be added to .gitignore
  • we already discussed that git is needed in the runtime image so AFAIS there is no need for a multistage build anymore
  • please use buster-slim instead of buster for debian

@sthuber90
Copy link
Author

The templating was inspired by the official Docker images available for NodeJS, Go, etc. and how those repositories are set up and how their images are build. It may not be necessary to got that far here, if your proposed ARG solution works.

Git will be added, as we discussed. Sorry, if I forgot it. Nonetheless, I think using multi-stage builds is valid. This way we will not package otherwise unneccessary files such as tests from the MagicMirror repo in the Docker image. Even though it may only be small size difference, I don't see the point why we shouldn't use multi-stage builds.

Instead of starting a discussion on buster-slim vs buster. Would it be acceptable to have a -slim tag?

Tag base image
2.12.0 node:lts-buster
2.12.0-slim node:lts-buster-slim
2.12.0-alpine node:lts-alpine

Instead of lts should the Node version be frozen to 12 (LTS) or 14 or just using the lts Docker tag?

@khassel
Copy link
Contributor

khassel commented Jul 21, 2020

I don't see the point why we shouldn't use multi-stage builds.

the idea was to do the stuff in one stage (see no image size diff to multi stage), this is untested:

FROM node:12-alpine

ENV NODE_ENV production

# get magic mirror
ARG branch=master
RUN set -o pipefail \
    apk update && apk add --no-cache git \
    && apk add --no-cache --update libintl \
    && apk add --no-cache --virtual build_deps gettext \
    && cp /usr/bin/envsubst /usr/local/bin/envsubst \
    && apk del build_deps \
    && mv /usr/local/bin/envsubst /usr/bin/envsubst
    && mkdir -p /tmp/magic_mirror \
    && cd /tmp/magic_mirror \
    && git clone --depth 1 -c advice.detachedHead=false -b ${branch} https://github.com/MichMich/MagicMirror.git .
    && mkdir -p /opt/magic_mirror \
    && cp -R config /opt/magic_mirror/default_config \
    && cp -R modules /opt/magic_mirror/default_modules \
    && npm set unsafe-perm true \
    && npm ci \
    # prune unnecessary files from ./node_modules, such as markdown, typescript source files, and so on. https://github.com/tj/node-prune
    && wget -q https://install.goreleaser.com/github.com/tj/node-prune.sh | sh \
    # it is intentional that modules are not copied to /opt/magic_mirror folder. Please keep alphabetically sorted
    && cp -R \
        .git \
        config \
        css \
        fonts \
        index.html \
        js \
        node_modules \
        package.json \ 
        package-lock.json \ 
        serveronly \
        translations \
        vendor /opt/magic_mirror \
    && rm -rf /tmp/magic_mirror

WORKDIR /opt/magic_mirror

EXPOSE 8080

The other questions (how many and which images) should be answered by the owner of this repo. We can think about many options, but he has to merge in the end ...

@sthuber90
Copy link
Author

We both just have different ways of solving the same problem, I guess. Personally, I am not so fond of big RUN statements. I agree with you though that whatever comes next needs to be @bastilimbach decision

@xbgmsharp
Copy link

That is great that this repository support multiple architecture.
However there is still a image size issue.
My docker image is half the bastilimbach/docker-magicmirror image for the same content.

# docker images | grep magic
xbgmsharp/docker-magicmirror      latest              1f351bd4aa49        20 hours ago        412MB
bastilimbach/docker-magicmirror   latest              95bfee9026c3        5 weeks ago         970MB

@sthuber90
Copy link
Author

@xbgmsharp as you can tell from the discussion, the changes I proposed to make the image smaller have not (yet) been merged.

@Legion2
Copy link
Contributor

Legion2 commented Nov 6, 2020

also see #48

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.

Use a smaller base image
5 participants