-
Notifications
You must be signed in to change notification settings - Fork 773
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
feat: Refactor Dockerfile
#3372
base: main
Are you sure you want to change the base?
Conversation
- Simplify and reduce layers where possible via HEREDOC syntax for `RUN` statements + DRY `COPY`. - `--target` is redundant during rustup install, instead only handle via `rustup target add`. - Shift order of directives such as ENV to minimize layers invalidated.
This may be easier to grok, avoiding the loop to build concurrently instead. Introduces more stages with fixed ENV instead of prior conditional branch with txt file. Still shares a common base with both target triples installed.
Brings the binary down to 24MB (AMD64) and 21MB (ARM64), reducing binary size by more than 70%. This could be further reduced.
Tagging @konstin for this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some contextual feedback, apologies for the verbosity. It's roughly what I've already covered in the PR description, but targeted, along with some additional insights if helpful.
Might help ease reviewing the diff directly 😎
COPY Cargo.toml Cargo.lock . | ||
ARG APP_NAME=uv | ||
ARG CARGO_HOME=/usr/local/cargo | ||
ARG RUSTFLAGS="-C strip=symbols -C relocation-model=static -C target-feature=+crt-static -C opt-level=z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used the RUSTFLAGS
ENV here, although you can configure these in Cargo.toml
+ .cargo/config.toml
(relocation-model
and target-feature
would still be rustflags IIRC). Presumably this is only relevant to the CI / Docker builds, so they're probably better managed here?
- I'm not sure if
opt-level
withz
(best size) is ideal? It should probably be compared/benched against a build for performance, since that is a focus ofuv
I doubt a little size savings is worth it if the performance regresses quite a bit. I'm not familiar with this project to know how you'd want to verify the impact. +crt-static
isn't necessary at the moment for themusl
targets being built, but there has been talk about future changes for these targets to default to dynamic linking, so I've included it as more of a precaution, it also better communicates the desire for a static linked build.relocation-model=static
tends to help save on binary size, I think this is ok. The default AFAIK is more of a security feature for memory layout, but I'm not sure if that's a concern foruv
as an attack vector (some attacker with access to read memory). It's related to PIE if you're familiar with that.
ARG TARGETPLATFORM | ||
RUN \ | ||
--mount=type=cache,target="/root/.cache/zig",id="zig-cache" \ | ||
# Cache mounts (dirs for crates cache + build target): | ||
# https://doc.rust-lang.org/cargo/guide/cargo-home.html#caching-the-cargo-home-in-ci | ||
# CAUTION: As cargo uses multiple lock files (eg: `${CARGO_HOME}/{.global-cache,.package-cache,.package-cache-mutate}`), do not mount subdirs individually. | ||
--mount=type=cache,target="${CARGO_HOME}",id="cargo-cache" \ | ||
# This cache mount is specific enough that you may not have any concurrent builds needing to share it, communicate that expectation explicitly: | ||
--mount=type=cache,target="target/",id="cargo-target-${APP_NAME}-${TARGETPLATFORM}",sharing=locked \ | ||
# These are redundant as they're easily reconstructed from cache above. Use TMPFS mounts to exclude from cache mounts: | ||
# TMPFS mount is a better choice than `rm -rf` command (which is risky on a cache mount that is shared across concurrent builds). | ||
--mount=type=tmpfs,target="${CARGO_HOME}/registry/src" \ | ||
--mount=type=tmpfs,target="${CARGO_HOME}/git/checkouts" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache mounting here is a bit excessive, I've added some context with comments. You're not really going to need to maintain much here AFAIK, so it's mostly out of sight/mind in practice.
If you're not familiar with this feature, the default sharing
type is shared
, which allows other concurrent builds to share the same data for each mount by matching id
, if no id
is configured it implicitly uses the target
path.
We have 3 caches here, zig
, cargo
, and the project specific target/
directory.
zig
I'm not familiar with it's cache system. I assume it's a global cache from the path and that it's fine for concurrent writers (akasharing=shared
, the implicit default).cargo
mounts the entire cargo home location as it doesn't yet have an isolated cache location.- This does mean if any other
Dockerfile
used theid
ofcargo-cache
they'd both be sharing the same contents, including whatever is installed inbin/
. - It'd normally not be safe for concurrent writers AFAIK, but due to the lock files cargo manages here, it's a non-issue.
- This does mean if any other
- The
target/
location has anid
assigned perTARGETPLATFORM
, and is also isolated byAPP_NAME
(uv
) as this is not useful to share with otherDockerfile
unrelated touv
project.- While there shouldn't be any concurrent writers, if someone was to to do several parallel builds with
RUSTFLAGS
arg being changed, thesharing=locked
will block them as they'd all want to compile the dependencies again (making the cache a bit less useful, but not as wasteful as individual caches viasharing=private
which aren't deterministic for repeat builds). - A previous commit did take a different approach, where the build process was part of the original builder stage and built both targets together, instead of relying on
TARGETPLATFORM
. If you don't mind always building for both targets, they could share a bit of a commontarget/release/
cache contents AFAIK. The logic for the two commands below was complicated a bit more with a bash loop (since static builds require and explicit--target
AFAIK, even when multiple targets are configured viarust-toolchain.toml
/.cargo/config.toml
?), so I opted for keeping the logic below simple to grok by using separateTARGETPLATFORM
caches (these can still build concurrently as I demo'd in the PR description).
- While there shouldn't be any concurrent writers, if someone was to to do several parallel builds with
The last two tmpfs
mounts are rather self-explanatory. There is no value in caching these to disk.
If you do want something that looks simpler, Earthly has tooling to simplify rust builds and cache management. However Dockerfile
is more common knowledge to have and troubleshoot/maintain, adding another tool/abstraction didn't seem worthwhile to contribute.
<<HEREDOC | ||
cargo zigbuild --release --bin "${APP_NAME}" --target "${CARGO_BUILD_TARGET}" | ||
cp "target/${CARGO_BUILD_TARGET}/release/${APP_NAME}" "/${APP_NAME}" | ||
HEREDOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using --release
, but I noticed in your Cargo.toml
you have a custom release profile that uses lto = "thin"
, it could be changed to use that profile instead, otherwise LTO should implicitly default to thin-local.
# Handle individual images differences for ARM64 / AMD64: | ||
FROM builder-base AS builder-arm64 | ||
ENV CARGO_BUILD_TARGET=aarch64-unknown-linux-musl | ||
|
||
FROM builder-base AS builder-amd64 | ||
ENV CARGO_BUILD_TARGET=x86_64-unknown-linux-musl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is effectively what you were doing earlier with the bash conditional statement on TARGETPLATFORM
arg. But instead of writing to a file (rust_target.txt
), a new intermediary stage is introduced and sets the expected target as an appropriate ENV.
This actually removes the need for the --target
option when building, but I kept it for clarity.
The next stage (builder-app
) refers to the TARGETARCH
implicit arg from BuildKit. Thus when building the Dockerfile
the stage selection here is determined from the final stage being built.
It'll remain as being built from the same BUILDPLATFORM
above, but diverge at this point. Due to the cache mounts used in builder-app
, this isn't a big concern, you really only have a few MB repeated from the COPY
instruction, followed by the binary cp
(since the target/
cache mount excludes the original binary from the image).
A prior commit in this PR history had an alternative approach where both targets were built, and these separate stages were located at the end of the Dockerfile
, where they instead used COPY --from
with the location to their respective target binary.
RUN <<HEREDOC | ||
# Install rustup, but skip installing a default toolchain as we only want the version from `rust-toolchain.toml`: | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal --default-toolchain none | ||
|
||
# When rustup installs the toolchain ensure it actually uses the minimal profile, avoiding excess layer weight: | ||
# https://github.com/rust-lang/rustup/issues/3805#issuecomment-2094066914 | ||
echo 'profile = "minimal"' >> rust-toolchain.toml | ||
echo 'targets = [ "aarch64-unknown-linux-musl", "x86_64-unknown-linux-musl" ]' >> rust-toolchain.toml | ||
# Add the relevant musl target triples (for a building binary with static linking): | ||
# Workaround until `ensure` arrives: https://github.com/rust-lang/rustup/issues/2686#issuecomment-788825744 | ||
rustup show | ||
HEREDOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When installing rustup
in the first line, as the commit history notes (and the PR description), setting --target
was redundant, it emits a warning that it will ignore it.
Likewise there was a mishap with the --profile minimal
being ignored when rust-toolchain.toml
exists and doesn't define it's own profile
key. If you'd like that file could add this line instead? I've raised an issue upstream to get the regression in rustup
fixed.
The next line added to rust-toolchain.toml
is probably not something you'd want to add to the actual file, since both targets aren't mandatory 🤷♂️
- I could instead add them individually with
rustup target add ...
in their respective stages that come afterwards - Due to the
--default-toolchain none
this still requires therustup show
workaround, otherwise you'd install the toolchain twice. An alternative is to use an ARG directive for passing in the toolchain version to use for--default-toolchain
, that way you could just build with the current stable release, but if you really need to install a toolchain that matchesrust-toolchain.toml
, provide that via the ARG instead?
RUN \ | ||
--mount=target=/var/lib/apt/lists,type=cache,sharing=locked \ | ||
--mount=target=/var/cache/apt,type=cache,sharing=locked \ | ||
<<HEREDOC | ||
# https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#example-cache-apt-packages | ||
# https://stackoverflow.com/questions/66808788/docker-can-you-cache-apt-get-package-installs#comment135104889_72851168 | ||
rm -f /etc/apt/apt.conf.d/docker-clean | ||
echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache | ||
|
||
apt update && apt install -y --no-install-recommends \ | ||
build-essential \ | ||
curl \ | ||
python3-venv \ | ||
cmake | ||
HEREDOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache mount here (and workaround for docker-clean
in this base image used), doesn't bring any savings to disk usage of this layer. It does however keep the cache external instead of the automatic clean, which is useful when the layer is invalidated, as it speeds up the build time.
ARG RUSTFLAGS="-C strip=symbols -C relocation-model=static -C target-feature=+crt-static -C opt-level=z" | ||
ARG TARGETPLATFORM | ||
RUN \ | ||
--mount=type=cache,target="/root/.cache/zig",id="zig-cache" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--mount=type=cache,target="/root/.cache/zig",id="zig-cache" \ | |
--mount=type=cache,target="$HOME/.cache/zig",id="zig-cache" \ |
Worth using $HOME
to be explicit
Hi, and thanks for looking into our dockerfile! I have two main concerns with the changes: Docker is a secondary target for us (most users are using the curl, powershell or pip installer), so we'd like to keep it simple to maintain. The main audience are projects with a fully dockerized CI pipeline who want to run ruff or uv as a docker step; to people creating docker images we recommend the pip or curl installers inside the dockerfile. We don't intend for users to build the uv container themselves, so we haven't optimized for builder size. There is one big exception though: Building the image for the release job is the bottleneck in our release process, if that could be improved that would be amazing! Performance is crucial for us, and we optimize performance over binary size. |
TL;DR: I'm happy to adjust the I can look into optimizing the workflow to bring down that bottleneck time with compilation, but I suspect the pragmatic approach might just be to take an existing
Sure! I can make an adjustment that prioritizes that over the other optimizations 👍
I looked at the recent workflow runs and see the bulk of the time is spent compiling As long as future runs wouldn't trigger compiling everything all over again if you had the However, you're already building I prefer having a |
We would like to retain the isolation of the Dockerfile for building the image. The build stage isn't used by us otherwise, we just want to publish the final image, everything else goes through |
I've tried to summarize the considerations in the TLDR, let me know which direction you'd like to take. TL;DR:
Just to confirm that isolation isn't serving any real purpose though? You could use the CI binaries you already built for AMD64/ARM64 if you wanted to right? If you just want the build stage to remain in the
At least that is just the more pragmatic / simple approach to take and doesn't complicate maintenance. To optimize the build stage for CI will require cache optimizations such as with the cache mounts I showed earlier, otherwise the layer that creates
You're looking at 1GB roughly after compilation. That can be compressed fairly quickly with zstd down to 300MB, there isn't much value in higher compression levels (the tradeoff is barely worth it for 50MB-ish less). So 600MB for both targets to build with. There is little value in caching anything else in the image itself, the toolchain and crates all install fairly fast that you're just wasting the 10GB cache storage available by doing that. I would only cache what is useful and would assist with the speedup. I haven't tried Finally one other trick you can use is to split it across two runners, one for each target. In your case this works quite well due to the minimal image you actually care about. If the cache upload is only your final scratch image with just the binary, then you can quite easily leverage cache to retrieve both platforms (using the release tag as the cache key for example) and have these two separate images combined into a multi-platform manifest (which is what's already happening implicitly). The added complexity here splits the bottleneck of 20+ min build time in half, so that it more closely aligns with the other jobs running. You may not need this optimization though if that |
Splitting into two separate jobs would be a good idea. I don't have experience with caching docker builds beyond layer caching, if you know a way i'm happy to use that.
We only build the docker container for release, we don't use those images ourselves otherwise.
The github docs [says]:
If we get into a release cadence of once a week the cache will already be evicted by then, i'm afraid this wouldn't work for us. |
TL;DR:
Technically you could probably have a scheduled build job that just builds and caches the image, or triggers when the main branch is updated (something we have at That said, I don't know if there's any nice ways to partition/manage the CI cache... and wow, I just looked at your current cache use: Your largest cache items are about 1GB of which multiple were uploaded within the same hours. So not quite sure what's happening there, perhaps a bit of a bug on GH side. You have one item (2MB) from almost a week ago, while everything else is within the past 2 days, so it's not like it's evicting the old items regularly when exceeding the 10GB capacity limit 🤔
The way this is handled for That way you can separately build and cache the two images on a schedule or when commits are pushed to main branch like described above, with the separate publishing workflow triggered only on tagging new releases? You wanted to keep the
I don't have any workflow examples to cite for this as I haven't done it yet myself. There is a GHA cache exporter for BuildKit that can be used.. oh you're already using it here: uv/.github/workflows/build-docker.yml Lines 64 to 65 in 0a055b7
It was experimental when I implemented the workflows for Alternatively, you could apply to So from a maintenance point of view, |
The cache mounts seem nice to have but yeah per my earlier attempt in #995 probably not worth it in GitHub Actions. I did consider https://github.com/reproducible-containers/buildkit-cache-dance but it didn't seem worth the dependency and complexity for us. It seems like the ecosystem is lagging a bit here and I've just been waiting for those upstream issues to be fixed. We could consider Sorry about the back and forth and hesitant here. This is a tough pull request to review, we appreciate that you're investing time into it but the release artifacts are a critical part of our software. And, since we're a small team, we're careful about taking on more maintenance and services to manage. |
This was written last week to respond on the same day but I didn't seem to send it (just came across the tab), I've still got this on my backlog and hopefully can tackle it this weekend instead 👍 Original response
No worries! If you'd prefer to leave the When I revisit this in the weekend I'll take this approach:
Alternatively (reduced complexity, minimal time to publish):
Cache Mounts + CI Action
Anything in particular about the complexity? It adds one additional step into the mix (technically two since you don't have - name: inject cache into docker
uses: reproducible-containers/buildkit-cache-dance@v3.1.0
with:
cache-map: |
{
"var-cache-apt": "/var/cache/apt",
"var-lib-apt": "/var/lib/apt"
}
skip-extraction: ${{ steps.cache.outputs.cache-hit }} All that's being configured is 1 or more locations in a container to cache, with a cache key to skip if an exact match ( On the FROM ubuntu:22.04
ENV DEBIAN_FRONTEND=noninteractive
RUN \
--mount=type=cache,target=/var/cache/apt,sharing=locked \
--mount=type=cache,target=/var/lib/apt,sharing=locked \
rm -f /etc/apt/apt.conf.d/docker-clean && \
echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' >/etc/apt/apt.conf.d/keep-cache && \
apt-get update && \
apt-get install -y gcc This is because they're:
With ubuntu images sometimes the I can look into this as a follow-up, but I think just caching the RUN \
--mount=type=cache,target="/path/to/cache",id="cache-dirs" \
<<HEREDOC
cargo zigbuild --release --bin "${APP_NAME}" --target "${CARGO_BUILD_TARGET}"
cp "target/${CARGO_BUILD_TARGET}/release/${APP_NAME}" "/${APP_NAME}"
HEREDOC or as two separate RUN --mount=type=cache,target="/path/to/cache",id="cache-dirs" \
cargo zigbuild --release --bin "${APP_NAME}" --target "${CARGO_BUILD_TARGET}"
RUN cp "target/${CARGO_BUILD_TARGET}/release/${APP_NAME}" "/${APP_NAME}" |
Summary
TL;DR:
RUN
layers.RUN
content.--target
until explicitly adding the target triple.When ignoring the final stage, the total disk usage for the builder image is about 4GB.
Expand to see breakdown (4.11GB image)
/root/.venv
creation/root/.venv/lib/python3.12/site-packages/ziglang
/root/.cache/pip/http-v2
(can be skipped withpip install --no-cache-dir
)/root/.venv/bin/cargo-zigbuild
rust toolchain target add
, (Over 600MB is from toolchain installing docs at~/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/share/doc/rust/html
, theminimal
rustup profile doesn't apply due torust-toolchain.toml
)uv
:/root/.cache/zig from cargo-zigbuild
/root/.cargo/registry/
(51MBcache/
, 21MBindex/
, 225MBsrc
)target/
dirtarget/release/{build,deps}
(180MB each)target/<triple>/release/
(168MBbuild/
, 667MBdeps/
)uv
(copied from target build dir)The
RUN --mount
usage allows for caching useful data locally that would otherwise be invalidated across builds when you'd rather have it available. It also minimizes the impact of unused layers from past builds piling up until a prune is run. This is mostly a benefit for local usage rather than CI runners.With the fix for the rustup profile and proper handling of cacheable content, the image size falls below 1.6GB and iterative builds of the image are much faster.
FROM --platform=${BUILDPLATFORM}
), you can just install both targets in advance? Then useTARGETPLATFORM
ARG to implicitly get the preferred platform binary build.builder
stage shares all the same common layers, instead of diverging at theTARGETPLATFORM
ARG. This is viable due tocargo-zigbuild
supporting cross-platform builds easily.uv
binary distributed via other install methods. You can use UPX to reduce this furth from 23.6MB to 6.7MB (<10% current published size for the image), at the expense of 300ms startup latency and potentially false positive to AV scanners.Test Plan
I've built the image locally, you still get the binary output successfully. Let me know if you'd like me to test something specific?