Unified Container Build with Cache Mounts#318
Open
yairpod wants to merge 1 commit into
Open
Conversation
yairpod
requested review from
Jakob-Naucke,
SpaceFace02,
alicefr,
bgartzi,
iroykaufman and
uril
July 23, 2026 10:40
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yairpod The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideUnifies all Rust component builds into a single cached builder stage and reuses its artifacts across multiple final images, replacing per-component Containerfiles and aligning Makefile targets with multi-stage build targets. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Containerfile" line_range="58-61" />
<code_context>
+ -p attestation-key-register \
+ $release_flag && \
+ mkdir -p /output && \
+ cp /build/target/${build_type}/operator /output/ && \
+ cp /build/target/${build_type}/compute-pcrs /output/ && \
+ cp /build/target/${build_type}/register-server /output/ && \
+ cp /build/target/${build_type}/attestation-key-register /output/
+
+# Distribution stages
</code_context>
<issue_to_address>
**issue (bug_risk):** The use of `build_type` directly in the target path can break builds for unexpected values.
This logic only works when `build_type` is `release` or `debug`: Cargo will still build to `debug` unless `--release` is used, but the `cp /build/target/${build_type}/...` paths will then be wrong for other values (e.g., `prod`) and the copy will fail. Either derive a `profile_dir` (`release`/`debug`) used consistently for both `cargo build` and copy paths, or validate `build_type` and fail early if it’s not one of the supported values.
</issue_to_address>
### Comment 2
<location path="Containerfile" line_range="32" />
<code_context>
sed -i '/\[dev-dependencies\]/,$d' operator/Cargo.toml && \
+ sed -i '/\[dev-dependencies\]/,$d' register-server/Cargo.toml && \
sed -i '/trusted-cluster-operator-test-utils/d' lib/Cargo.toml && \
+ git clone --depth 1 https://github.com/trusted-execution-clusters/reference-values && \
make crds-rs
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Unpinned `git clone` introduces non-reproducible builds and potential supply-chain risk.
Cloning `reference-values` without pinning a commit, tag, or at least a branch means builds can change unexpectedly as the upstream repo evolves and increases supply-chain risk. Please pin to a specific commit or tag (and ideally verify signatures/checksums) so the builder always uses a known, trusted revision and only changes when explicitly updated.
Suggested implementation:
```
ARG REFERENCE_VALUES_REV=v0.0.0
RUN sed -i 's/members = .*/members = ["lib", "operator", "compute-pcrs", "register-server", "attestation-key-register"]/' Cargo.toml && \
sed -i '/\[dev-dependencies\]/,$d' operator/Cargo.toml && \
sed -i '/\[dev-dependencies\]/,$d' register-server/Cargo.toml && \
sed -i '/trusted-cluster-operator-test-utils/d' lib/Cargo.toml && \
git clone --depth 1 --branch "${REFERENCE_VALUES_REV}" https://github.com/trusted-execution-clusters/reference-values && \
```
```
make crds-rs
ARG build_type=release
```
1. Replace the placeholder `v0.0.0` in `REFERENCE_VALUES_REV` with a real, trusted tag (or branch) from `trusted-execution-clusters/reference-values`. Using a tag is preferable for immutability.
2. If you decide to pin to a specific commit instead of a tag, drop `--depth 1` and `--branch` and use `git checkout <commit-sha>` after cloning; this will still be reproducible but with a slightly larger clone.
3. Optionally, you can add checksum or signature verification (e.g., verifying a signed tag) after cloning if your build environment supports that, to further reduce supply-chain risk.
</issue_to_address>
### Comment 3
<location path="Containerfile" line_range="69" />
<code_context>
+
+FROM quay.io/fedora/fedora:43 AS compute-pcrs
+COPY --from=builder /output/compute-pcrs /usr/bin
+COPY --from=builder /build/reference-values /reference-values
+
+FROM quay.io/fedora/fedora:43 AS register-server
</code_context>
<issue_to_address>
**suggestion (performance):** Copying the entire `reference-values` repo into the compute-pcrs image may bloat the runtime and include unnecessary content.
The `/build/reference-values` path in the final image likely contains the entire cloned repo (including `.git` and other non-runtime assets). If `compute-pcrs` only needs specific data or subdirectories, please adjust the Dockerfile to copy just those artifacts from the builder stage to minimize image size and avoid bundling unnecessary files into the runtime image.
Suggested implementation:
```
cp /build/target/${build_type}/compute-pcrs /output/ && \
cp /build/target/${build_type}/register-server /output/ && \
cp /build/target/${build_type}/attestation-key-register /output/ && \
mkdir -p /output/reference-values && \
cp -r /build/reference-values/runtime-data /output/reference-values
```
```
FROM quay.io/fedora/fedora:43 AS compute-pcrs
COPY --from=builder /output/compute-pcrs /usr/bin
COPY --from=builder /output/reference-values /reference-values
```
- Replace `/build/reference-values/runtime-data` with the actual path (subdirectory or artifact files) that `compute-pcrs` needs at runtime (for example, a `data/` or `artifacts/` directory, or specific JSON/YAML files).
- Ensure that whatever path you choose is produced/available in the builder stage (e.g., populated by `git clone`, build scripts, or other tooling) so the `cp -r` command succeeds.
- If `compute-pcrs` only requires individual files instead of a directory, adjust the `cp -r` command to copy just those files into `/output/reference-values` and keep the `COPY --from=builder /output/reference-values /reference-values` line unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| sed -i '/\[dev-dependencies\]/,$d' operator/Cargo.toml && \ | ||
| sed -i '/\[dev-dependencies\]/,$d' register-server/Cargo.toml && \ | ||
| sed -i '/trusted-cluster-operator-test-utils/d' lib/Cargo.toml && \ | ||
| git clone --depth 1 https://github.com/trusted-execution-clusters/reference-values && \ |
There was a problem hiding this comment.
🚨 suggestion (security): Unpinned git clone introduces non-reproducible builds and potential supply-chain risk.
Cloning reference-values without pinning a commit, tag, or at least a branch means builds can change unexpectedly as the upstream repo evolves and increases supply-chain risk. Please pin to a specific commit or tag (and ideally verify signatures/checksums) so the builder always uses a known, trusted revision and only changes when explicitly updated.
Suggested implementation:
ARG REFERENCE_VALUES_REV=v0.0.0
RUN sed -i 's/members = .*/members = ["lib", "operator", "compute-pcrs", "register-server", "attestation-key-register"]/' Cargo.toml && \
sed -i '/\[dev-dependencies\]/,$d' operator/Cargo.toml && \
sed -i '/\[dev-dependencies\]/,$d' register-server/Cargo.toml && \
sed -i '/trusted-cluster-operator-test-utils/d' lib/Cargo.toml && \
git clone --depth 1 --branch "${REFERENCE_VALUES_REV}" https://github.com/trusted-execution-clusters/reference-values && \
make crds-rs
ARG build_type=release
- Replace the placeholder
v0.0.0inREFERENCE_VALUES_REVwith a real, trusted tag (or branch) fromtrusted-execution-clusters/reference-values. Using a tag is preferable for immutability. - If you decide to pin to a specific commit instead of a tag, drop
--depth 1and--branchand usegit checkout <commit-sha>after cloning; this will still be reproducible but with a slightly larger clone. - Optionally, you can add checksum or signature verification (e.g., verifying a signed tag) after cloning if your build environment supports that, to further reduce supply-chain risk.
Until now our builds have compiled the same files 4 times, As the builds were done in separate containers and had no access to the compilation objects from the other containers or previous builds. By Unifying the Build stage into one Container and interceding build caches we will prevent unnecessary work and achieve faster builds. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: Opus:4.6
yairpod
force-pushed
the
build_optimizing
branch
from
July 23, 2026 12:03
964033f to
0b02453
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until now our builds have compiled the same files 4 times, As the builds were done in separate containers and had no access to the compilation objects from the other containers or previous builds. By Unifying the Build stage into one Container and interceding build caches we will prevent unnecessary work and achieve faster builds.
To check the effectiveness of these changes I used Opus 4.6 to write a build time benchmark script (see attached) and run a short test (also see attached).
The Highlights are these:
Without this PR the Operator container build was up to 30% faster the second build to the first build (~220 second time ~310 first time) , with the smaller gains for the other containers.
With the PR the difference is 70% (~110 second time ~305 first time) , but the other containers show a much larger improvement, as the speed up is about 90% (~30 seconds first build to ~3 in second build), also the build time of the first build of the other containers is reduced from the original code build time of ~220 seconds to ~30 in united build, is is due to the containers being build togather.
benchmark-builds.sh
results.csv
summary.txt
Summary by Sourcery
Unify container builds into a single cached builder stage that produces all Rust binaries and multi-stage images from one Containerfile.
New Features:
Enhancements:
Build:
Chores: