Skip to content

Commit

Permalink
Ignore missing providers for blobs w/ same chainid.
Browse files Browse the repository at this point in the history 
GetByBlob checks to see if there are any other blobs with the same
(uncompressed) ChainID and, if so, reuses their unpacked snapshot if it
exists.

The problem is if this code finds a match, it was trying to get the
matching record, but couldn't do so when the match is lazy because the
caller doesn't necessarily have descriptor handlers setup for it.

This commit changes the behavior to just ignore any match with the same
ChainID that's also lazy as they just aren't usable for the
snapshot-reuse optimization.

Signed-off-by: Erik Sipsma <[email protected]>
  • Loading branch information
@sipsma
sipsma committed Jul 7, 2021
1 parent 5fc0b3c commit 026d6d6
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 15 deletions.
3 changes: 2 additions & 1 deletion cache/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ func (cm *cacheManager) GetByBlob(ctx context.Context, desc ocispec.Descriptor,
var link ImmutableRef
for _, si := range sis {
ref, err := cm.get(ctx, si.ID(), opts...)
if err != nil && !IsNotFound(err) {
// if the error was NotFound or NeedsRemoteProvider, we can't re-use the snapshot from the blob so just skip it
if err != nil && !IsNotFound(err) && !errors.As(err, &NeedsRemoteProvidersError{}) {
return nil, errors.Wrapf(err, "failed to get record %s by chainid", sis[0].ID())
}
if ref != nil {
Expand Down
90 changes: 76 additions & 14 deletions cache/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,51 @@ func TestManager(t *testing.T) {
require.Equal(t, 0, len(dirs))
}

func TestLazyGetByBlob(t *testing.T) {
t.Parallel()
ctx := namespaces.WithNamespace(context.Background(), "buildkit-test")

tmpdir, err := ioutil.TempDir("", "cachemanager")
require.NoError(t, err)
defer os.RemoveAll(tmpdir)

snapshotter, err := native.NewSnapshotter(filepath.Join(tmpdir, "snapshots"))
require.NoError(t, err)

co, cleanup, err := newCacheManager(ctx, cmOpt{
snapshotter: snapshotter,
snapshotterName: "native",
})
require.NoError(t, err)
defer cleanup()
cm := co.manager

// Test for #2226 https://github.com/moby/buildkit/issues/2226, create lazy blobs with the same diff ID but
// different digests (due to different compression) and make sure GetByBlob still works
_, desc, err := mapToBlob(map[string]string{"foo": "bar"}, true)
require.NoError(t, err)
descHandlers := DescHandlers(make(map[digest.Digest]*DescHandler))
descHandlers[desc.Digest] = &DescHandler{}
diffID, err := diffIDFromDescriptor(desc)
require.NoError(t, err)

_, err = cm.GetByBlob(ctx, desc, nil, descHandlers)
require.NoError(t, err)

_, desc2, err := mapToBlob(map[string]string{"foo": "bar"}, false)
require.NoError(t, err)
descHandlers2 := DescHandlers(make(map[digest.Digest]*DescHandler))
descHandlers2[desc2.Digest] = &DescHandler{}
diffID2, err := diffIDFromDescriptor(desc2)
require.NoError(t, err)

require.NotEqual(t, desc.Digest, desc2.Digest)
require.Equal(t, diffID, diffID2)

_, err = cm.GetByBlob(ctx, desc2, nil, descHandlers2)
require.NoError(t, err)
}

func TestSnapshotExtract(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Depends on unimplemented containerd bind-mount support on Windows")
Expand All @@ -294,7 +339,7 @@ func TestSnapshotExtract(t *testing.T) {

cm := co.manager

b, desc, err := mapToBlob(map[string]string{"foo": "bar"})
b, desc, err := mapToBlob(map[string]string{"foo": "bar"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref1", bytes.NewBuffer(b), desc)
Expand All @@ -305,7 +350,7 @@ func TestSnapshotExtract(t *testing.T) {

require.Equal(t, false, snap.Info().Extracted)

b2, desc2, err := mapToBlob(map[string]string{"foo": "bar123"})
b2, desc2, err := mapToBlob(map[string]string{"foo": "bar123"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref1", bytes.NewBuffer(b2), desc2)
Expand Down Expand Up @@ -440,13 +485,13 @@ func TestExtractOnMutable(t *testing.T) {
snap, err := active.Commit(ctx)
require.NoError(t, err)

b, desc, err := mapToBlob(map[string]string{"foo": "bar"})
b, desc, err := mapToBlob(map[string]string{"foo": "bar"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref1", bytes.NewBuffer(b), desc)
require.NoError(t, err)

b2, desc2, err := mapToBlob(map[string]string{"foo2": "1"})
b2, desc2, err := mapToBlob(map[string]string{"foo2": "1"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref2", bytes.NewBuffer(b2), desc2)
Expand Down Expand Up @@ -562,7 +607,7 @@ func TestSetBlob(t *testing.T) {
require.NoError(t, err)
defer clean(context.TODO())

b, desc, err := mapToBlob(map[string]string{"foo": "bar"})
b, desc, err := mapToBlob(map[string]string{"foo": "bar"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref1", bytes.NewBuffer(b), desc)
Expand Down Expand Up @@ -594,7 +639,7 @@ func TestSetBlob(t *testing.T) {
snap2, err := active.Commit(ctx)
require.NoError(t, err)

b2, desc2, err := mapToBlob(map[string]string{"foo2": "bar2"})
b2, desc2, err := mapToBlob(map[string]string{"foo2": "bar2"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref2", bytes.NewBuffer(b2), desc2)
Expand All @@ -612,7 +657,7 @@ func TestSetBlob(t *testing.T) {
require.Equal(t, snap2.ID(), info2.SnapshotID)
require.Equal(t, info2.Extracted, true)

b3, desc3, err := mapToBlob(map[string]string{"foo3": "bar3"})
b3, desc3, err := mapToBlob(map[string]string{"foo3": "bar3"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref3", bytes.NewBuffer(b3), desc3)
Expand All @@ -637,7 +682,7 @@ func TestSetBlob(t *testing.T) {
require.Equal(t, snap2.ID(), snap4.ID())

// snap5 is same different blob but same diffID as snap2
b5, desc5, err := mapToBlob(map[string]string{"foo5": "bar5"})
b5, desc5, err := mapToBlob(map[string]string{"foo5": "bar5"}, true)
require.NoError(t, err)

desc5.Annotations["containerd.io/uncompressed"] = info2.DiffID.String()
Expand All @@ -658,7 +703,7 @@ func TestSetBlob(t *testing.T) {
require.Equal(t, digest.FromBytes([]byte(info.BlobChainID+" "+digest.FromBytes([]byte(desc5.Digest+" "+info2.DiffID)))), snap5.Info().BlobChainID)

// snap6 is a child of snap3
b6, desc6, err := mapToBlob(map[string]string{"foo6": "bar6"})
b6, desc6, err := mapToBlob(map[string]string{"foo6": "bar6"}, true)
require.NoError(t, err)

err = content.WriteBlob(ctx, co.cs, "ref6", bytes.NewBuffer(b6), desc6)
Expand Down Expand Up @@ -999,11 +1044,23 @@ func (b *buf) close() {
<-b.closed
}

func mapToBlob(m map[string]string) ([]byte, ocispec.Descriptor, error) {
type bufferCloser struct {
*bytes.Buffer
}

func (b bufferCloser) Close() error {
return nil
}

func mapToBlob(m map[string]string, compress bool) ([]byte, ocispec.Descriptor, error) {
buf := bytes.NewBuffer(nil)
gz := gzip.NewWriter(buf)
sha := digest.SHA256.Digester()
tw := tar.NewWriter(io.MultiWriter(sha.Hash(), gz))

var dest io.WriteCloser = bufferCloser{buf}
if compress {
dest = gzip.NewWriter(buf)
}
tw := tar.NewWriter(io.MultiWriter(sha.Hash(), dest))

for k, v := range m {
if err := tw.WriteHeader(&tar.Header{
Expand All @@ -1019,12 +1076,17 @@ func mapToBlob(m map[string]string) ([]byte, ocispec.Descriptor, error) {
if err := tw.Close(); err != nil {
return nil, ocispec.Descriptor{}, err
}
if err := gz.Close(); err != nil {
if err := dest.Close(); err != nil {
return nil, ocispec.Descriptor{}, err
}

mediaType := ocispec.MediaTypeImageLayer
if compress {
mediaType = ocispec.MediaTypeImageLayerGzip
}
return buf.Bytes(), ocispec.Descriptor{
Digest: digest.FromBytes(buf.Bytes()),
MediaType: ocispec.MediaTypeImageLayerGzip,
MediaType: mediaType,
Size: int64(buf.Len()),
Annotations: map[string]string{
"containerd.io/uncompressed": sha.Digest().String(),
Expand Down

0 comments on commit 026d6d6

Please sign in to comment.