Fix image size calculation in importer #9115

legionus · 2016-06-01T17:38:51Z

After upgrading docker/distibution V1Compatibility no longer contains Size. For
this reason, the image size calculation is wrong. A new way to calculate the
size repeats the way that dockerregistry calculates the size.

Fix #7706

@smarterclayton @pweil- @mfojtik @miminar @Kargakis

soltysh · 2016-06-01T17:45:37Z

pkg/image/importer/importer.go

@@ -397,6 +418,7 @@ func importRepositoryFromDocker(ctx gocontext.Context, retriever RepositoryRetri
 			importDigest.Err = err
 			continue
 		}
+		importDigest.Err = calculateImageSize(ctx, repo, importDigest.Image)


I'd rather we calculcate the size only when it's not there. Not all registries we use are hub.docker.com, for that cases (where v1 schema is supported) it's unnecessarily calculates the size.

This could be done as a followup to this issue, we need to unblock the queue, atm.

Please use the pattern we are using above (if err := ...; err != nil { importDigest.Err = err; continue }) for readability.

This definitely will be broken on v1, which does not have layer sizes.

Seems like a more natural fit inside of schema1ToImage to build a complete Image object. The additional fanout is less than beautiful, but it would keep the API looking more consistent. schema1ToImage would return a complete and valid look.

Can you make sure the integration test is properly testing v1 sizes against registry.access.redhat.com?

Schema v1 supported by hub.docker.com. Before that we used is not documented structure to calculate the size.

https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-1.md#manifest-field-descriptions

v1Compatibility string V1Compatibility is the raw V1 compatibility information. This will contain the JSON object describing the V1 of this image.

Why are we not using the descriptor on the layers reference to calculate
the size? We already have that data in schema on layers - in
schema2toManifest (which this PR should create) we would be able to
calculate the fake size.

On Wed, Jun 1, 2016 at 1:53 PM, Clayton Coleman ccoleman@redhat.com wrote:

It doesn't matter that it's not documented - it's what is deployed in
production in many places still.

On Wed, Jun 1, 2016 at 1:52 PM, Alexey Gladkov notifications@github.com
wrote:

In pkg/image/importer/importer.go
#9115 (comment):

@@ -397,6 +418,7 @@ func importRepositoryFromDocker(ctx gocontext.Context, retriever RepositoryRetri
importDigest.Err = err
continue
}

importDigest.Err = calculateImageSize(ctx, repo, importDigest.Image)

Schema v1 supported by hub.docker.com. Before that we used is not
documented structure to calculate the size.

https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-1.md#manifest-field-descriptions

v1Compatibility string

V1Compatibility is the raw V1 compatibility information. This will contain the JSON
object describing the V1 of this image.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9115/files/492baa03f23cf45ca229c3501c29ca4bccf546f3#r65409920,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p0YnP3kHQqbqW47WCSl_-TNMETGtks5qHcbcgaJpZM4IrzC2
.

I don't see the need to call stat

On Wed, Jun 1, 2016 at 1:56 PM, Clayton Coleman ccoleman@redhat.com wrote:

Why are we not using the descriptor on the layers reference to calculate
the size? We already have that data in schema on layers - in
schema2toManifest (which this PR should create) we would be able to
calculate the fake size.

On Wed, Jun 1, 2016 at 1:53 PM, Clayton Coleman ccoleman@redhat.com
wrote:

It doesn't matter that it's not documented - it's what is deployed in
production in many places still.

On Wed, Jun 1, 2016 at 1:52 PM, Alexey Gladkov notifications@github.com
wrote:

In pkg/image/importer/importer.go
#9115 (comment):

@@ -397,6 +418,7 @@ func importRepositoryFromDocker(ctx gocontext.Context, retriever RepositoryRetri
importDigest.Err = err
continue
}

importDigest.Err = calculateImageSize(ctx, repo, importDigest.Image)

Schema v1 supported by hub.docker.com. Before that we used is not
documented structure to calculate the size.

https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-1.md#manifest-field-descriptions

v1Compatibility string

V1Compatibility is the raw V1 compatibility information. This will contain the JSON
object describing the V1 of this image.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9115/files/492baa03f23cf45ca229c3501c29ca4bccf546f3#r65409920,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p0YnP3kHQqbqW47WCSl_-TNMETGtks5qHcbcgaJpZM4IrzC2
.

it's unnecessarily calculates the size.

@soltysh @smarterclayton I added the check to calculate the size only when it is equal to zero. This would eliminate unnecessary requests if we have all the information.

I'm still concerned because each of those stats could fail, resulting in
other failures. We probably don't want to fail import if size can't be
calculated. We probably don't want to add a fan-out of 20 requests on
setup.

I'd prefer to not fix this until we have schema2 support. I thought we
were getting a schema2 response without size.

On Wed, Jun 1, 2016 at 2:22 PM, Alexey Gladkov notifications@github.com
wrote:

In pkg/image/importer/importer.go
#9115 (comment):

@@ -397,6 +418,7 @@ func importRepositoryFromDocker(ctx gocontext.Context, retriever RepositoryRetri
importDigest.Err = err
continue
}

importDigest.Err = calculateImageSize(ctx, repo, importDigest.Image)

it's unnecessarily calculates the size.

@soltysh https://github.com/soltysh @smarterclayton
https://github.com/smarterclayton I added the check to calculate the
size only when it is equal to zero. This would eliminate unnecessary
requests if we have all the information.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9115/files/492baa03f23cf45ca229c3501c29ca4bccf546f3#r65415556,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3qVdLRkR2IkXDwpJ7kJ7xgChqOKks5qHc3zgaJpZM4IrzC2
.

I'm still concerned because each of those stats could fail, resulting in other failures. We probably don't want to fail import if size can't be calculated. We probably don't want to add a fan-out of 20 requests on setup.

Yes, this is possible. We use NewRetryRepository to make few retries. But fix is necessary for the scheme1.

deads2k · 2016-06-01T17:48:27Z

pkg/image/importer/importer.go

+	size := int64(0)
+	for i := range image.DockerImageLayers {
+		layer := &image.DockerImageLayers[i]
+		desc, err := bs.Stat(ctx, digest.Digest(layer.Name))


This fanout is unfortunate. TODO for a better solution or maybe a cache?

See my comment but I don't think we need the stat - we should already have desc.Size when we get the image metadata in schema1ToImage

schema1ToImage is only about image creation. It's not getting digests and do not have anything to calculate the size.

According to the v2 spec size must be filled out in layers. Is that not
the case?

On Wed, Jun 1, 2016 at 2:09 PM, Alexey Gladkov notifications@github.com
wrote:

In pkg/image/importer/importer.go
#9115 (comment):

@@ -282,6 +282,27 @@ func applyErrorToRepository(repository *importRepository, err error) {
}
}

+func calculateImageSize(ctx gocontext.Context, repo distribution.Repository, image *api.Image) error {

bs := repo.Blobs(ctx)

layerSet := sets.NewString()

size := int64(0)

for i := range image.DockerImageLayers {

layer := &image.DockerImageLayers[i]

desc, err := bs.Stat(ctx, digest.Digest(layer.Name))

schema1ToImage is only about image creation. It's not getting digests and
do not have anything to calculate the size.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9115/files/492baa03f23cf45ca229c3501c29ca4bccf546f3#r65413168,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p8C7_D83OxQ7joXFIweSMM4eSYOUks5qHcrygaJpZM4IrzC2
.

Schema v1 don't have the size:

https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-1.md#example-manifest

Schema v2 have it:

https://github.com/docker/distribution/blob/master/docs/spec/manifest-v2-2.md#example-image-manifest

But right now we talking about schema v1.

I think that's acceptable.

Our tests show that we had a regression after upgrading the hub.docker.com. But OK, you know better.

this is going to be less of a problem soon.

It will be less of a problem only then when there will be no schema1 on the servers.

we need to fix right now is the tests

I know that now is broken: TestImageStreamImport, TestImageStreamImportDockerHub, but there could be more.

I don't know how you can help upgrading the dockerregisrty. My PR is also affected by this issue.

It is acceptable if in the master branch of Origin we explicitly call out that importing schema1 from the DockerHub (that have been down converted from schema2) will not have size set, because we anticipate this problem resolving itself by a) more images on the hub being schema2, b) we want to support schema2 import on the rebase.

I'll open a PR to disable the tests now and we can lower the priority.

I'll open a PR to disable the tests now and we can lower the priority.

OK. Then I close this PR.

Opened test change in #9118

On Wed, Jun 1, 2016 at 3:41 PM, Alexey Gladkov notifications@github.com
wrote:

In pkg/image/importer/importer.go
#9115 (comment):

@@ -282,6 +282,27 @@ func applyErrorToRepository(repository *importRepository, err error) {
}
}

+func calculateImageSize(ctx gocontext.Context, repo distribution.Repository, image *api.Image) error {

bs := repo.Blobs(ctx)

layerSet := sets.NewString()

size := int64(0)

for i := range image.DockerImageLayers {

layer := &image.DockerImageLayers[i]

desc, err := bs.Stat(ctx, digest.Digest(layer.Name))

I'll open a PR to disable the tests now and we can lower the priority.

OK. Then I close this PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9115/files/492baa03f23cf45ca229c3501c29ca4bccf546f3#r65428383,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_Cre1dOt6e_KNPE4GhqBex9VKe5ks5qHeB4gaJpZM4IrzC2
.

legionus · 2016-07-05T14:41:55Z

[test]

legionus · 2016-07-05T18:18:15Z

@smarterclayton @soltysh I added caching for layer size.
What do you think about this solution ?

soltysh · 2016-07-06T09:48:29Z

pkg/image/api/helper.go

@@ -474,6 +474,8 @@ func ImageWithMetadata(image *Image) error {
 			image.DockerImageLayers[i].Name = layer.DockerBlobSum
 		}
 		if len(manifest.History) == len(image.DockerImageLayers) {
+			// This code does not work since hub.docker.com has been updated.
+			// The V1Compatibility no longer contains Size.


This will be true for docker hub, but for other repos supporting V1, it's not. I'd drop this comment entirely.

This comment is valid for all v2 -> v1 conversions.

Rephrase it in following way:
"This code does not work for images imported from v2 schema registries or converted from v2 to v1, since schema v2 does not contain size information."
Or something more appropriate, please.

@soltysh Fixed

soltysh · 2016-07-06T12:15:01Z

The changes itself looks good, but I won't let this in without tests. We're having too many problems with images to let any code in without at least simple unit test.

miminar · 2016-07-06T13:17:19Z

pkg/image/api/helper.go

@@ -474,6 +474,9 @@ func ImageWithMetadata(image *Image) error {
 			image.DockerImageLayers[i].Name = layer.DockerBlobSum
 		}
 		if len(manifest.History) == len(image.DockerImageLayers) {
+			// This code does not work for images imported from v2 schema
+			// registries or converted from v2 to v1, since schema v2 does not
+			// contain size information.


since schema v2 does not contain size information.

It does, it's just hidden in layer descriptors. This information is lost during conversion to v1 though.

So current comment is accurate.

So current comment is accurate.

It's not. Because schema v2 contains size information.

Agreed, the last part is wrong.

smarterclayton · 2016-07-07T02:03:42Z

pkg/image/importer/importer.go

@@ -78,17 +82,28 @@ func (i *ImageStreamImporter) contextImageCache(ctx gocontext.Context) map[manif
 	return cache
 }

+type imageStreamImporterCache struct {
+	ImageCache     map[manifestKey]*api.Image


We already have digestToRepositoryCache, why do we need a second cache?

miminar · 2016-07-08T16:05:27Z

pkg/image/importer/importer.go

@@ -50,6 +50,9 @@ type ImageStreamImporter struct {
 	limiter   flowcontrol.RateLimiter

 	digestToRepositoryCache map[gocontext.Context]map[manifestKey]*api.Image
+
+	// digestToLayerSizeCache maps layer digests to size.
+	digestToLayerSizeCache map[string]int64


Can we make it thread-safe. And let it be passed to NewImageStreamImporter so it can be shared among multiple importers invoked by image import controller?

I agree, but I think it should not be part of this PR.

If you're going to share it, then it needs to be an LRU.

miminar · 2016-07-08T16:20:46Z

test/integration/imageimporter_test.go

+				}
+			}
+			t.Fatalf("unexpected request %s: %#v", r.URL.Path, r)
+		}


There's a high potential for sharing most of this handler's code with the other tests.

It would make sense if there is another such test.

After upgrading docker/distibution V1Compatibility no longer contains Size. For this reason, the image size calculation is wrong. A new way to calculate the size repeats the way that dockerregistry calculates the size.

This reverts commit b30f322.

soltysh · 2016-07-11T12:09:24Z

Flake from #9766.

miminar · 2016-07-11T12:13:58Z

pkg/image/importer/importer.go

@@ -50,6 +50,9 @@ type ImageStreamImporter struct {
 	limiter   flowcontrol.RateLimiter

 	digestToRepositoryCache map[gocontext.Context]map[manifestKey]*api.Image


@smarterclayton Can you explain, why is it necessary to group the entries by context? Does the same scenario apply to the digestToLayerSizeCache below?

Because access to an image may only be allowed given the credentials associated with your context. So you cannot retry it. It does not apply to digest to layer size.

soltysh · 2016-07-11T12:25:09Z

LGTM, need @smarterclayton approval and answer @miminar question. Also please link an issue/PR addressing multithreaded access to the cache.

miminar · 2016-07-11T16:44:42Z

Created #9779 for a proper caching if this PR neglects it.

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

openshift-bot · 2016-07-12T14:25:46Z

Evaluated for origin test up to 26dad6d

legionus · 2016-07-12T14:29:46Z

@smarterclayton It's ready for merge ?

openshift-bot · 2016-07-12T15:39:50Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6110/)

miminar · 2016-07-12T16:31:25Z

LGTM

soltysh · 2016-07-13T13:30:32Z

I don't see any objections so merging.

[merge]

openshift-bot · 2016-07-13T13:35:23Z

Evaluated for origin merge up to 26dad6d

openshift-bot · 2016-07-13T14:30:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6110/) (Image: devenv-rhel7_4580)

soltysh reviewed Jun 1, 2016
View reviewed changes

soltysh added priority/P0 component/image labels Jun 1, 2016

deads2k reviewed Jun 1, 2016
View reviewed changes

legionus force-pushed the fix-image-metadata-size branch 2 times, most recently from 4ebd85b to 4bcc443 Compare June 1, 2016 18:18

legionus closed this Jun 1, 2016

legionus reopened this Jul 5, 2016

legionus force-pushed the fix-image-metadata-size branch from 4bcc443 to 4e57bf3 Compare July 5, 2016 13:08

soltysh reviewed Jul 6, 2016
View reviewed changes

legionus force-pushed the fix-image-metadata-size branch from 4e57bf3 to ce7b037 Compare July 6, 2016 11:08

miminar reviewed Jul 6, 2016
View reviewed changes

legionus force-pushed the fix-image-metadata-size branch from ce7b037 to 19fdd11 Compare July 6, 2016 18:44

smarterclayton reviewed Jul 7, 2016
View reviewed changes

legionus force-pushed the fix-image-metadata-size branch from 19fdd11 to add6457 Compare July 8, 2016 15:53

miminar reviewed Jul 8, 2016
View reviewed changes

legionus force-pushed the fix-image-metadata-size branch from add6457 to 7159082 Compare July 8, 2016 16:23

legionus and others added 2 commits July 8, 2016 18:41

Fix image size calculation in importer

5eefd92

After upgrading docker/distibution V1Compatibility no longer contains Size. For this reason, the image size calculation is wrong. A new way to calculate the size repeats the way that dockerregistry calculates the size.

Revert "Allow size of image to be zero when schema1 from Hub"

59e6a63

This reverts commit b30f322.

legionus force-pushed the fix-image-metadata-size branch from 7159082 to 9399f53 Compare July 8, 2016 16:41

miminar reviewed Jul 11, 2016
View reviewed changes

miminar mentioned this pull request Jul 11, 2016

Let's cache layers size globally #9779

Closed

miminar mentioned this pull request Jul 11, 2016

Docker registry allows to serve any blob user wants #9755

Closed

Add test for image import and conversion v2 schema to v1 schema

26dad6d

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus force-pushed the fix-image-metadata-size branch from 9399f53 to 26dad6d Compare July 12, 2016 14:21

openshift-bot merged commit 0f5afdc into openshift:master Jul 13, 2016

legionus mentioned this pull request Aug 23, 2016

Image size no longer in dockerImageMetadata #10592

Closed

		@@ -50,6 +50,9 @@ type ImageStreamImporter struct {
		limiter flowcontrol.RateLimiter

		digestToRepositoryCache map[gocontext.Context]map[manifestKey]*api.Image

Fix image size calculation in importer #9115

Fix image size calculation in importer #9115

Conversation

legionus commented Jun 1, 2016

soltysh Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionus commented Jul 5, 2016

legionus commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jul 11, 2016

miminar commented Jul 11, 2016

openshift-bot commented Jul 12, 2016

legionus commented Jul 12, 2016

openshift-bot commented Jul 12, 2016

miminar commented Jul 12, 2016

soltysh commented Jul 13, 2016 • edited Loading

openshift-bot commented Jul 13, 2016

openshift-bot commented Jul 13, 2016 • edited Loading

soltysh Jun 1, 2016 •

edited

Loading

soltysh commented Jul 13, 2016 •

edited

Loading

openshift-bot commented Jul 13, 2016 •

edited

Loading