Refactor docker image and CI pipeline #1624

keeler · 2022-06-01T05:45:47Z

Summary

Refactors the docker image for better layer caching and refactors the github action CI pipeline to run tests and linting in a docker container. The CI checks now run in ~2m instead of ~7m, and the size of the docker image is about 70% what it was before (796MB --> 564MB). The steps in the CI pipeline are encapsulated in a Makefile so that it's easier to reproduce those steps locally.

Details

Restructures the Dockerfile to install dependencies first, then download card data, then add sources and do a webpack build. Previously, installing dependencies + downloading card data + doing webpack build happened at the very end so any time a source file changed the dependencies would be re-installed and card info would be re-downloaded even if they didn't need to, dramatically slowing down repeated iterations of docker builds.
Refactors config/version.js to prefer using a VERSION_INFO env var that gets built into the docker image. This is so the git command and .git folder don't need to be built into the docker image which improves layer caching and saves some space in the image and docker build context.
Moves several files from backend/ to backend/core/. These files are required by the scripts in scripts/ and were moved so that backend/core/ could be added to the docker image before the rest of the backend code. This makes it so if the backend code outside of backend/core/ changes, the downloader scripts do not need to be re-run.

Notes for Future Improvements

Stuff that was out of scope for the effort in this PR but would perhaps be worth doing.

Use multi-stage docker build. Make a stage for building & running tests and a 'production' stage that copies built artifacts out of the 'builder' stage and only installs prod dependencies and whatever else is the bare minimum to run the app. This would likely further reduce the image size. Maybe not by much, though, as the downloaded card data accounts for ~350MB of the image.

keeler · 2022-06-02T05:02:09Z

.dockerignore

 node_modules
-frontend/lib


Removed frontend/lib and data/scores.json because they don't appear to exist anymore.

Added ignores for .git and .github because they really shouldn't be built into the docker image anyway. In particular, having .git built into the image means that making a commit required rebuilding the image from the COPY . . step that existed previously, even if that commit changed no code (e.g. a README change) which is unnecessary.

This is feedback from another PR, specifically: - dr4fters#1622 (comment) - dr4fters#1622 (comment)

keeler · 2022-06-08T05:15:18Z

.dockerignore

@@ -1,4 +1,11 @@
-data/*
-!data/scores.json
+**/.git


Previously, the .git folder was added to the docker image for the sake of building in a version number, namely a commit hash to display in the footer. Now that version number is built in as an environment variable. The other ignores here leave out files that are irrelevant to the docker image build. Any time these files changed required the entire image to be rebuilt.

keeler · 2022-06-08T05:17:33Z

.github/workflows/tests.yml

        with:
-          node-version: ${{matrix.node_version}}
+          fetch-depth: 0


Setting fetch-depth: 0 makes it so git describe --tags works in the make docker step.

keeler · 2022-06-08T05:18:57Z

Dockerfile


-# Install "git"


Installing git was apparently done only to allow running git describe --tags in config/version.js. This is slow and bloats the size of the image, and is IMHO unnecessary just to get a commit hash in the footer.

Node images come with a `node` user already. See https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user

keeler · 2022-06-19T04:21:16Z

Dockerfile


 # Set working dir as /app
 WORKDIR /app

-# Add sources to /app
-COPY . .
-RUN adduser -S dr4ftuser


Node images already include a non-root user called node, so adding another one is redundant. https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user

keeler · 2022-06-19T04:24:48Z

Dockerfile


 # Set working dir as /app
 WORKDIR /app

-# Add sources to /app
-COPY . .


Running COPY . . is not a good idea IMHO. When building a docker image instructions like RUN, COPY, etc. will be cached as a 'layer' in the image. When running COPY . . any file changing invalidates the cache for this layer, meaning this and everything below it needs to re-run.

keeler · 2022-06-19T04:32:05Z

Dockerfile

-# Add sources to /app
-COPY . .
-RUN adduser -S dr4ftuser
-RUN chown dr4ftuser -R .


Running chown in a separate RUN instruction like this has a sneaky behavior where it actually copies files in layers above it, increasing the size of the image. The COPY instruction includes a --chown flag that does not trigger this behavior, hence why it's used all over the place here.

keeler · 2022-06-19T04:33:33Z

Dockerfile

+COPY --chown=node scripts/ scripts/
+RUN npm run download_allsets \
+ && npm run download_booster_rules \
+ && chown node -R data/


Including this chown in the same RUN instruction does not increase the size of the image like it would if it were in its own separate RUN instruction.

keeler · 2022-06-19T04:38:47Z

Dockerfile


 # Install the dependencies
-RUN npm ci


Running npm ci at the end in combination with the COPY . . instruction above it meant that any time any source file changed, the dependencies would need to get reinstalled and the card info would be re-downloaded again regardless of whether it was necessary. This happens because a change in any source file (or really any file) invalidates the layer cache of the COPY . . layer. This slowed down repeated iterations of docker builds significantly.

Now, the dependencies and card info are added to the image before most of this code. You can test the difference this makes by running make docker to build the image, changing a file in frontend/ in some innocuous way (like adding a comment), then running make docker again. You'll see that the build picks up from the COPY frontend/ layer, skipping all the layers above it that don't need to change, including the dependency installation and card info download.

keeler · 2022-06-19T04:46:33Z

Dockerfile

+ && npm run download_booster_rules \
+ && chown node -R data/
+
+# Add sources to /app


I tried to order these in such a way that more frequently-changing files would be copied in later layers so the layer cache invalidation would only apply to lower layers. Open to suggestions.

Also, I think the app.json file could be excluded here.

tooomm · 2022-06-20T17:34:14Z

That's a bunch of changes again, nice!

Looks like you combined everything in one docker run.
That also means we no longer test against various Node versions to ensure compatibility and spot issues.

The CI checks now run in ~2m instead of ~7m

I'm looking at improving the CI runs since a while in #1506 and they finish in 1min: https://github.com/dr4fters/dr4ft/actions/runs/2530256044
Total time is ~2min as they run in parallel.
But they do test against all actively maintained and available node versions, including the given one in the .nvmrc file.

What do you think about the node test matrix?
Nothing set in stone here. But maybe its worth combining those?

Refactors the docker image for better layer caching and (...) the size of the docker image is about 70% what it was before (796MB --> 564MB).

The docker improvements and the reworked layering and caching sounds useful when running the app in a container.
But I can't judge too much about that.

keeler · 2022-06-20T20:17:26Z

I'm looking at improving the CI runs since a while in #1506 and they finish in 1min: https://github.com/dr4fters/dr4ft/actions/runs/2530256044 Total time is ~2min as they run in parallel. But they do test against all actively maintained and available node versions, including the given one in the .nvmrc file.

What do you think about the node test matrix? Nothing set in stone here. But maybe its worth combining those?

Nice, I wasn't aware of that PR, thank you for sharing.

I don't understand the need to test against different node versions. Is it because developers might be using an older version of node? Wouldn't establishing and clearly documenting the specific node version to use be much easier than testing and supporting a bunch of different node versions? Ultimately, the app will run using a single node version (current LTS) and that's the one worth testing, and IMHO developers should have no expectation that a different major version of node will necessarily work. Am I being too uncompromising?

Mainly, I replaced the CI steps with docker because it's much easier to reproduce the entire CI pipeline locally when/if there are issues. Also, I assume https://dr4ft.info runs from a docker container? If so, to me that is more reason to build & run everything in the CI pipeline from docker.

The docker improvements and the reworked layering and caching sounds useful when running the app in a container.

The main benefits of the changes in this PR are:

Can repro CI pipeline locally.
Faster to iteratively re-build the docker image while developing or addressing CI issues.
Smaller image, so faster to push/download and lighter on disk space.

If you feel strongly about testing different node versions, it would be possible to combine our ideas (test multiple node versions + run everything from docker) by using different base images in the docker build, e.g. docker build --build-arg BASE_IMAGE=${{base-image-from-matrix}} with a Dockerfile like this:

ARG BASE_IMAGE=node:lts-alpine

FROM $BASE_IMAGE
...

tooomm · 2022-06-20T21:30:54Z

I don't think Zach runs dr4ft.info from a docker container. Generally everything here was not centred around docker due to lack of knowledge. The Dockerfile is only added to the CI to have at least some basic commands run and see if it works at all (and keeps working).

Isn't it common practice to test against various Node versions in CI?
It's not only about devs, but also people running the app. They might have a different version or other services with different requirements on the same machine. Maybe I'm wrong?
Docker should not be mandatory for sure, but an option.

But that are things devs working with the code and actual web and docker ninjas need to answer.

ZeldaZach · 2022-06-20T23:47:50Z

With a docker container, we only need to theoretically test against the version the Docker container supports. I'd consider switching over to a container system for the main deployment once it's stable. Thanks for the CR, if it's ready for review please let me know!

keeler · 2022-06-21T23:44:16Z

Isn't it common practice to test against various Node versions in CI?

For libraries/packages intended for use by other projects, definitely yes. For web apps like this in my experience you pick a node version and test only that version.

It's not only about devs, but also people running the app. They might have a different version or other services with different requirements on the same machine.

Doesn't nvm solve that problem already? Wouldn't publishing an 'official' container image solve that problem as well? Have users asked for support across different node versions? Or is this an assumption?

Docker should not be mandatory for sure, but an option.

I think we can agree on this.

keeler added 15 commits May 28, 2022 23:20

chore: node 12.16.3 -> 16.15.0

d267051

chore: npm audit fix

da19113

chore: Add .nvmrc

9e00685

feat: Add Makefile

d636bfb

chore: Github workflow use npm ci

bf5ca74

wip: Attempt at better docker build caching

5fbc461

docs: Makefile help target

b62b9d3

feat: Don't need git in docker image

92327e9

refactor: CI pipeline use docker image

cfeb1bf

fix: Hide news if no message of the day

d37a9f3

fix: Broke editorconfig

528a1c0

fix: Docker env from arg invalidates cache

2bddf77

fix: Shallow clone doesn't include tags

d7f1a04

fix: Use fetch-depth 0 to get tag history

2700269

chore: Undo npm audit fix for now

5e4c63c

keeler marked this pull request as ready for review June 1, 2022 05:48

keeler mentioned this pull request Jun 1, 2022

ISSUE-1616: Upgrade to node 16 #1622

Merged

keeler commented Jun 2, 2022

View reviewed changes

keeler added 2 commits June 2, 2022 00:16

feedback: Use lts-alpine base and 16.15.1

efff286

This is feedback from another PR, specifically: - dr4fters#1622 (comment) - dr4fters#1622 (comment)

chore: Undo changes moved to PR 1623

c39a7ab

keeler marked this pull request as draft June 2, 2022 05:25

keeler added 5 commits June 3, 2022 00:59

chore: Docker ignore Vim swap files

1ecd506

refactor: Move npm ci above copying source

36c7690

feat: Makefile can specify --no-cache in docker build

bba12ac

perf: Update cards/sets before copying sources

d1a1f7e

Merge branch 'master' into refactor-docker-and-ci-pipeline

de384b6

keeler commented Jun 8, 2022

View reviewed changes

fix: Save space by combine chown data/ command

14092a1

keeler added 3 commits June 18, 2022 20:23

chore: dr4ftuser/dr4ftgroup -> dr4ft

90788bf

fix: No need for dr4ft user

145f284

Node images come with a `node` user already. See https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user

Merge branch 'master' into refactor-docker-and-ci-pipeline

548a9e9

keeler commented Jun 19, 2022

View reviewed changes

keeler marked this pull request as ready for review June 19, 2022 04:54

chore: Spacing in Dockerfile & Makefile

57f5775

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor docker image and CI pipeline #1624

Refactor docker image and CI pipeline #1624

keeler commented Jun 1, 2022 •

edited

keeler Jun 2, 2022

keeler Jun 8, 2022 •

edited

keeler Jun 8, 2022

keeler Jun 8, 2022

keeler Jun 19, 2022

keeler Jun 19, 2022 •

edited

keeler Jun 19, 2022 •

edited

keeler Jun 19, 2022

keeler Jun 19, 2022 •

edited

keeler Jun 19, 2022

tooomm commented Jun 20, 2022

keeler commented Jun 20, 2022

tooomm commented Jun 20, 2022

ZeldaZach commented Jun 20, 2022

keeler commented Jun 21, 2022 •

edited

		node_modules
		frontend/lib

Refactor docker image and CI pipeline #1624

Are you sure you want to change the base?

Refactor docker image and CI pipeline #1624

Conversation

keeler commented Jun 1, 2022 • edited

Summary

Details

Notes for Future Improvements

keeler Jun 2, 2022

Choose a reason for hiding this comment

keeler Jun 8, 2022 • edited

Choose a reason for hiding this comment

keeler Jun 8, 2022

Choose a reason for hiding this comment

keeler Jun 8, 2022

Choose a reason for hiding this comment

keeler Jun 19, 2022

Choose a reason for hiding this comment

keeler Jun 19, 2022 • edited

Choose a reason for hiding this comment

keeler Jun 19, 2022 • edited

Choose a reason for hiding this comment

keeler Jun 19, 2022

Choose a reason for hiding this comment

keeler Jun 19, 2022 • edited

Choose a reason for hiding this comment

keeler Jun 19, 2022

Choose a reason for hiding this comment

tooomm commented Jun 20, 2022

keeler commented Jun 20, 2022

tooomm commented Jun 20, 2022

ZeldaZach commented Jun 20, 2022

keeler commented Jun 21, 2022 • edited

keeler commented Jun 1, 2022 •

edited

keeler Jun 8, 2022 •

edited

keeler Jun 19, 2022 •

edited

keeler Jun 19, 2022 •

edited

keeler Jun 19, 2022 •

edited

keeler commented Jun 21, 2022 •

edited