remote cache: support arbitrary layers as results #2501

sipsma · 2021-12-01T18:15:26Z

This change enables inline cache to work as expected with MergeOp by
supporting a new type of result, DiffResult, which enables results to be
specified as a specific ordered set of layers inside an image.
Previously, results could only be specified with a singe layer index,
which meant that they had to start at the image's base layer and end at
that index. That meant that merge inputs couldn't be specified as they
are often a subset of the image layers that don't begin at the base.

Signed-off-by: Erik Sipsma erik@sipsma.dev

sipsma · 2021-12-01T18:28:13Z

cache/remotecache/v1/parse.go

+	// TODO: what the behavior on previous builders when there's no Result. Is it
+	// assumed that this record is Scratch? Or is it a cache miss? Want the latter.


@tonistiigi This is my main remaining question, the answer wasn't immediately obvious when jumping through the code so would appreciate any insight you can provide.

This case is important because now when a record can't be specified using the old style CacheResult and instead has to use the new DiffResults, an old buildkitd will see that the record exists but has empty CacheResult. The behavior I want is for these buildkitds to then just get a cache miss in this case.

If it turns out instead that they assume "no result" means "result is scratch" then we'll have to modify this slightly. We could add a new type to replace CacheRecord that exclusively uses DiffResults (so old builders won't even see records that use diff results). Or another option would be to have CacheRecord set its LayerIndex to -1 in these cases, which would mean that old buildkitds get an error when trying to import this cache. That's not ideal obviously but is better than them getting a cache hit with the incorrect result.

If you never call AddResult then it will not load any cache. Loading scratch should be possible with AddResult(nil) (maybe). In cache interface https://github.com/moby/buildkit/blob/master/docs/solver.md#cache-providers query and records() are separate concepts. So a vertex can match cache but there is no loadable record behind that match. This is because we can use the cache match to check for a cache match for the next vertex and that might be loadable now.

tonistiigi · 2022-01-07T01:08:48Z

cache/remotecache/v1/spec.go

-	Digest  digest.Digest  `json:"digest,omitempty"`
-	Inputs  [][]CacheInput `json:"inputs,omitempty"`
+	Results     []CacheResult  `json:"layers,omitempty"`
+	DiffResults []DiffResult   `json:"diffs,omitempty"`


One naming solution might be that if the old ones are called "layers" and only have one index then new ones are "chains" as they have a chain of indexes.

Changed the name to ChainedResult

At least in JSON I'd prefer just json:"chains,omitempty" . The results themselves are not chained. It's if a result points to layer index or a chain of layer indexes.

Updated to chains

tonistiigi · 2022-01-07T01:12:02Z

cache/remotecache/inline/inline.go

@@ -76,11 +77,50 @@ func (ce *exporter) ExportForLayers(ctx context.Context, layers []digest.Digest)
 	cache := map[int]int{}

 	// reorder layers based on the order in the image
+	blobIndexes := map[digest.Digest]int{}


nit: initialize with length

tonistiigi · 2022-01-07T01:20:30Z

cache/remotecache/inline/inline.go

+				}
+			}
+			if match {
+				n := getSortedLayerIndex(rr.LayerIndex, cfg.Layers, cache)


isn't this just len(resultBlobs) now?

Yeah that minus one, good call, fixed.

tonistiigi · 2022-01-07T01:23:57Z

cache/remotecache/inline/inline.go

+			}
+			var filteredResults []v1.CacheResult
+			for _, rr := range r.Results {
+				empty := v1.CacheResult{}


var doesn't seem needed here

Doing if rr != v1.CacheResult{} { is a syntax error unfortunately, i.e.: https://go.dev/play/p/kCPmFl4rpU0

So I think it's needed unless I'm missing something

rr != (v1.CacheResult{}) should work

Ah cool, fixed

tonistiigi · 2022-01-07T01:24:30Z

cache/remotecache/inline/inline.go

+				r.Results[j] = v1.CacheResult{}
+				r.DiffResults = append(r.DiffResults, diffResult)
+			}
+			var filteredResults []v1.CacheResult


add a comment that this removes the results that were converted to diffresults.

This change enables inline cache to work as expected with MergeOp by supporting a new type of result, DiffResult, which enables results to be specified as a specific ordered set of layers inside an image. Previously, results could only be specified with a singe layer index, which meant that they had to start at the image's base layer and end at that index. That meant that merge inputs couldn't be specified as they are often a subset of the image layers that don't begin at the base. Signed-off-by: Erik Sipsma <erik@sipsma.dev>

tonistiigi · 2022-01-16T05:19:14Z

I discovered a case that doesn't work very well with inline cache atm but it isn't directly related to this PR. We don't export cache for the roots atm. The thinking behind that was that if base image directly matches we might as well just pull it from upstream, not from cache. We also don't want to pull git sources from cache and instead should do a fresh clone. And of course caching the local context is just useless(there is another protection against that). Not caching images is actually maybe not that correct. There is issue where people are confused why the base image is not pulled from local directory with --cache-from type=local.

But related to inline cache and mergeop the interesting case is as follows.

llb.Merge([]llb.State{base, llb.Scratch().File(llb.Mkdir(),llb.Mkfile(),..), anotherstate})

Because llb.Scratch().File only makes a single layer with no inputs then if you look at the inline cache output it doesn't have a value for chains. So everything around it matches cache, including full mergeop, but if one of the inputs of mergeop gets invalidated then the llb.Scratch()... gets invalidates as well. That means that it will generate new blobs because timestamps change etc.

Even in the slightly different case:

llb.Merge([]llb.State{base, llb.Scratch().File(llb.Mkdir()).File(llb.Mkfile(),..), anotherstate})

The issue doesn't appear anymore (unless Mkfile() changes) as now there are chain values for the Mkfile part.

sipsma requested a review from tonistiigi December 1, 2021 18:15

sipsma commented Dec 1, 2021

View reviewed changes

sipsma mentioned this pull request Jan 6, 2022

Merge+Diff follow-ups #2548

Open

10 tasks

tonistiigi reviewed Jan 7, 2022

View reviewed changes

sipsma force-pushed the mergeop-inline-cache branch from c8e3211 to 5b45fae Compare January 11, 2022 20:37

sipsma marked this pull request as ready for review January 11, 2022 21:13

sipsma requested a review from tonistiigi January 11, 2022 21:13

sipsma force-pushed the mergeop-inline-cache branch from 5b45fae to ce012ab Compare January 11, 2022 21:29

tonistiigi approved these changes Jan 16, 2022

View reviewed changes

tonistiigi merged commit cfdaaa4 into moby:master Jan 16, 2022

crazy-max added this to the v0.10.0 milestone Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remote cache: support arbitrary layers as results #2501

remote cache: support arbitrary layers as results #2501

sipsma commented Dec 1, 2021 •

edited

Loading

sipsma Dec 1, 2021 •

edited

Loading

tonistiigi Jan 6, 2022

tonistiigi Jan 7, 2022

sipsma Jan 11, 2022

tonistiigi Jan 11, 2022

sipsma Jan 11, 2022

tonistiigi Jan 7, 2022

sipsma Jan 11, 2022

tonistiigi Jan 7, 2022

sipsma Jan 11, 2022

tonistiigi Jan 7, 2022

sipsma Jan 11, 2022

tonistiigi Jan 11, 2022

sipsma Jan 11, 2022

tonistiigi Jan 7, 2022

sipsma Jan 11, 2022

tonistiigi commented Jan 16, 2022

		// TODO: what the behavior on previous builders when there's no Result. Is it
		// assumed that this record is Scratch? Or is it a cache miss? Want the latter.

remote cache: support arbitrary layers as results #2501

remote cache: support arbitrary layers as results #2501

Conversation

sipsma commented Dec 1, 2021 • edited Loading

sipsma Dec 1, 2021 • edited Loading

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

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

tonistiigi commented Jan 16, 2022

sipsma commented Dec 1, 2021 •

edited

Loading

sipsma Dec 1, 2021 •

edited

Loading