mz936: dedup base image downloads - #937
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesShared base image caching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DockerfileParser
participant MakeKanikoStages
participant RenderStages
participant retrieveBaseImage
participant OCIBaseStore
participant Registry
DockerfileParser->>MakeKanikoStages: resolve base image digest
MakeKanikoStages->>RenderStages: classify shared base
RenderStages->>retrieveBaseImage: execute base action
retrieveBaseImage->>OCIBaseStore: load or store digest-keyed image
retrieveBaseImage->>Registry: retrieve image on local-store failure
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
55705d5 to
391989a
Compare
mzihlmann
changed the title
Performance resultsThree-way comparison against regression, following the methodology from #871. Real Docker Hub pulls, not loopback. Variants:
Each build has a Byte proof (debian:12, n=3, RX per run)
The flag collapses every case to a single base download. Cross-registry push saves one download even at N=1, and a 3-stage build drops from three downloads to one. Wall-time magnitude (nvidia/cuda:12.9.1-runtime-ubuntu24.04, ~2.65 GB, n=2)Median wall time / median RX.
base and off tie everywhere, so the added code is free on the default path. With the flag on, cross-registry push is ~1.7x faster and the 3-stage build ~1.4x faster. Known regression: single stage, no push is ~28% slower with the flag on196s vs 154-160s, same bytes, stddev 0.1s so it is real. With the flag on the base is written to the store and only then extracted, replacing the streaming path where download and extraction interleave. For a single stage that is never reused the serialization is pure overhead. Acceptable for a default-off flag, but the aim is a win in every case. The fix is to tee the compressed layer into the store with an |
mzihlmann
changed the title
Fix: decide the base action per stageThe first pass stored every remote base. That made a single no-push stage about 28% slower on a heavy base, because the base was downloaded into the store and then extracted, instead of streaming the download and extraction together. The fix decides per stage whether the base is read more than once, and only stores it then. A base is stored when several built stages share it, when a stage is kept for a later stage to build on, or when the built image is written out (push, tarball, or OCI layout), because all of those re-read the base layers. A base read exactly once streams as before, so it never pays a store round trip it cannot repay. The decision runs in
The dryrun plan prints Performance
proof2: single stage, no push (the regression check), n=5
branch-on median equals branch-off, and RX is 1x, so the single no-push stage now streams. The regression is gone, and branch-on has the tightest spread of the three. The pre-change median reads higher only because the harness ran it first every round and it caught the cold-network slots. A control round with the order flipped gives pre-change 138.2s, off 138.3s, on 139.7s, all within 1.5s. So pre-change, off, and on are equal for this case, as expected. proof1: cross-registry push, single stage, n=2
The push re-reads the base to upload it. With the flag on the base is stored once and the push reads it locally, saving one full download even at a single stage. proof3: multi-stage, 3 stages on the same base, no pushThree stages on the same base drop from 3 downloads to 1: RX about 8.0 GB to 2.68 GB, wall about 460s to 327s. This is unchanged by the elision, since a shared base is still stored and loaded. Summary
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@golden/testdata/test_issue_mz936/test.go`:
- Around line 25-27: Make the flag-off test case in the relevant test
configuration explicitly disable FF_KANIKO_SHARED_BASE_CACHE by setting it to
"0" or unsetting it in test.Env, so inherited environment values cannot enable
shared base caching and the Plan: "streamed" baseline remains deterministic.
In `@pkg/dockerfile/dockerfile.go`:
- Around line 419-432: Update the shared-cache planning loop in the
stage-processing logic to skip digestless base images before assigning
STORE/LOAD actions. Require a non-empty BaseImageDigest for both the current
stage and matching later stages, preserving existing planning for digest-keyed
bases.
In `@pkg/executor/build.go`:
- Around line 1587-1590: Update the error branch after img.Digest() in the
surrounding build function to emit a warning containing the digest error before
returning img, nil. Match the warning behavior and logging style used by the
other failure branches in that function, while preserving the existing return
behavior.
- Around line 1566-1610: Update retrieveBaseImage’s BaseImageLoad branch to
handle loadFromOCILayout failures without asserting or panicking: log a warning
and fall back to image_util.RetrieveSourceImage(stage, opts), propagating any
registry-fetch error. Preserve the local-store fast path when loading succeeds
and keep the existing Store-branch behavior unchanged.
- Line 1217: Update the cache lookahead path around retrieveBaseImage so every
non-local stage, including BaseImageLoad stages, resolves its base through
retrieveBaseImage instead of directly calling image_util.RetrieveSourceImage.
Preserve local-stage behavior and ensure shared remote bases are read from the
local OCI layout rather than redownloaded during precompute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1957a9ca-d685-4698-9d7c-af5b6f8f6063
📒 Files selected for processing (52)
README.mdgolden/golden_test.gogolden/testdata/test_issue_mz195/plans/fourthgolden/testdata/test_issue_mz195/plans/noisegolden/testdata/test_issue_mz195/plans/normalgolden/testdata/test_issue_mz195/plans/pushgolden/testdata/test_issue_mz333/plans/plangolden/testdata/test_issue_mz334/plans/cachedgolden/testdata/test_issue_mz334/plans/inferredgolden/testdata/test_issue_mz334/plans/plangolden/testdata/test_issue_mz338/plans/plangolden/testdata/test_issue_mz480/plans/finalgolden/testdata/test_issue_mz480/plans/final_testgolden/testdata/test_issue_mz487/plans/plangolden/testdata/test_issue_mz703/plans/legacy_stop_after_missgolden/testdata/test_issue_mz703/plans/probe_after_missgolden/testdata/test_issue_mz791/plans/resolved_onegolden/testdata/test_issue_mz791/plans/resolved_twogolden/testdata/test_issue_mz791/plans/unresolved_onegolden/testdata/test_issue_mz791/plans/unresolved_twogolden/testdata/test_issue_mz813/plans/resolved_onegolden/testdata/test_issue_mz813/plans/resolved_twogolden/testdata/test_issue_mz813/plans/unresolved_onegolden/testdata/test_issue_mz813/plans/unresolved_twogolden/testdata/test_issue_mz822/plans/resolved_onegolden/testdata/test_issue_mz822/plans/resolved_twogolden/testdata/test_issue_mz822/plans/unresolvedgolden/testdata/test_issue_mz936/Dockerfilegolden/testdata/test_issue_mz936/plans/pushgolden/testdata/test_issue_mz936/plans/sharedgolden/testdata/test_issue_mz936/plans/streamedgolden/testdata/test_issue_mz936/test.gogolden/testdata/test_unittests/plans/aliasgolden/testdata/test_unittests/plans/copyfrom_finalgolden/testdata/test_unittests/plans/final_wo_deps_buzzgolden/testdata/test_unittests/plans/final_wo_deps_finalgolden/testdata/test_unittests/plans/final_wo_deps_fizzgolden/testdata/test_unittests/plans/global_arggolden/testdata/test_unittests/plans/global_arg_stage1golden/testdata/test_unittests/plans/multiple_copygolden/testdata/test_unittests/plans/two_copyfrom_and_arg_basegolden/testdata/test_unittests/plans/two_copyfrom_and_arg_finalgolden/testdata/test_unittests/plans/two_copyfrom_finalgolden/testdata/test_unittests/plans/wo_copyfrom_devgolden/testdata/test_unittests/plans/wo_copyfrom_finalgolden/testdata/test_unittests/plans/wo_copyfrom_prodpkg/config/featureflags.gopkg/config/init.gopkg/config/stage.gopkg/dockerfile/dockerfile.gopkg/executor/build.gopkg/executor/build_test.go
|
Thanks for the review. Pushed 522563e. Rather than patch the store/load model in place, I collapsed it, which resolves two of these at the source. Load-branch panic (critical) and silent Digestless bases (major): the planner now skips bases with an empty digest, so Flag-off golden case (minor): we chose not to configure the flag off explicitly. The golden reads better with only active flags set, and Cache lookahead: unchanged, as agreed. It stays digest-only. New integration test |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/images.go`:
- Around line 392-393: Update the Dockerfile_test_issue_mz936 test expectation
for the shared-base: fallback warning so both stage warnings are required,
rather than allowing checkNoWarnings to succeed after the first match. Set the
expected occurrence count to two using the existing expectation mechanism, or
add a Dockerfile-specific output assertion that verifies both warnings.
In `@pkg/dockerfile/dockerfile.go`:
- Around line 418-420: Update the stage-filtering condition in the
duplicate-scan loop to stop treating BaseImageShared as a reason to continue.
Preserve skipping for locally stored base images and empty digests, while
allowing already-shared stages to participate in later same-digest scanning and
load the shared OCI layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d39afcf-f39e-4ec3-adc3-f3e33547b511
📒 Files selected for processing (8)
golden/testdata/test_issue_mz936/plans/pushgolden/testdata/test_issue_mz936/plans/sharedintegration/dockerfiles/Dockerfile_test_issue_mz936integration/images.gointegration/testdata/readonly-base-store/.gitkeeppkg/config/stage.gopkg/dockerfile/dockerfile.gopkg/executor/build.go
🚧 Files skipped from review as they are similar to previous changes (3)
- golden/testdata/test_issue_mz936/plans/shared
- golden/testdata/test_issue_mz936/plans/push
- pkg/executor/build.go
522563e to
3b82e0d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/build.go (1)
1564-1593: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up partial OCI layout writes on error.
writeImageLayoutcan leave a broken cache entry behind whenlayout.WriteorAppendImagefails, so laterBaseImageSharedlookups will keep hitting the same unreadable directory instead of starting clean. Removing the directory or writing to a temp path and renaming on success would avoid that stale state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/build.go` around lines 1564 - 1593, Update retrieveBaseImage and the writeImageLayout flow so failed OCI layout writes cannot leave a stale partial cache at the path derived from stage.BaseImageDigest. On writeImageLayout failure, remove the incomplete layout directory (or use an equivalent temporary-path-and-rename approach), while preserving the existing fallback to the retrieved registry image.
🧹 Nitpick comments (1)
pkg/executor/build.go (1)
1568-1570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging the store-miss before falling back to fetch.
Every other branch in this function logs (
Infof/Warnf) on hit, store, or fallback, but the initial store-miss (loadFromOCILayoutreturning an error before the registry fetch) is silent. A short debug/info log would help confirm the reported cache-hit-ratio improvements in production.♻️ Proposed addition
stored, err := loadFromOCILayout(path) if err == nil { logrus.Infof("shared-base: loading base %s from local store", stage.BaseImageDigest) return stored, nil } + logrus.Debugf("shared-base: base %s not found in local store (%v), fetching from registry", stage.BaseImageDigest, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/build.go` around lines 1568 - 1570, Add a concise debug or info log in the stage-loading flow immediately after loadFromOCILayout returns an error, before the registry-fetch fallback. Include the relevant stage or base-image identifier and indicate that the OCI layout store missed, while preserving the existing fallback behavior and nearby hit/store logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/executor/build.go`:
- Around line 1564-1593: Update retrieveBaseImage and the writeImageLayout flow
so failed OCI layout writes cannot leave a stale partial cache at the path
derived from stage.BaseImageDigest. On writeImageLayout failure, remove the
incomplete layout directory (or use an equivalent temporary-path-and-rename
approach), while preserving the existing fallback to the retrieved registry
image.
---
Nitpick comments:
In `@pkg/executor/build.go`:
- Around line 1568-1570: Add a concise debug or info log in the stage-loading
flow immediately after loadFromOCILayout returns an error, before the
registry-fetch fallback. Include the relevant stage or base-image identifier and
indicate that the OCI layout store missed, while preserving the existing
fallback behavior and nearby hit/store logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbe7c72a-a524-46d4-98e8-9632253c7e80
📒 Files selected for processing (7)
golden/testdata/test_issue_mz936/plans/pushgolden/testdata/test_issue_mz936/plans/sharedintegration/dockerfiles/Dockerfile_test_issue_mz936integration/images.gopkg/config/stage.gopkg/dockerfile/dockerfile.gopkg/executor/build.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/config/stage.go
- integration/dockerfiles/Dockerfile_test_issue_mz936
- golden/testdata/test_issue_mz936/plans/push
- golden/testdata/test_issue_mz936/plans/shared
- integration/images.go
3b82e0d to
46fe76c
Compare
d1496d8 to
73e186a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/config/init.go`:
- Around line 59-61: Update Cleanup to remove the shared-base directory
represented by KanikoBaseStagesDir alongside the other /kaniko work directories,
ensuring accumulated OCI layouts and blobs are deleted during cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ed080c4-a473-4d5e-8090-c0b44d8d6206
📒 Files selected for processing (54)
README.mdgolden/golden_test.gogolden/testdata/test_issue_mz195/plans/fourthgolden/testdata/test_issue_mz195/plans/noisegolden/testdata/test_issue_mz195/plans/normalgolden/testdata/test_issue_mz195/plans/pushgolden/testdata/test_issue_mz333/plans/plangolden/testdata/test_issue_mz334/plans/cachedgolden/testdata/test_issue_mz334/plans/inferredgolden/testdata/test_issue_mz334/plans/plangolden/testdata/test_issue_mz338/plans/plangolden/testdata/test_issue_mz480/plans/finalgolden/testdata/test_issue_mz480/plans/final_testgolden/testdata/test_issue_mz487/plans/plangolden/testdata/test_issue_mz703/plans/legacy_stop_after_missgolden/testdata/test_issue_mz703/plans/probe_after_missgolden/testdata/test_issue_mz791/plans/resolved_onegolden/testdata/test_issue_mz791/plans/resolved_twogolden/testdata/test_issue_mz791/plans/unresolved_onegolden/testdata/test_issue_mz791/plans/unresolved_twogolden/testdata/test_issue_mz813/plans/resolved_onegolden/testdata/test_issue_mz813/plans/resolved_twogolden/testdata/test_issue_mz813/plans/unresolved_onegolden/testdata/test_issue_mz813/plans/unresolved_twogolden/testdata/test_issue_mz822/plans/resolved_onegolden/testdata/test_issue_mz822/plans/resolved_twogolden/testdata/test_issue_mz822/plans/unresolvedgolden/testdata/test_issue_mz936/Dockerfilegolden/testdata/test_issue_mz936/plans/pushgolden/testdata/test_issue_mz936/plans/sharedgolden/testdata/test_issue_mz936/plans/streamedgolden/testdata/test_issue_mz936/test.gogolden/testdata/test_unittests/plans/aliasgolden/testdata/test_unittests/plans/copyfrom_finalgolden/testdata/test_unittests/plans/final_wo_deps_buzzgolden/testdata/test_unittests/plans/final_wo_deps_finalgolden/testdata/test_unittests/plans/final_wo_deps_fizzgolden/testdata/test_unittests/plans/global_arggolden/testdata/test_unittests/plans/global_arg_stage1golden/testdata/test_unittests/plans/multiple_copygolden/testdata/test_unittests/plans/two_copyfrom_and_arg_basegolden/testdata/test_unittests/plans/two_copyfrom_and_arg_finalgolden/testdata/test_unittests/plans/two_copyfrom_finalgolden/testdata/test_unittests/plans/wo_copyfrom_devgolden/testdata/test_unittests/plans/wo_copyfrom_finalgolden/testdata/test_unittests/plans/wo_copyfrom_prodintegration/dockerfiles/Dockerfile_test_issue_mz936integration/images.gopkg/config/featureflags.gopkg/config/init.gopkg/config/stage.gopkg/dockerfile/dockerfile.gopkg/executor/build.gopkg/executor/build_test.go
🚧 Files skipped from review as they are similar to previous changes (46)
- golden/testdata/test_issue_mz822/plans/unresolved
- golden/testdata/test_issue_mz334/plans/cached
- golden/testdata/test_unittests/plans/final_wo_deps_buzz
- golden/testdata/test_issue_mz334/plans/inferred
- golden/testdata/test_unittests/plans/wo_copyfrom_final
- golden/testdata/test_issue_mz480/plans/final_test
- golden/testdata/test_unittests/plans/final_wo_deps_final
- golden/testdata/test_issue_mz813/plans/unresolved_two
- golden/testdata/test_unittests/plans/wo_copyfrom_prod
- golden/testdata/test_unittests/plans/global_arg_stage1
- golden/testdata/test_issue_mz813/plans/resolved_one
- golden/testdata/test_issue_mz195/plans/noise
- integration/dockerfiles/Dockerfile_test_issue_mz936
- golden/testdata/test_issue_mz703/plans/legacy_stop_after_miss
- golden/testdata/test_unittests/plans/two_copyfrom_and_arg_base
- golden/testdata/test_issue_mz822/plans/resolved_two
- golden/testdata/test_issue_mz338/plans/plan
- golden/testdata/test_unittests/plans/global_arg
- golden/testdata/test_unittests/plans/two_copyfrom_and_arg_final
- golden/testdata/test_issue_mz813/plans/unresolved_one
- golden/testdata/test_issue_mz487/plans/plan
- golden/testdata/test_issue_mz703/plans/probe_after_miss
- golden/testdata/test_issue_mz791/plans/resolved_two
- golden/testdata/test_issue_mz813/plans/resolved_two
- golden/testdata/test_unittests/plans/alias
- golden/testdata/test_issue_mz791/plans/unresolved_two
- golden/testdata/test_issue_mz936/plans/streamed
- golden/testdata/test_issue_mz333/plans/plan
- golden/testdata/test_issue_mz791/plans/unresolved_one
- golden/testdata/test_unittests/plans/two_copyfrom_final
- pkg/config/stage.go
- golden/testdata/test_issue_mz334/plans/plan
- golden/testdata/test_issue_mz195/plans/normal
- golden/testdata/test_unittests/plans/copyfrom_final
- README.md
- golden/testdata/test_issue_mz791/plans/resolved_one
- golden/testdata/test_unittests/plans/wo_copyfrom_dev
- golden/testdata/test_unittests/plans/final_wo_deps_fizz
- golden/golden_test.go
- golden/testdata/test_issue_mz936/test.go
- pkg/config/featureflags.go
- golden/testdata/test_issue_mz936/plans/shared
- integration/images.go
- golden/testdata/test_issue_mz936/plans/push
- pkg/executor/build_test.go
- pkg/dockerfile/dockerfile.go
6daf5da to
82967c9
Compare
bcb82f4 to
887226b
Compare
30410d3 to
723c07b
Compare
0hlov3
left a comment
0hlov3
left a comment
There was a problem hiding this comment.
LGTM.
@mzihlmann overall everything looks good. Maybe we should mention that the /kaniko/bases are never cleaned up by config.Cleanup() with FF_KANIKO_SHARED_BASE_CACHE enabled, cause it can leave potentially large base layers behind, in repeated builds or long-lived executors this .
2 participants
Part of #936.
kaniko re-downloads a base image once per stage that uses it, so a multi-stage build fetches the same base repeatedly, and a cross-registry push re-fetches every base layer again to upload it. Behind
FF_KANIKO_SHARED_BASE_CACHE(default off) a remote base is stored once as an OCI layout under/kaniko/basesand reused by every later stage and by push, keyed by digest the same way the warmer keys its cache. A digest-pinned base is served straight from the store with no registry access, so a build can run against an unreachable source once the store holds it. Every base-retrieval site reads the store, but only the build loop downloads and populates it, so dependency calculation and cache lookahead still cost just a manifest.Summary by CodeRabbit
New Features
FF_KANIKO_SHARED_BASE_CACHE(off by default; becomes default in v1.29.0).Bug Fixes
Tests
/kaniko/basescase) and updated golden fixtures for shared/streamed scenarios.Documentation