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

Slim Down Docker Image (Attempt 2) #4481

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Nov 5, 2024

What it solves

This is a long awaited follow up to #3707, which reduces the Docker image size of the web interface from ~6GB to ~70MB. This iteration addresses build issues that it introduced in the previous version.

In particular, we now use a multi-stage Docker build, where building the web assets is done on the BUILDPLATFORM (i.e. the native build platform) instead of the target platform. This means that the image for linux/arm64 will first build the front-end on the native architecture of the CI runner (linux/amd64 by default) before copying. This should make building the Docker image MUCH quicker, while still producing a multi-platform image, and avoid the Docker build timeouts that were happening in CI. Documentation on how BUILDPLATFORM works and can be used for cross-compilation can be found here.

How this PR fixes it

The docker image size is significantly reduced by using a multistage build where the first stage builds the static website, and the actual image just hosts the static webside (here using BusyBox httpd which is nice and lightweight).

This gets the image down to around 70MB, around 1% of the original image size (sizes computed from Docker images built on amd64 Linux).

Note that there is one weird detail is that static HTTP servers typically don't have support for automatically adding .html endings to URL paths. It was worked around here with symlinks. See comment in the Dockerfile for more details.

How to test it

Build the docker image and run the container locally:

docker build . -t safe-wallet-web
docker run --rm -p 3000:3000 safe-wallet-web

This should give you a local interface at http://localhost:3000.

It was also able to successfully build in CI without timing out:

image

"Screenshots"

$ docker images
REPOSITORY                            TAG          IMAGE ID      CREATED             SIZE
localhost/safe-wallet-web             latest       a78bd066aacd  About a minute ago  67.1 MB

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
    • Analytics are not affected
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻
    • Not applicable

Copy link

github-actions bot commented Nov 5, 2024

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@nlordell
Copy link
Contributor Author

nlordell commented Nov 5, 2024

Copy link

github-actions bot commented Nov 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
74.08% (+0% 🔼)
13902/18767
🔴 Branches 52.71% 3424/6496
🔴 Functions
57.92% (+0.02% 🔼)
2029/3503
🟡 Lines
75.75% (+0.01% 🔼)
12631/16674
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟡
... / useSubmission.tsx
57.14% 0% 0% 61.54%

Test suite run success

1593 tests passing in 215 suites.

Report generated by 🧪jest coverage report action from 270b583

RUN apk add --no-cache libc6-compat git python3 py3-pip make g++ libusb-dev eudev-dev linux-headers
WORKDIR /app
COPY . .

# Fix arm64 timeouts
RUN yarn config set network-timeout 300000 && yarn global add node-gyp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer needed, we build on native arch.

@@ -1,24 +1,39 @@
FROM node:18-alpine
FROM --platform=$BUILDPLATFORM node:18-alpine AS build
ARG BUILDPLATFORM
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a built-in Docker arg that resolves to the native platform of the builder: https://docs.docker.com/build/building/multi-platform/#cross-compilation

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that, good to know

@nlordell nlordell force-pushed the chore/slim-down-docker-image-2 branch from 998d414 to 270b583 Compare November 5, 2024 16:20
@nlordell nlordell requested review from schmanu, katspaugh and Uxio0 and removed request for schmanu November 7, 2024 10:32
@Uxio0
Copy link
Member

Uxio0 commented Nov 7, 2024

@nlordell but this way we will not be able to set up environment variables on the run, right? Like we do here when running the docker image: https://github.com/safe-global/safe-infrastructure/blob/main/container_env_files/ui.env

@nlordell
Copy link
Contributor Author

nlordell commented Nov 8, 2024

Ah, I hadn't noticed (I thought it was just serving static files), but you are that it will not.

It looks like it does a yarn build to rebuild the interface on static-serve on container startup. So the container ships both a development and build environment including pre-built static files, instead of just the Safe{Wallet} interface. Doing a docker pull for a 6GB image is too much IMO. I would argue that safe-infrastructure could be changed to something like:

  ui:
    build: https://github.com/safe-global/safe-wallet-web.git#${UI_VERSION}
    env_file:
      - container_env_files/ui.env
    depends_on:
      - nginx
    expose:
      - 8080

The alternative, would be to implement env value substitution ONLY before serving the static
files (instead of pulling in the entire DEV environment). It may be possible with Next.js standalone builds.

@Uxio0
Copy link
Member

Uxio0 commented Nov 8, 2024

a docker pull for a 6GB image is too much IMO.

Agree, but also building was so slow for M1 computers in the past

It may be possible with Next.js standalone builds.

I tried that in the past but I couldn't make it, maybe @katspaugh knows a workaround

@nlordell
Copy link
Contributor Author

nlordell commented Nov 8, 2024

It may be possible with Next.js standalone builds

Just tried it out, and it isn't because of how the environment variables are used (they are directly replaced in the static files). Some hand-rolled thing could work - but may be brittle.

Agree, but also building was so slow for M1 computers in the past

I think the speed should be similar-ish to pulling a 6GB image 😅. I guess one strategy would be to split the build into different layers so that when replacing the .env file, most of the layers can be used from cache (so rebuilding is relatively inexpensive).

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.

2 participants