cache: don't skip unlazy without blob check

[tonistiigi]

fixes #3809

What's happening is that two builds interact with layer that has same uncompressed digest and different compressed digest. In that case we detect this and "link" the records so they will use the same snapshot and avoid duplicates. https://github.com/moby/buildkit/blob/master/cache/manager.go#L218 This means that because of lazy references the blob saved with getBlob() has not been pulled while snapshot exists.

In the #3809 problem arises when Filelist is called because of the SBOM attestation

buildkit/cache/filelist.go

Lines 37 to 50 in 3d44ec2

	// lazy blobs need to be pulled first
	if err := sr.Extract(ctx, s); err != nil {
	return nil, err
	}
	desc, err := sr.ociDesc(ctx, sr.descHandlers, false)
	if err != nil {
	return nil, err
	}
	ra, err := sr.cm.ContentStore.ReaderAt(ctx, desc)
	if err != nil {
	return nil, err
	}

. The "not found" error happens inside the ReaderAt because it can't find the blob. While there is an Unlazy step before this call to make sure we have local data, the current check only looks for the snapshot and not the blob. I guess another way to trigger this would be to try to push the second build into a different registry that doesn't already have any of the base layers.

Marking as draft for now as I try to find a way to trigger this with integration test.

@sipsma

[aaronlehmann]

Perhaps a long shot, but I wonder if #3856 might be fixed by this or have a similar cause?

[tonistiigi]

@aaronlehmann I think it's unlikely

[tonistiigi]

@ktock Looks like this PR has some effect on stargz cache import tests. Could you take a look?

[ktock]

@tonistiigi Thanks for notifying me. Extract() method with remote snapshotters doesn't guarantee that the layer content exists in the content store (because it defers downloading the layer until the actual image creation). And the tests check that behaviour.

I agree with the current implementation of FileList() that computes the filelist from the raw tarball rather than snapshot differ. So can we add a flag to unlazy() to enable forcibly downloading the layer contents to the content store even on remote snapshotters? And this should be enabled only from FileList().

Example patch based on your commit is like this : ktock@2c8acb0

[tonistiigi]

@ktock I used your commit as a base and changed it around a bit. PTAL.

[ktock]

LGTM about unlazy-related codes.
Left a comment about the containerd worker's differ.

[ktock]

containerd-rootless test uses containerd worker + native snapshotter. In this configuration, buildkitd relies on containred's differ via containerd client that doesn't support changing the compression level. So this doesn't produce the difference for the created blob. Can we use something like non-compressed tar or zstd here so that we can make sure the same layer with different compressed blobs?

[pjonsson]

@ktock I ran into the compression-level directive being ignored by exporters in another context, is that behavior documented somewhere?

[ktock]

@pjonsson FYI #4275 solves the issue. Please post an issue if that doesn't fix it.

[ktock]

@tonistiigi This PR seems accidentally closed? #4275 doesn't include commits of this PR so could you rebase this PR on top of #4275 ?

[ktock]

d733225 is still needed to pass CI.

[tonistiigi]

@ktock Sorry, I rebased the wrong branch.