Proposal to add DockerImageReference to ImageStreamTag

[soltysh]

This idea came from tackling https://bugzilla.redhat.com/show_bug.cgi?id=1268000. The general flow is as follows:

1. Image import controller imports images in non-deterministic order, which for v1 registries with multiple tags pointing to the same id means, that the first seen image will be imported, all the others will always point to this one. This also means that the Image.DockerImageReference will always point to that first imported image.
2. First build occurs, triggered by ConfigChange trigger, this build saves the DockerImageReference returned from Image.DockerImageReference (GetImageStreamTag call in generator.go).
3. ImageChange controller notices change to ruby:2.0 image, resolves that, but since it's reading straight from stream, it uses LatestTaggedImage(stream, tag) which returns TagEvent which has DockerImageReference that might be different from Image.DockerImageReference from 1 and might sometimes trigger the 2nd build. Usually the DockerImageReference in TagEvent is similar to what was specified in image spec.

There are two solutions to this problem:

1. Rework resolveImageStreamReference so that for ImageStreamTag it reads the stream and then uses LatestTaggedImage(stream, tag) to get the same value we do in ImageChange controller (step 3).
2. Extend ImageStreamTag with DockerImageReference, especially that under the cover (see imagestreamtag.Get is using LatestTaggedImage(stream, tag) and has that information.

My preference, as the title points to, is no 2. Because this will also fix the problem of getting proper information when doing oc get istag/ruby:2.0. Additionally we'll have consistent behavior all over the places. The former solution is a quick fix for this particular place and not the whole problem. When I was talking to @ncdc he also mentioned removing (or at least trying to) Image.DockerImageReference field, which the latter could help with. This would also allow better address the problem in #5391 where we return 404 for tags that are references. We could return empty image, but still provide valid metadata information about it.

@miminar @ncdc @bparees @smarterclayton opinions?

[miminar]

This would also allow better address the problem in #5391 where we return 404 for tags that are references.

To fill DockerImageReference, an image digest/ID is needed. Which is known after being imported.
As @smarterclayton pointed out, initial tag events may look like this:

  status:
    dockerImageRepository: ""
    tags:
    - items:
      - created: 2015-10-26T01:41:23Z
        dockerImageReference: openshift/origin:v1.0.6
        image: ""
      tag: latest

Note the image entry.

Can't we import latest image as first unless particular tag is requested?

[soltysh]

To fill DockerImageReference, an image digest/ID is needed.

Not necessarily, we have that information from TagEvent returned here. It's that we used in certain places and in the others we don't, thus the above problem I'm describing. Making this consistent across all the calls should simplify things.

[smarterclayton]

ImageStreamTag already returns dockerImageReference?

On Thu, Oct 29, 2015 at 6:23 AM, Maciej Szulik notifications@github.com
wrote:

To fill DockerImageReference, an image digest/ID is needed.

Not necessarily, we have that information from TagEvent returned here
https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestreamtag/rest.go#L64.
It's that we used in certain places and in the others we don't, thus the
above problem I'm describing. Making this consistent across all the calls
should simplify things.

—
Reply to this email directly or view it on GitHub
#5481 (comment).

[miminar]

@soltysh If I oc create -f installable.yaml. oc get istag/installable:latest gets me Error from server: imageStreamTag "installable:latest" not found until an image is imported. That's because api.LatestTaggedImage returns nil event. I expected it to return event with Image empty instead but import/conversion machinery deleted those events.

What I'm trying to say is that you may have both Image and DockerImageReference empty when an image stream exists but its image does not. In such a case 404 would be correct answer if no such tag exists in Spec.Tags, empty Image (and optionally DockeImageReference) might be correct otherwise.

[ncdc]

@smarterclayton

ImageStreamTag already returns dockerImageReference?

It does, but it's embedded in the Image. And as we know, the Image's DockerImageReference is only ever the first pull spec it ever saw for that image id. And with v1 images where multiple tags may point to the same id, this is problematic because you may get the "wrong" pull spec for the ImageStreamTag.

[ncdc]

@miminar

That's because api.LatestTaggedImage returns nil event. I expected it to return event with Image empty instead but import/conversion machinery deleted those events.

I don't think it's returning a nil event. I think it's returning an event whose Image is empty. But the end result in your example is the same - you get a 404.

I like what @soltysh is proposing. Here's where things break down currently. If I oc get is/foo -o yaml, you could have this in status.tags[latest].items[0]:

1. dockerImageReference = openshift/foo:latest
2. image = 6e84ab2bc62488cda1a6219499b999d44dd0394dba352e0e60d14b0ecdb0d1a8

Now, if you oc get image/6e84ab2bc62488cda1a6219499b999d44dd0394dba352e0e60d14b0ecdb0d1a8, you might get:

1. dockerImageReference = openshift/foo:v1.2.3

In this case, both tags v1.2.3 and latest point to the same v1 image id. So if you get the ImageStreamTags for both tags, you end up with the same Image and the single DockerImageReference of openshift/foo:v1.2.3. This is a problem because you really want the DockerImageReference for oc get istag/foo:latest to be openshift/foo:latest, not openshift/foo:v1.2.3.

Adding a DockerImageReference as a top-level field to ImageStreamTag would allow clients to see the value from imageStream.status.tags[<tag>].items[0].dockerImageReference, which is the correct pull spec and currently missing from the response.

[ncdc]

And this doesn't attempt to address the 404 issue... that's separate.

[soltysh]

And this doesn't attempt to address the 404 issue... that's separate.

The 404 can leverage that field to provide metadata information about the image (including the pull spec), but still with empty Image field in ImageStreamTag. This is specifically important in the case where reference is True where the Image field will always be empty, thus we'll always get 404 for referenced images. But the tag is there.

[ncdc]

Good point

[smarterclayton]

The value you describe is what I would expect imagestreamtag to have set on
it right now - DockerImageReference should only ever be a pointer to a
location in a registry to pull from.

On Thu, Oct 29, 2015 at 9:38 AM, Maciej Szulik notifications@github.com
wrote:

And this doesn't attempt to address the 404 issue... that's separate.

The 404 can leverage that field to provide metadata information about the
image (including the pull spec), but still with empty Image field in
ImageStreamTag. This is specifically important in the case where reference
is True where the Image field will always be empty, thus we'll always get
404 for referenced images. But the tag is there.

—
Reply to this email directly or view it on GitHub
#5481 (comment).

[smarterclayton]

Actually, " imageStream.status.tags[].items[0].dockerImageReference"
is not the correct pull spec - how are they different?

On Thu, Oct 29, 2015 at 9:43 AM, Clayton Coleman ccoleman@redhat.com
wrote:

The value you describe is what I would expect imagestreamtag to have set
on it right now - DockerImageReference should only ever be a pointer to a
location in a registry to pull from.

On Thu, Oct 29, 2015 at 9:38 AM, Maciej Szulik notifications@github.com
wrote:

And this doesn't attempt to address the 404 issue... that's separate.

The 404 can leverage that field to provide metadata information about the
image (including the pull spec), but still with empty Image field in
ImageStreamTag. This is specifically important in the case where
reference is True where the Image field will always be empty, thus we'll
always get 404 for referenced images. But the tag is there.

—
Reply to this email directly or view it on GitHub
#5481 (comment).

[soltysh]

Per discussion on IRC, we've settled that the dockerImageReference will be overridden to the value from TagEvent. So no new field. Will work on a PR.

[ncdc]

For clarification, the field to override is ImageStreamTag.Image.DockerImageReference

[liggitt]

Pretty sure the UI uses this to correlate builds, deployments, and image streams:
https://github.com/openshift/origin/blob/master/assets/app/scripts/services/imageStreamResolver.js#L58

Will the proposed change affect this?

[ncdc]

@liggitt I don't believe so. That function operates on ImageStream.status data.