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

Improve to use multi-arch images when building services #250

Closed
wants to merge 3 commits into from

Conversation

glours
Copy link
Member

@glours glours commented Oct 4, 2022

I updated redis, postgres images and all the base images used in Docker files
Also rename docker-compose-* files to compose-* to follow the Compose specification.
And used --platform=BUILDPLATFORM to be sure the user platform will be use when building the stack

glours added 3 commits October 4, 2022 15:14
…tion step is done using the user platform

Signed-off-by: Guillaume Lours <[email protected]>
@glours glours requested review from BretFisher and a team and removed request for BretFisher October 4, 2022 15:06
@@ -1,4 +1,4 @@
FROM python:3.9-slim
FROM --platform=$BUILDPLATFORM python:3.10-bullseye
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the --platform=$BUILDPLATFORM now prevent me from using QEMU to build for other architectures?

--platform=$BUILDPLATFORM works if the stage does a cross-compile (so toolset is "native" but the artefacts produced are for another architecture), but in this case, no cross-compile is happening, which means that if we would build with docker build --platform=linux/amd64 on an arm64 machine, we would produce an image with linux/amd64, but the content actually being linux/arm64

docker-compose -f docker-compose-windows.yml build
docker-compose -f compose-windows.yml build
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit on the fence on renaming docker-compose*.yml to compose.yml (also still curious what led to the decision to make it the recommended name); I think docker-compose.yml is more familiar to users (and more recognisable) than compose.yml. (every time I see compose.yml, I confuse it for composer.json

FROM node:10-slim
FROM --platform=$BUILDPLATFORM node:18.0-bullseye-slim
Copy link
Member

Choose a reason for hiding this comment

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

While we're updating these files, we should probably add a # syntax line, to follow best practice (and to make sure we're always building with the current syntax); (Also see my other comment w.r.t. --platform)

Suggested change
FROM node:10-slim
FROM --platform=$BUILDPLATFORM node:18.0-bullseye-slim
# syntax=docker/dockerfile:1
FROM node:18.0-bullseye-slim

Some other suggestions:

  • As we're not always keeping a close eye on this repository to keep it up-to-date, perhaps we should add build-args (ARG) to allow overriding versions
  • That said; I'm a bit on the fence on adding the -bullseye. The "pro" is that the image wouldn't unexpectedly switch to bookworm (debian 12), but I think for these examples, things would likely "just work" with newer versions. Pinning to bullseye may also mean that (if we do add build-args) switching to (say) Node 27.0 may mean that there's no longer a bullseye variant (but only a bookworm variant).
Suggested change
FROM node:10-slim
FROM --platform=$BUILDPLATFORM node:18.0-bullseye-slim
# syntax=docker/dockerfile:1
ARG NODE_VERSION=18.0
FROM node:${NODE_VERSION}-slim

I wonder if we should switch back to -alpine (looks to be still ~30MB smaller); I see it was changed from -alpine to -slim in #140, because there was a bug in npm (npm/uid-number#7), but that one looks to be fixed now.

@@ -57,7 +57,7 @@ services:
- back-tier

db:
image: postgres:9.4
image: postgres:12.12-bullseye
Copy link
Member

Choose a reason for hiding this comment

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

I see this compose file bind-mounts various things, including health-checks; would it be "better practice" to have a build: for these, so that the healtchchecks are part of the image? That way;

  • the compose file can also run on a remote Docker Engine, not just on a local machine
  • the versions are defined in the Dockerfile (not spread over Dockerfile and compose-file)

@@ -1,4 +1,4 @@
FROM node:10-slim
FROM --platform=$BUILDPLATFORM node:18.0-bullseye-slim

# add curl for healthcheck
RUN apt-get update \
Copy link
Member

Choose a reason for hiding this comment

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

(can't comment on the lines further down)

Perhaps we should make this a multi-stage Dockerfile, and move the tini download to a separate stage that we --copy from 🤔

# Add Tini for proper init of signals
ENV TINI_VERSION v0.19.0
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
RUN chmod +x /tini

Now that ADD also supports --chmod, we can skip the RUN, so it can be a FROM scratch;

FROM scratch AS tini
ARG TINI_VERSION=v0.19.0
ADD --chmod=0755 https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini

Copy link
Member

Choose a reason for hiding this comment

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

Erm... looks like there's more to fix; this doesn't take multi-arch into account, and always downloads x86 (also looks like it's not statically linked?); https://github.com/krallin/tini/releases/tag/v0.19.0

docker build -t foo -<<'EOF'
FROM scratch AS tini
ARG TINI_VERSION=v0.19.0
ADD --chmod=0755 https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
EOF

docker run --rm foo /tini --version
qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory

And doesn't work on alpine 😞

ldd /tini
	/lib/ld-linux-aarch64.so.1 (0xffffb203e000)
	libc.so.6 => /lib/ld-linux-aarch64.so.1 (0xffffb203e000)
Error relocating /tini: __fprintf_chk: symbol not found

This looks to work though;

FROM scratch AS tini
ARG TINI_VERSION=v0.19.0
ARG TARGETARCH
ADD --chmod=0755 https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini-${TARGETARCH} /tini

FROM debian:bullseye-slim
COPY --from=tini /tini /tini
docker run --rm foo /tini --version
tini version 0.19.0 - git.de40ad0
docker run --rm foo /tini --version
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
tini version 0.19.0 - git.de40ad0

@mikesir87
Copy link
Member

Hey all! Thanks for the PR @glours and the conversation. Seeing there are unresolved conversations, a lot of updates in a single PR, and the PR is in conflict, we're going to go ahead and close this one. But, there are several pieces to this PR we do want to bring forward in future PRs.

@mikesir87 mikesir87 closed this Dec 16, 2022
@BretFisher
Copy link
Collaborator

Thanks, @glours and @thaJeztah for this effort. We've resolved some of the issues addressed in this PR over the last few weeks and are creating tasking to ensure we include the rest of @glours ideas.

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.

4 participants