Enable multiple exporters #2760

a-palchikov · 2022-03-30T14:23:03Z

fixes #1555
This is a follow-up on #1788.

Multiple exporters

This PR enables buildkit to output to multiple destinations:

$ buildctl build \
	    --frontend=dockerfile.v0 \
	    --local context=. \
	    --local dockerfile=. \
	    --export-cache type=inline \
	    --output type=local,dest=dump \
	    --output type=local,dest=dump2 \
	    --output type=image,name=localhost:5000/out:test \
	    --output type=image,name=localhost:5000/out2:latest \
	    --output type=tar,dest=out.tar \
	    --output type=tar,dest=out2.tar \
	    --output type=oci,dest=oci.tar \
	    --output type=docker,dest=docker.tar \
	    --metadata-file metadata.json

No attempt was made to avoid metadata collisions that multiple exporter instances might be running into at this point.

tonistiigi · 2022-03-30T23:06:45Z

Have not reviewed this yet but making the returned metadata into array with objects with exporter types does not seem right. I think all exporters would just add their keys to the metadata. Exporters should be careful to not include keys that another exporter might use for a different value.

a-palchikov · 2022-03-31T09:54:36Z

Have not reviewed this yet but making the returned metadata into array with objects with exporter types does not seem right. I think all exporters would just add their keys to the metadata. Exporters should be careful to not include keys that another exporter might use for a different value.

The reasoning behind an array is the freedom it provides regarding the use of multiple instances of the same exporter(s). But if the mental model of the export is the use of unique exporters - e.g. that no exporters can reuse keys / should be careful about overwriting existing keys, then I understand that a single map is a more appealing and simpler approach.

a-palchikov · 2022-04-11T22:38:58Z

@tonistiigi I updated the implementation to pack multiple exporter responses into a single map.

tonistiigi · 2022-04-12T01:23:09Z

@a-palchikov Check the CI results.

a-palchikov · 2022-04-13T20:13:44Z

@tonistiigi Let me know if there's anything I need to do on my side.

tonistiigi

Some tests still look red in CI.

tonistiigi · 2022-04-23T04:34:58Z

api/services/control/control.proto

 message SolveResponse {
-	map<string, string> ExporterResponse = 1;
+	ExporterResponse ExporterResponse = 1;


Is this backward compatible in proto level?

Even if it is, I'll go back to the previous message layout - there is no more need for this.

tonistiigi · 2022-04-23T04:35:56Z

api/services/control/control.proto

+	map<string, string> Response = 1;
+	// Name identifies the exporter. It is empty
+	// for the common exporter metadata (e.g. cache exporter)
+	string Name = 2;


Afaics this is unused?

Yes - will remove.

tonistiigi · 2022-04-23T04:36:45Z

client/client_test.go

@@ -1893,6 +1895,74 @@ func testUser(t *testing.T, sb integration.Sandbox) {
 	checkAllReleasable(t, c, sb, true)
 }

+func testPreventsMultipleExportWithSameExporter(t *testing.T, sb integration.Sandbox) {
+	integration.SkipIfDockerd(t, sb, "multiple exporters")


Multiple local exporters should be allowed in dockerd.

tonistiigi · 2022-04-23T04:40:26Z

client/client_test.go

+}
+
+func testMultipleExporters(t *testing.T, sb integration.Sandbox) {
+	integration.SkipIfDockerd(t, sb, "multiple exporters")


Skip should not be required here but need to change the image exporter name to "moby" if Dockerd. Eg. look at testBuildInfoExporter().

tonistiigi · 2022-04-23T04:41:11Z

client/client_test.go

+				Attrs: map[string]string{
+					"name": strings.Join([]string{target1, target2}, ","),
+				},
+			},


Add tar exporters here as well for good measure.

Oh, just realized that this could be a bigger issue with the multiple file sender implementations like local and tar exporters. There needs to be some sort of fan-out for the multiple file exporters on the gRPC level.

control/control.go

exporter/containerimage/exptypes/types.go

solver/llbsolver/solver.go

tonistiigi · 2022-04-23T04:58:06Z

solver/llbsolver/solver.go

-			exporterResponse, err = e.Export(ctx, inp, j.SessionID)
-			return err
+		if err := inBuilderContext(ctx, j, exp.Name(), "", func(ctx context.Context, _ session.Group) error {
+			resp, err := exp.Export(ctx, inp, j.SessionID)


I wonder if these should run in parallel if multiple set.

I will give it a try and see if it can be made to run in parallel. Hopefully there's no mutable state to share.

a-palchikov · 2022-04-27T21:38:50Z

My recent changes had another undesired ripple effect - the propagation of compression to the inline cache exporter. I traced the changes to this PR and now have a question regarding this behavior in the presence of multiple exporters. The behavior could be extended as following:

inline cache exporter inherits the compression from the exporter if only one exporter is used
inline cache exporter inherits compression configuration from some exporter (e.g. the first configured) if multiple exporters have been specified. This is going to be confusing.
inline cache exporter inherits compression configuration from an image exporter (OCI, image).

Has a support for explicit compression configuration for the inline cache exporter been considered? (sorry if I'm out of the loop here):

     --output type=image,name=registry:5000/image:1,push=true,compression=zstd \
     --export-cache type=inline,compression=zstd

I know this was possibly a valid choice with just one export target, but now that we want multiple exporters at a time and I think it's valid to assume they might have different compression requirements.

tonistiigi · 2022-05-10T02:54:53Z

Has a support for explicit compression configuration for the inline cache exporter been considered? (sorry if I'm out of the loop here):

Inline cache export should not define a compression because it shouldn't create any new layers. Its job is to match the compression of the exported image. So there is always 1-to-1 match between image exporter and inline cache exporter. If there are multiple image exporters with multiple compressions then the inline cache needs to also run multiple times with each result connected to the correct image export.

Did this appear from making the exporters run in parallel or just a new aspect you didn't think about before? If former then we could leave that to follow-up.

FYI @ktock

a-palchikov · 2022-05-11T20:22:43Z

No, it was something I did not think about before. Thanks for explaining - I get it now. Also sorry it is taking that much time - I have actually started looking into a way to make multiple client exporters (file and directory) work together after realizing this and thinking about your suggestion.

Currently, it is not possible to run two or more local exporters (i.e. by changing the destination) or write multiple tarballs due to the limitation of a single FileSendServer. I'm running with an idea of implementing a generic FileSendServer for multiple sinks (multiple files or multiple directories) and we'll see if that pans out.

a-palchikov · 2022-06-28T18:55:27Z

client/solve.go

+	var exporters []exporter
+	ids := make(map[string]int)
+	for _, exp := range opt.Exports {
+		if id, ok := ids[exp.Type]; !ok {


A naive way to auto-id the exporters. Potentially could also be provided id from configuration.

a-palchikov · 2022-06-28T18:56:14Z

exporter/containerimage/export.go

+}
+
+func (e *imageExporterInstance) ExportImage(ctx context.Context, src exporter.Source, inlineCache exptypes.InlineCache, sessionID string) (map[string]string, error) {
+	meta := make(map[string][]byte)


Copy of the metadata as the exporters are executed concurrently.

a-palchikov · 2022-06-28T18:58:12Z

session/filesync/diffcopy.go

@@ -113,10 +124,29 @@ func syncTargetDiffCopy(ds grpc.ServerStream, dest string) error {
 				return true
 			}
 		}(),
-	}))
+	})
+	for _, dest := range dests[1:] {


This runs unconditionally of the previous error as fsutil.Receive seems to alwalys fail with context.Cancelled (even on master) and I did not look into this just yet.

a-palchikov · 2022-06-28T19:00:28Z

session/filesync/filesync.go

+}
+
+func CopyToCaller(ctx context.Context, fs fsutil.FS, c session.Caller, progress func(int, bool)) error {
+	method := session.MethodURL(_FSSend_serviceDesc.ServiceName, "diffcopy")


I chose to add another grpc server to sync directory for the local exporter since it was impossible (well, complicated) to make both file exporters and the local exporter run on the same protocol.

I think it could have already been the precedent with the FileSync service but I might be wrong.

We shouldn't need another service here.

We can do what we use for multiplexing input directories, using the grpc header. So in the filesend handler, we should get the header value (and use something similar to the DirSource interface).

a-palchikov · 2022-06-28T19:01:52Z

I also left the metadata conflict resolution up for discussion and potentially another PR.

AkihiroSuda · 2022-10-04T11:06:40Z

Needs rebase

jedevc

@a-palchikov sorry for the massively delayed review.

I've had a glance through, I think the protocol changes make sense - I've just got a couple notes around the local exporting, I think we can simplify some stuff there.

It's been a while - I'm happy to help carry this, otherwise, I'll definitely stay on top of reviews and feedback for this, really sorry for the delays again ❤️

jedevc · 2023-06-29T15:28:01Z

exporter/containerimage/export.go

+	src := *inp
+	src.Metadata = meta


If we're making a copy, we should overwrite inp with the copy so we don't accidentally use it later.

Done. I also updated the inputs to be values instead of references to present a stronger cue that they're not supposed to relay values back to caller.

jedevc · 2023-06-29T15:35:05Z

frontend/dockerui/build.go

+	r.mu.Unlock()
+}
+
+type mutableResult struct {


Hm, this is a good point.

I'd rather we rework the Result type though (making all of the public types into private ones), instead of having wrapper types around it.

jedevc · 2023-06-29T15:46:00Z

session/filesync/filesync.go

+}
+
+func CopyToCaller(ctx context.Context, fs fsutil.FS, c session.Caller, progress func(int, bool)) error {
+	method := session.MethodURL(_FSSend_serviceDesc.ServiceName, "diffcopy")


We shouldn't need another service here.

We can do what we use for multiplexing input directories, using the grpc header. So in the filesend handler, we should get the header value (and use something similar to the DirSource interface).

jedevc · 2023-06-29T15:48:18Z

solver/llbsolver/solver.go

+					}
+					return inBuilderContext(ctx, job, exp.Name(), job.SessionID+"-export", func(ctx context.Context, _ session.Group) (err error) {
+						var dref exporter.DescriptorReference
+						resps[i], dref, err = imageExporter.ExportImage(ctx, inp, *inlineCache, sessionID)


We can avoid the interface juggling by passing a function that runs the runInlineCacheExporter function. Exporters that don't support inline cache just don't need to call that function.

The interface helps to determine whether it's necessary to run the inline cache exporter in the first place - if that were a simple function / or placed into Source as additional metadata before the corresponding exporter's Export is entered, such optimization wouldn't be possible. I'm ok dropping it if it really simplifies things by a lot in your POV.

On the Result type and the need to handle concurrent updates - I see the reasons of simplicity of having a single thread-safe type but I also don't see how to do this with as little changes as possible - this change will be massive, penetrating a lot of places due to potential change of APIs - e.g. instead of allowing direct attribute access, it would have getters/setters instead to exercise greater control (that concerns the metadata of all attributes).

Which also makes me wonder if there won't be a strong case for the 2 type hierarchy - a simple Result not safe for concurrent use (and suiting the majority of use-cases) but also a thread-safe wrapper to compensate for cases where there is a legitimate need to populate the result from multiple goroutines. So far, I could only find a single place where that was necessary. Please let me know if I'm in the wrong here.

With that said, can we also reach some sort of intermediate ground and delay this refactoring to a dedicated issue?

jedevc · 2023-06-29T15:49:29Z

client/solve.go

+					Attrs: exp.Attrs,
+				})
+			} else if localExporter == nil {
+				// TODO(dima): different options per local exporter?


I think if we do my suggestion below for multi-plexing between multiple output directories, this issue should go away? We then shouldn't need to do the de-duplication.

a-palchikov · 2023-07-03T10:50:20Z

@jedevc No worries - I'll look into your suggestions, thanks!

molinch · 2023-07-18T09:56:17Z

Do you already have a clue if this feature will land in a release, or in a preview release?

a-palchikov · 2023-07-21T10:27:23Z

Do you already have a clue if this feature will land in a release, or in a preview release?

I'm working on the review comments, albeit slowly. Sorry for the delay.

In order to support multiple local exporters, a new gRPC API has been added in order to distinguish the origin of the message packet. Also, I felt that the solver/result package was slightly confusing with its use of the mutex to protect some metadata access patterns while not providing enough protection for potential data races. Instead, where necessary to protect against data races, the result values are explicitly duplicated (including the metadata). Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com> Signed-off-by: a-palchikov <deemok@gmail.com>

…h the implementations Signed-off-by: a-palchikov <deemok@gmail.com>

remains as to what to do to possibly different options per local exporter since it's currently replicated on the client but the attestation files are generated on the server. Signed-off-by: a-palchikov <deemok@gmail.com>

Signed-off-by: a-palchikov <deemok@gmail.com>

* assign the input source in exporters to avoid re-using the stale reference * remove the FSSend gRPC service - instead rely on gRPC metadata to communicate the functional protocol Signed-off-by: a-palchikov <deemok@gmail.com>

thread-safe Result. Address review comments: * Re-use the input variable in exporters to avoid accidental re-use of stale values Signed-off-by: a-palchikov <deemok@gmail.com>

molinch · 2023-09-06T06:18:55Z

Do you already have a clue if this feature will land in a release, or in a preview release?

I'm working on the review comments, albeit slowly. Sorry for the delay.

No worries thanks a lot for the feedback !
I see that some of it was merged, but not all yet, I guess nothing can be tested as of now. Correct?

a-palchikov · 2023-09-06T11:30:06Z

No worries thanks a lot for the feedback ! I see that some of it was merged, but not all yet, I guess nothing can be tested as of now. Correct?

There's actually a replacement PR for this - I'll close this one to avoid confusion.

a-palchikov force-pushed the dima/enable-multi-exporters branch from 9d597fe to 97804ac Compare March 30, 2022 15:07

a-palchikov force-pushed the dima/enable-multi-exporters branch from 6d899f4 to 083e4c5 Compare April 11, 2022 22:35

a-palchikov force-pushed the dima/enable-multi-exporters branch 2 times, most recently from 664635e to 1a24044 Compare April 12, 2022 21:08

tonistiigi self-requested a review April 13, 2022 21:18

tonistiigi reviewed Apr 23, 2022

View reviewed changes

a-palchikov force-pushed the dima/enable-multi-exporters branch from 1a24044 to 44e035f Compare April 26, 2022 23:14

a-palchikov force-pushed the dima/enable-multi-exporters branch from 44e035f to 1496c15 Compare April 27, 2022 22:29

sipsma mentioned this pull request Apr 28, 2022

Improve CI Caching dagger/dagger#2140

Closed

14 tasks

a-palchikov marked this pull request as draft May 2, 2022 13:54

a-palchikov force-pushed the dima/enable-multi-exporters branch from 1496c15 to 384c0ee Compare May 27, 2022 09:48

a-palchikov mentioned this pull request Jun 6, 2022

Receive multiple dests tonistiigi/fsutil#135

Closed

a-palchikov force-pushed the dima/enable-multi-exporters branch from 384c0ee to 60ff7c7 Compare June 28, 2022 10:32

a-palchikov commented Jun 28, 2022

View reviewed changes

a-palchikov marked this pull request as ready for review June 28, 2022 19:01

a-palchikov requested a review from tonistiigi June 28, 2022 19:02

jedevc mentioned this pull request Jul 7, 2022

Ensure exporter progress logs are properly keyed #2953

Merged

a-palchikov force-pushed the dima/enable-multi-exporters branch from 60ff7c7 to 0f4c73d Compare August 2, 2022 10:29

a-palchikov force-pushed the dima/enable-multi-exporters branch from 0f4c73d to a8ec866 Compare October 6, 2022 19:07

adminy approved these changes Nov 19, 2022

View reviewed changes

a-palchikov force-pushed the dima/enable-multi-exporters branch from 1fea6d7 to 27a1987 Compare May 1, 2023 15:53

a-palchikov requested a review from crazy-max May 8, 2023 10:40

tonistiigi assigned jedevc May 22, 2023

tonistiigi added this to the v0.12.0 milestone May 22, 2023

a-palchikov force-pushed the dima/enable-multi-exporters branch from 27a1987 to ae8e22d Compare May 29, 2023 14:34

jedevc requested changes Jun 29, 2023

View reviewed changes

tonistiigi modified the milestones: v0.12.0, v0.13 Jul 12, 2023

a-palchikov marked this pull request as draft July 22, 2023 18:49

a-palchikov force-pushed the dima/enable-multi-exporters branch from ae8e22d to 5555bd5 Compare August 2, 2023 10:21

fahedouch and others added 7 commits August 2, 2023 12:49

Update the signature of the expected image exporter interface to matc…

7a9405e

…h the implementations Signed-off-by: a-palchikov <deemok@gmail.com>

Fix a concurrent map write. Fix nil map write

75b1237

Signed-off-by: a-palchikov <deemok@gmail.com>

Address review comments

7b845fd

Signed-off-by: a-palchikov <deemok@gmail.com>

Address review comments:

9741d62

* assign the input source in exporters to avoid re-using the stale reference * remove the FSSend gRPC service - instead rely on gRPC metadata to communicate the functional protocol Signed-off-by: a-palchikov <deemok@gmail.com>

Add a new MutableResult type to solver/result package that implements

d1192f4

thread-safe Result. Address review comments: * Re-use the input variable in exporters to avoid accidental re-use of stale values Signed-off-by: a-palchikov <deemok@gmail.com>

a-palchikov force-pushed the dima/enable-multi-exporters branch from 5555bd5 to d1192f4 Compare August 2, 2023 10:50

a-palchikov marked this pull request as ready for review August 2, 2023 13:31

a-palchikov requested a review from jedevc August 2, 2023 13:32

This was referenced Aug 8, 2023

chore: tidy up filesync encode headers handling #4124

Merged

filesync: append rather than replace grpc md. #4049

Merged

Enable multiple exporters (alternative) #4134

Merged

Add session callback before export completion #4139

Open

a-palchikov closed this Sep 6, 2023

Enable multiple exporters #2760

Enable multiple exporters #2760

Conversation

a-palchikov commented Mar 30, 2022 • edited by crazy-max Loading

Multiple exporters

tonistiigi commented Mar 30, 2022

a-palchikov commented Mar 31, 2022

a-palchikov commented Apr 11, 2022

tonistiigi commented Apr 12, 2022

a-palchikov commented Apr 13, 2022

tonistiigi left a 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

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

a-palchikov commented Apr 27, 2022 • edited Loading

tonistiigi commented May 10, 2022 • edited Loading

a-palchikov commented May 11, 2022

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

a-palchikov commented Jun 28, 2022

AkihiroSuda commented Oct 4, 2022

jedevc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-palchikov commented Jul 3, 2023

molinch commented Jul 18, 2023

a-palchikov commented Jul 21, 2023

molinch commented Sep 6, 2023

a-palchikov commented Sep 6, 2023

a-palchikov commented Mar 30, 2022 •

edited by crazy-max

Loading

a-palchikov commented Apr 27, 2022 •

edited

Loading

tonistiigi commented May 10, 2022 •

edited

Loading