Allow importing tags from ImageStreams pointing to external registries #4853

soltysh · 2015-09-29T16:34:43Z

This is the initial version of improved importer, to address card 514.

@ncdc I know it's bit messy still, I'm working on getting it DRY-ed and most importantly fully tested, still I'd appreciate first raw pass, if this makes sense. The general flow is following:

if no tags are defined import all from default spec
if tags are defined import only the ones defined, both from default spec and from external repos

@pweil- fyi

ncdc · 2015-09-29T16:35:50Z

cc @pweil- @miminar

ncdc · 2015-09-29T16:43:12Z

pkg/image/controller/controller.go

+	return c.done(stream, "", retryCount)
+}
+
+// getDefaultTags reads all tags from the stream's default registry,


s/registry/image repository/

soltysh · 2015-10-01T14:26:46Z

I'm done with the implementation, @ncdc ptal.

ncdc · 2015-10-01T14:53:22Z

pkg/image/controller/controller.go

+// 2. spec.DockerImageRepository + tags defined - import all tags from default registry,
+//    and all the specified which (if name matches) will overwrite the default ones.
+//    Additionally:
+//    for kind == DockerImage import or reference underlying image, iow. exact tag (no provided means latest),


s/no provided/not provided/

soltysh · 2015-10-05T19:30:14Z

@ncdc comments addressed and reorganized the code per your suggestion. ptal.

[test] in the mean time.

smarterclayton · 2015-10-06T18:46:27Z

pkg/image/api/v1beta3/types.go

@@ -63,6 +63,8 @@ type NamedTagReference struct {
 	Name        string                `json:"name"`
 	Annotations map[string]string     `json:"annotations,omitempty"`
 	From        *kapi.ObjectReference `json:"from,omitempty"`
+	// Referenace states if the tag will be imported. Default value is false, which means the tag will be imported.
+	Reference bool `json:"reference,omitempty" description:"flag informing if tag is a reference (true) or is imported"`


if true consider this tag a reference only and do not attempt to import metadata about the image

ncdc · 2015-10-06T18:50:54Z

I will review this tomorrow morning when I'm back from PTO.

smarterclayton · 2015-10-06T18:51:52Z

pkg/image/controller/controller.go

+				dockerImage, err = conn.ImageByTag(ref.Namespace, ref.Name, ref.Tag)
+			}
+			switch {
+			case dockerregistry.IsRepositoryNotFound(err), dockerregistry.IsRegistryNotFound(err):


This is no longer the case where it should terminate an import. Instead, you need to make as much progress as possible and then fail at the end. It would be better to accumulate errors and continue. c.done() should not be invoked in this loop. This iteration of the loop should be extracted into a function.

We don't have any reasonable error handling for IS. Do we want to use
events for that or you're talking about just logging errors here? I agree
we should process as much as possible, but still we want to inform user
about import problems in a reasonable way.
On Oct 6, 2015 8:51 PM, "Clayton Coleman" notifications@github.com wrote:

In pkg/image/controller/controller.go
#4853 (comment):

var dockerImage *dockerregistry.Image

if image, ok := retrieved[ref.ID]; ok {

dockerImage = image

} else {

// TODO insecure applies to the stream's spec.dockerImageRepository, not necessarily to an external one!

conn, err := client.Connect(ref.Registry, insecure)

if err != nil {

return err

}

if len(ref.ID) > 0 {

dockerImage, err = conn.ImageByID(ref.Namespace, ref.Name, ref.ID)

} else {

dockerImage, err = conn.ImageByTag(ref.Namespace, ref.Name, ref.Tag)

}

switch {

case dockerregistry.IsRepositoryNotFound(err), dockerregistry.IsRegistryNotFound(err):

This is no longer the case where it should terminate an import. Instead,
you need to make as much progress as possible and then fail at the end. It
would be better to accumulate errors and continue. c.done() should not be
invoked in this loop. This iteration of the loop should be extracted into a
function.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4853/files#r41306035.

We do, it goes in the annotation. But you don't even need to do more than
report the first error.

On Tue, Oct 6, 2015 at 5:36 PM, Maciej Szulik notifications@github.com
wrote:

In pkg/image/controller/controller.go
#4853 (comment):

var dockerImage *dockerregistry.Image

if image, ok := retrieved[ref.ID]; ok {

dockerImage = image

} else {

// TODO insecure applies to the stream's spec.dockerImageRepository, not necessarily to an external one!

conn, err := client.Connect(ref.Registry, insecure)

if err != nil {

return err

}

if len(ref.ID) > 0 {

dockerImage, err = conn.ImageByID(ref.Namespace, ref.Name, ref.ID)

} else {

dockerImage, err = conn.ImageByTag(ref.Namespace, ref.Name, ref.Tag)

}

switch {

case dockerregistry.IsRepositoryNotFound(err), dockerregistry.IsRegistryNotFound(err):

We don't have any reasonable error handling for IS. Do we want to use
events for that or you're talking about just logging errors here? I agree
we should process as much as possible, but still we want to inform user
about import problems in a reasonable way.
… <#1503f1535551d805_>
On Oct 6, 2015 8:51 PM, "Clayton Coleman" notifications@github.com
wrote: In pkg/image/controller/controller.go <
https://github.com/openshift/origin/pull/4853#discussion_r41306035>: > +
var dockerImage *dockerregistry.Image > + if image, ok :=
retrieved[ref.ID]; ok { > + dockerImage = image > + } else { > + // TODO
insecure applies to the stream's spec.dockerImageRepository, not
necessarily to an external one! > + conn, err :=
client.Connect(ref.Registry, insecure) > + if err != nil { > + return err >

} > + if len(ref.ID) > 0 { > + dockerImage, err =
conn.ImageByID(ref.Namespace, ref.Name, ref.ID) > + } else { > +
dockerImage, err = conn.ImageByTag(ref.Namespace, ref.Name, ref.Tag) > + }

switch { > + case dockerregistry.IsRepositoryNotFound(err),
dockerregistry.IsRegistryNotFound(err): This is no longer the case where it
should terminate an import. Instead, you need to make as much progress as
possible and then fail at the end. It would be better to accumulate errors
and continue. c.done() should not be invoked in this loop. This iteration
of the loop should be extracted into a function. — Reply to this email
directly or view it on GitHub <
https://github.com/openshift/origin/pull/4853/files#r41306035>.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4853/files#r41325904.

0xmichalis · 2015-10-06T21:18:13Z

Is this ready for review @soltysh ?

soltysh · 2015-10-17T09:18:09Z

I'm closing and reopening to kick travis, I'll also kick the jenkins to have it retested. All of the failures are passing on my local machine.

soltysh · 2015-10-20T10:35:34Z

This seems to fail only with hack/test-end-to-end-docker.sh, the local version works as expected. Let's give a jenkins another try.

soltysh · 2015-10-20T17:34:07Z

I had to use ParseDockerImageReference instead of the initial DockerImageReferenceForStream in getTags method. ParseDockerImageReference was used in the original implementation, whereas the other one came from a discussion with @ncdc.

soltysh · 2015-10-20T19:10:53Z

re-[test] the errors are flake, I can't repro locally.

smarterclayton · 2015-10-20T22:31:34Z

The test failure in hack/test-cmd.sh may be your changes. Can you run it on a loop on your laptop and verify it completes? Did not see that failure before your changes.

soltysh · 2015-10-21T07:07:10Z

@smarterclayton I can't reproduce the error from hack/test-cmd.sh, it's working ok, I'll try to run that in the loop as you proposed, though. What worries me more is the hack/test-end-to-end-docker.sh which runs perfectly ok as hack/test-end-to-end.sh but the docker version fails on AWS. I'm working on it... stilll...

soltysh · 2015-10-21T14:05:16Z

@smarterclayton I figured out what is causing this problem. Due to my changes in the import the time needed for importing the image is a bit longer than previously. This infects the flow in registry, where if IS is missing upon ISM creation we create new IS and right after that ISM, which is getting version mismatch error: Error creating image stream mapping: imageStream \"ruby-20-centos7\" cannot be updated: the object has been modified; please apply your changes to the latest version and try again. I've talked with @miminar and @ncdc I'm working on adding a retry to the ISM creation, that should solve our problem.

soltysh · 2015-10-22T09:45:35Z

I'm waiting for #5301 to land and then I'll rebase to catch the solution to failing tests in this PR.

…s pointing to external registries.

soltysh · 2015-10-26T11:45:16Z

Rebased to pick up the solution to failing tests.

openshift-bot · 2015-10-26T11:48:52Z

Evaluated for origin test up to e496c31

soltysh · 2015-10-26T13:26:51Z

Finally, 👊 upon having 2 LGTMs:
[merge]

pweil- · 2015-10-26T13:43:45Z

🎉 🎈 🍰

openshift-bot · 2015-10-26T13:45:41Z

Evaluated for origin merge up to e496c31

Merged by openshift-bot

ncdc reviewed Sep 29, 2015
View reviewed changes

soltysh force-pushed the card514 branch from 9d8cb7a to a168f6e Compare October 1, 2015 14:25

soltysh changed the title ~~[WIP] platformmanagement_public_514 - Allow importing tags from ImageStreams pointing to external registries~~ Allow importing tags from ImageStreams pointing to external registries Oct 1, 2015

ncdc reviewed Oct 1, 2015
View reviewed changes

This was referenced Oct 2, 2015

prune images: Conform to the hacking guide #4451

Merged

Support deleting image stream status tags #3982

Merged

soltysh force-pushed the card514 branch from a168f6e to 681ebfa Compare October 5, 2015 19:29

smarterclayton reviewed Oct 6, 2015
View reviewed changes

soltysh closed this Oct 17, 2015

soltysh reopened this Oct 17, 2015

soltysh force-pushed the card514 branch from 04c69ef to 25657f6 Compare October 20, 2015 10:32

soltysh force-pushed the card514 branch 2 times, most recently from 85f449a to 5d8530f Compare October 20, 2015 17:32

soltysh mentioned this pull request Oct 21, 2015

Retry ISM save upon conflicting IS error #5301

Merged

This was referenced Oct 22, 2015

ImageStream tag improvements #3484

Closed

Adding information about reference for image stream tags and about dockerimage pointed tags openshift/openshift-docs#1104

Merged

openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015

platformmanagement_public_514 - Allow importing tags from ImageStream…

e496c31

…s pointing to external registries.

soltysh force-pushed the card514 branch from 5d8530f to e496c31 Compare October 26, 2015 11:44

openshift-bot pushed a commit that referenced this pull request Oct 26, 2015

Merge pull request #4853 from soltysh/card514

a410188

Merged by openshift-bot

openshift-bot merged commit a410188 into openshift:master Oct 26, 2015

soltysh deleted the card514 branch October 27, 2015 08:28

soltysh mentioned this pull request Nov 17, 2015

Bug1281928 - fix image stream tagging for DockerImage type images. #5932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow importing tags from ImageStreams pointing to external registries #4853

Allow importing tags from ImageStreams pointing to external registries #4853

soltysh commented Sep 29, 2015

ncdc commented Sep 29, 2015

ncdc Sep 29, 2015

soltysh commented Oct 1, 2015

ncdc Oct 1, 2015

soltysh commented Oct 5, 2015

smarterclayton Oct 6, 2015

soltysh Oct 9, 2015

ncdc commented Oct 6, 2015

smarterclayton Oct 6, 2015

soltysh Oct 6, 2015

smarterclayton Oct 6, 2015

0xmichalis commented Oct 6, 2015

soltysh commented Oct 17, 2015

soltysh commented Oct 20, 2015

soltysh commented Oct 20, 2015

soltysh commented Oct 20, 2015

smarterclayton commented Oct 20, 2015

soltysh commented Oct 21, 2015

soltysh commented Oct 21, 2015

soltysh commented Oct 22, 2015

soltysh commented Oct 26, 2015

openshift-bot commented Oct 26, 2015

soltysh commented Oct 26, 2015

pweil- commented Oct 26, 2015

openshift-bot commented Oct 26, 2015

Allow importing tags from ImageStreams pointing to external registries #4853

Allow importing tags from ImageStreams pointing to external registries #4853

Conversation

soltysh commented Sep 29, 2015

ncdc commented Sep 29, 2015

ncdc Sep 29, 2015

Choose a reason for hiding this comment

soltysh commented Oct 1, 2015

ncdc Oct 1, 2015

Choose a reason for hiding this comment

soltysh commented Oct 5, 2015

smarterclayton Oct 6, 2015

Choose a reason for hiding this comment

soltysh Oct 9, 2015

Choose a reason for hiding this comment

ncdc commented Oct 6, 2015

smarterclayton Oct 6, 2015

Choose a reason for hiding this comment

soltysh Oct 6, 2015

Choose a reason for hiding this comment

smarterclayton Oct 6, 2015

Choose a reason for hiding this comment

0xmichalis commented Oct 6, 2015

soltysh commented Oct 17, 2015

soltysh commented Oct 20, 2015

soltysh commented Oct 20, 2015

soltysh commented Oct 20, 2015

smarterclayton commented Oct 20, 2015

soltysh commented Oct 21, 2015

soltysh commented Oct 21, 2015

soltysh commented Oct 22, 2015

soltysh commented Oct 26, 2015

openshift-bot commented Oct 26, 2015

soltysh commented Oct 26, 2015

pweil- commented Oct 26, 2015

openshift-bot commented Oct 26, 2015