Enable multiple exporters (alternative) #4134

jedevc · 2023-08-10T10:07:56Z

Some small reworks of #2760.

@a-palchikov would you be able to review this to check that this still hits the spirit of the original? The logic behind splitting this out was that while I was reviewing I found myself just endlessly nitpicking, which is quite a slow way to do review, so I thought I'd just try fixing them myself. If you're happy with the changes I've made, I'm happy to push to #2760, or try and continue the discussion in here, whichever works for you best.

Notable changes from the original include:

Split into separate small commits to simplify reviews.
Ensure client-side backwards compatibility by continuing to set the deprecated fields.
Moves the exporter ID into the Resolve method for the exporter (which means that even exporters of the same type are guaranteed to have unique IDs).
Addition of multiplexed DiffCopy using gRPC headers (similar to the existing approach for exposing containerd content stores), which allows for multiple local/tar exporters.
Deferring the result.MutableResult refactor (which was used primarily by frontends to avoid concurrent map-access). We can follow this up later.
Implement the inline cache optimization suggestion I made. This means we don't need interface juggling from the ExportImage method, while preserving the ability for exporters to request inline cache on demand (instead of requiring access through the metadata map).
Adds a new capability to be handled by high-level clients.
Adds support for multiple exporters (and image descriptors) to the build history (and fixes a possible leak, where we didn't end up freeing all the references).

api/services/control/control.proto

jedevc · 2023-08-10T10:22:06Z

solver/llbsolver/solver.go

+	// TODO: separate these out, and return multiple exporter responses to the
+	// client
+	for _, resp := range resps {
+		for k, v := range resp {
+			if exporterResponse == nil {
+				exporterResponse = make(map[string]string)
 			}
+			exporterResponse[k] = v
 		}


This is the only aspect of the overall design I'm a little unsure on. I think for now, I'd be ok this, since we already have the precedent with multiple cache exporters (see the similar comment I've added there). However, long term, I think we should split out the exporter response more (while keeping this merging behavior for the deprecated field):

message SolveResponse { map<string, string> ExporterResponseDeprecated = 1; repeated ExporterResponse ExporterResponses = 2; repeated ExporterCacheResponse CacheExporterResponses = 2; FrontendResponse FrontendRessponse = 3; }

If we take this option, I'm happy to follow-up asap with a PR to do this, but I don't think the handling for this belongs in this one, for two reasons: 1. #2760 has already been open for more than a year and 2. we should fix this for the multiple cache exporters as well, which has already merged and been released in v0.11.

Looking through the original PR, I see notice this discussed previously: #2760 (comment). This is also a viable approach, where we could make sure that exporters didn't overwrite each other's keys - though this is potentially a non-backwards compatible change (depending on details).

aaronlehmann · 2023-08-14T15:45:16Z

This will be very useful!

exporter/containerimage/writer.go

session/filesync/filesync.go

client/solve.go

a-palchikov

@jedevc There is no need to push to the original PR - this branch works just fine. I admit I did not quite understand some of your suggestions - reading the code made things much more clearer, thanks.

session/filesync/filesync.go

solver/result/result.go

solver/pb/caps.go

exporter/containerimage/writer.go

session/filesync/filesync.go

solver/llbsolver/history.go

a-palchikov · 2023-09-08T11:18:12Z

session/filesync/filesync.go

 	}
+	f, ok := sp.fs[id]
+	if !ok {
+		return errors.Errorf("exporter %q not found", id)


This would be confusing for the user if internal ids were used (e.g. local1). Maybe use the path as ID (see below)?

Given that hitting this is indicative of a buildkit internal error, I'd rather have the id printed here.

This would mean that somehow buildkit has requested to export to an exporter that the client never created.

session/filesync/filesync.go

control/control.go

a-palchikov

lgtm

api/services/control/control.proto

AkihiroSuda · 2023-09-19T18:25:24Z

client/client_test.go

+
+	require.Equal(t, resp.ExporterResponse["image.name"], target2)
+	require.FileExists(t, filepath.Join(destDir, "out.tar"))
+	require.FileExists(t, filepath.Join(destDir, "out2.tar"))


Can we validate the contents of the image and the tar?

AkihiroSuda · 2023-09-19T18:28:54Z

exporter/containerimage/writer.go

+			if err != nil {
+				return nil, err
+			}
+		}


Probably these lines didn't need to be moved

Likely not, I was just trying to match the structure of the non-map case - I can move it back if it's significant.

solver/result/result.go

jedevc · 2023-10-15T18:59:06Z

@tonistiigi are there any blockers on this? I'd really like to be able to close this piece out, feels like it could be a strong feature to include in v0.13.

brunocascio · 2023-11-08T15:06:13Z

Subscribed to this also 👍

This is useful when you have a workflows which use the image as an artifact without a registry in between but the local storage

AkihiroSuda · 2023-11-15T21:15:27Z

Needs rebase

AkihiroSuda · 2023-12-13T13:52:00Z

Needs rebase again

AkihiroSuda · 2024-01-05T04:31:15Z

@jedevc Could you rebase?

Signed-off-by: Justin Chadwell <[email protected]>

Co-authored-by: fahed dorgaa <[email protected]> Co-authored-by: a-palchikov <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

Co-authored-by: a-palchikov <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

Signed-off-by: Justin Chadwell <[email protected]>

This preps for the case where a single exporter source does not map neatly onto a single inline cache - such as introducing multiple exporters. We also introduce laziness here - each exporter chooses to attempt extracting the inline cache on demand, which ensures that we avoid creating inline caches for exporters that do not support them. Co-authored-by: a-palchikov <[email protected]> Co-authored-by: fahed dorgaa <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

This patch adds multi-plexing to the local file transfer protocol (from server to client). This is implementation-wise similar to the multiplexing from the containerd content store transfer protocol, using a GRPC header to select the appropriate target. Signed-off-by: Justin Chadwell <[email protected]>

Co-authored-by: a-palchikov <[email protected]> Co-authored-by: fahed dorgaa <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

This patch adds support for multiple exporters at the control API, and propogates the resulting required changes through the client and the solver. A few notable changes: - Each exporter instance now has an associated identifier - Build records in the build history now have multiple possible descriptors to built content - Exporter responses are all merged together (like we currently do with multiple cache exporters). We likely will need to revisit this design later, since now cache exporters do not line up one-to-one with exporters. For backwards compatability, new clients will continue to produce requests that contain the now deprecated exporter fields, as well as the new ones. New servers will attempt to use deprecated fields if they are present. Co-authored-by: a-palchikov <[email protected]> Co-authored-by: fahed dorgaa <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

Higher-level clients should be able to check server-side caps to ensure that when they attempt to use multiple exporters, that the feature is actually supported. Signed-off-by: Justin Chadwell <[email protected]>

We can derive exporter ids from their place in the exporter array in a SolveRequest - this removes the need to manually generate and handle multiple sets of IDs. Signed-off-by: Justin Chadwell <[email protected]>

jedevc · 2024-01-05T12:06:33Z

Rebased. Since this has an approval, once the tests pass, I'm going to merge it.

Signed-off-by: Justin Chadwell <[email protected]>

crazy-max

LGTM thanks for the effort on this long-waiting feature!

I'm a bit worry of the exporter responses handling on client side but your suggestion in #4134 (comment) seems like a good approach as follow-up.

As discussed we could start looking at making changes in buildx (and GHA as well to deal correctly with whatever digest user wants to consume). We also already consume the master image in our e2e tests https://github.com/docker/buildx/blob/6b63e7e3de2ef20c5d7349d7b9b277d6f0a58755/.github/workflows/e2e.yml#L70 but we should do the same for the integration tests in https://github.com/docker/buildx/blob/master/.github/workflows/build.yml when running the workflow manually so we can specify a custom image.

That being said, could we make a list of tasks that should be fulfilled in #1555 for follow-ups similar to moby/moby#45897 (a new issue is fine too)

crazy-max reviewed Aug 10, 2023

View reviewed changes

api/services/control/control.proto Show resolved Hide resolved

jedevc commented Aug 10, 2023

View reviewed changes

jedevc force-pushed the enable-multi-exporters branch 3 times, most recently from 949750f to 6728e37 Compare August 10, 2023 11:47

jedevc force-pushed the enable-multi-exporters branch from 6728e37 to 0f3e7ff Compare August 14, 2023 15:54

jedevc changed the title ~~Enable multiple exporters (testing)~~ Enable multiple exporters (alternative) Aug 14, 2023

jedevc marked this pull request as ready for review August 14, 2023 15:54

crazy-max reviewed Aug 17, 2023

View reviewed changes

exporter/containerimage/writer.go Show resolved Hide resolved

session/filesync/filesync.go Outdated Show resolved Hide resolved

client/solve.go Outdated Show resolved Hide resolved

a-palchikov reviewed Aug 25, 2023

View reviewed changes

a-palchikov mentioned this pull request Sep 6, 2023

Enable multiple exporters #2760

Closed

jedevc force-pushed the enable-multi-exporters branch 2 times, most recently from b111cb3 to deb9b41 Compare September 7, 2023 12:24

a-palchikov reviewed Sep 11, 2023

View reviewed changes

a-palchikov reviewed Sep 13, 2023

View reviewed changes

control/control.go Show resolved Hide resolved

jedevc force-pushed the enable-multi-exporters branch 2 times, most recently from 606823e to 16a74c3 Compare September 18, 2023 13:10

a-palchikov approved these changes Sep 18, 2023

View reviewed changes

AkihiroSuda reviewed Sep 19, 2023

View reviewed changes

api/services/control/control.proto Show resolved Hide resolved

AkihiroSuda reviewed Sep 19, 2023

View reviewed changes

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

AkihiroSuda reviewed Sep 19, 2023

View reviewed changes

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

AkihiroSuda approved these changes Sep 19, 2023

View reviewed changes

AkihiroSuda requested a review from tonistiigi September 19, 2023 18:34

jedevc force-pushed the enable-multi-exporters branch from 16a74c3 to 5438a4b Compare October 1, 2023 18:41

jedevc and others added 11 commits January 5, 2024 12:03

chore: remove function indirection for cache exporters

9c5816a

Signed-off-by: Justin Chadwell <[email protected]>

filesync: add doc comments in proto

578e810

Signed-off-by: Justin Chadwell <[email protected]>

exporter: avoid modifying exporter inputs

da231fb

Co-authored-by: fahed dorgaa <[email protected]> Co-authored-by: a-palchikov <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

session: create helper type for exporter file output

a80b485

Co-authored-by: a-palchikov <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

exporter: make ParseKey platform parameter optional

d970f86

Signed-off-by: Justin Chadwell <[email protected]>

exporter: add new id parameter to exporter instances

6cd8c2a

Co-authored-by: a-palchikov <[email protected]> Co-authored-by: fahed dorgaa <[email protected]> Signed-off-by: Justin Chadwell <[email protected]>

solver: add cap for multiple exporters

8822113

Higher-level clients should be able to check server-side caps to ensure that when they attempt to use multiple exporters, that the feature is actually supported. Signed-off-by: Justin Chadwell <[email protected]>

exporter: use implicit ids for exporters

1c1777b

We can derive exporter ids from their place in the exporter array in a SolveRequest - this removes the need to manually generate and handle multiple sets of IDs. Signed-off-by: Justin Chadwell <[email protected]>

jedevc force-pushed the enable-multi-exporters branch from 044b717 to 1c1777b Compare January 5, 2024 12:04

control: fix typo in exporter slice comment

cfd320c

Signed-off-by: Justin Chadwell <[email protected]>

crazy-max approved these changes Jan 5, 2024

View reviewed changes

jedevc merged commit 15b7b54 into moby:master Jan 8, 2024
60 checks passed

jedevc deleted the enable-multi-exporters branch January 8, 2024 11:19

This was referenced Jan 8, 2024

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

Open

Enable multiple exporters docker/buildx#2181

Closed

This was referenced Jan 16, 2024

chore: update buildkit upstream to master dagger/dagger#6428

Merged

filesync: append metadata for CopyToCaller #4560

Merged

blampe mentioned this pull request Feb 6, 2024

Upgrade to docker v25 pulumi/pulumi-docker#958

Closed

DaAwesomeP mentioned this pull request Feb 12, 2024

Feature Request: Save and Push docker/build-push-action#727

Closed

crazy-max mentioned this pull request Feb 16, 2024

Support push and load being set together docker/build-push-action#1056

Closed

MatTheCat mentioned this pull request Mar 7, 2024

[BUG] build --push does not push when building for a single platform using a docker-container builder instance docker/compose#11072

Closed

tonistiigi mentioned this pull request May 1, 2024

support multiple exporters for build #1555

Closed

frankgrimes97 mentioned this pull request May 14, 2024

Scans only work with amd64 images crazy-max/ghaction-container-scan#9

Open

crazy-max mentioned this pull request Jun 11, 2024

history: fix empty Exporters attribute #5017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multiple exporters (alternative) #4134

Enable multiple exporters (alternative) #4134

jedevc commented Aug 10, 2023 •

edited

Loading

jedevc Aug 10, 2023

aaronlehmann commented Aug 14, 2023

a-palchikov left a comment

a-palchikov Sep 8, 2023 •

edited

Loading

jedevc Sep 13, 2023

a-palchikov left a comment

AkihiroSuda Sep 19, 2023

AkihiroSuda Sep 19, 2023

jedevc Oct 1, 2023

jedevc commented Oct 15, 2023

brunocascio commented Nov 8, 2023

AkihiroSuda commented Nov 15, 2023

AkihiroSuda commented Dec 13, 2023

AkihiroSuda commented Jan 5, 2024

jedevc commented Jan 5, 2024

crazy-max left a comment

Enable multiple exporters (alternative) #4134

Enable multiple exporters (alternative) #4134

Conversation

jedevc commented Aug 10, 2023 • edited Loading

jedevc Aug 10, 2023

Choose a reason for hiding this comment

aaronlehmann commented Aug 14, 2023

a-palchikov left a comment

Choose a reason for hiding this comment

a-palchikov Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

jedevc Sep 13, 2023

Choose a reason for hiding this comment

a-palchikov left a comment

Choose a reason for hiding this comment

AkihiroSuda Sep 19, 2023

Choose a reason for hiding this comment

AkihiroSuda Sep 19, 2023

Choose a reason for hiding this comment

jedevc Oct 1, 2023

Choose a reason for hiding this comment

jedevc commented Oct 15, 2023

brunocascio commented Nov 8, 2023

AkihiroSuda commented Nov 15, 2023

AkihiroSuda commented Dec 13, 2023

AkihiroSuda commented Jan 5, 2024

jedevc commented Jan 5, 2024

crazy-max left a comment

Choose a reason for hiding this comment

jedevc commented Aug 10, 2023 •

edited

Loading

a-palchikov Sep 8, 2023 •

edited

Loading