Skip to content
This repository was archived by the owner on Oct 18, 2023. It is now read-only.

Make cargo-chef effective for CI builds #763

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Make cargo-chef effective for CI builds #763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69ff903
Optimize runtime container layers
Oct 11, 2023
fa94d1b
Use Github Actions cache for Docker
Oct 11, 2023
be58650
Move rm -rf /var/lib/apt/lists/* to its own line
Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/pr-images.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ jobs:
with:
submodules: recursive

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Log in to the Container registry
uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9
with:
Expand All @@ -42,3 +45,7 @@ jobs:
push: true
tags: ${{ steps.meta.outputs.tags }}-${{ github.event.pull_request.head.sha }}
labels: ${{ steps.meta.outputs.labels }}
cache-from: type=gha
cache-to: type=gha,mode=max


6 changes: 6 additions & 0 deletions .github/workflows/publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ jobs:
with:
submodules: recursive

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Log in to the Container registry
uses: docker/login-action@f054a8b539a109f9f41c372932f1ae047eff08c9
with:
Expand All @@ -44,3 +47,6 @@ jobs:
push: true
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}
cache-from: type=gha
cache-to: type=gha,mode=max

23 changes: 15 additions & 8 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@ RUN apt update \
build-essential tcl protobuf-compiler file \
libssl-dev pkg-config git\
&& apt clean \
&& cargo install cargo-chef
&& rm -rf /var/lib/apt/lists/*

# We need to install and set as default the toolchain specified in rust-toolchain.toml
# Otherwise cargo-chef will build dependencies using wrong toolchain
# This also prevents planner and builder steps from installing the toolchain over and over again
COPY rust-toolchain.toml rust-toolchain.toml
RUN cat rust-toolchain.toml | grep "channel" | awk '{print $3}' | sed 's/\"//g' > toolchain.txt \
&& rustup update $(cat toolchain.txt) \
&& rustup default $(cat toolchain.txt) \
&& rm toolchain.txt rust-toolchain.toml
&& rm toolchain.txt rust-toolchain.toml \
&& cargo install cargo-chef
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a regression. If rust-toolchain.toml changes cargo install cargo-chef will be re-run. Previously it would be cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the toolchain changes we'll have to rebuild everything anyway, the cost of running cargo install cargo-chef is marginal.
I think the benefit of having all rust tools built by the same toolchain outweighs the downsides.
Let me know if you still want me to move it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine. Just don't name the commit "Optimize ... layers" when in fact you're de-optimizing it.
If you have named your intend of "cleaning up" the layers, I wouldn't add such comment at all.


FROM chef AS planner
COPY . .
Expand All @@ -27,14 +29,19 @@ RUN cargo build -p sqld --release

# runtime
FROM debian:bullseye-slim
COPY --from=builder /target/release/sqld /bin/sqld

EXPOSE 5001 8080
VOLUME [ "/var/lib/sqld" ]

RUN groupadd --system --gid 666 sqld
RUN adduser --system --home /var/lib/sqld --uid 666 --gid 666 sqld
RUN apt-get update && apt-get install -y ca-certificates
COPY docker-entrypoint.sh /usr/local/bin
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
VOLUME [ "/var/lib/sqld" ]
WORKDIR /var/lib/sqld
USER sqld
EXPOSE 5001 8080

COPY docker-entrypoint.sh /usr/local/bin

COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
Comment on lines +41 to +43
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that reverse order would be better since docker-entrypoint.sh probably changes more often than ca-certificates.

Also don't you need to add apt-get install -y ca-certificates to builder stage?
I think it would be better to add apt-get install -y ca-certificates to chef stage and copy certificates from there so that changed dependencies wouldn't trigger reinstallation of certificates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that reverse order would be better since docker-entrypoint.sh probably changes more often than ca-certificates.

That is true, but the entrypoint changes pretty rarely, and when that happens we'll only save a layer that weights the size of ca-certificates.crt. I preferred to keep it tidy by grouping together COPYs from the same place.

Let me know if you still want me to reorder.

Also don't you need to add apt-get install -y ca-certificates to builder stage?
I think it would be better to add apt-get install -y ca-certificates to chef stage and copy certificates from there so that changed dependencies wouldn't trigger reinstallation of certificates.

We don't have to because our Rust base image already does it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation about the rust base image containing ca-certificates.
Regarding the order. I'm fine with whatever order you pick. Just please name your intend correctly. If the commit was saying "reorder the layers to make them more readable at the expense of build speed" I wouldn't even add such comment. Saying that you're speeding something up when in fact you're doing the opposite is confusing.

COPY --from=builder /target/release/sqld /bin/sqld

ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
CMD ["/bin/sqld"]