Perform an O(1) check to find layers in an image #101

smarterclayton · 2018-06-12T04:25:52Z

Using the new imagestream/layers API, avoid making O(N) image fetches in the case where we don't have a cached association for the reference.

API in openshift/origin#19969

smarterclayton · 2018-06-12T06:57:45Z

@bparees

bparees · 2018-06-12T18:15:29Z

hack/test-local-registry.sh

+
+REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY="${REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY:-/tmp/registry}" \
+  REGISTRY_OPENSHIFT_SERVER_ADDR="${url}" \
+	dockerregistry images/dockerregistry/config.yml


i assume this expects a cluster to be running somewhere, how are the coordinates of the cluster being supplied to the registry?

add a usage comment at the top of the file?

bparees · 2018-06-12T18:16:05Z

pkg/dockerregistry/server/blobdescriptorservice.go

 	context.GetLogger(ctx).Debugf("(*blobDescriptorService).Stat: could not stat layer link %s in repository %s: %v", dgst.String(), bs.repo.Named().Name(), err)

+	// we couldn't find the layer link


comment should elucidate the implications of not finding the layer link

this is just a refactor of the method, i'm happy to make better comments

bparees · 2018-06-12T18:25:24Z

pkg/imagestream/imagestreamhasblob.go

+			firstImage = name
+			break
+		}
+		return logFound(&imageapiv1.Image{


what if layers.Layers was empty?

that means the image stream has no layers

yeah but we shouldn't returning found in that case, right?

if we don't have a layer we return on line 43 with nil, and the if block fails, and we return ErrBlobUnknown.

bparees · 2018-06-12T18:29:57Z

pkg/imagestream/imagestreamhasblob.go

+		}
+		var firstImage string
+		for name := range layers.Layers {
+			firstImage = name


we don't actually need to find the specific image that contains the blob? any image in the imagestream will do?

If we have the list of blobs, we don't need the image object anymore, we should just return that list.

bparees · 2018-06-12T18:30:50Z

...ub.com/openshift/client-go/image/clientset/versioned/typed/image/v1/imagestream_expansion.go

+	Layers(name string) (*v1.ImageStreamLayers, error)
+}
+
+// Bind applies the provided binding to the named pod in the current namespace (binding.Namespace is ignored).


godoc is wrong

dmage · 2018-06-13T13:44:06Z

pkg/imagestream/imagestreamhasblob.go

+			ObjectMeta: metav1.ObjectMeta{
+				Name: firstImage,
+			},
+			DockerImageLayers: copied,


The configuration blob for v2 scheme is missed here.

RememberLayersOfImage will report the error "image %s does not have metadata" for this image. We trying to reduce amount of logs.

I’m going to remove that message. This is purely a blob access check.

This error message means that we got incomplete image and we do not have all necessary data (we do not have the digest of config blob) and can't to cache them all. For the RememberLayersOfImage this message is truely error.

Your handmade image object is really incomplete and hacky.

Then I'll split this into RememberLayers and RememberLayersOfImage.

legionus · 2018-06-13T14:23:51Z

pkg/imagestream/imagestreamhasblob.go

+			firstImage = name
+			break
+		}
+		return logFound(&imageapiv1.Image{


Regression. The requireManaged was ignored. It will break config.Pullthrough.Enabled flag.

https://github.com/openshift/image-registry/pull/101/files#diff-0c454c8033229f303d88c165f82eb0d9L82

I’m not sure pullthrough enabled false is something we should support. In 8 release we’ve never had a reason to trn it off. At this point, I’d probably prefer to drop the use case.

I don’t really consider the distinction between managed an unmanaged images important anymore. If a layer is available it should be served. The image it comes from is irrelevant.

Specifically, for the case where we are attempting to find the layers, I think this method should be returning a set of layers that are considered valid (which is added to the cache).

OK. Please, open an issue to make pullthrough not optional to address this important change. Because right now this functionality can be turned off and removing this option will need to be written to the log as a warning in the case of config.Pullthrough.Enabled = false.

/cc @bparees @dmage

smarterclayton · 2018-06-14T01:37:02Z

I still need to add the e2e test to try and access content. Will do that once we clear up the case.

legionus

I think it is not necessary to add new features, which from the start leads to hacks. I mean the creation of images that are not images. Such image do not have all information about blobs (image blobs != image layers).

This will lead to the fact that the last code can no longer process images identically. This will greatly complicate the work with images.

dmage · 2018-06-14T12:46:44Z

pkg/imagestream/imagestreamhasblob.go

@@ -37,6 +38,27 @@ func (is *imageStream) HasBlob(ctx context.Context, dgst digest.Digest, requireM
 		return found
 	}

+	if layers, err := is.imageStreamGetter.layers(); err == nil {


Why do we need to fallback to the old code?

For 1 release we should tolerate the cluster not having /layers or the admin not configuring permissions. We can then remove the old code path in 3.12.

smarterclayton · 2018-07-08T20:38:40Z

Updated with the latest version of the api, all feedback added to the PR.

The biggest change is the signature to HasBlob - it now returns (bool, *ImageStreamLayers, *Image), and the caller uses the two objects if they are available as "things to add to the cache". When we remove the legacy path in 3.12/4.0 we can drop that segment and simplify the signature back down.

Unit tests added, the e2e test will be added to the origin PR.

smarterclayton · 2018-07-09T00:05:43Z

/retest

bparees · 2018-07-09T20:59:23Z

looks reasonable to me

smarterclayton · 2018-07-10T02:14:32Z

/retest

smarterclayton · 2018-07-10T22:48:07Z

Bumped dependencies in #113, which I will rebase on master after that merges.

Was accidentally dropped when moved over from image-registry

Makes it easier to read

smarterclayton · 2018-07-11T16:01:23Z

Updated on bumps, ready for final review.

Added one commit to switch us to building on go1.10 for travis.

Because this must work during a rolling update we fall back to the older path if we get an error. A future release can drop support for the fallback path (3.12+).

bparees · 2018-07-12T01:31:54Z

lgtm. Any final concerns @dmage or @legionus ?

smarterclayton · 2018-07-13T14:09:04Z

No comments? I'm happy to do follow ups (I need to improve the e2e tests in origin for pulling images.

bparees · 2018-07-13T15:19:17Z

/lgtm

openshift-ci-robot · 2018-07-13T15:19:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2018

openshift-ci-robot requested review from liggitt and sttts June 12, 2018 04:25

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 12, 2018

smarterclayton force-pushed the fetch_layers branch from f01bb41 to 5acfc3d Compare June 12, 2018 06:54

bparees requested changes Jun 12, 2018

View reviewed changes

smarterclayton changed the title ~~WIP - Perform an O(1) check to find layers in an image~~ WIP - 3.11 - Perform an O(1) check to find layers in an image Jun 12, 2018

dmage reviewed Jun 13, 2018

View reviewed changes

legionus reviewed Jun 13, 2018

View reviewed changes

openshift-ci-robot requested review from bparees and dmage June 14, 2018 07:16

legionus suggested changes Jun 14, 2018

View reviewed changes

legionus mentioned this pull request Jun 14, 2018

pullthrough is not optional #103

Merged

dmage reviewed Jun 14, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2018

smarterclayton force-pushed the fetch_layers branch from 5acfc3d to 30f40a9 Compare July 8, 2018 20:32

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2018

smarterclayton force-pushed the fetch_layers branch from 30f40a9 to cb8bb2b Compare July 8, 2018 20:36

smarterclayton changed the title ~~WIP - 3.11 - Perform an O(1) check to find layers in an image~~ Perform an O(1) check to find layers in an image Jul 8, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2018

smarterclayton added 2 commits July 11, 2018 10:54

Add a test script to assist in running a local registry

8fd43e4

Was accidentally dropped when moved over from image-registry

Reorganize blobdescriptorservice Stat to exit early

1fd7f26

Makes it easier to read

smarterclayton force-pushed the fetch_layers branch from cb8bb2b to 237f9d4 Compare July 11, 2018 15:49

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 11, 2018

smarterclayton force-pushed the fetch_layers branch from b4b2837 to 1bc02cd Compare July 11, 2018 17:26

smarterclayton added 3 commits July 11, 2018 14:55

Use imagestream/layers call to make verify O(1)

b665e71

Because this must work during a rolling update we fall back to the older path if we get an error. A future release can drop support for the fallback path (3.12+).

Add support for layers in fake client testing and unit tests

359fc89

Update to Go 1.10 for travis and build to get t.Helper

82a0a6d

smarterclayton force-pushed the fetch_layers branch from 1bc02cd to 82a0a6d Compare July 11, 2018 18:55

bparees approved these changes Jul 12, 2018

View reviewed changes

openshift-ci-robot assigned bparees Jul 13, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2018

openshift-merge-robot merged commit d46b6b6 into openshift:master Jul 13, 2018

		context.GetLogger(ctx).Debugf("(*blobDescriptorService).Stat: could not stat layer link %s in repository %s: %v", dgst.String(), bs.repo.Named().Name(), err)

		// we couldn't find the layer link

Perform an O(1) check to find layers in an image #101

Perform an O(1) check to find layers in an image #101

Conversation

smarterclayton commented Jun 12, 2018 • edited Loading

smarterclayton commented Jun 12, 2018

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

bparees Jun 12, 2018 • 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

smarterclayton commented Jun 14, 2018

legionus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 8, 2018

smarterclayton commented Jul 9, 2018

bparees commented Jul 9, 2018

smarterclayton commented Jul 10, 2018

smarterclayton commented Jul 10, 2018

smarterclayton commented Jul 11, 2018

bparees commented Jul 12, 2018

smarterclayton commented Jul 13, 2018

bparees commented Jul 13, 2018

openshift-ci-robot commented Jul 13, 2018

smarterclayton commented Jun 12, 2018 •

edited

Loading

bparees Jun 12, 2018 •

edited

Loading