Custom inline cache implementation #60

mikejholly · 2024-05-14T18:46:43Z

This PR includes a new & simplified inline cache implementation that's compatible with the simple solver.

See comments in PR.

mikejholly · 2024-05-16T18:22:17Z

cache/remotecache/import.go

-	Cache   []byte `json:"moby.buildkit.cache.v0"`
-	History []struct {
+	Cache              []byte `json:"moby.buildkit.cache.v0"`
+	EarthlyInlineCache []byte `json:"earthly.inlinecache.v0"`


This field is added to an image's "config" and can be viewed using something like crane config. The map of cache keys to registry layer digests is stored here.

mikejholly · 2024-05-16T18:23:02Z

executor/runcexecutor/monitor_stats.go

@@ -53,7 +53,7 @@ func (w *runcExecutor) monitorContainerStats(ctx context.Context, id string, sam
 	for {
 		select {
 		case <-ctx.Done():
-			bklog.G(ctx).Infof("stats collection context done: %v", ctx.Err())
+			bklog.G(ctx).Debugf("stats collection context done: %v", ctx.Err())


This helps with log noise.

mikejholly · 2024-05-16T18:24:18Z

exporter/containerimage/writer.go

+		if err != nil {
+			return nil, err
+		}
+		m["earthly.inlinecache.v0"] = dt


This is where the cache key to layer digest data is added.

mikejholly · 2024-05-16T18:25:54Z

solver/llbsolver/bridge.go

+		}
+
+		err := inBuilderContext(ctx, b.builder, "earthly inline cache", "", func(ctx context.Context, g session.Group) error {
+			inlineCacheRemotes, err = registry.EarthlyInlineCacheRemotes(ctx, b.sm, w, b.registryHosts, g, cacheImport.Attrs)


This is where remote manifests (including cache relation info) are fetched for the inline cache source.

mikejholly · 2024-05-16T18:26:44Z

solver/llbsolver/solver.go

+		return nil, err
+	}
+
+	sources := worker.NewCombinedResultSource(


Here's a new abstraction that allows the simple solver to load from a set of result sources. The inline cache remote source is specified here.

mikejholly · 2024-05-16T18:27:37Z

solver/llbsolver/solver.go

+		// }
+
+		if hasInlineCacheExporter(exp.CacheExporters) {
+			meta, err := earthlyInlineCache(ctx, j, e, cached)


Here, we determine if inline caching is enabled. If so, the cache relationship map is added to the image metadata before the image is passed to the Earthly exporter.

mikejholly · 2024-05-16T18:30:35Z

solver/simple.go

+
+		// Hijack the CacheKey type in order to export a reference from the new cache key to the ref ID.
+		expKeys = append(expKeys, ExportableCacheKey{
+			CacheKey: &CacheKey{


In the BuildKit remote caching approach, a complex chain of cache keys are added to ExportableCacheKey. Here, I'm able to stick with the return type (needed by the caller & caller parent) while using a simplified one-to-one cache key to worker ref ID relation.

mikejholly · 2024-05-16T18:32:03Z

solver/simple.go

 	mu        sync.Mutex
 }

 type cacheMapDep struct {
-	selector string
-	computed string
+	selector digest.Digest


I switched to digest.Digest in a few places because using strings were leading to annoying conversions. They're just strings with some helper methods.

mikejholly · 2024-05-16T18:39:24Z

solver/simple.go

-	return &inMemCache{cache: map[string]Result{}}
+// RefIDStore uses a BoltDB database to store links from computed cache keys to
+// worker ref IDs.
+type RefIDStore struct {


I split the ref ID storage out from the result loader so that the former can be used in the solver & worker/simple.go.

cache/manager.go

alexcb · 2024-05-16T21:10:17Z

cache/remotecache/inline.go

+	}
+
+	layerDone := progress.OneOff(ctx, fmt.Sprintf("inferred cache manifest type: %s", manifestType))
+	layerDone(nil)


I'm guessing this is intentional since I see it done in cache/remotecache/azblob/importer.go, but as someone who's new to the OneOff, this seems confusing. but since it's a pattern that's already used in buildkit maybe it's fine.

Much of this code was taken from the preexisting inline cache code: https://github.com/earthly/buildkit/blob/earthly-main/cache/remotecache/import.go#L58

cache/remotecache/inline.go

cache/remotecache/v1/cachestorage.go

cmd/buildkitd/main.go

solver/llbsolver/bridge.go

alexcb · 2024-05-16T21:22:46Z

solver/llbsolver/bridge.go

 		if err != nil {
-			return nil, err
+			bklog.G(ctx).Warnf("failed to import inline cache: %v", err)


did you intentionally mean to drop the return here?

Yes, it's probably not necessary to fail the build on an error. But now that you mention it, users may want to know. What do you think?

Updated to return the error and fail the build. It's likely useful to know when a network error has occurred.

alexcb · 2024-05-16T21:23:29Z

solver/llbsolver/bridge.go

+		}
+	}
+
+	// MH: Old-school remote & inline cache imports disabled.


should we just delete this instead?

solver/llbsolver/inline.go

solver/llbsolver/solver.go

alexcb

looking good! just a few minor questions.

alexcb

looks good; but please squash your changes before merging.

Once squashed, you can also do a git merge --ff-only via the cli to get an even cleaner git history.

mikejholly force-pushed the mh/next-inline-cache-redux branch 3 times, most recently from a917e12 to 4afd23a Compare May 16, 2024 17:59

mikejholly commented May 16, 2024

View reviewed changes

mikejholly force-pushed the mh/next-inline-cache-redux branch from 4afd23a to 9ea0325 Compare May 16, 2024 18:36

mikejholly marked this pull request as ready for review May 16, 2024 18:37

mikejholly mentioned this pull request May 16, 2024

Update BK for new inline cache (won't merge) earthly/earthly#4112

Closed

mikejholly commented May 16, 2024

View reviewed changes

mikejholly force-pushed the mh/next-inline-cache-redux branch 4 times, most recently from 750aa06 to bab4556 Compare May 16, 2024 20:21