perf(image): link against musl and land on the static base - #347
Conversation
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
There was a problem hiding this comment.
The runtime claims hold up where they can be checked from the tree: reqwest is default-features = false with webpki-roots, rustls is pinned to ring, jsonwebtoken to rust_crypto, zstd builds its vendored C, and nothing pulls openssl or native-tls, so distroless/static carries everything the binary asks for. binaries.yml already ships both musl targets, so the image is not the first musl build of this code.
The workflow pin bump from 2ea7662 to 15286b3 adds build-ulimits and nothing else, and the reusable job passes it through as buildah --ulimit only when set, so the site jobs on the same pin are unaffected. The zig 0.14.1 tarball URL resolves and cargo-zigbuild 0.23.4 exists on crates.io.
Nothing blocking. Three nits inline: the tool-install layer sits below the source COPY and so is rebuilt on every release, the zig tarball is unverified, and no CI job on this PR runs the image it builds.
| chmod +x /usr/local/bin/sccache; \ | ||
| rm -rf /tmp/sccache* | ||
|
|
||
| # The binary is linked against musl so the runtime image can be the static one, |
There was a problem hiding this comment.
Nit: this block sits below COPY server ./server / COPY cli ./cli, so any source change invalidates it and the build re-downloads zig (~45 MB) and recompiles cargo-zigbuild from source, once per platform. A release bumps the version line in server/Cargo.toml, which is the first COPY, so that is every release build.
Nothing in either tool install depends on the sources. Moving this block and the sccache one above the three COPY lines makes them layers the registry cache can actually reuse. Left as prose rather than a suggestion because it is a move across lines the diff does not all cover.
| ARG ZIG_VERSION=0.14.1 | ||
| ARG ZIGBUILD_VERSION=0.23.4 | ||
| RUN set -eux; \ | ||
| url="https://ziglang.org/download/${ZIG_VERSION}/zig-x86_64-linux-${ZIG_VERSION}.tar.xz"; \ | ||
| curl -fsSL "$url" -o /tmp/zig.tar.xz; \ | ||
| mkdir -p /opt/zig; \ | ||
| tar -xJf /tmp/zig.tar.xz -C /opt/zig --strip-components=1; \ | ||
| ln -s /opt/zig/zig /usr/local/bin/zig; \ | ||
| rm /tmp/zig.tar.xz; \ | ||
| cargo install cargo-zigbuild --version "${ZIGBUILD_VERSION}" --locked |
There was a problem hiding this comment.
Nit: the tarball that becomes the C toolchain for the published binary is extracted with no integrity check, so a compromised or truncated download is linked into the release image and the build still passes. ziglang.org publishes the sum in its download index; keeping it in an ARG next to the version means they get updated together.
| ARG ZIG_VERSION=0.14.1 | |
| ARG ZIGBUILD_VERSION=0.23.4 | |
| RUN set -eux; \ | |
| url="https://ziglang.org/download/${ZIG_VERSION}/zig-x86_64-linux-${ZIG_VERSION}.tar.xz"; \ | |
| curl -fsSL "$url" -o /tmp/zig.tar.xz; \ | |
| mkdir -p /opt/zig; \ | |
| tar -xJf /tmp/zig.tar.xz -C /opt/zig --strip-components=1; \ | |
| ln -s /opt/zig/zig /usr/local/bin/zig; \ | |
| rm /tmp/zig.tar.xz; \ | |
| cargo install cargo-zigbuild --version "${ZIGBUILD_VERSION}" --locked | |
| ARG ZIG_VERSION=0.14.1 | |
| ARG ZIG_SHA256=24aeeec8af16c381934a6cd7d95c807a8cb2cf7df9fa40d359aa884195c4716c | |
| ARG ZIGBUILD_VERSION=0.23.4 | |
| RUN set -eux; \ | |
| url="https://ziglang.org/download/${ZIG_VERSION}/zig-x86_64-linux-${ZIG_VERSION}.tar.xz"; \ | |
| curl -fsSL "$url" -o /tmp/zig.tar.xz; \ | |
| echo "${ZIG_SHA256} /tmp/zig.tar.xz" | sha256sum -c -; \ | |
| mkdir -p /opt/zig; \ | |
| tar -xJf /tmp/zig.tar.xz -C /opt/zig --strip-components=1; \ | |
| ln -s /opt/zig/zig /usr/local/bin/zig; \ | |
| rm /tmp/zig.tar.xz; \ | |
| cargo install cargo-zigbuild --version "${ZIGBUILD_VERSION}" --locked |
The sum is the one https://ziglang.org/download/index.json gives for 0.14.1 / x86_64-linux. The sccache install above has the same gap, which is pre-existing and not for this PR to fix.
| # emulated one, so the second platform costs a compile and not an hour. | ||
| platforms: linux/amd64,linux/arm64 | ||
| sccache-gha: true | ||
| build-ulimits: nofile=65536:65536 |
There was a problem hiding this comment.
Nit: verify builds both platforms and never runs the result, so the acceptance criterion in the description proves the musl link succeeds and says nothing about whether the static image starts. Changing libc and base image in one commit is the case where a binary links cleanly and then fails on first execution, for a missing loader path or an empty /etc/passwd entry rather than a compile error.
The release build job's smoke-test-cmd gates the push, so the failure mode is a broken release rather than a broken published image, which is why this is a nit. Closing it needs a second amd64-only verify job carrying the same smoke-test-cmd: the reusable workflow builds into the local store for the smoke test and documents that as single-platform, so it cannot be added to this job without giving up the arm64 cross-compile check that #331 was about.
There was a problem hiding this comment.
ecb4cfc is right: reusable-docker-build declares all five permissions at workflow level, so a caller job has to grant the same set or the run never starts. verify and verify-site do start now.
That is what exposed the next thing: Build without publishing / hadolint fails, so Build & push was skipped and the acceptance criterion in the description is still unproven on both architectures. One blocking comment inline.
Nothing else changed since the last review.
There was a problem hiding this comment.
f2b2c44 clears the blocking finding: hadolint is green and Build without publishing / Build & push is running rather than skipped. Thread resolved.
The acceptance criterion is not met yet, only unblocked. Build & push is still in progress at the time of this review, so neither the amd64 nor the arm64 musl link has finished. Approving the change, not the evidence: hold auto-merge until both legs are green, as the description already says.
Nothing else changed since the last review.
|
Verified end to end before merging, since CI proves the image builds but never that it runs: Built DNS and TLS both work from The allocator question is the one the hardware cannot answer. No size claim from here. Docker 29 reports disk usage rather than content size, so what I measured locally is not comparable to the 15.4 MB in #327, which is what the registry serves. The number that belongs in the README is the published compressed one, and I will take it off GHCR after the release rather than quote a laptop. |
f2b2c44 to
b0e45a6
Compare
|
All three taken, and the nit on #349 too. Toolchain layers moved above the COPY lines. You were right that this was every release build, not an edge case: the version bump in The zig tarball is checksummed. I did not take the sum from the review: Added #349's comment is corrected here rather than in a fourth PR, since it is two lines in a file this branch already touches. It now says the top level stays read and that every job calling the reusable build grants the full set regardless of |
Second attempt at the musl half of #327. The first one (#329) built the image on a laptop and died in CI, and #331 reverted it.
The size win was never in doubt: 4.4 MB against 12.8 MB, measured locally on both architectures. What failed was the method.
cargo-zigbuildcompiles compiler_rt and libunwind for the target and holds a descriptor open per source, and buildah gives aRUN1024 of them, soft and hard. Docker Desktop hands out 1048576, which is why every local build passed:A hard limit cannot be lifted from inside the process it constrains, so
ulimit -nin the Dockerfile was never going to work. FerrLabs/.github#327 added abuild-ulimitsinput to the reusable image workflow, and this passesnofile=65536:65536to it. The Dockerfile now prints the limit it got rather than trying to change it, so the next time this class of failure happens the number is in the log.The rest is #329 restored unchanged: musl targets for both architectures through
cargo zigbuild,distroless/static-debian12instead ofcc-debian12, and the zig cache pointed at/tmpbecause the default under$HOMEis not writable in the rootless builder.What was checked
Nothing needs glibc at runtime. rustls is pinned to ring, the trust roots are compiled in through webpki-roots rather than read from a system store, and zstd is statically linked.
distroless/staticcarries neither glibc nor libgcc, which is where the ten megabytes go.The allocator. musl's is slower than glibc's under heavy multithreaded allocation, which is the shape of this server.
bench.ymlgrewhostandmusllegs for exactly this question and the answer is that the runners cannot settle it: four runs of the same commit spread upload from 150 to 273 MiB/s and small objects from 1.3 to 16 ms. There is no regression visible above that noise, and there is no measurement that would justify mimalloc either.docs/performance.mdsays so rather than quoting a number the hardware did not earn.DNS. musl resolves through
/etc/resolv.confand does no NSS, which is what a Kubernetes pod provides anyway. The forge lookups go through that path.Acceptance
A green
Build without publishingon bothlinux/amd64andlinux/arm64in this CI, not on a laptop. That is what #331 taught. Auto-merge stays off until the arm64 leg is green.Part of #327