Add fallback behavior if performance optimizations fail #81
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When saving an image to the docker daemon we currently omit base image layers because docker does not require them and fetching them requires a slow image save. However, some runtimes that implement the daemon api have stricter validation so we will fallback to the slower behavior if the first save fails Signed-off-by: Emily Casey <[email protected]>
* Tests run faster * Avoids dockerhub rate limiting Signed-off-by: Emily Casey <[email protected]>
Fixes performance hack fallback edge case where GetLayer is called before Rebase. Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
jromero
reviewed
Dec 7, 2020
| return nil, err | ||
| } | ||
| } | ||
| return os.Open(i.layerPaths[l]) |
There was a problem hiding this comment.
An edge case for sure but wouldn't it be better to check if i.layerPaths[l] is valid/non-empty after downloading the base layers? Otherwise, a very cryptic error may be thrown if that edge case is ever encountered.
Signed-off-by: Emily Casey <[email protected]>
micahyoung
previously requested changes
Dec 9, 2020
There was a problem hiding this comment.
Looks good to me, except I'm running into some temp file or test image leakage or something - each time I run the local tests on Windows, it seams about 1GB of data gets left behind. Still investigating but wanted to let you know in the meantime.
Update: nevermind, same thing is happening on main, I'll file a separate issue.
micahyoung
approved these changes
Dec 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #80
Resolves #63
When saving a daemon image we now first attempt a performant save with base layers omitted and, if that fails, fetch the base layers by
docker saveing the image, and try again. I have also removed more complicated logic that was intended to omit layers reused from the previous image whenever possible. With the addition of the launch cache in lifecycle, neither lifecycle nor pack currently take advantage of this optimization and it adds complexity.While intended to resolve #80 this also resolves #63 as a side effect because we now save an image to disk by ID, avoiding situations where
manifest.jsonmight contain multiple entries. This previously happened because we were passing a tag to docker save without expanding an implicit:latesttag. Whiledocker inspectwill treatmy/imageasmy/image:latestdocker savewill return all images with repositorymy/image. While we could save using the expanded tag, ID is the most explicit and also avoid any potential race conditions.This PR is intended to provide fixes for immediate problems with as little disruption as possible. I intend to follow up in the next two weeks with a redesign of the imgutil interface that addresses some of the domain modeling problems and improves the usage.
I attempted to test this manually with
podmanbut ran into issues where the result ofdocker inspectwas missing fields. I believe this is an incompatibility betweenpodmanand the docker client library rather than animgutilproblem. I did temporarily comment out the performance hack and rerun tests to ensure the fallback isn't broken. More robust/automated testing of the fallback will be easier to introduce after the refactor mentioned above so I have omitted it for now.