add pkg/blobcache from Buildah

[vrothberg]

Buildah's blobcache is a cache to speed up builds by avoiding CPU
intensive (de)compressing of blobs.

Move the code from Buildah into containers/image to allow for extending
internal interfaces (e.g., image destination/source) and supporting
newly added features in the blobcache.

There are a number of planned features (e.g., sigstore/cosign) which
will likely be implemented entirely by extending the internal interfaces
and by casting to internal types in containers/image/copy as already
done. Having Buildah's blobcache here allows for extending its internal
interface more easily.

Signed-off-by: Valentin Rothberg vrothberg@redhat.com

@mtrmac @nalind PTAL
Once merged, I can prepare a follow-up PR in Buildah and do the aliasing.

[vrothberg]

Added two commits on top to remove the dependencies on buidah and libimage.

[nalind]

I'd been looking at this at the end of last week, and there's more that needs to be done:

• the cache only supports blobs that have been compressed with gzip
• supporting other types of compression opens up the possibility that a registry will deny a push that tries to reuse a base layer, originally pulled from a different registry, that was compressed with an algorithm (and corresponding media type referencing the layer in a manifest) that it doesn't recognize
• this means the cache needs to expose information that the cache user can use to determine if the cache may have caused a copy attempt to fail, and that it might succeed if retried without it
• the CacheLookupReferenceFunc function is a bad fit for this, since we need to keep a reference to the cache object in order to be able to query it for that information after a copy, and it doesn't allow for that, so its callers will be better off using anonymous functions of their own instead. Drop the function, and you can drop the new exported type that it demands.

[vrothberg]

Thanks, @nalind! I am mostly interested in moving the code here so we can work on cosign without the blobcache removing signatures.

The points you mention above seem outside of this scope and independent from where the code lives.

[mtrmac]

ACK to moving the code, and not touching the implementation as part of this PR, unless necessary.

I worry, at this point, about the API, because we need to keep that stable, though. Primarily:

• Should we have a “longer-term” BlobCache object that holds the directory/configuration, and only allow wrapping references through that?
• Is a single ”remap any reference to a reference with a cache” the right API, or should we have separate ones for ({c/storage, the registry}, {cache writer, cache consumer})? Or a single “remap (reference, shouldConsumeCache, shouldUpdateCache)”) API?

[mtrmac]

I’d prefer a pair of CacheWriterImageReference(ref …) and CacheConsumerImageReference(ref …) methods (or some other naming for the “local c/storage” vs. “remote registry” split?).

The caller usually knows which one of the two (whatever “the two” means) is intended, and having that cleanly separated would allow implementing the two differently.

(The naming is not just a “source”/“destination”, because AFAICS an ImageSource is typically the “cache writer”, so a “source” name is confusing.)

Once we add a single method like this, we won’t be able to split the two (without continuing to support the single-method approach.) OTOH if we understand how the two conceptually differ, the two public functions can, for now, just call the single implementation internally.

---

Or is there some structural reason why a single reference is always both a cache writer and consumer, and the code should behave the same on both wrapped references participating? It does make sense that “if we have the blob in cache, reading it from there is cheaper than both reading from a remote registry and constructing a tarball from c/storage”. OTOH, if we wrap a registry reference, I don’t think we actually want to ever write a blob that we are pushing to the registry also to disk, do we? (Or is that inherently caller-situation specific and do we eventually need an API for the caller to choose?)

It’s fairly likely I’m thinking about this wrong.

[mtrmac]

@nalind PTAL

[nalind]

The cache is populated in PutBlob() (ImageDestination) and consulted in GetBlob() (ImageSource). It could conceivably also have populated the cache on GetBlob(), but it didn't occur to me to do that at the time.
An ImageReference can be asked to create both ImageSources and ImageDestinations. I didn't see much benefit in having two functions which took an ImageReference argument and returned a different ImageReference when the implementation logic didn't require them to be different. If an ImageSource was also writing to the cache, I might have thought differently.

[mtrmac]

tl;dr — skip to the bottom

---

Personally I just found it hard to think about the properties… let me see if I get this right:

On (new, pull):

Activated through libimageOptions.DestinationLookupReferenceFunc, i.e. a c/storage storageDest is cache-wrapped, the registry dockerSrc isn’t. The compress argument to NewBlobCache is ignored because the cache isn’t used on a source (and anyway, it is set to PreserveOriginal which means “don’t change LayerInfosForCopy”).

• The original manifest + dockerSrc.LayerInfosForCopy is used without modification
• storageDest.TryReusingBlob, if the layer isn’t in c/storage already, checks for a cached blob; if so, it writes it to storageDest, and we don’t contact the registry. (This can AFAICS happen only if the cache is longer-lived than the c/storage backend.)
• dockerSrc.GetBlob just contacts the registry
• storageDest.PutBlob always stores the original layer blob to the cache, and if it is gzip-compressed, also stores an uncompressed variant to the cache.

I.e. on pull, we almost always record the compressed versions in cache (on the first pull of that layer), and usually the uncompressed version.

On push:

Activated through libimageOptions.SourceLookupReferenceFunc, i.e. a c/storage storageSrc is cache-wrapped, the registry dockerDest isn’t. compress defaults to types.Compress unless the push invoker disables compression.

• storageSrc.LayerInfosForCopy starts with the storageSrc version, but can replace digests of uncompressed storage layers with digests of cached compressed versions.
• dockerDest.TryReusingBlob is not modified, but it can check the registry first for the compressed version per the update above, before falling back to BlobInfoCache. [The difference is that TryReusingBlob always queries the registry for the primary digest, but BlobInfoCache-known associations are, currently, only used if we saw that blob in the same registry. So using a compressed-digest version for the physical query has a higher chance of avoiding an upload.]
• storageSrc.GetBlob first checks the cache, which helps both for a compressed artifact (no need to re-compress) and an uncompressed one (no need to traverse the graph driver’s fragmented storage and re-assemble tar-split; the uncompressed data is then compressed by the generic code).
• dockerDest.PutBlob just contacts the registry

I.e. on push, for any pre-existing layers we first check for an on-registry version, and if it isn’t there, we push them from the cache, not reading from c/storage.Store.Diff. Only new layers are compressed+written — and we don’t update the cache with any newly created layers (uncompressed nor compressed).

On commit:

(commit source = buildah.containerImageSource; reads from a prepared blob or from c/storage. commit destination = usually a c/storage destination, but not quite always.)

Activated manually; both the containerSrc source and commitDest destination is cache-wrapped. compress defaults to types.PreserveOriginal unless the commit invoker enables compression.

• containerSrc.LayerInfosForCopy starts with the containerImageSource version (= the newly-built manifest contents); it can replace digests of uncompressed storage layers with digests of cached compressed versions, but it doesn’t do so by default.
• commitDest.TryReusingBlob, if the layer isn’t in the destination already, checks for a cached blob; if so, it writes it to commitDest, and we don’t use the containerSrc version. (This can AFAICS basically never happen for a c/storage commit destination, where the layer either exists in the same c/storage, or it is entirely new; we would read the cache only for other destination types.)
• containerSrc.GetBlob first checks the cache, which does nothing for a c/storage destination — either the layer already exists there and we don’t get to GetBlob, or it is genuinely new and not in the cache. For other commit destinations, this potentially helps both for a compressed artifact (no need to re-compress) and an uncompressed one (no need to traverse the graph driver’s fragmented storage and re-assemble tar-split; the generic code can compress).
• commitDest.PutBlob, if the blob is not being compressed on the fly, stores the original layer blob to the cache (it is not gzip-compressed as coming from from containerSrc). If it is being compressed on the fly, it is not stored in the cache.

I.e. for a c/storage commit destination, this has the net effect of storing the uncompressed version of any new layers to the cache (and that in turn means that a future push won’t need a Store.Diff to read that). For non-c/storage commit destinations, this uses the cache similarly to a push, using it to optimize writes of previously-pulled layers (but not any commit-created layers).

So, in principle,

• on a commit to c/storage, we would only need to cache-wrap the destination (which would be usefully restricted, “BlobCache is only ever used to wrap c/image/storage”)
• direct commits to non-c/storage behave more like pushes (where we want to wrap the source); in rare cases (direct commits without compression) we potentially benefit from caching the commit outcome, which happens in the PutBlob destination, but mostly we only need to cache-wrap the source.

---

So the pull/push cases are straightforward; I was originally primarily focusing on the commit case (the only direct use of NewCache), which is non-obvious, tends to do nothing for most layers, but what it does do (avoid a second .Diff for pushing newly created layers) might actually be quite relevant for performance.

---

Conceptually I quite like the idea that “the cache is an implementation optimization of a c/storage user (both c/storage/image and buildah.containerImageSrc)”, where both PutBlob and GetBlob can update the cache. Going even further …

Hypothetically, in some far-future idealized world, the cache could be integrated into c/image/storageImageDestination so that its PutBlob doesn’t need to do any private copies just to compute digests, and into buildah.containerImageSrc so that it can write the uncompressed blobs for the new layers directly into the cache instead of making a private copy first. That might be a performance improvement for low-memory / IO-starved systems. And integration of that kind would look so completely different from the current ImageReference wrapper (direct cache support in the two transports) that it’s really not worth worrying about — we would probably provide a “cache” object with some kind of “get blob” / “record blob” API. So that’s clearly out of scope and not worth worrying about short-term, apart from maybe defining that “cache” object now.

So, yes, I was thinking about this wrong — splitting the uses into “producer”/“consumer”, as I was suggesting originally, is not really the right approach. The cache would be inherently better if it were directly integrated, which is a completely different API.

---

So, on balance, let’s keep the one wrapping function here for now, and just document that

• it updates the cache on PutBlob and PutManifest (destinations), reads from it on GetBlob , GetManifest (sources) and TryReusingBlob (destinations) — that is enough for callers to get the effects they need,
• it might use the cache in more ways in the future — that gives us flexibility.

Separately, I think it still makes sense to introduce a BlobCache type, with a WrapImageReference method replacing this NewBlobCache function, but that’s a weak preference and not blocking I guess.

[mtrmac]

Should this have an Options struct, or “a functional options” interface?

I suppose we can always add a new API of that kind later…

[vrothberg]

Agreed

[mtrmac]

• supporting other types of compression opens up the possibility that a registry will deny a push that tries to reuse a base layer, originally pulled from a different registry, that was compressed with an algorithm (and corresponding media type referencing the layer in a manifest) that it doesn't recognize

Isn’t that generic issue with the existing TryReusingBlob API? The transport doesn’t know which formats would be acceptable to the manifest, and if a blob is accepted by the destination, c/image/copy would never try writing the layer in a different format.

• this means the cache needs to expose information that the cache user can use to determine if the cache may have caused a copy attempt to fail, and that it might succeed if retried without it

Is it just “retried without it”? Ideally we try to use a cached zstd, and if that fails, we should still try to use cached gzip, not just turn off the cache entirely. That seems to me to, ideally, involve some kind of extra data to TryReusingBlob on the second attempt, whether using a cache or not, not a specific “might a cache have done this?” heuristic. Such a heuristic could work for many cases well enough, OTOH I’m not sure we need a “ask the cache if it could be the cause” API, we could just blindly try without the cache again, anyway — that would be a worse heuristic, but assuming TryReusingBlob on things we have already pushed is trivially cheap, it might be cheap enoug.

• the CacheLookupReferenceFunc function is a bad fit for this, since we need to keep a reference to the cache object in order to be able to query it for that information after a copy, and it doesn't allow for that, so its callers will be better off using anonymous functions of their own instead. Drop the function, and you can drop the new exported type that it demands.

Do you propose a “cache” object that can be used to remap references, and survives any one remapped reference? That does seem to be a more accurate mapping of the underlying concepts. OTOH the “determine if the cache may have caused a copy attempt to fail” question feels actually scoped to a specific cache-wrapped reference — if a “single cache” is involved in 10 copies, and the first one involves such a risky remapping, a per-cache bit would be set for all 10 following copies (… in abstract principle. In practice we would have a single-copy.Image cache object, at least right now.)

[nalind]

• supporting other types of compression opens up the possibility that a registry will deny a push that tries to reuse a base layer, originally pulled from a different registry, that was compressed with an algorithm (and corresponding media type referencing the layer in a manifest) that it doesn't recognize

Isn’t that generic issue with the existing TryReusingBlob API? The transport doesn’t know which formats would be acceptable to the manifest, and if a blob is accepted by the destination, c/image/copy would never try writing the layer in a different format.

Yes.

• this means the cache needs to expose information that the cache user can use to determine if the cache may have caused a copy attempt to fail, and that it might succeed if retried without it

Is it just “retried without it”? Ideally we try to use a cached zstd, and if that fails, we should still try to use cached gzip, not just turn off the cache entirely. That seems to me to, ideally, involve some kind of extra data to TryReusingBlob on the second attempt, whether using a cache or not, not a specific “might a cache have done this?” heuristic. Such a heuristic could work for many cases well enough, OTOH I’m not sure we need a “ask the cache if it could be the cause” API, we could just blindly try without the cache again, anyway — that would be a worse heuristic, but assuming TryReusingBlob on things we have already pushed is trivially cheap, it might be cheap enough.

That's a good point.

• the CacheLookupReferenceFunc function is a bad fit for this, since we need to keep a reference to the cache object in order to be able to query it for that information after a copy, and it doesn't allow for that, so its callers will be better off using anonymous functions of their own instead. Drop the function, and you can drop the new exported type that it demands.

Do you propose a “cache” object that can be used to remap references, and survives any one remapped reference? That does seem to be a more accurate mapping of the underlying concepts. OTOH the “determine if the cache may have caused a copy attempt to fail” question feels actually scoped to a specific cache-wrapped reference — if a “single cache” is involved in 10 copies, and the first one involves such a risky remapping, a per-cache bit would be set for all 10 following copies (… in abstract principle. In practice we would have a single-copy.Image cache object, at least right now.)

For writing, yes, I suppose scope could be used to infer if a given destination supported a given compression algorithm. It seems we need a more expressive way to tell it what types of substitutions it can/should make.

[vrothberg]

I am not sure whether this PR can be merged as is or not. Most (all?) seem like follow-up items?

[mtrmac]

Primarily I worry about committing to a an API we couldn’t evolve.

• So, at minimum, this shouldn’t return an interface, or we can’t reasonably extend it.
• If we ever add a “cache” type separate from a reference wrapper, now would be a good time (it’s not absolutely unavoidable, we could add that later, but then we would want to provide a new reference wrapper API that returns a reference without the ClearCache etc. methods)
• Replacing the compress parameter to NewBlobCache is also much easier now.
• Do we want to add an options struct / functional options mechanism now, so that we don’t need to add an …WithOptions later? I don’t feel strongly at all about this one.
• (Just to be explicit, I no longer think we should split this into a source/destination/… wrapper API.)

Most of the review comments are actually aimed at the documentation, and the ~newly introduced reference mapping API. At least to the extent it is entirely or almost entirely new, and not a long-standing buildah/pkg/… documentation, I’d prefer for it to be just cleaned up now rather than committing a new to-do item — the effort of tracking that for later is relatively large vs. just fixing it now.

[vrothberg]

Addressed some of the issues, most notably turned BlobCache into a struct, see individual commits.

[mtrmac]

Thanks! LGTM.