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

Validate the cache image and previous image in the analyze phase #1060

Merged
merged 7 commits into from
May 9, 2023

Conversation

matthewrobertson
Copy link
Contributor

Updates the analyzer to only reuse layers from the previous app image or cache image if they are not corrupt. This uses the new imgutil::Valid() that was added in buildpacks/imgutil#190

Fixes #1044

@matthewrobertson matthewrobertson requested a review from a team as a code owner April 5, 2023 06:08
analyzer.go Outdated Show resolved Hide resolved
cache/image_cache.go Outdated Show resolved Hide resolved
@matthewrobertson
Copy link
Contributor Author

I actually tested this end to end by creating a custom builder with my updated lifecycle and it seems like this is not sufficient to solve the issue. I can see the corrupt image is ignored during analysis but I am now getting a new error while exporting:

ERROR: failed to export: previous image did not have layer with diff id "sha256:7dd44d3eb001c70a8cfc002f048f29a3ee3ba335356f1b4d2f7cf8d0cc29173a"
ERROR: failed to build: executing lifecycle: failed with status code: 62

@natalieparellano natalieparellano added this to the lifecycle 0.17.0 milestone Apr 5, 2023
@matthewrobertson matthewrobertson force-pushed the validate-images branch 2 times, most recently from f8afa85 to e13b372 Compare April 6, 2023 07:52
@matthewrobertson
Copy link
Contributor Author

OK figured it out, all is well. I got confused by some nested conditionals... I have now tested end to end and confirmed that this is working properly and it handles previous app images and cache images with missing layers.

@natalieparellano @jabrown85 this is ready for review now.

analyzer.go Show resolved Hide resolved
@matthewrobertson
Copy link
Contributor Author

windows failures are fixed by this PR in imgutil: buildpacks/imgutil#196

Signed-off-by: Matthew Robertson <mattrobertson@google.com>
Signed-off-by: Matthew Robertson <mattrobertson@google.com>
Signed-off-by: Matthew Robertson <mattrobertson@google.com>
@jabrown85
Copy link
Contributor

@matthewrobertson looks like we still have failures in windows about corrupt images. https://github.com/buildpacks/lifecycle/actions/runs/4758563102/jobs/8456787984?pr=1060#step:6:6897

If you don't have any ideas maybe we can get someone with a windows rig to help take a look

@matthewrobertson
Copy link
Contributor Author

I still see logs like "Ignoring cache image "xxx" because it was corrupt" in test output. I will poke around to see if I can figure out why they are failing the validation.

Signed-off-by: Matthew Robertson <mattrobertson@google.com>
@matthewrobertson
Copy link
Contributor Author

@jabrown85 looks like the code was doing what it was supposed to do 😄 Windows base image layers weren't being pushed to the registry because of this: https://docs.docker.com/engine/reference/commandline/dockerd/#allow-push-of-nondistributable-artifacts

Should be fixed now 🤞

Copy link
Contributor

@jabrown85 jabrown85 left a comment

Choose a reason for hiding this comment

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

Nice work! I'll let @natalieparellano weigh in as well before merge but it looks great to me and all the tests are now passing 😄

natalieparellano and others added 2 commits April 25, 2023 22:27
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Matthew Robertson <mattrobertson@google.com>
Signed-off-by: Matthew Robertson <mattrobertson@google.com>
@natalieparellano natalieparellano merged commit e907258 into buildpacks:main May 9, 2023
7 checks passed
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.

Reusing nonexistent layers when exporting
3 participants