Multiple cache exports #3024

a-palchikov · 2022-08-12T00:22:00Z

a-palchikov · 2022-08-15T17:51:21Z

There seems to be a regression in Go 1.18.5 visible in flightcontrol package tests - they seem to be up to 6 times slower:
median for last 4 PRs prior to Go version bump was 72.97s, after Go was updated it jumped (see this PR):

ok  	github.com/moby/buildkit/util/flightcontrol	420.389s	coverage: 65.8% of statements

And now it's not fitting the allotted 10min.

crazy-max · 2022-08-15T17:58:21Z

There seems to be a regression in Go 1.18.5

Seems to be the same behavior in #3022

remidebette · 2022-08-29T15:31:42Z

There seems to be a regression in Go 1.18.5

Seems to be the same behavior in #3022

So was the unit test issue solved there?

a-palchikov · 2022-08-29T16:00:16Z

So was the unit test issue solved there?

Yes, I believe so - by bumping the test timeout to 30m here.
I'll fix the conflicts and update shortly.

jedevc · 2022-09-02T08:56:17Z

solver/llbsolver/solver.go

-		if err := inBuilderContext(ctx, j, "exporting cache", j.SessionID+"-cache", func(ctx context.Context, _ session.Group) error {
-			prepareDone := progress.OneOff(ctx, "preparing build cache for export")
-			if err := res.EachRef(func(res solver.ResultProxy) error {
+func exportInlineCache(ctx context.Context, e exporter.ExporterInstance, inlineExporter *RemoteCacheExporter, j *solver.Job, res *frontend.Result) (exporterResponse map[string]string, err error) {


This function needs to be split out more - I think we can keep everything except the if inlineExporter != nil in the Solve function, so that this function handles only the inline exporter.

The name might not be the most appropriate - I can rename to exportWithInlineCacheExporter - otherwise I believe it already contains the parts only relevant for the (non-cache) export.

The name is better for sure - I think we should split up the function to only contain the inline cache export though and to do the non-cache export completely separately. There is some dependency there, but we can do the inline cache export by modifying a pointer to an exporter.Source object.

jedevc · 2022-09-02T08:59:22Z

solver/llbsolver/solver.go

-		})
+		var inlineExporter *RemoteCacheExporter
+		cacheExporters, inlineExporter = splitExporters(cacheExporters)
+		exporterResponse, err = exportInlineCache(ctx, e, inlineExporter, j, res)


nit: we could put the check for inlineExporter != nil here in Solve for clarity.

jedevc · 2022-09-02T09:03:56Z

solver/llbsolver/solver.go

+	for _, exp := range exporters {
+		exp := exp
+		eg.Go(func() error {
+			return inBuilderContext(ctx, j, "exporting cache", j.SessionID+"-cache", func(ctx context.Context, _ session.Group) error {


We should change "exporting cache" to also include the cache type here (probably shouldn't have any of the other parameters, though we possibly could), and make sure that the ID field (currently j.SessionID + "-cache") is unique for each exporter, or we'll get confusing build log output.

Personally, I'd like to have a .Name method on the remotecache.Exporter interface, similar to how we already have for exporter.ExporterInstance.

Agree - I'll add the Name API on the remotecache.Exporter.

jedevc · 2022-09-02T09:10:29Z

solver/llbsolver/solver.go

+	eg, ctx := errgroup.WithContext(ctx)
+	g := session.NewGroup(j.SessionID)
+	var cacheExporterResponse map[string]string
+	respCh := make(chan map[string]string, len(exporters))


The convention for these when we have a known number of outputs in parallel is to have an array and to assign to indexes in it: see here.

cache/remotecache/azblob/exporter.go

solver/llbsolver/solver.go

jedevc · 2022-09-06T08:34:23Z

solver/llbsolver/solver.go

-			return res.Result(ctx)
-		})
+		var inlineCacheExporter *RemoteCacheExporter
+		cacheExporters, inlineCacheExporter = splitCacheExporters(cacheExporters)


Should this split be unconditional? If we reach a scenario where exp.Exporter == nil, we shouldn't attempt to build inline cache using runCacheExporters below, right?

Ugh, right. Good point.

jedevc · 2022-09-06T08:43:36Z

solver/llbsolver/solver.go

+				return inBuilderContext(ctx, j, fmt.Sprint("exporting ", exp.Exporter.Name()), id, func(ctx context.Context, _ session.Group) error {
+					prepareDone := progress.OneOff(ctx, "preparing build cache for export")
+					if err := res.EachRef(func(res solver.ResultProxy) error {
+						r, err := res.Result(ctx)


This isn't something changed in this PR, but I think we want to avoid calling Result twice. I think if we can do #3024 (comment) by moving the result conversion steps back into .Solve, we can just pass the cached variable into this function instead of res.

I see that I can make use of both - cached and the immutable ref result. I'll update.

jedevc · 2022-09-14T10:33:08Z

LGTM (assuming CI passes) 🎉

PTAL @tonistiigi

solver/result/result.go

crazy-max · 2022-10-03T12:47:06Z

solver/llbsolver/solver.go

+	for i, exp := range exporters {
+		func(exp RemoteCacheExporter, i int) {


Like cache imports I think we should store a cache export ID so we make sure the ref (registry, local, scope, ...) is dedup:

buildkit/solver/llbsolver/bridge.go

Line 83 in f2c41e1

if prevCm, ok := b.cms[cmID]; !ok {

I'm looking into that.

@crazy-max added deduplication of cache exporters in 702ba77

AkihiroSuda · 2022-11-03T00:45:45Z

Please squash commits

Fix cache import/export tests w.r.t inline caching. Signed-off-by: a-palchikov <deemok@gmail.com>

a-palchikov · 2022-11-03T14:41:42Z

@AkihiroSuda done in cf45d28

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from ce0b54d to 8115476 Compare August 15, 2022 12:38

crazy-max mentioned this pull request Aug 16, 2022

ci: align test flags #3031

Merged

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 8115476 to 864825f Compare August 30, 2022 14:43

tonistiigi requested a review from jedevc September 2, 2022 05:10

jedevc reviewed Sep 2, 2022

View reviewed changes

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 864825f to e436b23 Compare September 5, 2022 21:38

jedevc reviewed Sep 6, 2022

View reviewed changes

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from e436b23 to 6f3bbad Compare September 13, 2022 15:00

jedevc approved these changes Sep 14, 2022

View reviewed changes

crazy-max added the status/code-review label Sep 14, 2022

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 6f3bbad to 5f4124f Compare September 26, 2022 11:30

jedevc reviewed Sep 29, 2022

View reviewed changes

solver/result/result.go Outdated Show resolved Hide resolved

jedevc requested a review from tonistiigi September 29, 2022 10:02

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 5f4124f to 188c5ad Compare October 3, 2022 12:20

crazy-max reviewed Oct 3, 2022

View reviewed changes

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 188c5ad to 88eb404 Compare October 5, 2022 19:53

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 88eb404 to 702ba77 Compare October 31, 2022 11:33

Add support for multiple cache exports.

cf45d28

Fix cache import/export tests w.r.t inline caching. Signed-off-by: a-palchikov <deemok@gmail.com>

a-palchikov force-pushed the dima/2818/multiple-cache-exports branch from 702ba77 to cf45d28 Compare November 3, 2022 13:13

jedevc merged commit ac4f39a into moby:master Nov 3, 2022

This was referenced Nov 10, 2022

(*service).Write failed when running multiple local cache exporters in parallel #3270

Open

Don't allow duplicate cache exporters #3271

Merged

andreafalzetti mentioned this pull request Apr 12, 2023

Upgrade Buildkit [timeboxed 2 days] okteto/okteto#3548

Closed

3 tasks

arkid15r mentioned this pull request Nov 3, 2023

Docker Buildkit multiple cache exporters support docker/docs#18590

Open

1 task

jedevc mentioned this pull request Jan 8, 2024

Split out SolveResponse to avoid key collision with multiple exporters/cache exporters #4532

Open

huonw mentioned this pull request Mar 11, 2024

Docker: Support multiple cache_from values for docker_image. pantsbuild/pants#20600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple cache exports #3024

Multiple cache exports #3024

a-palchikov commented Aug 12, 2022

a-palchikov commented Aug 15, 2022

crazy-max commented Aug 15, 2022

remidebette commented Aug 29, 2022

a-palchikov commented Aug 29, 2022

jedevc Sep 2, 2022

a-palchikov Sep 5, 2022

jedevc Sep 6, 2022

jedevc Sep 2, 2022 •

edited

jedevc Sep 2, 2022

a-palchikov Sep 5, 2022

jedevc Sep 2, 2022

jedevc Sep 6, 2022

a-palchikov Sep 13, 2022

jedevc Sep 6, 2022

a-palchikov Sep 13, 2022

jedevc commented Sep 14, 2022

crazy-max Oct 3, 2022 •

edited

a-palchikov Oct 6, 2022

a-palchikov Oct 31, 2022

AkihiroSuda commented Nov 3, 2022

a-palchikov commented Nov 3, 2022

		for i, exp := range exporters {
		func(exp RemoteCacheExporter, i int) {

Multiple cache exports #3024

Multiple cache exports #3024

Conversation

a-palchikov commented Aug 12, 2022

a-palchikov commented Aug 15, 2022

crazy-max commented Aug 15, 2022

remidebette commented Aug 29, 2022

a-palchikov commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedevc Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedevc commented Sep 14, 2022

crazy-max Oct 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 3, 2022

a-palchikov commented Nov 3, 2022

jedevc Sep 2, 2022 •

edited

crazy-max Oct 3, 2022 •

edited