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

build: use alpine for parent image, not nimlang/nim #64

Merged
merged 6 commits into from
Apr 26, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Apr 21, 2021

I'll tidy up a few more things (e.g. the builder names; symlinking;
maybe use just two build stages instead of three), but I think this PR
is pretty close to complete.

The resulting Docker image is now smaller than any Nim image I found
in the wild.
I have some ideas to improve it further, but that'll be for a later PR.

The commit message is reproduced below.


This commit reduces the size of our Docker image. Previously, we were
using an image from the nimlang/nim Docker Hub repo that itself uses
alpine:latest. We now use an alpine image directly, thereby removing
some features/dependencies we don't need in the test runner environment.

Image size before this commit: 249.53 MB
Image size with this commit: 139.66 MB

Some advantages:

  • Reduced attack surface
  • Reduced time to download and extract the parent image
  • Reduced time to transfer our built image from/to the container
    registry
  • We can update Nim even if https://hub.docker.com/r/nimlang/nim wasn't
    updated yet
  • We can update Alpine even if there hasn't been a new Nim version

Disadvantages:

  • As we now build the Nim compiler ourselves, docker build now takes
    something like 5 minutes during CI, rather than something like 30
    seconds. We could add caching to the CI workflow if this becomes too
    inconvenient.
  • Some added complexity.

This commit removes from the image:

  • nodejs, and the ability to run nim js (compile Nim to JavaScript)
  • g++, which removes the ability to run nim cpp (compile Nim to C++)
  • The ability to run nim doc (generate documentation for a Nim file)
  • Some build dependencies, for example: curl, tar, xz
  • Unnecessary contents of the Nim installation

To build the Nim compiler, this commit adds a new script - this is
better than writing the commands in the Dockerfile directly because we
can:

  • run and test the script independently
  • run shellcheck on it

This commit also adds a shellcheck workflow.

This commit reduces the size of our Docker image. We were using a
`nimlang/nim` image that is itself based off `alpine:latest`, but we can
just use an `alpine` image directly to avoid some bloat.

Before: 249.53 MB
After:  139.60 MB

Other advantages:
- Reduced attack surface: we get more control over the image contents -
  we can remove things that we don't need in the test runner
  environment, like `nodejs`.
- We are no longer dependent on `nimlang/nim` for Nim version updates.
- We can update Alpine even if there hasn't been a new Nim version.

Disadvantages:
- `docker build` now takes something like 5 minutes during CI, rather
  than something like 30 seconds.
- Some added complexity.

This commit removes:
- `nodejs`
- The C++ compiler. `nim cpp` is now impossible
- The ability to run `nim doc`
- The ability to run `nim js`
- Some build dependencies: `curl`, `tar`, `xz`
- Unnecessary Nim directories

With this commit, we now build Nim ourselves. We do this by adding a
script, which is better than inlining the commands in the Dockerfile
because we can:
- run and test the script independently.
- run `shellcheck` on it.

This commit also adds a `shellcheck` workflow.
@ynfle
Copy link
Contributor

ynfle commented Apr 21, 2021

Is the 5 minutes delay worth it?
Do we need the optimizations performed here?

@ee7
Copy link
Member Author

ee7 commented Apr 22, 2021

Is the 5 minutes delay worth it?

Absolutely. Improvements at run-time are often worth costs at build-time. And here I'd say that the improvement is big and the cost is small - I can probably minimize that extra build time anyway (Edit: see below post).

Do we need the optimizations performed here?

"need" is a strong word, but a near-2x reduction in the image size is a pretty big win. For security reasons, it's best practice for an image to contain as little as possible. This is especially important for our use case: running untrusted, user-provided code. Containers have enough security problems already.

And from the exercism docker docs:

The Dockerfile should create the minimal image needed for the tooling to function correctly and speedily.

The Dockerfile should produce an image with as a small a size as possible while maximising (and prioritising) performance.

The end-user may also benefit from a faster container startup time, depending on how things are deployed.

It also doesn't hurt financially. Exercism is using Amazon ECR, but hopefully Exercism optimizes its usage to suit their pricing anyway. (Edit: apparently our current usage of ECR is indeed "basically free")

I should have mentioned that I can add caching to the CI workflow, which theoretically means we'd only see that extra time when e.g. the Nim version is updated. We already have caching in the deployment workflow, see:

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@154c24e1f33dbb5865a021c99f1318cfebf27b32 # 1.1.1
- name: Cache Docker layers
uses: actions/cache@26968a09c0ea4f3e233fdddbafd1166051a095f6 # 2.1.4
with:
path: /tmp/.buildx-cache
key: ${{ runner.os }}-buildx-${{ github.sha }}
restore-keys: |
${{ runner.os }}-buildx-

The main hurdle there is that the cache is deleted if it isn't accessed for a week (that's pretty short - it was 45 days on Travis CI). We can keep it alive via a scheduled job, which might be nice to have anyway. Edit: Actually, we'd want the scheduled job to periodically rebuild the cache, at least while we're using Alpine.

@ee7 ee7 marked this pull request as ready for review April 25, 2021 08:18
@ee7 ee7 requested a review from a team as a code owner April 25, 2021 08:18
@ee7 ee7 requested a review from ynfle April 25, 2021 08:19
@ee7
Copy link
Member Author

ee7 commented Apr 25, 2021

Made some tweaks - ready for review.

I'd like to omit caching for now - we can consider adding it later.

Another problem with adding caching is that we want to use the latest versions of packages in order to get security updates (we can't reliably pin package versions on Alpine anyway), which means that we want to periodically invalidate the cache. We can do that by either:

  • leaving more than 7 days between docker build runs (so that the cache action deletes the cache)
  • adding a scheduled job for every 6 days that runs docker build --no-cache

(Note that this also applies to the existing caching in the deployment workflow).

But overall, I think it would take longer to set-up + test + maintain than the time it would save, so it's a low priority for me.

Bumping the Nim version should be a separate change.
@ee7 ee7 merged commit fc024ce into exercism:main Apr 26, 2021
@ee7 ee7 deleted the build-use-alpine-directly branch April 26, 2021 13:27
@ee7 ee7 changed the title build: use alpine directly, not nimlang/nim build: use alpine for parent image, not nimlang/nim Apr 26, 2021
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