Docker: Mount rust from image at build time instead of downloading it #1522
Conversation
pkg/docker/Dockerfile.go1.22
Outdated
| @@ -8,30 +10,16 @@ LABEL org.opencontainers.image.documentation="https://unit.nginx.org/installatio | |||
| LABEL org.opencontainers.image.vendor="NGINX Docker Maintainers <[email protected]>" | |||
| LABEL org.opencontainers.image.version="1.34.0" | |||
|
|
|||
| RUN set -ex \ | |||
| RUN --mount=type=bind,target=/rust,from=rust,rw \ | |||
There was a problem hiding this comment.
Because this is from= a stage, it would be mostly OK, but it's technically not officially supported in the DOI build system / dependency calculation, so adding it makes me a little nervous: https://github.com/docker-library/bashbrew/blob/f71d6ef63e1f5abe6d5cb06a62a3e5e42d68566e/cmd/bashbrew/docker.go#L96-L190
(For example, if we accept this, and then the FROM ... AS rust gets removed later, we won't necessarily remember the implications of that meaning that this line now implies from=rust:latest which will "just work" but not be accounted for correctly in our dependency calculations.)
There was a problem hiding this comment.
Would using a target that isn't matching an image solve this such as rust-build?
There was a problem hiding this comment.
Yes, but I've opened docker-library/bashbrew#113 now to help me quiet the voices (in my head). ❤️
There was a problem hiding this comment.
(So to be more clear, this is no longer a blocking concern from me -- maybe we double check with @yosifkit 👀)
Since this encodes an implicit dependency on BuildKit, it would be good to include Builder: buildkit explicitly like https://github.com/docker-library/official-images/blob/520cbf428492c155a9674a3f6ebe69118f3499fa/library/golang#L7 too 👍
pkg/docker/Dockerfile.go1.22
Outdated
| @@ -1,3 +1,5 @@ | |||
| FROM rust:1.83.0-bookworm AS rust | |||
There was a problem hiding this comment.
The rust image only maintains a single version at any given time, so this is only reasonable if it's going to be proactively kept up-to-date. 😬
There was a problem hiding this comment.
This concern, however, is still valid. 🙈
There was a problem hiding this comment.
Yeah, that's something that we probably can only update when we're releasing a new Unit version, and not really when the upstream Rust image is updated to a newer version...
There was a problem hiding this comment.
I mean, we can probably just use FROM rust:bookworm... and let the fate decide if our stuff is still buildable with a new compiler, but in the long run that will still exhibit the same issue, when Forky is released & bookworm is dropped from the Rust image.. But that is probably going to happen not very soon from now on.
Does that work?
There was a problem hiding this comment.
You could use FROM rust:bookworm, but that seems like it'll just lead to lots of failing unit image builds after a rust update that won't be addressed until the next Unit release (and so leave the images without DOI base image rebuilds).
Given your need for a specific, tested version of Rust, I recommend not copying/mounting from the rust image. Especially since it is already updated to 1.84 today, so we wouldn't accept the unit changes because it is no longer from a supported DOI image (rust:1.83.0-bookworm).
I would also not recommend the COPY of unitd* from the minimal image unless the base OS is the same (i.e. both the minimal and target images being Debian Bookworm) since, as a non-static binary, there isn't a guarantee that it will run correctly in an Ubuntu Jammy based image (like the Temurin jdk image). Or maybe making sure unitd --version and unitd-debug --version work in the copied-to image after purging the build deps is enough to mitigate this?
There was a problem hiding this comment.
So I feel like this PR actually makes things worse from multiple standpoints (maintainability being probably the worst); although admittedly improving on one - speeding up the builds for leaf images.
LaurentGoderre
force-pushed
the
optimize-docker
branch
6 times, most recently
from
8e1f00d to
bbefca2
Compare
|
Hmm, I think we can just re-use the "minimal" image instead of creating yet another "base" one. I'll try to give it a shot. |
This ensures that rust is not left behind in the image
Instead of rebuilding unitd binaries every time a module image is built, copy them from the minimal image.
We need to have this image built to copy the unitd binaries to the modules images.
This helps to debug the issues.
|
I've pushed some commits that implement the "mount rust" idea, as well as removing build duplication into https://github.com/thresheek/unit/commits/optimize-docker-take2/. I can create another PR, or force-push to @LaurentGoderre's branch if @LaurentGoderre doesnt mind so we can review them here. I've split the changes into reviewable items, so it should be easier to catch issues this way. One thing I don't like is that the majority of "modules" don't really need a Rust image mounted since they won't be building any Rust code - the only users are |
|
@thresheek feel free to use my branch however you need! |
thresheek
force-pushed
the
optimize-docker
branch
from
bbefca2 to
e846d28
Compare
|
There we go. Please take a look. |
LaurentGoderre
force-pushed
the
optimize-docker
branch
2 times, most recently
from
bbefca2 to
e846d28
Compare
LaurentGoderre
force-pushed
the
optimize-docker
branch
from
e846d28 to
cd6032e
Compare
|
@thresheek LGTM! |
|
It also looks good to me. Can we have a final clarification if those resulting Dockerfiles will be acceptable in the Library, as questioned in #1522 (comment) ? Thanks! |
|
I'm pretty sure I address that. @tianon can you confirm? |
|
I was hoping this approach would remove the need for Rust in all the images (so this could be limited to just one variant and thus cleaner), but the modules make that complicated. 😅 |
This ensures that rust is not left behind in the image