Import/export support for OCI compatible image manifest version of cache manifest (opt-in on export, inferred on import) #3724

kattmang · 2023-03-17T05:45:03Z

Import/export support for OCI compatible image manifest version of cache manifest (opt-in on export, inferred on import)

This feature addition is an extension to the remote and local cache feature. This generally follows @joadrp's Proposal in #2251 while also taking in Tonis' comment on thread regarding an opt-in key for exporting with this new cache manifest format. #2251 (comment)

To summarize, this commit adds:

README change detailing the new export-cache key for both local and registry types
added logic to plumb through image-manifest bool key with default False (opt-in) to the contentCacheExporter.
added export contentCacheImporter logic to support both the new Image Manifest approach as well as the existing Image Index approach based on above key
added import cache manifest inference logic to support and detect both the old and new cache manifest format (also drops and errors on when it can't be inferred).

Tested to both local and registry, and tested for regression to the old manifest type, using ECR as the registry destination (At ECR we do NOT support the Image Index format as it isn't strict to the OCI spec for valid mediaTypes in the manifest list).

Open to additional testing suggestions.

kattmang · 2023-03-17T05:56:16Z

Also, looking for maintainer feedback on how we can get this change into the next upcoming release branch for Buildkit/Docker CLI support.

kattmang · 2023-03-17T06:40:17Z

Converted to draft as I see some integ tests failures on tests I wasn't able to test locally due to work network limitations.

EDIT: error appears to be for all/most to be related to cache manifest push tests (on the old image index format):

x.go:281: time="2023-03-17T05:41:55Z" level=debug msg="fetch response received" response.header.content-length=82 response.header.content-type="application/json; charset=utf-8" response.header.date="Fri, 17 Mar 2023 05:41:55 GMT" response.header.docker-distribution-api-version=registry/2.0 response.status="400 Bad Request" spanID=42abbcfb23d7bac7 traceID=193289b2c6357960aab7bacaa62a7bb2 url="http://localhost:46109/v2/buildkit/buildkit/testdiffself-cache/manifests/latest"
2023-03-17T05:43:20.8581033Z     sandbox.go:281: time="2023-03-17T05:41:55Z" level=debug msg="unexpected response" body="{\"errors\":[{\"code\":\"MANIFEST_INVALID\",\"message\":\"manifest invalid\",\"detail\":{}}]}\n" resp="&{400 Bad Request 400 HTTP/1.1 1 1 map[Content-Length:[82] Content-Type:[application/json; charset=utf-8] Date:[Fri, 17 Mar 2023 05:41:55 GMT] Docker-Distribution-Api-Version:[registry/2.0]] {0xc00021cdc0} 82 [] false false map[] 0xc00021f700 <nil>}" spanID=42abbcfb23d7bac7 traceID=193289b2c6357960aab7bacaa62a7bb2
2023-03-17T05:43:20.8582004Z     sandbox.go:281: time="2023-03-17T05:41:55Z" level=error msg="/moby.buildkit.v1.Control/Solve returned error: rpc error: code = Unknown desc = error writing manifest blob: failed commit on ref \"sha256:cb8f036590ad3c123d37457ab73199281279b099405d7ce2f33a93bf6a3600ed\": unexpected status from PUT request to http://localhost:46109/v2/buildkit/buildkit/testdiffself-cache/manifests/latest: 400 Bad Request"

Image Index cache manifest works fine on my box to my local cache with the PR commit

docker run \
    -it \
    --rm \
    --privileged \
    -v /home/kangmatt/buildctl-test:/tmp \
    -v $HOME/.docker:/root/.docker \
    -v /home/kangmatt/.aws:/root/.aws \
    --entrypoint buildctl-daemonless.sh \
    ecr-buildkit:local \
    --debug build \
    --frontend=dockerfile.v0 \
    --local context=/tmp \
    --local dockerfile=/tmp \
    --output type=image,name=010500926215.dkr.ecr.us-west-2.amazonaws.com/buildkit-cache:image,push=true \
    --import-cache type=local,src=/tmp/bkcache3 \
      --export-cache type=local,dest=/tmp/bkcache3
DEBU[2023-03-17T07:38:33Z] serving grpc connection                       spanID=cbf32771e6c1da9f traceID=e0acf1b700a4dab25577a75c9e97c368
[+] Building 2.7s (10/11)
 => => transferring dockerfile: 102B                                                                                                                 0.0s
 => [internal] load metadata for docker.io/library/alpine:3.4                                                                                        1.4s
 => [internal] load .dockerignore                                                                                                                    0.0s
 => => transferring context: 2B                                                                                                                      0.0s
 => importing cache manifest from local:16548731276217466773                                                                                         0.0s
 => => inferred cache manifest type: Manifest List                                                                                                   0.0s
 => [1/4] FROM docker.io/library/alpine:3.4@sha256:b733d4a32c4da6a00a84df2ca32791bb03df95400243648d8c539e7b4cce329c                                  0.0s
 => => resolve docker.io/library/alpine:3.4@sha256:b733d4a32c4da6a00a84df2ca32791bb03df95400243648d8c539e7b4cce329c                                  0.0s
 => CACHED [2/4] RUN apk update                                                                                                                      0.0s
 => CACHED [3/4] RUN apk add vim                                                                                                                     0.0s
 => CACHED [4/4] RUN apk add curl                                                                                                                    1.0s
 => => sha256:c1e54eec4b5786500c19795d1fc604aa7302aee307edfe0554a5c07108b77d48 2.39MB / 2.39MB                                                       0.0s
 => => extracting sha256:c1e54eec4b5786500c19795d1fc604aa7302aee307edfe0554a5c07108b77d48                                                            0.1s
 => => sha256:ce0647ccd38e3c1f8dcd3726a16b73d221486587ecb489433dd78991bd913662 769.08kB / 769.08kB                                                   0.0s
 => => extracting sha256:ce0647ccd38e3c1f8dcd3726a16b73d221486587ecb489433dd78991bd913662                                                            0.0s
 => => sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138 8.48MB / 8.48MB                                                       0.1s
 => => extracting sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138                                                            0.5s
 => => sha256:9493ae436659cad0a53e3cd7d047c9620770455786d80b3f00eb671d47fe213e 847.77kB / 847.77kB                                                   0.0s
 => => extracting sha256:9493ae436659cad0a53e3cd7d047c9620770455786d80b3f00eb671d47fe213e                                                            0.1s
 => exporting to image                                                                                                                               0.2s
 => => exporting layers                                                                                                                              0.0s
 => => exporting manifest sha256:2a0a27c9a62b5d3df27ff008886be597b816c987551c24a13010f2a516b39079                                                    0.0s
 => => exporting config sha256:c1c3409a7c2d8449a12ad3bfd30fc70cc03cc2c308428be865305817ce0b9f6a                                                      0.0s
 => => pushing layers                                                                                                                                0.1s
 => => pushing manifest for 010500926215.dkr.ecr.us-west-2.amazonaws.com/buildkit-cache:image@sha256:2a0a27c9a62b5d3df27ff008886be597b816c987551c24  0.1s
 => [auth] sharing credentials for 010500926215.dkr.ecr.us-west-2.amazonaws.com                                                                      0.0s
 => exporting cache to client directory                                                                                                              0.0s
 => => preparing build cache for export                                                                                                              0.0s
DEBU[2023-03-17T07:38:36Z] (*service).Write started                      expected="sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138[+] Building 2.7s (11/11) FINISHED                                                                                                                         => [internal] load build definition from Dockerfile                                                                                                 0.0s
 => => transferring dockerfile: 102B                                                                                                                 0.0se => [internal] load metadata for docker.io/library/alpine:3.4                                                                                        1.4s5 => [internal] load .dockerignore                                                                                                                    0.0s
 => => transferring context: 2B                                                                                                                      0.0s8 => importing cache manifest from local:16548731276217466773                                                                                         0.0s7 => => inferred cache manifest type: Manifest List                                                                                                   0.0s
 => [1/4] FROM docker.io/library/alpine:3.4@sha256:b733d4a32c4da6a00a84df2ca32791bb03df95400243648d8c539e7b4cce329c                                  0.0s2 => => resolve docker.io/library/alpine:3.4@sha256:b733d4a32c4da6a00a84df2ca32791bb03df95400243648d8c539e7b4cce329c                                  0.0s5 => CACHED [2/4] RUN apk update                                                                                                                      0.0s
 => CACHED [3/4] RUN apk add vim                                                                                                                     0.0s4 => CACHED [4/4] RUN apk add curl                                                                                                                    1.0s6 => => sha256:c1e54eec4b5786500c19795d1fc604aa7302aee307edfe0554a5c07108b77d48 2.39MB / 2.39MB                                                       0.0s
 => => extracting sha256:c1e54eec4b5786500c19795d1fc604aa7302aee307edfe0554a5c07108b77d48                                                            0.1s9 => => sha256:ce0647ccd38e3c1f8dcd3726a16b73d221486587ecb489433dd78991bd913662 769.08kB / 769.08kB                                                   0.0s6 => => extracting sha256:ce0647ccd38e3c1f8dcd3726a16b73d221486587ecb489433dd78991bd913662                                                            0.0s
 => => sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138 8.48MB / 8.48MB                                                       0.1s
 => => extracting sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138                                                            0.5sI => => sha256:9493ae436659cad0a53e3cd7d047c9620770455786d80b3f00eb671d47fe213e 847.77kB / 847.77kB                                                   0.0s
 => => extracting sha256:9493ae436659cad0a53e3cd7d047c9620770455786d80b3f00eb671d47fe213e                                                            0.1se => exporting to image                                                                                                                               0.2s
 => => exporting layers                                                                                                                              0.0si => => exporting manifest sha256:2a0a27c9a62b5d3df27ff008886be597b816c987551c24a13010f2a516b39079                                                    0.0sh => => exporting config sha256:c1c3409a7c2d8449a12ad3bfd30fc70cc03cc2c308428be865305817ce0b9f6a                                                      0.0s
 => => pushing layers                                                                                                                                0.1sD => => pushing manifest for 010500926215.dkr.ecr.us-west-2.amazonaws.com/buildkit-cache:image@sha256:2a0a27c9a62b5d3df27ff008886be597b816c987551c24  0.1s
 => [auth] sharing credentials for 010500926215.dkr.ecr.us-west-2.amazonaws.com                                                                      0.0s7 => exporting cache to client directory                                                                                                              0.0s
 => => preparing build cache for export                                                                                                              0.0s
 => => writing layer sha256:3c3bb6b86aebf4589683a24f265145b10dabc83f495a06e48befa488ba79d138                                                         0.0s
 => => writing layer sha256:9493ae436659cad0a53e3cd7d047c9620770455786d80b3f00eb671d47fe213e                                                         0.0s
 => => writing layer sha256:c1e54eec4b5786500c19795d1fc604aa7302aee307edfe0554a5c07108b77d48                                                         0.0s
 => => writing layer sha256:ce0647ccd38e3c1f8dcd3726a16b73d221486587ecb489433dd78991bd913662                                                         0.0s
 => => writing config sha256:2134fc6fc8b4ff0f425570b9d8b3f0eba3b6611482a7dd419a0b90cee98c96a4                                                        0.0s
 => => writing cache manifest sha256:758738f665e3b7108b4ce18b4e885b00714eef6256e6ba23381a0e30c10bcc19                                                0.0s

kattmang · 2023-03-17T09:37:59Z

Fixed in amended commit, existing integ tests are passing now. Humbly looking for guidance on where to add integ tests for the new ImageManifest type :)

kattmang · 2023-03-17T22:28:26Z

Linter errors fixed as well.

tonistiigi

Look at the current integration tests, eg. testBasicCacheImportExport, testCacheImportExport and add variants where there is opt-in to this version.

tonistiigi · 2023-03-20T03:56:20Z

cache/remotecache/export.go

@@ -75,20 +76,23 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 	}

 	// own type because oci type can't be pushed and docker type doesn't have annotations
-	type manifestList struct {
+	type abstractManifest struct {


Use different types for this and don't try to combine them together. I believe missing MediaType issue was fixed in latest oci spec so probably custom types are not needed anymore at all.

Yeah, I think ocispecs.Manifest and ocispecs.Index would work respectively. I think if that's the case though, for the rest of the export code I would either need to:

create an wrapper struct around Manifest and Index (seems to still have the same concern you have here) and implement functions that map the generated cache descriptors to the appropriate spec/oci object aka WrapperObj.addCacheDescriptor()

OR

Have two object references for Manifest and Index and interchange which is used when the same "flat" way the code is structured now (bifurcation/ifs where the content descriptors are actually being set)

Is the concern more around the possibility of generating a nonconforming OCI Index/Manifest that has a superset of fields? If so, going with Approach 2 would create a new problem of writing to the wrong Object altogether and at the end of Finalize encode an empty object to string. I kind of prefer Approach 1 as the wrapping struct could have a setCacheFormat(), which would remove all of the if ce.imageManifest references in Finalize. I only worry that with Approach 1 we may be abstracting in the wrong direction (should Finalize become be a switchboard to what generated descriptors go where or should it only concern itself about the generation of the descriptors?)

I'm going to go with Approach 1 as I can formalize it in my head better but open to alternatives.

I think we probably would want to do option 3: use the ocispecs.Index struct, and simply set the mediaType depending on whether oci-mediatypes has been appropriately configured: see

buildkit/exporter/containerimage/writer.go

Lines 170 to 180 in 3187d2d

idx := ocispecs.Index{

MediaType: ocispecs.MediaTypeImageIndex,

Annotations: opts.Annotations.Platform(nil).Index,

Versioned: specs.Versioned{

SchemaVersion: 2,

},

}

if !opts.OCITypes {

idx.MediaType = images.MediaTypeDockerSchema2ManifestList

}

for how we do it for the containerimage exporter.

@jedevc I'm not sure how Index can be adapted to support both Index and Manifest (Manifest is the format that being introduced into both import.go and export.go here). Maybe I'm missing something here.

EDIT: point taken on oci-mediatypes

See new commit: I went ahead with option one. I also added checks on oci-mediatype compatibility with image manifest: #3724 (comment)

tonistiigi · 2023-03-20T03:57:36Z

cache/remotecache/export.go

@@ -122,7 +132,11 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 	}
 	configDone(nil)

-	mfst.Manifests = append(mfst.Manifests, desc)
+	if ce.imageManifest {
+		mfst.Config = &desc


Is the mediatype correct in here for the config descriptor? @sudo-bmitch

I think this still looks correct to me. We should definitely avoid changing the config media type here if at all possible since some registries have a hardcoded list of allowed config media types (as demonstrated in #3610) - if we keep it the same, we won't have the same problem, and this new way of storing cache should "just work".

I think that looks right. We've added guidance in image-spec for packaging artifacts (like the build cache) in the image manifest. My advice is to stick with the first example for now, which I believe you're already doing: https://github.com/opencontainers/image-spec/blob/933f91770d22839aca9c0ae46abc01321ca4ee70/manifest.md#guidelines-for-artifact-usage

tonistiigi · 2023-03-20T04:00:11Z

cache/remotecache/import.go

 		}
+
+		for _, m := range mfst.Layers {
+			if m.MediaType == v1.CacheConfigMediaTypeV0 {


This does not look correct. It is not layer mediatype that is CacheConfigMediaTypeV0 but the config's.

Good catch, yeah. This indeed popped when I added integ tests :)

Addressed in new commit.

tonistiigi · 2023-03-20T04:02:16Z

cache/remotecache/import.go

@@ -97,6 +150,37 @@ func (ci *contentCacheImporter) Resolve(ctx context.Context, desc ocispecs.Descr
 	return solver.NewCacheManager(ctx, id, keysStorage, resultStorage), nil
 }

+// extends support for "new"-style image-manifest style remote cache manifests and determining downstream
+// handling based on inference of document structure (is this a new or old cache manifest type?)
+func inferManifestType(ctx context.Context, dt []byte) (ManifestType, error) {


We already have a function called DetectManifestMediaType

Heh, I feel very lucky it already exists :) Good learning for me to see a better quality implementation as well. I also like that it uses the containerd/oci mediatypes which seems more direct and extensible than my fake-enum approach.

kattmang · 2023-03-20T10:06:06Z

Thanks for the feedback and pointers on new integ tests, @tonistiigi .Very insightful and will address yours and others' feedback items point by point after vacation in April!

jedevc · 2023-03-20T10:08:38Z

cache/remotecache/export.go

 	mfst.SchemaVersion = 2
 	mfst.MediaType = images.MediaTypeDockerSchema2ManifestList
-	if ce.oci {
+	if ce.oci && !ce.imageManifest {


We should still respect the value of oci-mediatypes here, even when image-manifest=true.

The default for oci-mediatypes is false, so it will still work out of the box with ECR, but users may use other registries and want to use OCI media types instead, so we should give them the option to enable this.

Probably oci-mediatypes=false and image-manifest=true would be invalid configuration and should just error.

What would be a better name for image-manifest=true? Is it oci-artifact-manifest oci-artifact? I'm quite confused about what they mean, but image-manifest seems too generic and doesn't convey the meaning that this uses a config with custom mediatyoe.

Funnily enough in my initial work for this I did use oci-artifact as the key name here, but after working with some colleagues more closely associated with the OCI image-spec 1.1 work ( tagging @michaelb990 and @jlbutler ), we found that the term artifact has become semantically loaded. More specifically there's two artifacts in people's mind in the space:

small "a" artifacts: image manifest + layers with a nonstandardized config.mediaType (this aligns with the cache manifest approach in this PR)

capital "A" Artifacts: artifact manifest + blobs (currently being pulled out / made optional in 1.1). This is an approach we could take for cache manifest if/when this manifest type becomes more prevalent.

As I work through this happy to consider other names for the key.

Will think through oci-mediatypes: I think I lean on erroring here though.

Thinking about it more: ImageManifest mode could support docker's image manifest version (which I don't think makes sense given registry support is wide enough for OCI manifest), so I am going to error if oci-mediatypes=false and image-manifest=true as invalid and down the road that can be implemented as needed/warranted. I also will update the readme to instruct client users to need to toggle both for Image Manifest remote caching to work.

kattmang · 2023-05-01T18:53:40Z

Added Client and Dockerfile Integ tests, and addressed feedback, primarily the largest thing being a bugfix to the import on imageManifest being broken on config mediatype being checked on layers instead of Config (thanks @tonistiigi ). Also tore out my manifest type inference code in favor of DetectManifestMediaType in utils.

kattmang · 2023-05-01T21:03:40Z

one second, rebuilding branch.

EDIT: FIxed, ready for review

tonistiigi · 2023-05-02T23:15:10Z

cache/remotecache/export.go

+type ExportableCache struct {
+	exportedManifest ocispecs.Manifest
+	exportedIndex    ocispecs.Index
+	cacheType        int


Add actual type for this with iota const values.

tonistiigi · 2023-05-02T23:16:18Z

cache/remotecache/export.go

-	cc := v1.NewCacheChains()
-	return &contentCacheExporter{CacheExporterTarget: cc, chains: cc, ingester: ingester, oci: oci, ref: ref, comp: compressionConfig}
+type ExportableCache struct {
+	exportedManifest ocispecs.Manifest


Add a comment in here that there are two styles of exports. Index of blobs and OCI artifact manifest.

tonistiigi · 2023-05-02T23:19:17Z

cache/remotecache/export.go

-
-		// Manifests references platform specific manifests.
-		Manifests []ocispecs.Descriptor `json:"manifests"`
+	cache := ExportableCache{}


iiuc the public functions SetMediaType and SetSchemaVersion are not needed. You can just make a constructor NewExportableCache(type) and everything is set up internally in that function.

For the oci check, this can be before NewExportableCache is called.

I remember when I was writing this I had it like that and I changed it to this (I have no idea why and on reflection this just makes more sense).

tonistiigi · 2023-05-02T23:21:49Z

cache/remotecache/export.go

+}
+
+func (ec *ExportableCache) FinalizeCache(ctx context.Context, ce *contentCacheExporter) {
+	// Nothing needed here for Manifest-type cache manifests


Are you sure ConvertAllLayerMediaTypes is not needed? Is it guaranteed to be OCI already?

From my tests I think it "accidentally" was OCI compatible, but I think you're right in that there's no guarantee that whatever the solver gives is going to be a consistent OCI Layer type. We should call this regardless of cache type to ensure the entire cache manifest is internally consistent.

Comment in here was not removed

Literally had my IDE open to remove it as I got this message :)

tonistiigi · 2023-05-02T23:22:38Z

cache/remotecache/export.go

+	}
+}
+
+func (ec *ExportableCache) GetCacheJSON() ([]byte, error) {


Call this MarshalJSON().

tonistiigi · 2023-05-02T23:23:29Z

cache/remotecache/export.go

@@ -133,9 +193,14 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string
 	desc = ocispecs.Descriptor{
 		Digest:    dgst,
 		Size:      int64(len(dt)),
-		MediaType: mfst.MediaType,
+		MediaType: cache.GetMediaType(),


After removing SetMediaType, Get prefix is not needed here.

tonistiigi · 2023-05-02T23:24:01Z

cache/remotecache/import.go

+		if mfst.Config.MediaType == v1.CacheConfigMediaTypeV0 {
+			configDesc = mfst.Config
+		} else {
+			err = errors.Wrapf(err, "Image Manifest cache is missing expected cache Config mediatype")


lowercase errors

err is nil in here. This needs to be errors.Errorf. Include the mediatype that doesn't match in the error message.

tonistiigi · 2023-05-02T23:27:17Z

cache/remotecache/import.go

 		}
+	default:
+		err = errors.Wrapf(err, "Unsupported or uninferrable manifest type")
+		return nil, err


how is the inline case still hit in line 112?

I think it would work fine if the ref'd manifest happens to be a Manifest List/Index and had an inline cache in its config (can that even happen?), but it would fail to hit the inline case if the ref is an Image Manifest because of line 88. I propose just ripping out lines 88 through 91 and letting any malform failures happen in importInlineCache.

Also, my apologies for not seeing the integ test failure indicating this issue: I did see some integ tests fail without any changes from my part so I thought that was attributable to other changes in master. Very good catch!

I just tested in my local, ripping out 88-91 does indeed fix the integ tests failing below.

tonistiigi · 2023-05-02T23:28:22Z

Also rebase to remove the merge commit from the commit chain.

kattmang · 2023-05-03T07:25:05Z

I've addressed comments and rebased.

kattmang · 2023-05-03T17:22:14Z

Integ tests look clean from my repo: https://github.com/kattmang/buildkit-cache-manifest-oci/actions/runs/4868808356/jobs/8682618226

kattmang · 2023-05-05T21:06:43Z

Just to confirm it looks like the single test failure here is for a test unrelated to this change, but want to see what if you've seen this one: TestDiffDeleteDoesNotFollowSymlink .

Happy to pull from the latest master and resubmit.

EDIT: looks like reruns did the trick and is part of a subset of tests that are flakey. Corresponded over slack with @tonistiigi and appears related to #3401

tonistiigi

Tested this and seemed to work correctly

tonistiigi · 2023-05-11T05:01:58Z

cache/remotecache/export.go

+			CacheType: cacheType,
+		}
+	}
+	return &ExportableCache{ExportedManifest: ocispecs.Manifest{


add switch for the cacheType with error case for unknown value.

tonistiigi · 2023-05-11T05:04:00Z

cache/remotecache/export.go

+	CacheType        CacheType
+}
+
+func NewExportableCache(cacheType CacheType, mediaType string, schemaVersion specs.Versioned) *ExportableCache {


why is schemaVersion a parameter? It is always 2 and can't be configured.

Instead of mediatype this should take oci parameter so that the type switch only happens once. And caller can't make a mistake setting incompatible cacheType and mediaType.

I take this to mean we should pull in scope from Finalize where we determine mediatype and cachetype more into the Cache object itself, which makes sense.

tonistiigi · 2023-05-11T05:09:13Z

cache/remotecache/import.go

-			configDesc = m
-			continue
+	switch manifestType {
+	case images.MediaTypeDockerSchema2ManifestList, ocispecs.MediaTypeImageIndex:


This can be follow-up, but I guess manifest list of "cache image manifests" should also be a valid configuration.

Yeah, I'm not sure I should tackle this because I don't know how such a cache would/could be exported.

tonistiigi · 2023-05-11T05:15:34Z

frontend/dockerfile/dockerfile_test.go

+
+	dt2, err = os.ReadFile(filepath.Join(destDir, "unique"))
+	require.NoError(t, err)
+	require.Equal(t, string(dt), string(dt2))


Ideally, you should also test in here that the manifest in the registry is an image manifest and it has a config with correct mediatype. Otherwise, this test would pass without any of your changes as well because image-manifest=true would just be ignored.

… of cache manifest (opt-in on export, inferred on import) moby/buildkit moby#2251 Signed-off-by: Kang, Matthew <impulsecss@gmail.com>

kattmang · 2023-05-11T23:00:35Z

Made the integ test change and the ExportableCache refactor @tonistiigi . LMK if we're good here.

tonistiigi · 2023-05-12T01:25:01Z

cache/remotecache/export.go

+
+func NewExportableCache(oci bool, imageManifest bool) (*ExportableCache, error) {
+	var mediaType string
+	if oci && !imageManifest {


This doesn't read very well.

var mediaType string cacheType := ManifestList if imageManifest { cacheType = ImageManifest if oci {} else {} } else { if oci {} else {} }

tonistiigi · 2023-05-12T01:26:03Z

cache/remotecache/export.go

 const (
 	// ExportResponseManifestDesc is a key for the map returned from Exporter.Finalize.
 	// The map value is a JSON string of an OCI desciptor of a manifest.
 	ExporterResponseManifestDesc = "cache.manifest"
+	CacheManifestSchemaVersion   = 2


This doesn't need to be public.

If you combine the schemaVersion to a single definition, then this doesn't need to be defined as constant at all.

tonistiigi · 2023-05-12T01:29:20Z

cache/remotecache/export.go

+func (ec *ExportableCache) FinalizeCache(ctx context.Context, ce *contentCacheExporter) {
+	// Nothing needed here for Manifest-type cache manifests
+	if ec.CacheType == ManifestList {
+		ec.ExportedIndex.Manifests = compression.ConvertAllLayerMediaTypes(ctx, ce.oci, ec.ExportedIndex.Manifests...)


Just store the .oci in NewExportableCache so it can be read from ec.

tonistiigi · 2023-05-12T22:19:36Z

cache/remotecache/export.go

+}
+
+func (ec *ExportableCache) FinalizeCache(ctx context.Context, ce *contentCacheExporter) {
+	// Nothing needed here for Manifest-type cache manifests


Comment in here was not removed

tonistiigi · 2023-05-12T22:20:33Z

cache/remotecache/export.go

+	ExportedManifest ocispecs.Manifest
+	ExportedIndex    ocispecs.Index
+	CacheType        CacheType
+	Oci              bool


nit: if it is public then OCI

tonistiigi · 2023-05-12T22:23:16Z

cache/remotecache/export.go

+
+func (data CacheType) String() string {
+	switch data {
+	case NotSet:


nit: case NotSet: is not needed here

Signed-off-by: Matt Kang <impulsecss@gmail.com>

kattmang · 2023-05-13T04:35:17Z

Thanks for your hard work in the review @tonistiigi ! Excited to see if we can fold this into 0.12.

jxlwqq · 2023-06-16T10:23:11Z

Can't wait for the new release.

If you want to test or use it early, you can use the master tag https://hub.docker.com/r/moby/buildkit/tags

This gives us access to moby/buildkit#3724 which (hopefully) unblocks errors such as https://github.com/firezone/firezone/actions/runs/6425322959/job/17447712176?pr=2258#step:6:158 when trying to pull the cache. --------- Signed-off-by: Thomas Eizinger <thomas@eizinger.io> Co-authored-by: Jamil <jamilbk@users.noreply.github.com>

kattmang marked this pull request as draft March 17, 2023 06:38

kattmang force-pushed the master branch from 25c466f to 660c18c Compare March 17, 2023 09:25

kattmang marked this pull request as ready for review March 17, 2023 09:38

kattmang force-pushed the master branch from 660c18c to 777efc5 Compare March 17, 2023 22:28

tonistiigi reviewed Mar 20, 2023

View reviewed changes

jedevc reviewed Mar 20, 2023

View reviewed changes

milosgajdos mentioned this pull request Apr 24, 2023

Unable to push manifest list to registy distribution/distribution#3863

Open

kattmang force-pushed the master branch from 5330646 to 3c9f176 Compare May 1, 2023 20:23

kattmang closed this May 1, 2023

kattmang force-pushed the master branch from 3c9f176 to b0c05cd Compare May 1, 2023 20:26

kattmang reopened this May 1, 2023

kattmang mentioned this pull request May 1, 2023

[ECR] [request]: support cache manifest aws/containers-roadmap#876

Closed

tonistiigi reviewed May 2, 2023

View reviewed changes

kattmang force-pushed the master branch 2 times, most recently from badd8b2 to ba2c7dc Compare May 3, 2023 06:03

kattmang requested a review from tonistiigi May 3, 2023 18:35

tonistiigi reviewed May 11, 2023

View reviewed changes

added import/export support for OCI compatible image manifest version…

797156a

… of cache manifest (opt-in on export, inferred on import) moby/buildkit moby#2251 Signed-off-by: Kang, Matthew <impulsecss@gmail.com>

kattmang force-pushed the master branch from ba2c7dc to a057921 Compare May 11, 2023 16:50

kattmang force-pushed the master branch from a057921 to 5006d17 Compare May 11, 2023 22:57

kattmang requested a review from tonistiigi May 11, 2023 23:00

tonistiigi reviewed May 12, 2023

View reviewed changes

kattmang force-pushed the master branch 2 times, most recently from 5ebb4b3 to a7f450c Compare May 12, 2023 05:00

kattmang requested a review from tonistiigi May 12, 2023 05:00

tonistiigi reviewed May 12, 2023

View reviewed changes

feedback changes for moby/buildkit moby#2251

29fd071

Signed-off-by: Matt Kang <impulsecss@gmail.com>

kattmang force-pushed the master branch from a7f450c to 29fd071 Compare May 12, 2023 23:05

kattmang requested a review from tonistiigi May 12, 2023 23:06

tonistiigi approved these changes May 12, 2023

View reviewed changes

tonistiigi merged commit 995d908 into moby:master May 13, 2023

tonistiigi mentioned this pull request May 13, 2023

Support remote cache based on OCI Image Manifest layout #2251

Closed

theomessin mentioned this pull request May 14, 2023

GCR and Explicit Cache earthly/earthly#1291

Open

TeamTeaTime mentioned this pull request May 17, 2023

Fix buildkit cache export to harbor. goharbor/harbor#18105

Closed

5 tasks

beastawakens mentioned this pull request May 18, 2023

BuildKit socket timeout on 3.12.0 convox/convox#641

Closed

MShekow mentioned this pull request Jun 1, 2023

Cache misses (registry, gha) for multi-stage images #3925

Closed

imeoer mentioned this pull request Jun 30, 2023

ERROR: error writing manifest blob: failed commit on ref unexpected status: 404 Not Found #3781

Open

anmarkoulis mentioned this pull request Jul 7, 2023

feat: add caching in reusable workflows on build and push step Orfium/orfium-github-actions#54

Merged

4 tasks

thomaseizinger mentioned this pull request Oct 5, 2023

ci: manually update to buildkit 12 firezone/firezone#2259

Merged

TBBle mentioned this pull request Oct 14, 2023

OCI Images used as cache SHOULD not have empty layers list #4331

Closed

bjones mentioned this pull request Mar 15, 2024

podman build fails with: --cache-from: repository must contain neither a tag nor digest containers/buildah#4935

Open

julmb mentioned this pull request Mar 17, 2024

Manifest push fails when using Gitea container registry with Docker BuildKit caching go-gitea/gitea#28973

Closed

dvdksn mentioned this pull request Aug 12, 2024

buildx: image-manifest param for registry and local cache backends docker/docs#20576

Merged

	idx := ocispecs.Index{
	MediaType: ocispecs.MediaTypeImageIndex,
	Annotations: opts.Annotations.Platform(nil).Index,
	Versioned: specs.Versioned{
	SchemaVersion: 2,
	},
	}

	if !opts.OCITypes {
	idx.MediaType = images.MediaTypeDockerSchema2ManifestList
	}

Import/export support for OCI compatible image manifest version of cache manifest (opt-in on export, inferred on import) #3724

Import/export support for OCI compatible image manifest version of cache manifest (opt-in on export, inferred on import) #3724

Conversation

kattmang commented Mar 17, 2023