build: take referencePolicy into account when resolving istag #12767

mfojtik · 2017-02-02T08:54:55Z

@miminar @bparees just a WIP (will return to this after meetings.,..)

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1412029

mfojtik · 2017-02-02T08:55:00Z

[test]

miminar

Please, handle "ImageStreamImage" as well. It's more common in cluster.

If the image is tagged exactly once (or even multiple times) in the image stream with local resolve policy, it shall be honored.

mfojtik · 2017-02-02T13:17:30Z

pkg/build/generator/generator_test.go

+					Status: imageapi.ImageStreamStatus{
+						DockerImageRepository: originalImage,
+						Tags: map[string]imageapi.TagEventList{
+							"tag1": {


@bparees this looks ugly but I haven't found a better way to inject the tag the trigger expects to change :-)

mfojtik · 2017-02-02T13:19:04Z

pkg/build/generator/generator_test.go

@@ -1421,7 +1453,7 @@ func TestResolveImageStreamRef(t *testing.T) {
 				Name: imageRepoName + ":" + tagName,
 			},
 			expectedSuccess:   true,
-			expectedDockerRef: latestDockerReference,
+			expectedDockerRef: dockerReference,


@bparees this one is weird... you are trying to resolve the "foo:test" to "foo:latestDockerReference", can you or somebody explain the intention of this test?

we're trying to resolve the image stream tag "testRepo:test" to the docker image reference "latestDockerReference"

the intent being to prove we properly convert imagestreamtags (which builds can't consume) to dockerimagereferences (which they can)

Since you've switched from using the mocked GetImageStreamTagFunc to the GetImageStreamFunc, you're getting different behavior. The GetImageStreamTag func always returned a DockerImageRef of "latestDockerReference" but the GetImageStreamFunc returns two tags, one for dockerReference and one for latestDockerReference. So your change is probably fine.

mfojtik · 2017-02-02T13:20:36Z

@miminar added handing for ImageStreamImage as well by lookup to images and finding the appropriate tag. PTAL.

miminar · 2017-02-02T13:32:47Z

pkg/build/generator/generator.go

+			return "", err
+		}
+		tag := ""
+		for tagName, imageList := range stream.Status.Tags {


This will pick up tags in random order, which means I can have different results for two runs on exact same ImageStream.

If I have an image tagged under is:tag somewhere in an ancient history and I re-tag it again as is:latest and I update the tag with:

oc patch istag/is:latest -p '{"tag":{"referencePolicy":{"type":"Local"}}}'

I'd probably expect that all the new build(configs) will resolve it as a local image.

With this code, I get completely random result.

miminar · 2017-02-02T13:40:04Z

pkg/build/generator/generator_test.go

+							"tag1": {
+								Items: []imageapi.TagEvent{
+									{
+										DockerImageReference: "ref@" + name + ":tag1",


This doesn't look like a valid reference. Is it on purpose?

"ref/"+name+":tag1" is saner.

This doesn't look like a valid reference. Is it on purpose?

@mfojtik bump on this question.

@bparees @miminar i think it was like this before, I can fix it to reflect the reality

bparees · 2017-02-02T18:26:18Z

lgtm aside from @miminar's question about the tag ref.
[testextended][extended:core(builds)]

bparees · 2017-02-02T20:44:54Z

extended test failure "Extended.[builds][Slow] update failure status Build status failed assemble runtime artifacts should contain the failure reason related to failing to fetch runtime image artifacts" is a known flake.

#12341

mfojtik · 2017-02-06T11:24:27Z

@miminar @bparees PTAL (this should be ready to go)

bparees · 2017-02-06T12:54:55Z

lgtm, will merge after extended tests pass.

miminar · 2017-02-06T13:09:44Z

pkg/build/generator/generator.go

+			glog.V(2).Info(err)
+			return "", err
+		}
+		reference, ok := imageapi.ResolveLatestTaggedImage(stream, tag)


This is wrong. Returned reference may be for completely different image than requested.

Imagine image stream is with a tag having two images in history:

"latest": { "items": [ "image": "a", "image": "b" ]}

Now this is called with a reference to image stream tag is:b. The imageapi.LatestImageTagEvent returns the right tag event with "image": "b". However, this call takes the tag, looks into is and takes the first image a. So the returned reference contains image name a instead of b.

oh you're right I forgot about this case, thanks.

@miminar you mean to say that the "tag" argument to ResolveLatestTaggedImage is being effectively ignored? That wasn't my interpretation of how ResolveLatestTaggedImage works, but if it is that sounds like bad behavior for the helper function and not aligned with the godoc which says:

ResolveLatestTaggedImage returns the appropriate pull spec for a given tag in the image stream, handling the tag's reference policy if necessary to return a resolved image.

Ok nm, I see the scenario you're describing is more subtle than what I was thinking.

@miminar added special function to handle ImageStreamImage. In that case we try to find the image stream that references this image and also lookup the "tag" which the given image exists. If there is matching tag event, we use the docker reference from it and read the referencePolicy...

miminar

Need to fix ImageStreamImage handling.

miminar · 2017-02-07T08:28:12Z

pkg/image/api/helper.go

+// DockerImageReferenceForImage returns the docker reference for specified image. Assuming
+// the image stream contain the image and the image has corresponding tag, this function
+// will try to find this tag and take the reference policy into the account.
+// The the image stream does not reference the image or the image does not have


miminar · 2017-02-07T08:28:47Z

pkg/image/api/helper.go

@@ -610,6 +634,34 @@ func ResolveLatestTaggedImage(stream *ImageStream, tag string) (string, bool) {
 	}
 }

+// DockerImageReferenceForImage returns the docker reference for specified image. Assuming
+// the image stream contain the image and the image has corresponding tag, this function


miminar · 2017-02-07T09:03:30Z

pkg/image/api/helper.go

+	}
+	switch ref.ReferencePolicy.Type {
+	case LocalTagReferencePolicy:
+		ref, err := ParseDockerImageReference(event.DockerImageReference)


Use stream.Status.DockerImageRepository as a base reference. Override its ID and unset its Tag.

mfojtik · 2017-02-07T10:19:12Z

@miminar thanks! all fixed, ptal.

openshift-bot · 2017-02-07T10:21:19Z

Evaluated for origin test up to 6f33f96

openshift-bot · 2017-02-07T10:21:27Z

Evaluated for origin testextended up to 6f33f96

miminar

LGTM. Some unit test for ImageStreamImage handling would be nice though.

mfojtik · 2017-02-07T10:47:22Z

@miminar will add as follow up.

[merge]

openshift-bot · 2017-02-07T10:53:24Z

Evaluated for origin merge up to 6f33f96

openshift-bot · 2017-02-07T11:44:38Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13648/) (Base Commit: 4ec486d)

openshift-bot · 2017-02-07T11:53:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13650/) (Base Commit: feb429e) (Image: devenv-rhel7_5868)

openshift-bot · 2017-02-07T12:21:21Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1070/) (Base Commit: 4ec486d) (Extended Tests: core(builds))

mfojtik force-pushed the resolve-image-by-reference branch from 3579a13 to a3d4df7 Compare February 2, 2017 08:56

miminar suggested changes Feb 2, 2017

View reviewed changes

mfojtik force-pushed the resolve-image-by-reference branch from a3d4df7 to d1b7d03 Compare February 2, 2017 13:16

mfojtik commented Feb 2, 2017

View reviewed changes

miminar suggested changes Feb 2, 2017

View reviewed changes

mfojtik force-pushed the resolve-image-by-reference branch from d1b7d03 to 5e8076b Compare February 2, 2017 16:05

bparees self-assigned this Feb 2, 2017

mfojtik force-pushed the resolve-image-by-reference branch from 5e8076b to 7e1c633 Compare February 6, 2017 11:24

miminar reviewed Feb 6, 2017

View reviewed changes

miminar suggested changes Feb 6, 2017

View reviewed changes

mfojtik force-pushed the resolve-image-by-reference branch from 7e1c633 to fdc60f5 Compare February 6, 2017 15:47

miminar suggested changes Feb 7, 2017

View reviewed changes

build: take referencePolicy into account when resolving istag

6f33f96

mfojtik force-pushed the resolve-image-by-reference branch from fdc60f5 to 6f33f96 Compare February 7, 2017 10:19

miminar approved these changes Feb 7, 2017

View reviewed changes

openshift-bot merged commit 37ef206 into openshift:master Feb 7, 2017

mfojtik deleted the resolve-image-by-reference branch September 5, 2018 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: take referencePolicy into account when resolving istag #12767

build: take referencePolicy into account when resolving istag #12767

mfojtik commented Feb 2, 2017 •

edited

Loading

mfojtik commented Feb 2, 2017

miminar left a comment

mfojtik Feb 2, 2017

mfojtik Feb 2, 2017

bparees Feb 2, 2017 •

edited

Loading

bparees Feb 2, 2017

mfojtik commented Feb 2, 2017

miminar Feb 2, 2017

miminar Feb 2, 2017

bparees Feb 3, 2017

mfojtik Feb 6, 2017

mfojtik Feb 6, 2017

bparees commented Feb 2, 2017

bparees commented Feb 2, 2017

mfojtik commented Feb 6, 2017

bparees commented Feb 6, 2017

miminar Feb 6, 2017

mfojtik Feb 6, 2017

bparees Feb 6, 2017

bparees Feb 6, 2017

mfojtik Feb 6, 2017

miminar left a comment

miminar Feb 7, 2017

miminar Feb 7, 2017

miminar Feb 7, 2017

mfojtik commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

miminar left a comment

mfojtik commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017 •

edited

Loading

openshift-bot commented Feb 7, 2017

build: take referencePolicy into account when resolving istag #12767

build: take referencePolicy into account when resolving istag #12767

Conversation

mfojtik commented Feb 2, 2017 • edited Loading

mfojtik commented Feb 2, 2017

miminar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Feb 2, 2017

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 commented Feb 2, 2017

bparees commented Feb 2, 2017

mfojtik commented Feb 6, 2017

bparees commented Feb 6, 2017

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

miminar 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

mfojtik commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

miminar left a comment

Choose a reason for hiding this comment

mfojtik commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017

openshift-bot commented Feb 7, 2017 • edited Loading

openshift-bot commented Feb 7, 2017

mfojtik commented Feb 2, 2017 •

edited

Loading

bparees Feb 2, 2017 •

edited

Loading

openshift-bot commented Feb 7, 2017 •

edited

Loading