Multiple exporter responses #5586
Conversation
github-actions
bot
added
area/client
area/api
area/exporter
area/session
area/solver
labels
a-palchikov
force-pushed
the
dima/multiple-exporter-responses-dev
branch
2 times, most recently
from
7aa45a8 to
07fafeb
Compare
client/graph.go
Outdated
| ExporterResponse map[string]string | ||
| ExporterResponse map[string]string | ||
| ExporterResponses []ExporterResponse |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
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. |
|
@crazy-max How will this affect |
We would need to handle new list of 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 The other case is in our GHA where we use the metadata to set 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 |
a-palchikov
force-pushed
the
dima/multiple-exporter-responses-dev
branch
from
07fafeb to
1799133
Compare
a-palchikov
force-pushed
the
dima/multiple-exporter-responses-dev
branch
from
1799133 to
9264400
Compare
unfinished in the sense that exporter responses (which are abstracted as a map of key/value pairs) are combined into a single map at the end of the export: https://github.com/moby/buildkit/blob/55a7483b0564a7ad5b2ce5e62512789dce327bca/solver/llbsolver/solver.go#L808-L809 In order to provide the correct exporter response, each response (currently at least each container image exporter) needs to be dedicated to the exporter instance. To achieve this, assign and propagate exporter IDs from the client and have corresponding exporters annotate their responses with the respective ID so the client can order outputs per exporter instance. Fixes moby#5556. Output descriptors in container image exporters with backwards-compatible fallbacks. Move the exporter set up to a public helper API so that users that custom-initialize the session can benefit from being able to configure exporters correctly. Remove ids from the exporter implementation further shifting the burden of managing exporter identities to the control/client so it can be unified and kept an implementation detail as much as possible. Implement support for multiple exporter responses in APIs. Signed-off-by: a-palchikov <[email protected]>
image formats generated different metadata. Signed-off-by: a-palchikov <[email protected]>
Signed-off-by: a-palchikov <[email protected]>
by adding IDs similar to output exporters. Signed-off-by: a-palchikov <[email protected]>
a-palchikov
force-pushed
the
dima/multiple-exporter-responses-dev
branch
from
9264400 to
3e59e14
Compare
|
@jedevc @tonistiigi @crazy-max Updated to keep cache export responses separate as well. |
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
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