moby / moby/buildkit

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

Open
#4,532 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
10.3k
Forks
1.5k
Avg merge
1d 23h
Merged PRs (30d)
48

Description

Follow-up to https://github.com/moby/buildkit/pull/4134#discussion_r1289910449.

With #3024 #4134 we subtly introduced an issue when multiple cache exporters and exporters are used, if multiple of the same type are used.

The issue is that we unconditionally just merge all the exporter responses together (even when there are possibly conflicting keys): https://github.com/moby/buildkit/blob/15b7b542ae352a1650716c58f8eeec41f5b38c28/solver/llbsolver/solver.go#L573-L582

My proposed solution is to split out the SolveResponse struct from:

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

into:

message SolveResponse {
	map<string, string> ExporterResponseDeprecated = 1;

	repeated ExporterResponse ExporterResponses = 2;
	repeated ExporterCacheResponse CacheExporterResponses = 2;
	FrontendResponse FrontendRessponse = 3;
}

We would keep returning the combined responses in the deprecated field, but would then also return these new fields that have all the data correctly split out.

This then gives clients the option to easily distinguish between which exporters did what and produced which results. One of the open questions is how to address the --metadata-file option:

https://github.com/moby/buildkit/blob/15b7b542ae352a1650716c58f8eeec41f5b38c28/cmd/buildctl/build.go#L353-L358

To really solve this issue, we need to make sure that the solve response stays separated out here - not just falling back to the deprecated field. However: we should avoid breaking existing usage (that relies on the current format) of the --metadata-file.

One possible option is to nest the exporter responses into the file (we should do this transformation client-side):

{
	"<deprecated key>": "<deprecated value>",
	"exporters": [
		{
			"<new key>": "<new value>"
		}
	],
	"cache": [
		{
			"<new key>": "<new value>"
		}
	],
	"frontend": {
		"<new key>": "<new value>"
	]
}

Essentially we can keep the format as is, but add some new fields that have the split out properties correctly. So new usages can rely on those, and can fallback to the old properties (e.g. the github actions, cc @crazy-max), while users relying on the old usage wouldn't break. This would allow us to deprecate and then remove the old format eventually, though I'm unsure of exact timelines.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with solver/llbsolver/solver.go at the linked response-merging code, then inspect the protobuf SolveResponse definition and cmd/buildctl/build.go around the --metadata-file handling. Resolve how separated exporter, cache-exporter, and frontend responses should be represented while preserving the deprecated combined field and existing metadata-file users. Done means clients can distinguish response sources without breaking the current format.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, build-system
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.