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
Reviewer's GuideUnifies all Rust-based container images into a single multi-stage Containerfile using a shared builder with cached cargo mounts, adds reference-values as a workspace dependency, and updates the Makefile and ignore rules to consume the unified build pipeline while removing per-component Containerfiles. 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.
yairpod
force-pushed
the
build_optimizing
branch
from
July 23, 2026 12:03
964033f to
0b02453
Compare
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
Member
There was a problem hiding this comment.
very neat with the .containerignore also
Contributor
|
uril
reviewed
Jul 29, 2026
SpaceFace02
reviewed
Jul 29, 2026
yairpod
force-pushed
the
build_optimizing
branch
2 times, most recently
from
July 30, 2026 14:47
c5b2b35 to
108c2b0
Compare
Member
Author
Added cache cleaning to "make clean" command. |
Member
|
@yairpod is this PR closed intentionally? |
Member
Author
No, thanks for pointing this out |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
RUNstep that parsesCargo.lockto derive thereference-valuesrepo/commit looks quite brittle (multiplegrep/sedassumptions on lockfile layout) — consider a more robust approach (e.g.,cargo metadata, a fixed git URL/commit arg, or at least explicit error handling if the expected entries are not found). - Adding
$(CONTAINER_CLI) builder prune --all --forcetomake cleanis fairly destructive and may remove unrelated builder cache; it might be safer to scope this to the project (e.g., labels) or guard it behind an opt-in variable/target. - The unified
cargo buildstep in theContainerfilebundles several concerns (build, copy, reference-values sync) into one long shell line, which makes it harder to maintain and debug; consider splitting this into smallerRUNblocks or a helper script to separate build, artifact copy, and reference-values preparation logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `RUN` step that parses `Cargo.lock` to derive the `reference-values` repo/commit looks quite brittle (multiple `grep`/`sed` assumptions on lockfile layout) — consider a more robust approach (e.g., `cargo metadata`, a fixed git URL/commit arg, or at least explicit error handling if the expected entries are not found).
- Adding `$(CONTAINER_CLI) builder prune --all --force` to `make clean` is fairly destructive and may remove unrelated builder cache; it might be safer to scope this to the project (e.g., labels) or guard it behind an opt-in variable/target.
- The unified `cargo build` step in the `Containerfile` bundles several concerns (build, copy, reference-values sync) into one long shell line, which makes it harder to maintain and debug; consider splitting this into smaller `RUN` blocks or a helper script to separate build, artifact copy, and reference-values preparation logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
yairpod
force-pushed
the
build_optimizing
branch
from
August 3, 2026 10:40
108c2b0 to
5a32f4f
Compare
Jakob-Naucke
approved these changes
Aug 3, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jakob-Naucke, 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 |
Member
|
ah yes, the verified signatures |
yairpod
force-pushed
the
build_optimizing
branch
from
August 3, 2026 13:03
5a32f4f to
60dff63
Compare
|
New changes are detected. LGTM label has been removed. |
uril
reviewed
Aug 3, 2026
| cargo clean | ||
| rm -rf bin manifests $(CRD_YAML_PATH) $(CRD_RS_PATH) | ||
| rm -f trusted-cluster-gen config/rbac/role.yaml .crates.toml .crates2.json | ||
| $(CONTAINER_CLI) builder prune --all --force --filter label=project=trusted-cluster-operator |
Contributor
There was a problem hiding this comment.
should "builder" be replaced with -- image , system, volume, other ?
Member
Author
There was a problem hiding this comment.
builder prune is different meaning the the others:
builder prune - Removes BuildKit build cache (intermediate layers)
image prune - Removes Dangling/unused images
system prune - Removes Everything (containers, images, networks, cache)
volume prune - Removes Unused volumes
So we need to clean the BuildKit build cache, but I just found that it does not clean the mount caches.
I will add the command "$(CONTAINER_CLI) builder prune --force --filter type=exec.cachemount" for that, note that filter and labels are not supported here! so we will have to clean all mount caches in the system, or none. I think that in this case cleaning all of them should be acceptable.
uril
reviewed
Aug 3, 2026
|
|
||
| image: operator-image compute-pcrs-image reg-server-image attestation-key-register-image | ||
| image: | ||
| $(CONTAINER_CLI) build $(IMAGE_BUILD_OPTIONS) --target operator -t $(OPERATOR_IMAGE) -f Containerfile . |
Contributor
There was a problem hiding this comment.
What's the purpose of this change -- to always build all 4 containers ?
Member
There was a problem hiding this comment.
To avoid performing identical steps in parallel with multithreaded make. If we want to keep the targets, one could also do
Suggested change
| $(CONTAINER_CLI) build $(IMAGE_BUILD_OPTIONS) --target operator -t $(OPERATOR_IMAGE) -f Containerfile . | |
| $(MAKE) operator-image | |
| $(MAKE) compute-pcrs-image | |
| $(MAKE) reg-server-image | |
| $(MAKE) attestation-key-register-image |
Member
Author
There was a problem hiding this comment.
Also, the caches are behind a lock, so multithreading is sequential anyway.
Member
There was a problem hiding this comment.
oh, but when on your previous revision 108c2b0, make -j4 image yields something like
[1/5] STEP 17/23: RUN make crds-rs
--> 1a27730f3d14
[1/3] STEP 16/23: 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
--> 376893283ee7
[1/4] STEP 17/23: RUN make crds-rs
--> 33d731781ac3
[1/2] STEP 17/23: RUN make crds-rs
Updating crates.io index
Updating git repository `https://github.com/trusted-execution-clusters/compute-pcrs`
--> 4d4aca09ee8d
[1/3] STEP 17/23: RUN make crds-rs
Updating crates.io index
Updating git repository `https://github.com/trusted-execution-clusters/compute-pcrs`
Updating crates.io index
Updating git repository `https://github.com/trusted-execution-clusters/compute-pcrs`
Updating git repository `https://github.com/latchset/clevis-pin-trustee`
Updating git repository `https://github.com/latchset/clevis-pin-trustee`
Updating crates.io index
Updating git repository `https://github.com/trusted-execution-clusters/compute-pcrs`
Updating git repository `https://github.com/latchset/clevis-pin-trustee`
Updating git repository `https://github.com/latchset/clevis-pin-trustee`
Downloading crates ...
Downloading crates ...
which doesn't look so sequential to me?
Member
Author
There was a problem hiding this comment.
I'm not sure, I read about the Lock and did not experiment with it.
But that seems to be parallelization of line 40 "RUN make crds-rs" and the first use of --mount=type=cache (where the lock should be) is line 43 (in revision 108c2b0) .
Also that is a huge waste of build effort as a sequential build would not run these stages 4 times, the layers would be pulled from the intermediate layer cache (not to be confused with the mount cache).
Member
Author
There was a problem hiding this comment.
I added mount caches to the make crds-rs stage, it shows improvement in build times when the libs are modified.
Benchmarks:
New is with make crds-rs caches, old is the PR without them 60dff635
approach build_type repetition cache_state edit_target total_time_s
new debug 1 cold none 402.5
new debug 2 warm lib 92.5
new debug 3 warm lib 93.1
new debug 2 warm operator 22.5
new debug 3 warm operator 23.3
old debug 1 cold none 404.0
old debug 2 warm lib 114.0
old debug 3 warm lib 119.4
old debug 2 warm operator 22.5
old debug 3 warm operator 23.3
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 Testing additional cacheing
yairpod
force-pushed
the
build_optimizing
branch
from
August 4, 2026 09:26
60dff63 to
c5c0aa7
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
4 participants
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 Rust binary container builds into a single cached builder stage and derive multiple runtime images from it to reduce duplicate compilation and improve build performance.
New Features:
Enhancements:
Build:
Tests: