Provide layers referenced by an image stream as layers subresource #19969

smarterclayton · 2018-06-12T02:13:09Z

Adds a new GET endpoint to an image stream as a subresource layers that
returns an array of every layer referenced by the image stream and the tags
and images included by the image.

The subresource is fed by a store driven informer that caches and indexes
only the layers. Clients get a 500 retry error if the cache has not
initialized yet (the client will silently retry).

Turns the registry access check for a given layer into an O(1) check
instead of O(N) where N is the number of images in the image stream.

This is not a general purpose layer cache, but lays the foundation for it.

Depends on openshift/api#56

Detected by https://bugzilla.redhat.com/show_bug.cgi?id=1589994

@bparees

sttts · 2018-06-12T11:30:31Z

pkg/image/registry/imagestream/etcd/image.go

+var _ metav1.Object = &ImageLayers{}
+
+func (l *ImageLayers) GetObjectKind() schema.ObjectKind { return &metav1.TypeMeta{} }
+func (l *ImageLayers) DeepCopyObject() runtime.Object {


why not generated this?

Probably could, although I'm hesitant to involve the stack there.

sttts · 2018-06-12T11:32:09Z

pkg/image/registry/imagestream/etcd/image.go

+
+var _ metav1.Object = &ImageLayers{}
+
+func (l *ImageLayers) GetObjectKind() schema.ObjectKind { return &metav1.TypeMeta{} }


looks like we are forcing ImageLayers to be an runtime.Object. At the very least this neads a very good reason documented as a comment above.

@smarterclayton why not making this an actual API object?

This is a minimal cache. Cache doesn't actually need runtime object except there are bugs in cache. I'm going to correct those, which is why there is a comment. Informers are supposed to depend on list-watch (which requires resource version) but the cache key is arbitrary.

mfojtik · 2018-06-12T11:38:18Z

@smarterclayton for UPSTREAM: openshift/api: Add an image layers API aren't you supposed to bump the external repo (openshift/api) first and then just run ./hack/update-deps ? /cc @deads2k

smarterclayton · 2018-06-12T13:30:01Z

It’s not ready to merge On Jun 12, 2018, at 7:38 AM, Michal Fojtik <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton> for UPSTREAM: openshift/api: Add an image layers API aren't you supposed to bump the external repo (openshift/api) first and then just run ./hack/update-deps ? /cc @deads2k <https://github.com/deads2k> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19969 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p-bJzcLmLVc3FHzilxjqCtU98PaPks5t76gugaJpZM4UjqsE> .

dmage · 2018-06-13T13:34:15Z

pkg/image/registry/imagestream/etcd/image.go

+	}
+	return &imageapi.ImageLayer{
+		Name:      image.DockerImageMetadata.ID,
+		MediaType: image.DockerImageManifestMediaType,


The function name is misleading. And the return value too.

For v2 manifests DockerImageMetadata.ID contains a digest of the image configuration. It differs from a manifest.

The MediaType for this blob is a though question, but it definitely shouldn't be DockerImageManifestMediaType.

As LayerSize is not filled here, it shouldn't be used anywhere.

I’ll probably say we just mark layer size as optional in the api. It’s not fundamental for this call (although I would prefer it be correct). Is there a reason we didn’t record it on the image object? Or did we just lose that info when we moved back to storing this in the registry? We have it when we post a new image to the registry, correct?

You’re right that this should be the configuration mimetype.

Probably can make LayerSize a pointer and have it be nil if unknown.

Updated to make layer size be a pointer and nil if unknown.

legionus · 2018-06-13T14:20:02Z

pkg/image/registry/imagestream/etcd/image.go

+			for i, image := range list.Items {
+				out.Items[i] = &ImageLayers{
+					Name:     image.Name,
+					Layers:   image.DockerImageLayers,


These layers have different order in different images. ImageAPI reorder them and add imageapi.DockerImageLayersOrderAnnotation, but this code drop this infomation. After that, these layers are copied as is, but without an annotation indicating the order.

Also there is no imageapi.ManagedByOpenShiftAnnotation for image. Therefore, then the registry does not have the ability to understand this.

I’ll reorder them before returning.

With regards to pullthrough off, I think we probably should drop support for disabling it. We will depend on it for secured images (when registry access for redhat is made protected), and local resolution depends on it. Is there a good use case for continuing to support it?

Is there a good use case for continuing to support it?

You tell me :) We always had the option to turn on/off pullthrough. Because of this, the registry's code is more complex than it could be because we always need to remember the type of the image and sometimes pretend that we do not know about it.

If you think that we can drop it, then let's do it. Please open an issue to remove it.

But right now without imageapi.ManagedByOpenShiftAnnotation pullthrough will be broken.

Unfortunately, we can not completely remove imageapi.ManagedByOpenShiftAnnotation.

Let me chat with Ben - if we think we're dependent on pull through now for protected image access, then I think it's time to remove the support for pull through false. Will respond back today.

Do we need the annotation for anything else specific to the use of this call (in the place where we check whether an image stream has "access" to a layer)?

A long time ago we talked about changing the meaning of the annotation (or replacing it with a new annotation) which was:

The person who imported this image metadata has proven they have access to each of the layers in the image they imported.

A background controller (the mirror controller) would not set the annotation until it had either pulled all layers from the source (in the background) or otherwise verified the source had those layers (check that the source can serve the contents). Before the annotation was set, only pull through would work, and users would be blocked from tagging the image into another image stream. Once the annotation was set, the image was now effectively managed (we've imported the image and the metadata). The downside of this is that it forces mirroring which can be expensive (today mirroring is mostly controlled by whether you set ReferencePolicy in practice).

Let me chat with Ben - if we think we're dependent on pull through now for protected image access, then I think it's time to remove the support for pull through false.

OCP will be, Origin won't be. Perhaps that's sufficient though.

(Technically someone could also setup an OCP cluster that got protected images from elsewhere like an internal mirror, so it is possible to run an OCP cluster w/o pullthrough, also).

But i'm certainly a fan of simplifying.

I think we closed on this, right? Dropping pullthrough: false support?

yeah, being removed here: openshift/image-registry#103

i think it's currently stuck on that test that needs the docker socket for which i have a PR open to mark as local.

smarterclayton · 2018-06-24T19:47:41Z

/test launch-gcp

smarterclayton · 2018-06-24T23:12:09Z

/test launch-gcp

smarterclayton · 2018-06-25T02:26:42Z

/test launch-gcp

smarterclayton · 2018-06-25T03:42:41Z

e2e test should be passing, initial comments addressed. Will be updating the registry PR soon.

liggitt · 2018-07-09T13:30:38Z

vendor/github.com/openshift/api/image/v1/types.go

+	// may have zero layers.
+	// +optional
+	Layers []string `json:"layers" protobuf:"bytes,1,rep,name=layers"`
+	// manifest, if set, is the blob that contains the image manifest. Some images do


this is the sha of the manifest ("name of the blob containing the image manifest"), not the blob itself, right?

The manifest is a blob, and the digest / name for both is the same (the SHA of the contents)

liggitt · 2018-07-09T13:31:33Z

vendor/github.com/openshift/api/image/v1/types.go

+type ImageBlobReferences struct {
+	// layers is the list of blobs that compose this image, from base layer to top layer.
+	// All layers referenced by this array will be defined in the blobs map. Some images
+	// may have zero layers.


is it valid to have zero layers and no manifest?

talked offline: yes, this could be a scratch image with inline manifest

liggitt · 2018-07-09T13:32:05Z

vendor/github.com/openshift/api/image/v1/types.go

+	// optional if the necessary information about size is not available.
+	LayerSize *int64 `json:"size" protobuf:"varint,1,opt,name=size"`
+	// MediaType of the referenced object.
+	MediaType string `json:"mediaType" protobuf:"bytes,2,opt,name=mediaType"`


is this always known?

Yes, we always know what this value is in the Image object as part of that API (since 3.5 or 3.6)

liggitt · 2018-07-09T13:34:11Z

Any other API comments? Updating that PR and will put the label on it otherwise

a couple questions, no other concerns from me

bparees

seems like there are some un-addressed comments in this PR also.

bparees · 2018-07-09T18:52:08Z

...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

The upstreams are only for testing, they'll be pulled in via a bump.

i didn't look closely to see if you pulled this in from upstream. as long as it's correct upstream, then, i guess.

bparees · 2018-07-09T20:22:09Z

test/extended/images/layers.go

+	var oc *exutil.CLI
+	var ns []string
+
+	g.AfterEach(func() {


wait for builder service account in a beforeeach

don't builds wait for builder service account?

I don't try to access anything before builder service account is available.

don't builds wait for builder service account?

well, I should have said "wait for the builder service account's secrets". (the waitforserviceaccount helper does both)

if the SA exists but the secrets don't, the build will fail (because it can't pull the base image or push the app image) We do have some logic in the build controller now that waits for build secrets in certain cases:

https://github.com/openshift/origin/blob/master/pkg/build/controller/build/build_controller.go#L525-L543

given that i'm actually not sure why we saw some of the pull issues we saw earlier, unless those were only happening on deployments, not builds.

smarterclayton · 2018-07-11T03:42:41Z

Updated on top of an API bump (i think i finally fixed glide to not be horribad)

Regressed when we started using our own logic.

Adds a new GET endpoint to an image stream as a subresource `layers` that returns an array of every layer referenced by the image stream and the tags and images included by the image. The subresource is fed by a store driven informer that caches and indexes only the layers. Clients get a 500 retry error if the cache has not initialized yet (the client will silently retry). Turns the registry access check for a given layer into an O(1) check instead of O(N) where N is the number of images in the image stream.

deads2k · 2018-07-11T12:07:01Z

Updated on top of an API bump (i think i finally fixed glide to not be horribad)

Any other changes? I made it through my review before and it lgtm

smarterclayton · 2018-07-11T14:26:55Z

Nothing new. Ben's comments were resolved

smarterclayton · 2018-07-11T16:24:33Z

/retest

deads2k · 2018-07-11T17:55:41Z

/lgtm

openshift-ci-robot · 2018-07-11T17:55:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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~~ [deads2k,smarterclayton]

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

smarterclayton · 2018-07-12T03:45:16Z

/retest

danwinship · 2018-07-12T13:11:26Z

/retest

smarterclayton · 2018-07-12T17:24:42Z

/retest

danwinship · 2018-07-12T23:11:58Z

/retest

smarterclayton · 2018-07-13T01:58:42Z

/retest

openshift-ci-robot · 2018-07-13T07:56:06Z

@smarterclayton: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/gcp	`e9536dd`	link	`/test gcp`
ci/openshift-jenkins/extended_image_ecosystem	`aac71d5`	link	`/test extended_image_ecosystem`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot requested review from dmage and tnozicka June 12, 2018 02:13

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

smarterclayton force-pushed the image_layers branch from 0480134 to fbe0541 Compare June 12, 2018 06:56

This was referenced Jun 12, 2018

Add Layers(name string) as a subresource call to image stream openshift/client-go#47

Closed

Perform an O(1) check to find layers in an image openshift/image-registry#101

Merged

sttts reviewed Jun 12, 2018

View reviewed changes

smarterclayton force-pushed the image_layers branch from fbe0541 to 47e4243 Compare June 12, 2018 15:04

smarterclayton changed the title ~~Provide layers referenced by an image stream as layers subresource~~ WIP - 3.11 - Provide layers referenced by an image stream as layers subresource Jun 12, 2018

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

smarterclayton force-pushed the image_layers branch from 47e4243 to 4744012 Compare June 13, 2018 04:19

dmage reviewed Jun 13, 2018

View reviewed changes

legionus reviewed Jun 13, 2018

View reviewed changes

legionus mentioned this pull request Jun 14, 2018

pullthrough is not optional openshift/image-registry#103

Merged

smarterclayton force-pushed the image_layers branch 6 times, most recently from 00de183 to e9536dd Compare June 23, 2018 05:18

smarterclayton force-pushed the image_layers branch from e9536dd to 360d99f Compare June 25, 2018 03:41

smarterclayton changed the title ~~WIP - 3.11 - Provide layers referenced by an image stream as layers subresource~~ Provide layers referenced by an image stream as layers subresource Jun 25, 2018

liggitt reviewed Jul 9, 2018

View reviewed changes

bparees requested changes Jul 9, 2018

View reviewed changes

smarterclayton mentioned this pull request Jul 10, 2018

bump(*): Pick up new API changes #20280

Closed

smarterclayton force-pushed the image_layers branch from 9de949f to 25a4e3b Compare July 11, 2018 03:41

smarterclayton force-pushed the image_layers branch from 25a4e3b to 57a223b Compare July 11, 2018 04:16

smarterclayton added 4 commits July 11, 2018 01:06

bump(*): Pick up new API changes

1b3d5dc

Allow the -delete-namespace flag to work on origin tests

16d3f5d

Regressed when we started using our own logic.

Add internal types for ImageStreamLayers

81e6e1c

smarterclayton force-pushed the image_layers branch from 57a223b to aac71d5 Compare July 11, 2018 05:29

openshift-ci-robot assigned deads2k Jul 11, 2018

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

deads2k mentioned this pull request Jul 11, 2018

Reduce HAProxy reloads - adds support to use the haproxy dynamic config api #19073

Merged

bparees approved these changes Jul 11, 2018

View reviewed changes

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


		var _ metav1.Object = &ImageLayers{}

		func (l *ImageLayers) GetObjectKind() schema.ObjectKind { return &metav1.TypeMeta{} }

Provide layers referenced by an image stream as layers subresource #19969

Provide layers referenced by an image stream as layers subresource #19969

Conversation

smarterclayton commented Jun 12, 2018 • edited

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

mfojtik commented Jun 12, 2018

smarterclayton commented Jun 12, 2018 via email

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 24, 2018

smarterclayton commented Jun 24, 2018

smarterclayton commented Jun 25, 2018

smarterclayton commented Jun 25, 2018

liggitt Jul 9, 2018 • edited

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

liggitt commented Jul 9, 2018

bparees left a 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 Jul 11, 2018

deads2k commented Jul 11, 2018

smarterclayton commented Jul 11, 2018

smarterclayton commented Jul 11, 2018

deads2k commented Jul 11, 2018

openshift-ci-robot commented Jul 11, 2018

smarterclayton commented Jul 12, 2018

danwinship commented Jul 12, 2018

smarterclayton commented Jul 12, 2018

danwinship commented Jul 12, 2018

smarterclayton commented Jul 13, 2018

openshift-ci-robot commented Jul 13, 2018

smarterclayton commented Jun 12, 2018 •

edited

liggitt Jul 9, 2018 •

edited