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 flakiness during import of a cache with empty layers removed #2372

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

jgiannuzzi
Copy link
Contributor

This PR aims at fixing issues #1980 and #2198. It does so by ensuring that all results get returned by cache.remotecache.v1.(*cacheResultStorage).LoadWithParents, and not only partial results, as can be the case when 2 or more vertexes point to the same result.

See #2198 (comment) for more information about my understanding of the issue.

Signed-off-by: Jonathan Giannuzzi <jonathan@giannuzzi.me>
@jgiannuzzi
Copy link
Contributor Author

Tests are running fine on my repo: https://github.com/jgiannuzzi/buildkit/actions/runs/1261157927

I guess this is a transient error. Could a maintainer please re-run all jobs? 🙏

@crazy-max
Copy link
Member

@jgiannuzzi Done

@jgiannuzzi
Copy link
Contributor Author

Thanks @crazy-max!

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to figure this out.

@tonistiigi tonistiigi added this to the v0.9.1 milestone Sep 24, 2021
@tonistiigi tonistiigi merged commit deb1440 into moby:master Sep 24, 2021
@tonistiigi
Copy link
Member

@jgiannuzzi Now that you are familiar with this code, what is your thinking about removing the empty layer blob removal and always exporting empty layer blobs. It messes up the history array in image config and therefore reproducible builds and I don't have any better solution to fix that.

I was also looking at

res, err := cm.results.Load(ctx, res)
if it would need a similar change but I think it doesn't atm as the cachekey of that result is not being used. The whole Load vs LoadWithParents thing could use some cleanup though. Initially, there was only Load but it wasn't enough for all use cases.

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.

3 participants