Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Existing images are tagged with correct reference on push #12525

Merged
merged 1 commit into from
Jan 26, 2017
Merged

Existing images are tagged with correct reference on push #12525

merged 1 commit into from
Jan 26, 2017

Conversation

miminar
Copy link

@miminar miminar commented Jan 17, 2017

Prior to this patch, when docker pushed an image with manifest of schema 2 where the image already existed in the integrated registry, the tag event used to point to the original image stream.

With this patch, the dockerImageReference of the tag event will be rewritten to the target image stream of image stream mapping.

Resolves bz#1408993

@miminar
Copy link
Author

miminar commented Jan 17, 2017

@legionus, @soltysh PTAL

@miminar miminar added component/image component/imageregistry kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Jan 17, 2017
@miminar
Copy link
Author

miminar commented Jan 17, 2017

[test]

legionus
legionus suggested changes Jan 17, 2017
View reviewed changes
pkg/image/registry/imagestreammapping/rest_test.go
@@ -291,7 +420,7 @@ func TestAddExistingImageAndTag(t *testing.T) {
Name: "existingImage",
Namespace: "default",
},
DockerImageReference: "localhost:5000/someproject/somerepo:imageID1",
DockerImageReference: "localhost:5000/someproject/somerepo@sha256@" + testImageID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miminar Typo ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

pkg/image/registry/imagestreammapping/rest_test.go
},
DockerImageReference: "localhost:5000/someproject/somerepo:" + imageID,
DockerImageReference: "localhost:5000/somens/somerepo:" + imageID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miminar imageID is a tag ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from 12d6b8d to 25b394c Compare January 18, 2017 09:29
@miminar
Copy link
Author

miminar commented Jan 18, 2017

Yum flake in https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12971/

failed: [ci-prtest95-ig-m-ndhc] (item=[u'iproute', u'python-dbus', u'python-six', u'PyYAML', u'yum-utils']) => {"failed": true, "item": ["iproute", "python-dbus", "python-six", "PyYAML", "yum-utils"], "msg": "Failure talking to yum: rhui-client-rhel7-x86_64: Check uncompressed DB failed"}

@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from 25b394c to 5aefe9c Compare January 18, 2017 12:00
mfojtik
mfojtik reviewed Jan 18, 2017
View reviewed changes
pkg/image/registry/imagestreammapping/rest.go
@@ -64,13 +64,25 @@ func (s *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, err
tag = api.DefaultImageTag
}

if err := s.imageRegistry.CreateImage(ctx, &image); err != nil && !errors.IsAlreadyExists(err) {
imageCreateErr := s.imageRegistry.CreateImage(ctx, &image)
if imageCreateErr != nil && !errors.IsAlreadyExists(imageCreateErr) {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return imageCreateErr ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, good catch!

mfojtik
mfojtik reviewed Jan 18, 2017
View reviewed changes
pkg/image/registry/imagestreammapping/rest.go
return nil, err
}

// prefer dockerImageReference set on image for the tagEvent if the image is new
ref := image.DockerImageReference
if errors.IsAlreadyExists(imageCreateErr) && image.Annotations[api.ManagedByOpenShiftAnnotation] == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kapi.ConditionTrue ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotation is always checked against "true", kapi.ConditionTrue is capitalized.

mfojtik
mfojtik reviewed Jan 18, 2017
View reviewed changes
pkg/image/registry/imagestreammapping/rest.go
if errors.IsAlreadyExists(imageCreateErr) && image.Annotations[api.ManagedByOpenShiftAnnotation] == "true" {
// the image is managed by us and, most probably, tagged in some other image stream
// let's make the reference local to this stream
if streamRef, err := api.DockerImageReferenceForStream(stream); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to log the error case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might. I'll add a debug statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging the error now.

@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from 5aefe9c to a6932ba Compare January 18, 2017 15:41
@miminar
Copy link
Author

miminar commented Jan 18, 2017

Comments addressed.

@miminar
Copy link
Author

miminar commented Jan 19, 2017

@legionus if you have some time again, could you take another look?

soltysh
soltysh reviewed Jan 23, 2017
View reviewed changes
pkg/image/registry/imagestreammapping/rest.go
streamRef.ID = image.Name
ref = streamRef.Exact()
} else {
utilruntime.HandleError(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason to use this, instead of plain old glog?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really convinced one is more preferred than the other. In strategy and rest files under pkg/image/registry/ the use of utilruntime.HandleError() vs glog.V(?).Info is 7 to 8 .

But I'll rewrite it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from a6932ba to ebcbf19 Compare January 24, 2017 09:55
@miminar
Copy link
Author

miminar commented Jan 24, 2017

Thanks for the review. @legionus is it any better now?

@soltysh
Copy link
Contributor

soltysh commented Jan 24, 2017

LGTM

[merge]

@soltysh
Copy link
Contributor

soltysh commented Jan 24, 2017

@miminar looks like you missed this one:

# github.com/openshift/origin/pkg/image/registry/imagestreammapping
pkg/image/registry/imagestreammapping/rest.go:11: imported and not used: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/runtime" as utilruntime

@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from ebcbf19 to 119fbbe Compare January 24, 2017 13:36
@miminar
Copy link
Author

miminar commented Jan 24, 2017

@miminar looks like you missed this one:

Ouch! Fixed.

@miminar
Copy link
Author

miminar commented Jan 25, 2017

Flake #12661

Existing images are tagged with correct reference on push
4b5918e 
Prior to this patch, when docker pushed an image with manifest of
schema 2 where the image already existed in the integrated registry, the
tagevent used to point to the original image stream.

With this patch, the dockerImageReference of the tag event will be
rewritten to the target image stream of image stream mapping.

Signed-off-by: Michal Minář <[email protected]>
@miminar miminar force-pushed the existing-images-pushed-with-correct-ref branch from 119fbbe to 4b5918e Compare January 25, 2017 09:55
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 4b5918e

@openshift-bot
Copy link
Contributor

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

@miminar
Copy link
Author

miminar commented Jan 26, 2017

@soltysh can you please try to merge one more time?

@soltysh
Copy link
Contributor

soltysh commented Jan 26, 2017

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 4b5918e

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13335/) (Base Commit: 249ec86) (Image: devenv-rhel7_5786)

@openshift-bot openshift-bot merged commit 39e4f8c into openshift:master Jan 26, 2017
@miminar miminar deleted the existing-images-pushed-with-correct-ref branch April 29, 2017 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik mfojtik left review comments

@soltysh soltysh soltysh left review comments

@legionus legionus legionus approved these changes

Assignees
No one assigned
Labels
component/image component/imageregistry kind/bug Categorizes issue or PR as related to a bug. priority/P1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@miminar @soltysh @openshift-bot @mfojtik @legionus