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

Refactor Dockerfile for multi-stage build #144

Merged
merged 6 commits into from
Nov 28, 2024

Conversation

geraldwuhoo
Copy link
Contributor

I noticed that when building the Dockerfile, the resultant image had a size of 1.15GB! This did not pass the smell test for a small static Rust binary with an alpine base container.

The current Dockerfile uses a single-stage build, which means that the entire package cache, cargo cache, build cache, etc. are saved in a Docker image layer. This results in an absolutely enormous image.

This PR refactors the Dockerfile to use a multi-stage build, which first uses the rust:alpine Docker image to build the project. The second stage starts from a base alpine container, and only copies the statically compiled binary and the start.sh entrypoint script, which reduces size from 1.15GB to 11.4MB.

I quickly tested this out by using the new Docker image to serve a page, which I could access using amfora. I am not entirely familiar with all the bells and whistles of this project, so I recommend the primary maintainers further test the new Dockerfile before merging.

Passing thoughts

Use of alpine as final base instead of scratch

Just some additional thoughts regarding the Dockerfile -- the only reason I used alpine:latest as the final build stage instead of scratch is due to the start.sh script, which requires a shell to interpret it. However, the start script itself is very simple, being only one command. This command could easily be inlined directly in Dockerfile. The only problem is that CMD command doesn't understand ENV variables. In addition, as scratch does not contain a shell, shell expansion of the environment variable won't work regardless.

If agate itself can be refactored to be configured by environment variables in addition to traditional command line flags, we can use the ENV directive in the Dockerfile to configure the runtime arguments of agate, and skip the startup script entirely. This will allow us to use scratch as the base container, which would save ~5.9MB -- the size of the alpine base container (non-insignificant compared to currently proposed final size of 11.4MB).

This can be addressed in a future PR if there is sufficient interest, and I am glad to implement that as well. I just want to make sure it is actually a desired feature before spending time to implement it -- don't want to spend time writing it just to be shot down due to differing design philosophies.

@kallisti5
Copy link

kallisti5 commented Apr 26, 2022

I can confirm this is a good change. Multi-stage builds will greatly reduce the container size. I'd like to add a few more features, but really can't until this one is merged.

@mbrubeck are you ok with this one as-is?

As for scratch, i generally shy away from it since it makes containers extremely difficult to debug (no exec bash), but it could make sense in this case given the simplicity of agate. One feature i'd like to see is "automatically pulling a gemini website via git and hosting it on startup" (instead of relying on a volume mount). This wouldn't be possible with scratch.

@Johann150
Copy link
Collaborator

I am currently waiting for the things from #145 to be modified into this PR.

@geraldwuhoo
Copy link
Contributor Author

Sorry folks, this slipped PR my mind. I have time to work on this now so hopefully we can get this merged soon :).

@ldotlopez
Copy link

Hi,

How is this PR going?
I have an alternative aproach at: https://github.com/ldotlopez/agate which builds the container from source (instead of using wget) and multi-stage build.

I would like to open a new PR for my branch if this PR is not merge.

@geraldwuhoo
Copy link
Contributor Author

Hi all, apologies for the long delay. I have updated the PR branch with the additional asks. If all looks good, I can update the docs as well with the new Docker build process.

I tested this with:

$ docker build -t agate .
$ docker run --rm --name agate -p 1965:1965 -v $(pwd)/content:/gmi:ro -v $(pwd)/certificates:/app/.certificates -e HOSTNAME=localhost localhost/agate

@geraldwuhoo geraldwuhoo marked this pull request as ready for review August 18, 2024 02:21
@mbrubeck
Copy link
Owner

Thank for the update! I don’t really have the time or expertise to review this properly right now. If anyone else in this discussion can take a look, I would be very grateful.

Copy link
Contributor

@jphastings jphastings left a comment

Choose a reason for hiding this comment

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

Hello! I'd like to see this PR merged, and happen to have significant experience with Docker, so I figured I'd offer a review here 😊

I've been slightly opinionated (in the direction of consistency with the agate binary), but I've tried to explain my reasoning so either of you (@geraldwuhoo as the kind author of the PR, or @mbrubeck as the much appreciated author of agate) can pick and chose from my suggestions as you see fit.

If it'd be useful I can also submit a PR after this one to add a new Github Action to build this container and attach it to the repo as a package. (This is how the container would be available with docker run ghcr.io/mbrubeck/agate — it would also close #279)

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
tools/docker/start.sh Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@geraldwuhoo
Copy link
Contributor Author

Thank you for the review! I agree with all of these. I particularly like the envvar change. Much more extensible. I will commit and test these shortly.

geraldwuhoo and others added 5 commits November 21, 2024 01:31
Co-authored-by: JP Hastings-Edrei <hello@byjp.me>
Co-authored-by: JP Hastings-Edrei <hello@byjp.me>
Co-authored-by: JP Hastings-Edrei <hello@byjp.me>
Co-authored-by: JP Hastings-Edrei <hello@byjp.me>
@geraldwuhoo
Copy link
Contributor Author

I've rebuilt the Dockerfile and tested it. It works great. I like the changes.
Hope to get this merged soon if all looks well.

@jphastings
Copy link
Contributor

Thanks @geraldwuhoo! @mbrubeck is there anything else I can do to help with this PR?

@mbrubeck
Copy link
Owner

@jphastings If you haven’t already, it would be great if you could do a final review of the latest commits. Then I’ll go ahead and merge this. Thanks you both for your work on this!

Copy link
Contributor

@jphastings jphastings left a comment

Choose a reason for hiding this comment

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

These changes look good to me! Thanks for the nudge @mbrubeck — I'd forgotten to review the extra commits 😊

@mbrubeck mbrubeck merged commit e2d9b8f into mbrubeck:master Nov 28, 2024
3 checks passed
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.

6 participants