Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip nil cache result #6378

Merged
merged 1 commit into from
Jan 10, 2024
Merged

fix: skip nil cache result #6378

merged 1 commit into from
Jan 10, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jan 9, 2024

Somehow ImmutableRefs can be nil - not 100% sure how this happens, but upstream buildkit does include checks in appropriate places for this case. We should probably do the same as well.

Should hopefully mitigate #6379.

Somehow ImmutableRefs can be nil - not 100% sure how this happens, but
upstream buildkit does include checks in appropriate places for this
case. We should probably do the same as well.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc requested a review from sipsma January 9, 2024 12:01
@jedevc
Copy link
Member Author

jedevc commented Jan 9, 2024

Ok, in buildkit upstream, we call CacheResultStorage.Load in CacheExporter.Export, and pass it to ResolveRemotes - the functions passed in here call WorkerRef.GetRemotes, which explicitly checks for ImmutableRef being nil: https://github.com/moby/buildkit/blob/8a7e5c73174063ce5482b49b5316e4706534c1d3/worker/result.go#L45-L47.

This check was added in moby/buildkit@1835ef5 (#2434) by @sipsma:

This case happens if you do something contrived like Merge(Diff(Image("busybox"), Image("busybox")), foo), i.e. you do a diff where upper+lower end up being the same ref and thus simplifying to nil, which then gets plugged into merge. Basically, a nil input is treated as Scratch here.

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

LGTM, I guess in general this means that we can't currently distinguish between a cache key that has no results and a cache key that explicitly was evaluated and ended up being nothing. The cases where this makes any difference are obscure to start and in those cases the performance hit is so minuscule that I'm inclined to just not care for now.

The reason nil is used for those cases where something evaluates out to be nothing is that the alternative would have been to create an entire snapshot that's only ever going to be empty, which is wasteful in and of itself but would also result in images having unnecessary empty layers, etc.

There's probably some way of addressing that upstream by having a special immutable ref that represents "evaluated to nothing" and handling it all over the entire buildkit codebase, but doesn't feel worth it right now relative to just being aware that a cache ref can be nil.

@jedevc jedevc merged commit e355c57 into dagger:main Jan 10, 2024
42 of 43 checks passed
@jedevc jedevc deleted the skip-nil-cache-ref branch January 10, 2024 11:01
@jedevc jedevc added this to the v0.9.6 milestone Jan 10, 2024
jedevc added a commit that referenced this pull request Jan 11, 2024
Signed-off-by: Justin Chadwell <me@jedevc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants