Multiple exporter responses

[a-palchikov]

This is an attempt to fix #5556 by implementing support for separate exporter responses.

While working on this, I realized that I don't quite understand the preinitialized sessions and how custom-initialized sessions play with the exporter configuration:

buildkit/client/solve.go

Lines 49 to 50 in 7d7a919

	SharedSession *session.Session // TODO: refactor to better session syncing
	SessionPreInitialized bool // TODO: refactor to better session syncing

I've moved some code that sets up exporter configuration in the session to a public API but this is definitely something I'd like more feedback and context on. Basically, I was thinking along the lines of allowing users setting up sessions on their own to reuse some work that buildkit does internally.

Otherwise, on to the actual issue: I'm trying to re-introduce explicit exporter IDs as I believe it's hard to avoid this at this point. And unless there's going to be major architectural changes around exports soon, I think this should lay some ground for further improvements with the cache exports.
By re-introducing IDs, I also removed them from actual exporter implementations as I think they carry little weight there and were introducing a point of inconsistency with the assumption that they are indices in some array. Instead - these are completely under control of the client for now.

I'm still working on tests.

Let me know if this is the direction to go or there are other options to explore.

@tonistiigi @jedevc @crazy-max @LaurentGoderre

[jedevc]

SGTM, I think I suggested this before in #4134 (comment) (but never got around to it 😢)

While we're here though, we should also split out the CacheExporterResponses as well, since those currently suffer from exactly the same issue.

[a-palchikov]

I'll focus on the image exporters for now with the tests. Will see if supporting cache responses is something that can be included or done separately.

[jedevc]

Yeah, atm, all the exporters and cache export results are merged into this one ExporterResponse map.

If all the exporters are made their own item in a list, then the caches should be split out too. We definitely shouldn't try and merge all the cache exporters into all the ExporterResponses (that's possibly the most confusing behavior).

[jedevc]

Not 100% convinced on re-introducing exporter IDs - that said, no objection, I think I liked them removed because it felt simpler, so if it feels necessary to add them back, I don't think it's too much of an issue.

Bringing them back might actually be as simple as reverting and fixing the rebase issues in 1c1777b? Although we still need the backwards-compat with old clients / servers.

[tonistiigi]

@crazy-max How will this affect --metadata-file in buildx?

[crazy-max]

@crazy-max How will this affect --metadata-file in buildx?

We would need to handle new list of resp.ExporterResponses instead of single map resp.ExporterResponse: https://github.com/docker/buildx/blob/3e3242cfdda23ab9077dde54479a74d26fcc2242/commands/build.go#L368-L400

Main case is to get the image id from it and from what I see we would need to pick the right one (docker exporter) for --iidfile flag. I recall we wanted to deprecate iidfile at some point, maybe it's time now 😅

The other case is in our GHA where we use the metadata to set imageid and digest: https://github.com/docker/build-push-action/?tab=readme-ov-file#outputs, see https://github.com/docker/build-push-action/blob/11be14d908760a0756f045980728ec5fb7880f74/src/main.ts#L120

Would need some changes in our toolkit to check if multiple exporter responses are available: https://github.com/docker/actions-toolkit/blob/ba72b5ac36b2cacdd458ba242d8ec94f5de373bf/src/buildx/build.ts#L113-L124 but not sure what "digest" would be the right one. Maybe the very first one for backward compat and we could have a new digests input that would be a map but then we would need some kind of ID if user wants to pick a specific one.

[a-palchikov]

@jedevc @tonistiigi @crazy-max Updated to keep cache export responses separate as well.

[jedevc]

@a-palchikov thanks for keeping this rebased for so long (sorry, I've been a bit inactive here).

I'd really like to move this forward, currently this information gets completely lost, so if we can get the API to serve it that's perfect. I don't know if your intention is to get this merged, or whether you're just treating as a place to keep a fork up-to-date.

I started reviewing, I have a couple things:

• Some of the refactoring are a bit far reaching, e.g. I'm not sure the refactoring in solve.go is all necessary in this change. Not to say we shouldn't work to tidy up that logic, but, it feels orthoganal to getting the API in the right shape.

• I still don't feel convinced on the exporter IDs. I know it's a while back, but using the array indices seems fine to me (also avoids us needing to update clients to create and send those).

  If we do add IDs here though, we should probably do the same for the cache exporters?

  Don't really know, (tbf, don't really care, but switching seems like more work in updating clients, even if it's less work in updating the engine). Happy to defer to @tonistiigi on this point.

I'm happy to post a longer review, or to just hack on this myself, I feel like it's been left hanging for ages (again, sorry, I dropped the ball here).

[a-palchikov]

@jedevc No worries, I figured the delay could be due to multiple things including the unattractiveness of the unnecessary changes to "tidy up" the interface. I was planning to drop those (in favor of a separate PR perhaps), just to keep the changeset clean. So hard agree from me there and expect changes in this context soon.

I will also ponder a bit around the need to propagate IDs around so client don't need to bother.