cache: Fix walkBlob to call callback only for unlazied contents #5595
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
There were two lines that checked if err != nil and return nil error. This seems likely to be a typo, especially since there is support for ignoring errors during cache export but on a completely different level of abstraction (in llbsolver). There were bugs causing errors during cache export (fixed in subsequent commits) which ended up getting silently dropped and causing cache exports to be missing mysteriously. Now errors are returned and only ignored if cache export errors are configured to be ignored. Signed-off-by: Erik Sipsma <[email protected]>
Before this change, lazy blobs were handled during cache export by providing descHandlers from the ref being exported in llbsolver. However, this didn't handle some max cache export cases that involve use of read-write mounts. Specifically, if you exported cache for a ref from a read-write mount in an ExecOp, the ref's descHandlers didn't include handlers for any refs from the the rootfs of the ExecOp. If any of those refs for the rootfs involved lazy blobs, any error would get hit during cache export about lazy blobs. It's possible for the rootfs to have lazy blobs in a few different ways, but the one tested in the integ test added here involves two images with layers that get deduped by chainID (i.e. uncompress to the same layer but have different compressions). Image layer refs that find an existing ref w/ same chainID will get a snapshot for free but stay lazy in terms of their blobs, thus making it possible for an exec to run on top of them while still considered lazy. The fix here puts the CacheOptGetter logic in the cache export code directly so that it can use the solver's information on dependencies to find all possible descHandlers, including those for the rootfs in the read-write mount case. Signed-off-by: Erik Sipsma <[email protected]>
Before this change, if a cache mount had base layers from a ref and those layers were lazy, you could hit missing blob errors when trying to reload an existing mutable ref for the cache mount. It's possible to have lazy refs in the base layers when blobs get deduped by chainID. The fix is just to handle the lazy blob error and reload with descHandlers set. Signed-off-by: Erik Sipsma <[email protected]>
For lazy remote cache cases, we figure out the descriptor handlers to use during loading of cache rather than during a CacheMap operation. In order to make those descHandlers available as CacheOpts we need to plumb them through to the shared op and allow withAncestorCacheOpts to check those in addition to the CacheOpts from CacheMaps. This allows loading of lazy refs during cache export when there are refs resolved with cache imports. Signed-off-by: Erik Sipsma <[email protected]>
Signed-off-by: Erik Sipsma <[email protected]>
Signed-off-by: Erik Sipsma <[email protected]>
Signed-off-by: Kohei Tokunaga <[email protected]>
github-actions
bot
added
area/testing
area/storage
area/client
area/solver
area/worker
labels
ktock
changed the title
tonistiigi
reviewed
Dec 18, 2024
| @@ -848,6 +848,11 @@ func getBlobWithCompression(ctx context.Context, cs content.Store, desc ocispecs | |||
| } | |||
|
|
|||
| func walkBlob(ctx context.Context, cs content.Store, desc ocispecs.Descriptor, f func(ocispecs.Descriptor) bool) error { | |||
| if _, err := cs.Info(ctx, desc.Digest); errors.Is(err, cerrdefs.ErrNotFound) { | |||
There was a problem hiding this comment.
Do we know a more defined call path for this case? I assume the blob is missing because it was not pulled and is missing the descHandlers. Is this expected or did we forget to pass the descHandlers? Or is this a callpath where we would want to skip a blob if we only have access to it remotely and not locally?
There was a problem hiding this comment.
This is called from getAvailableBlobs and in this context, this function doesn't have access to refs(and descHandlers) to compression variants thus GetRemtoes currently treats only local blobs as available compression variants. To change GetRemote to return all remotely accessible variants, we need to change getAvailableBlobs somehow accessible to all compression variant's refs and descHandlers to trigger unlazying of them.
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.
Testing c00304f as a following-up patch for #5560 (comment) This PR contains patches from #5560 so this should be rebased after that PR is merged. Or, this should be cherry-picked to that PR. Note that this PR doesn't contain the commit 07cf45e in that PR to check if the patch in this PR fixes the issue.
walkBlobwalks compression variants in the content store with calling a callback for the found content. The current implementation doesn't check the existence of the specified content and this resulted in a test failure described in #5560 (comment) . This commit fixes this issue.