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.

Sign up for GitHub

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

cache: don't skip unlazy without blob check #4210

Merged
merged 2 commits into from
Sep 26, 2023
Merged

cache: don't skip unlazy without blob check #4210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e371069
cache: don't skip unlazy without blob check
tonistiigi Sep 9, 2023
5dccc0a
Ensure layers in the local content store on FileList API
ktock Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cache/filelist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (sr *immutableRef) FileList(ctx context.Context, s session.Group) ([]string
}

// lazy blobs need to be pulled first
if err := sr.Extract(ctx, s); err != nil {
if err := sr.ensureLocalContentBlob(ctx, s); err != nil {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion cache/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ func (cm *cacheManager) GetByBlob(ctx context.Context, desc ocispecs.Descriptor,

ref := rec.ref(true, descHandlers, nil)
if s := unlazySessionOf(opts...); s != nil {
if err := ref.unlazy(ctx, ref.descHandlers, ref.progress, s, true); err != nil {
if err := ref.unlazy(ctx, ref.descHandlers, ref.progress, s, true, false); err != nil {
return nil, err
}
}
Expand Down
40 changes: 28 additions & 12 deletions cache/refs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,14 @@ func (sr *immutableRef) Mount(ctx context.Context, readonly bool, s session.Grou
return mnt, nil
}

func (sr *immutableRef) ensureLocalContentBlob(ctx context.Context, s session.Group) error {
if (sr.kind() == Layer || sr.kind() == BaseLayer) && !sr.getBlobOnly() {
return nil
}

return sr.unlazy(ctx, sr.descHandlers, sr.progress, s, true, true)
}

func (sr *immutableRef) Extract(ctx context.Context, s session.Group) (rerr error) {
if (sr.kind() == Layer || sr.kind() == BaseLayer) && !sr.getBlobOnly() {
return nil
Expand All @@ -1002,14 +1010,14 @@ func (sr *immutableRef) Extract(ctx context.Context, s session.Group) (rerr erro
if rerr = sr.prepareRemoteSnapshotsStargzMode(ctx, s); rerr != nil {
return
}
rerr = sr.unlazy(ctx, sr.descHandlers, sr.progress, s, true)
rerr = sr.unlazy(ctx, sr.descHandlers, sr.progress, s, true, false)
}); err != nil {
return err
}
return rerr
}

return sr.unlazy(ctx, sr.descHandlers, sr.progress, s, true)
return sr.unlazy(ctx, sr.descHandlers, sr.progress, s, true, false)
}

func (sr *immutableRef) withRemoteSnapshotLabelsStargzMode(ctx context.Context, s session.Group, f func()) error {
Expand Down Expand Up @@ -1149,25 +1157,33 @@ func makeTmpLabelsStargzMode(labels map[string]string, s session.Group) (fields
return
}

func (sr *immutableRef) unlazy(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group, topLevel bool) error {
func (sr *immutableRef) unlazy(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group, topLevel bool, ensureContentStore bool) error {
_, err := g.Do(ctx, sr.ID()+"-unlazy", func(ctx context.Context) (_ struct{}, rerr error) {
if _, err := sr.cm.Snapshotter.Stat(ctx, sr.getSnapshotID()); err == nil {
return struct{}{}, nil
if !ensureContentStore {
return struct{}{}, nil
}
if blob := sr.getBlob(); blob == "" {
return struct{}{}, nil
}
if _, err := sr.cm.ContentStore.Info(ctx, sr.getBlob()); err == nil {
return struct{}{}, nil
}
}

switch sr.kind() {
case Merge, Diff:
return struct{}{}, sr.unlazyDiffMerge(ctx, dhs, pg, s, topLevel)
return struct{}{}, sr.unlazyDiffMerge(ctx, dhs, pg, s, topLevel, ensureContentStore)
case Layer, BaseLayer:
return struct{}{}, sr.unlazyLayer(ctx, dhs, pg, s)
return struct{}{}, sr.unlazyLayer(ctx, dhs, pg, s, ensureContentStore)
}
return struct{}{}, nil
})
return err
}

// should be called within sizeG.Do call for this ref's ID
func (sr *immutableRef) unlazyDiffMerge(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group, topLevel bool) (rerr error) {
func (sr *immutableRef) unlazyDiffMerge(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group, topLevel bool, ensureContentStore bool) (rerr error) {
eg, egctx := errgroup.WithContext(ctx)
var diffs []snapshot.Diff
sr.layerWalk(func(sr *immutableRef) {
Expand All @@ -1177,13 +1193,13 @@ func (sr *immutableRef) unlazyDiffMerge(ctx context.Context, dhs DescHandlers, p
if sr.diffParents.lower != nil {
diff.Lower = sr.diffParents.lower.getSnapshotID()
eg.Go(func() error {
return sr.diffParents.lower.unlazy(egctx, dhs, pg, s, false)
return sr.diffParents.lower.unlazy(egctx, dhs, pg, s, false, ensureContentStore)
})
}
if sr.diffParents.upper != nil {
diff.Upper = sr.diffParents.upper.getSnapshotID()
eg.Go(func() error {
return sr.diffParents.upper.unlazy(egctx, dhs, pg, s, false)
return sr.diffParents.upper.unlazy(egctx, dhs, pg, s, false, ensureContentStore)
})
}
case Layer:
Expand All @@ -1192,7 +1208,7 @@ func (sr *immutableRef) unlazyDiffMerge(ctx context.Context, dhs DescHandlers, p
case BaseLayer:
diff.Upper = sr.getSnapshotID()
eg.Go(func() error {
return sr.unlazy(egctx, dhs, pg, s, false)
return sr.unlazy(egctx, dhs, pg, s, false, ensureContentStore)
})
}
diffs = append(diffs, diff)
Expand Down Expand Up @@ -1223,7 +1239,7 @@ func (sr *immutableRef) unlazyDiffMerge(ctx context.Context, dhs DescHandlers, p
}

// should be called within sizeG.Do call for this ref's ID
func (sr *immutableRef) unlazyLayer(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group) (rerr error) {
func (sr *immutableRef) unlazyLayer(ctx context.Context, dhs DescHandlers, pg progress.Controller, s session.Group, ensureContentStore bool) (rerr error) {
if !sr.getBlobOnly() {
return nil
}
Expand All @@ -1250,7 +1266,7 @@ func (sr *immutableRef) unlazyLayer(ctx context.Context, dhs DescHandlers, pg pr
parentID := ""
if sr.layerParent != nil {
eg.Go(func() error {
if err := sr.layerParent.unlazy(egctx, dhs, pg, s, false); err != nil {
if err := sr.layerParent.unlazy(egctx, dhs, pg, s, false, ensureContentStore); err != nil {
return err
}
parentID = sr.layerParent.getSnapshotID()
Expand Down
143 changes: 143 additions & 0 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ func TestIntegration(t *testing.T) {
testLLBMountPerformance,
testClientCustomGRPCOpts,
testMultipleRecordsWithSameLayersCacheImportExport,
testSnapshotWithMultipleBlobs,
testExportLocalNoPlatformSplit,
testExportLocalNoPlatformSplitOverwrite,
)
Expand Down Expand Up @@ -5494,6 +5495,148 @@ func testMultipleRecordsWithSameLayersCacheImportExport(t *testing.T, sb integra
ensurePruneAll(t, c, sb)
}

// moby/buildkit#3809
func testSnapshotWithMultipleBlobs(t *testing.T, sb integration.Sandbox) {
workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter)
c, err := New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

// build two images with same layer but different compressed blobs

registry, err := sb.NewRegistry()
if errors.Is(err, integration.ErrRequirements) {
t.Skip(err.Error())
}
require.NoError(t, err)

now := time.Now()

st := llb.Scratch().File(llb.Copy(llb.Image("alpine"), "/", "/alpine/", llb.WithCreatedTime(now)))

def, err := st.Marshal(sb.Context())
require.NoError(t, err)

name1 := registry + "/multiblobtest1/image:latest"

_, err = c.Solve(sb.Context(), def, SolveOpt{
Exports: []ExportEntry{
{
Type: "image",
Attrs: map[string]string{
"name": name1,
"push": "true",
"compression-level": "0",
},
},
},
}, nil)
require.NoError(t, err)

ensurePruneAll(t, c, sb)

st = st.File(llb.Mkfile("test", 0600, []byte("test"))) // extra layer so we don't get a cache match based on image config rootfs only

def, err = st.Marshal(sb.Context())
require.NoError(t, err)

name2 := registry + "/multiblobtest2/image:latest"

_, err = c.Solve(sb.Context(), def, SolveOpt{
Exports: []ExportEntry{
{
Type: "image",
Attrs: map[string]string{
"name": name2,
"push": "true",
"compression-level": "9",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

},
},
},
}, nil)
require.NoError(t, err)

ensurePruneAll(t, c, sb)

// first build with first image
destDir := t.TempDir()

out1 := filepath.Join(destDir, "out1.tar")
outW1, err := os.Create(out1)
require.NoError(t, err)

st = llb.Image(name1).File(llb.Mkfile("test", 0600, []byte("test1")))

def, err = st.Marshal(sb.Context())
require.NoError(t, err)

_, err = c.Solve(sb.Context(), def, SolveOpt{
Exports: []ExportEntry{
{
Type: ExporterOCI, // make sure to export so blobs need to be loaded
Output: fixedWriteCloser(outW1),
},
},
}, nil)
require.NoError(t, err)

// make sure second image does not cause any errors
out2 := filepath.Join(destDir, "out2.tar")
outW2, err := os.Create(out2)
require.NoError(t, err)

st = llb.Image(name2).File(llb.Mkfile("test", 0600, []byte("test2")))

def, err = st.Marshal(sb.Context())
require.NoError(t, err)

_, err = c.Solve(sb.Context(), def, SolveOpt{
FrontendAttrs: map[string]string{
"attest:sbom": "",
},
Exports: []ExportEntry{
{
Type: ExporterOCI, // make sure to export so blobs need to be loaded
Output: fixedWriteCloser(outW2),
},
},
}, nil)
require.NoError(t, err)

// extra validation that we did get different layer blobs
dt, err := os.ReadFile(out1)
require.NoError(t, err)

m, err := testutil.ReadTarToMap(dt, false)
require.NoError(t, err)

var index ocispecs.Index
err = json.Unmarshal(m["index.json"].Data, &index)
require.NoError(t, err)

var mfst ocispecs.Manifest
err = json.Unmarshal(m["blobs/sha256/"+index.Manifests[0].Digest.Hex()].Data, &mfst)
require.NoError(t, err)

dt, err = os.ReadFile(out2)
require.NoError(t, err)

m, err = testutil.ReadTarToMap(dt, false)
require.NoError(t, err)

err = json.Unmarshal(m["index.json"].Data, &index)
require.NoError(t, err)

err = json.Unmarshal(m["blobs/sha256/"+index.Manifests[0].Digest.Hex()].Data, &index)
require.NoError(t, err)

var mfst2 ocispecs.Manifest
err = json.Unmarshal(m["blobs/sha256/"+index.Manifests[0].Digest.Hex()].Data, &mfst2)
require.NoError(t, err)

require.NotEqual(t, mfst.Layers[0].Digest, mfst2.Layers[0].Digest)
}

func testExportLocalNoPlatformSplit(t *testing.T, sb integration.Sandbox) {
workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureMultiPlatform)
c, err := New(sb.Context(), sb.Address())
Expand Down
Loading