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.

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

Update DockerImageReferences when tagging across image streams #9238

Merged
merged 6 commits into from
Jun 13, 2016

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 9, 2016

The docker image reference to an internal image (in the local registry) should be updated during a tag, so if the original image stream is deleted pulls don't fail.

I.e.

  1. Create image stream foo
  2. Push image @1 to it
  3. Tag @1 to image stream bar
  4. Delete image stream foo
  5. Try to pull bar@1 - fails

Also adds:

  • Allow dockerImageReference on an Image to be updated
  • Add oc create imagestream
  • ImageStreamMapping should default dockerImageReference if not set
  • Add --reference to oc tag
  • Add tests

@smarterclayton
Copy link
Contributor Author

@miminar @liggitt this leads up to migrating docker image references when the user changes repository names.

Printer ObjectPrinter
}

// NewCmdCreateServiceAccount is a macro command to create a new image stream
Copy link

Choose a reason for hiding this comment

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

Outdated godoc.

@miminar
Copy link

miminar commented Jun 9, 2016

Could you please better describe commit message of "Allow mutation of image dockerImageReference"?

// New returns a new ImageStreamMapping for use with Create.
func (r *REST) New() runtime.Object {
return &api.ImageStreamMapping{}
func NewImageStreamMappingStrategy(defaultRegistry api.DefaultRegistry) Strategy {
Copy link

Choose a reason for hiding this comment

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

NewStrategy? Also the godoc doesn't match.

@miminar
Copy link

miminar commented Jun 9, 2016

We should set up a controller for fixing such cases for existing entries. It could handle a lot of other cases we don't address today:

  • post-corrections for re-deployment of docker-registry service - dockerImageReferences outdated
  • deleting tags having just references to images no longer in internal registry
  • updating of some global map of blob ↔️ repository mapping - useful for pullthrough and image pruning

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jun 9, 2016 via email

@@ -310,8 +310,8 @@ func TestUpdateResetsMetadata(t *testing.T) {
t.Errorf("unexpected docker image: %#v", image.DockerImageMetadata)
return false
}
if image.DockerImageReference != "openshift/ruby-19-centos-2" {
t.Errorf("image reference changed: %s", image.DockerImageReference)
if image.DockerImageReference == "openshift/ruby-19-centos-2" {
Copy link

Choose a reason for hiding this comment

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

Could you also check against expected value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miminar
Copy link

miminar commented Jun 10, 2016

One nit. LGTM otherwise.

@ncdc
Copy link
Contributor

ncdc commented Jun 10, 2016

Can we finally get around to removing storing image.dockerImageReference in etcd?

switch len(args) {
case 0:
return fmt.Errorf("image stream name is required")
case 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we at least have --image or something like that, which will create a non-empty ImageStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here. That's what tag does.

@smarterclayton
Copy link
Contributor Author

I think i've flipped - we want to store the image reference. It's strictly less complex than a mapping solution, and now that we have a migration story developing it's manageable.

@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 11, 2016

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4782/)

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 11, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4789/) (Image: devenv-rhel7_4363)

Reference tags are not imported, but simply linked
Enables simple creation of an image stream
Admins should be able to update dockerImageReference on an image so that
migrations can be performed via the API.
If the caller does not provide a DIR, set the DIR to be the pullthrough
location for this image stream. This enables non-registry clients to
submit images more easily, primarily for testing, but will also be used
in future registry to avoid the need to set the value.
The hasScheduled flag was being reset
If the image reference we are copying in an image stream points to the
internal registry, update the docker pull spec to the current image
stream so that deleting the original image stream has no impact.

Add more extensive tests of this behavior.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 33b455a

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 33b455a

@openshift-bot openshift-bot merged commit c02ce07 into openshift:master Jun 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants