The blobcache improvments

[Honny1]

I made some improvements based on the review by @mtrmac.

[Honny1]

PTAL @mtrmac @nalind

[giuseppe]

LGTM

[mtrmac]

Thanks for the follow-up!

I’m not sure about the wantFirstAvailableAlgo code. Will it be used?

• It is non-deterministic (iterating over the noted algorithms in a Go-determined order)
• What will Buildah use? Due to containers/buildah#6660 , I think Buildah would not want to randomly use Zstd.
• If we ever build the complex “any pre-existing layer” (with dependence on the destination manifest and users’ copy.Options) discussed in #731 (review) , I think a natural way to obtain that would be to call NewBlobCache without WithCompressAlgorithm. If we now define that call to be “first available without more conditions”, we would later need to add a WithSmartCompressSelection() to opt into the better behavior. That would not be much of a problem, but if we can’t see any current user of wantFirstAvailableAlgo, maybe it would be better to drop that (and continue conservatively using Gzip as in the previous version).

---

(Non-blocking: The commit split is not correct; the branch does not compile at one of the commits.)

[mtrmac]

(In #731 (comment) I meant primarily

Suggested change

	logrus.Debugf("suggesting cached blob with digest %q, type %q (decompress) in place of blob with digest %q", replaceDigest.String(), info.MediaType, info.Digest.String())
	info.CompressionOperation = s.reference.compress
	logrus.Debugf("suggesting cached blob with digest %q, type %q (decompress) in place of blob with digest %q", replaceDigest.String(), info.MediaType, info.Digest.String())
	info.CompressionOperation = types.Decompress

but you’re right that the debug log does not need to include the %q either.)

[Honny1]

Thanks for the follow-up!

I’m not sure about the wantFirstAvailableAlgo code. Will it be used?

• It is non-deterministic (iterating over the noted algorithms in a Go-determined order)
• What will Buildah use? Due to containers/buildah#6660 , I think Buildah would not want to randomly use Zstd.
• If we ever build the complex “any pre-existing layer” (with dependence on the destination manifest and users’ copy.Options) discussed in #731 (review) , I think a natural way to obtain that would be to call NewBlobCache without WithCompressAlgorithm. If we now define that call to be “first available without more conditions”, we would later need to add a WithSmartCompressSelection() to opt into the better behavior. That would not be much of a problem, but if we can’t see any current user of wantFirstAvailableAlgo, maybe it would be better to drop that (and continue conservatively using Gzip as in the previous version).

You're right, I have an idea for a simple solution: let's change the requested algorithm parameter to an array. This would allow callers to specify multiple acceptable compressions, and we would return the first match. Since they already know the destination, they can make the choice from there. They can use manifest.CandidateCompressionMatchesReuseConditions along with dest.SupportedManifestMIMETypes() to filter the candidate algorithms. WDYT?

[mtrmac]

Structurally, the BlobCache use is:

srcRef := NewBlobCache(srcref)
copy.Image(…, srcRef, copy.Options{
  ForceManifestMIMEType: …,
  EnsureCompressionVariantsExist: …,
  ForceCompressionFormat: …,
  DestinationCtx: &types.SystemContext{CompressionFormat: …},
})
// hypothetically repeated copies from the same srcRef

i.e. the options governing the copy are not yet available in NewBlobCache, and some of them are not constant (EnsureCompressionVariantsExist might cause one gzip and one zstd variant to be created during a single copy.Image).

I.e. the caller would need to duplicate a bunch of that logic (now that isn’t exposed, CandidateCompressionMatchesReuseConditions is internal; suppose we expose that, the caller would still need to be dealing with that when it’s not something the caller distinctly cares about), and the non-constant cases couldn’t be accommodated at all.

If you want all of this resolved now and perfectly, passing the relevant data to a new LayerInfosForCopyWithOptions should not be too difficult, it’s just some mechanical work. (It would be nice to have, it would simplify the Buildah caller, and perhaps be a better semantic fit than the strict WithCompressAlgorithm matching. We might even get to drop that option before making a release with that API.)

I think I’d prefer dropping that part from this PR and fixing primarily the DetectCompressionFormat call (and whatever else is already done) now, so that that part isn’t lost. The LayerInfosForCopyWithOptions part would be large enough that having that in a separate PR (even if it were to follow immediately) would probably simplify the discussion.

[Honny1]

Thanks for the explanation. I've addressed all the nits, and I think it's ready to merge.

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[Honny1]

PTAL @mtrmac @nalind @containers/podman-maintainers

[mtrmac]

Thanks!