mount layers the destination registry already holds - #990
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:
📝 WalkthroughWalkthroughThe pull request adds ChangesCross-repository mounting
Build-plan output updates
Estimated code review effort: 4 (Complex) | ~45 minutes 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 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
golden/testdata/test_issue_mz989/test.go (1)
5-53: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the flag-on mount contract with same-registry and flag-off cases.
golden/golden_test.gosetsFF_KANIKO_CROSS_REPO_MOUNT=1globally, so theplaincomment on line 15 is already misleading. Split the current upload cases into explicitFF_KANIKO_CROSS_REPO_MOUNT=falsebaselines, then addFF_KANIKO_CROSS_REPO_MOUNT=truescenarios with the base repo present on registryexample.com; those should record mounts forplain/cache_hitand preserve uploads forrekeyed. Cross-registry source repos should only verify uploads, not mounts.🤖 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 `@golden/testdata/test_issue_mz989/test.go` around lines 5 - 53, Update the golden test cases in Tests to explicitly set FF_KANIKO_CROSS_REPO_MOUNT=false for baseline upload scenarios, then add flag-enabled cases with the base repository available on example.com that expect mounts for plain and cache_hit while keeping rekeyed uploads. Include a cross-registry source-repository case that verifies uploads only, and revise misleading comments and plan fixtures accordingly.
🤖 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/executor/push.go`:
- Around line 321-322: Gate the mounts.RecordImage call in
pkg/executor/push.go:321-322 so pushed images are recorded only when
config.FF.CrossRepoMount is enabled. Apply the same conditional guard to the
cache image recording call in pkg/cache/cache.go:85; both sites should avoid
updating provenance when the feature is disabled.
---
Nitpick comments:
In `@golden/testdata/test_issue_mz989/test.go`:
- Around line 5-53: Update the golden test cases in Tests to explicitly set
FF_KANIKO_CROSS_REPO_MOUNT=false for baseline upload scenarios, then add
flag-enabled cases with the base repository available on example.com that expect
mounts for plain and cache_hit while keeping rekeyed uploads. Include a
cross-registry source-repository case that verifies uploads only, and revise
misleading comments and plan fixtures accordingly.
🪄 Autofix
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: 7a4733ec-08b6-43f6-a796-45baa0694d32
📒 Files selected for processing (61)
README.mdgolden/golden_test.gogolden/testdata/test_issue_mz195/Dockerfilegolden/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/eliminatedgolden/testdata/test_issue_mz334/plans/inferredgolden/testdata/test_issue_mz334/plans/plangolden/testdata/test_issue_mz338/plans/plangolden/testdata/test_issue_mz480/Dockerfilegolden/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_mz989/Dockerfilegolden/testdata/test_issue_mz989/plans/cache_hitgolden/testdata/test_issue_mz989/plans/cache_missgolden/testdata/test_issue_mz989/plans/plaingolden/testdata/test_issue_mz989/plans/rekeyedgolden/testdata/test_issue_mz989/plans/storedgolden/testdata/test_issue_mz989/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/images.gopkg/cache/cache.gopkg/config/featureflags.gopkg/executor/build.gopkg/executor/push.gopkg/mounts/mounts.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
mzihlmann
changed the title
b4c95a3 to
69e9107
Compare
|
Gated both recording sites on Not taking the golden suggestion. It rests on The deeper reason is that no golden can record a The byte claim is backed by registry access logs in the PR description rather than by a fixture. Turning that into an assertion needs a counting proxy in front of the integration registry, which is tracked in #989. |
3cc7aa5 to
a6aa597
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/executor/build.go`:
- Around line 1226-1241: Update mountSource to accept the registry-mapping
resolution context and resolve both the source repository and each destination
before comparing them. Use the resolved repository contexts with
mounts.Mountable, and return the resolved mount source so plan action selection
uses mapped registries and can choose MOUNT instead of UPLOAD.
🪄 Autofix
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: 4dde1f73-665b-4bb8-b232-12dc5d911776
📒 Files selected for processing (7)
golden/testdata/test_issue_mz989/plans/cache_hitgolden/testdata/test_issue_mz989/plans/cache_missgolden/testdata/test_issue_mz989/plans/rekeyedgolden/testdata/test_issue_mz989/test.gopkg/executor/build.gopkg/executor/push.gopkg/mounts/mounts.go
🚧 Files skipped from review as they are similar to previous changes (4)
- golden/testdata/test_issue_mz989/plans/cache_miss
- golden/testdata/test_issue_mz989/plans/cache_hit
- golden/testdata/test_issue_mz989/plans/rekeyed
- pkg/executor/push.go
50dfe91 to
5fde9d8
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/executor/build.go`:
- Around line 1171-1196: In RenderStages, compute the same shouldTakeSnapshot
condition used by stageBuilder.build before planning cache actions. Gate cache
upload setup, planned digest assignment, and the pushedLayer append on that
condition; also require the retrieved cache image to contain a layer before
appending a pushed layer, so single-snapshot plans emit only the final snapshot
and never create empty-cache push actions.
🪄 Autofix
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: 9bc8cf5c-4c90-4d19-805a-10c1d0b7a215
📒 Files selected for processing (4)
golden/golden_test.gogolden/testdata/test_issue_mz989/test.gopkg/executor/build.gopkg/mounts/mounts.go
🚧 Files skipped from review as they are similar to previous changes (1)
- golden/testdata/test_issue_mz989/test.go
38934f9 to
fc8b4a0
Compare
fc8b4a0 to
fb1dc8e
Compare
Performance measurementThree configs, five rounds, config order shuffled within every round and the slot recorded.
Against a private registry over a VPN link, so bytes cost what they cost in production. Bytes are the host interface TX delta, since a remote registry has no access log to read. The layer is incompressible, so it costs a real 50 MB upload rather than compressing away. Destination and cache repo are both on that registry, which is the only case where a mount is possible. Two scenarios:
Every run pushes to a repository that did not hold the blob beforehand, otherwise Bytes sent, median of five:
Wall clock, median of five:
About 21% off the total build for the miss and 22% for the hit, on a build where the upload is one of several costs. The The miss halves because the layer goes to the cache repo as it is built and then goes up again inside the image, and the second send becomes a mount. The hit drops to the config blob because the layer was read out of the cache repo minutes earlier and the push mounts it from there. A no-cache control, run against a local Bound on the claimThe cache hit only wins when the cache entry and the image disagree on media type vendor. A pilot against an older docker-schema2 base produced a docker-typed image and a docker-typed cache entry. So against a docker-media-type base the flag adds nothing to a cache hit. It still halves the miss, because that layer is built locally and never had a reference to lose. Modern base images are OCI, which is the configuration measured above. CaveatsThe shuffle did not balance slots. Not measured: several destinations on one registry, where the first push becomes the source for the rest, and cross-registry pushes, where the mount is refused and the bytes go up regardless. |
2 participants
Fixes #989
A registry will copy a blob between its own repositories for free, but only if it is told which repository already holds it, and kaniko loses that the moment it copies a layer locally. With
--cachethat costs an upload per layer on every build: the layer goes to the cache repo as it is built, and the same bytes go up again inside the image a moment later. A cache hit pays it too, uploading a layer that was just read out of the cache repo. Both repositories usually sit on the registry being pushed to, so the bytes are already there.pkg/mountskeeps a note of which repositories hold which layer and re-tags them on the way intoremote.Write. Keyed by digest rather than carried on the layer, because the relabel inconvertLayerMediaTypereplaces the layer object while keeping its bytes, and a rewrite that does change the bytes changes the digest and simply misses. Only repositories this build read from or wrote to are recorded, sinceremote.Writefails a push outright if the token request for a mount source is refused.Measured against a local registry with the cache repo and destination on it, the built layer is mounted instead of uploaded on both a cache miss and a cache hit, leaving only the config blob to upload. Behind
FF_KANIKO_CROSS_REPO_MOUNT, default off.The first commit is separate and changes no behaviour: the dryrun plan now accounts for every layer each push sends, so the effect of this and the remaining producers in #989 shows up as a plan diff rather than only in registry logs.
Summary by CodeRabbit
New Features
FF_KANIKO_CROSS_REPO_MOUNTfeature flag, disabled by default.Bug Fixes
Documentation