image export: Use correct media type when creating new layer blobs. #1541

sipsma · 2020-06-23T18:57:53Z

There are a few bugs in the image export related code being fixed here. GetMediaTypeForLayers was iterating over diffPairs in the wrong order, resulting in it always returning nil for images with more than one layer. This actually worked most of the time because it accidentally triggered a separate codepath meant to handle v0.6 migrations where mediatypes left empty get filled in. However, fixing that bug revealed another existing bug where the "oci" parameter in the image exporter was never passed to GetDiffPairs, resulting in newly created images to always have oci layer media types even when docker types are used for the rest of the image descriptors (manifests, config, etc.).

Due to the interaction between these various bugs, the only practical end effect previously was that single-layer images could incorrectly use oci layer media types when docker types were supposed to be used. An existing test has been expanded to cover that case in a previous commit.

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

Note: This PR currently also includes the commits from #1538 so that tests can run in full. I'll rebase once #1538 is merged.

tonistiigi

As we discussed in slack this should also use conversion between equivalent docker and oci layer types when manifest is created so the manifest always contains the correct information. Did you plan that as a separate PR? Tbh when we have that fix I don't think it would be very bad if differ always created oci types. The information about the mediatype that was used when blob was first created is kind of meaningless apart from the compression.

The fix in GetMediaTypeForLayers LGTM

tonistiigi · 2020-06-23T23:39:17Z

cache/blobs/compression.go

@@ -102,21 +102,30 @@ func detectCompressionType(cr io.Reader) (CompressionType, error) {

 // GetMediaTypeForLayers retrieves media type for layer from ref information.
 func GetMediaTypeForLayers(diffPairs []DiffPair, ref cache.ImmutableRef) []string {


Add a comment that on error, slice with full length and partial data would be returned. This confused me in the beginning.

sipsma · 2020-06-24T00:00:12Z

As we discussed in slack this should also use conversion between equivalent docker and oci layer types when manifest is created so the manifest always contains the correct information.

Ah okay, I think I slightly misunderstood originally. I thought I had addressed this already by including these lines but now that you mention it those lines won't handle the case where there is already a blob associated with a layer but the media type stored in cache metadata is different from what the exporter wants.

I'll update to do the mediatype conversion on a higher level in the exporter rather than GetDiffPairs, which I imagine is closer to what you were originally suggesting.

tonistiigi · 2020-06-24T00:03:45Z

I'll update to do the mediatype conversion on a higher level in the exporter rather than GetDiffPairs, which I imagine is closer to what you were originally suggesting.

You can add a public helper function that takes an array of descriptors and makes sure they are either docker or oci. Then if someone calls GetRemote() and wants to have layers for a specific mediatype (eg. maybe in the exported cache) they can reuse this.

fuweid · 2020-06-24T02:53:54Z

client/client_test.go

-		ContainerdAddress() string
-	}); !ok {
+	cdAddress := sb.ContainerdAddress()
+	if cdAddress == "" {
 		return


could you mind to add some log for skip check? It will be nice to debug!

Yeah good call, updated with that

There is a bug in the way images are pushed that results in oci types being used for layers even when docker types should be used, but only in single-layer images. Signed-off-by: Erik Sipsma <erik@sipsma.dev>

sipsma · 2020-06-24T17:16:11Z

You can add a public helper function that takes an array of descriptors and makes sure they are either docker or oci. Then if someone calls GetRemote() and wants to have layers for a specific mediatype (eg. maybe in the exported cache) they can reuse this.

Makes sense, but when I went to actually make the updates for this PR I remembered that as it currently stands the exporter uses a slice of diffpairs and slice of mediatype strings rather than a slice of descriptors, so for now I updated this PR to have a public helper func ConvertLayerMediaType that just converts a single mediatype string. However, there is more refactoring in #1475 that switches the exporter to use a slice of descriptors, so when I rebase 1475 I'll update the helper func to operate on that instead.

tonistiigi · 2020-06-24T17:16:55Z

cache/blobs/compression.go

+		converted = toDockerLayerType[mediaType]
+	}
+	if converted == "" {
+		return "", fmt.Errorf("unhandled layer media type %q", mediaType)


I think ignoring error (and maybe printing the warning would be more appropriate). Especially because the conversion doesn't currently handle the alternative compressions (zstd etc).

tonistiigi · 2020-06-24T17:20:23Z

cache/blobs/compression.go

-		tref = tref.Parent()
+		parent := tref.Parent()
+		tref.Release(context.TODO())
+		tref = parent
 	}


in an odd case where parent chain length does not match the length of diffPairs (shouldn't happen) we should make sure we don't leave the tref referenced after returning.

sipsma · 2020-06-24T18:02:10Z

Updated from all the comments, think travis just needs a retry due to context deadline exceeded

tonistiigi · 2020-06-24T19:05:35Z

exporter/containerimage/writer.go

@@ -141,7 +141,7 @@ func (ic *ImageWriter) Commit(ctx context.Context, inp exporter.Source, oci bool
 	return &idxDesc, nil
 }

-func (ic *ImageWriter) exportLayers(ctx context.Context, compression blobs.CompressionType, refs ...cache.ImmutableRef) ([][]blobs.DiffPair, error) {
+func (ic *ImageWriter) exportLayers(ctx context.Context, compression blobs.CompressionType, oci bool, refs ...cache.ImmutableRef) ([][]blobs.DiffPair, error) {


oci bool unused in here now

oh duh... fixed

There are a few bugs in the image export related code being fixed here. GetMediaTypeForLayers was iterating over diffPairs in the wrong order, resulting in it always returning nil for images with more than one layer. This actually worked most of the time because it accidentally triggered a separate codepath meant to handle v0.6 migrations where mediatypes left empty get filled in. However, fixing that bug revealed another existing bug where the "oci" parameter in the image exporter was not being honored except when the v0.6 codepath got followed, resulting in images to always have oci layer media types even when docker types are used for the rest of the image descriptors. Due to the interaction between these various bugs, the only practical end effect previously was that single-layer images could use the wrong layer media type. An existing test has been expanded to cover that case in a previous commit. Signed-off-by: Erik Sipsma <erik@sipsma.dev>

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

tonistiigi reviewed Jun 23, 2020

View reviewed changes

fuweid reviewed Jun 24, 2020

View reviewed changes

Test media type after first push in TestBuildExportWithUncompressed

9e08704

There is a bug in the way images are pushed that results in oci types being used for layers even when docker types should be used, but only in single-layer images. Signed-off-by: Erik Sipsma <erik@sipsma.dev>

sipsma force-pushed the fix-media-type branch from 31cbaf2 to 45d22c5 Compare June 24, 2020 17:12

tonistiigi reviewed Jun 24, 2020

View reviewed changes

sipsma force-pushed the fix-media-type branch from 45d22c5 to 4a58727 Compare June 24, 2020 17:34

tonistiigi reviewed Jun 24, 2020

View reviewed changes

sipsma added 2 commits June 24, 2020 12:07

client test: log skip in checkAllReleasable.

43b58b2

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

sipsma force-pushed the fix-media-type branch from 4a58727 to 43b58b2 Compare June 24, 2020 19:10

tonistiigi approved these changes Jun 24, 2020

View reviewed changes

tonistiigi merged commit 17c11d9 into moby:master Jun 24, 2020

tonistiigi mentioned this pull request Jul 23, 2020

vendor: update buildkit to 4d1f260e8 moby/moby#41234

Merged

tonistiigi mentioned this pull request Sep 1, 2020

Error 400 on push to quay.io docker/buildx#300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image export: Use correct media type when creating new layer blobs. #1541

image export: Use correct media type when creating new layer blobs. #1541

sipsma commented Jun 23, 2020

tonistiigi left a comment

tonistiigi Jun 23, 2020 •

edited

Loading

sipsma commented Jun 24, 2020 •

edited

Loading

tonistiigi commented Jun 24, 2020

fuweid Jun 24, 2020

sipsma Jun 24, 2020

sipsma commented Jun 24, 2020

tonistiigi Jun 24, 2020

tonistiigi Jun 24, 2020

sipsma commented Jun 24, 2020

tonistiigi Jun 24, 2020

sipsma Jun 24, 2020

		@@ -102,21 +102,30 @@ func detectCompressionType(cr io.Reader) (CompressionType, error) {

		// GetMediaTypeForLayers retrieves media type for layer from ref information.
		func GetMediaTypeForLayers(diffPairs []DiffPair, ref cache.ImmutableRef) []string {

image export: Use correct media type when creating new layer blobs. #1541

image export: Use correct media type when creating new layer blobs. #1541

Conversation

sipsma commented Jun 23, 2020

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

sipsma commented Jun 24, 2020 • edited Loading

tonistiigi commented Jun 24, 2020

fuweid Jun 24, 2020

Choose a reason for hiding this comment

sipsma Jun 24, 2020

Choose a reason for hiding this comment

sipsma commented Jun 24, 2020

tonistiigi Jun 24, 2020

Choose a reason for hiding this comment

tonistiigi Jun 24, 2020

Choose a reason for hiding this comment

sipsma commented Jun 24, 2020

tonistiigi Jun 24, 2020

Choose a reason for hiding this comment

sipsma Jun 24, 2020

Choose a reason for hiding this comment

tonistiigi Jun 23, 2020 •

edited

Loading

sipsma commented Jun 24, 2020 •

edited

Loading