Skip to content

Comments

WIP [DO_NOT_MERGE]: Read registry configuration from Docker daemon in new-app#10637

Closed
mfojtik wants to merge 1 commit intoopenshift:masterfrom
mfojtik:new-app-image-lookup
Closed

WIP [DO_NOT_MERGE]: Read registry configuration from Docker daemon in new-app#10637
mfojtik wants to merge 1 commit intoopenshift:masterfrom
mfojtik:new-app-image-lookup

Conversation

@mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Aug 25, 2016

This patch will use the engine-api client to get list of Docker registries (ADD_REGISTRY) configured for a Docker daemon and use that list to extended the search for the Docker image during the new-app.

In other words, if you have Dockerfile in your application source which has FROM rhel7 and you have Docker that runs with --add-registry registry.access.redhat.com option, the oc new-app will include that registry when searching for image match.

@smarterclayton I think we should do something similar in image importer (we default to docker.io and not respecting the Docker configuration).

@csrwng you might have opinions about this.

Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1369769

@mfojtik mfojtik force-pushed the new-app-image-lookup branch from 9eea00e to 296a449 Compare August 25, 2016 12:25
@csrwng
Copy link
Contributor

csrwng commented Aug 25, 2016

@mfojtik My concern would be that we're assuming that your local Docker is configured with the registries that you'd want to use in your cluster. It's highly likely that it is... just don't have a feel for a probability.

@mfojtik
Copy link
Contributor Author

mfojtik commented Aug 25, 2016

@csrwng we still search the cluster images first (if I understand that code right) and do this in a fallback. In that case, instead of just defaulting to "docker.io" which is hardcoded in importer, we at least try more registries. If there is a match, the IS created will point to that exact registry, so in case that registry does not exists in the cluster you will notice that, but we won't prevent your from creating the application because our resolution does not see registries other than "docker.io" when you don't specify the full path (which I think should be the "best practice" ;-)

Namespace: OriginNamespace,
}

// Attempt to discovert default registries
Copy link
Contributor

Choose a reason for hiding this comment

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

s/discovert/discover/

}
}
extendedTerms = append(extendedTerms, terms...)
glog.V(5).Infof("fallbacking to generic search (%q, %#+v)", precise, extendedTerms)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/fallbacking/falling back/

@mfojtik mfojtik force-pushed the new-app-image-lookup branch from 296a449 to c25ee66 Compare August 25, 2016 13:18
@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 25, 2016 via email

@mfojtik
Copy link
Contributor Author

mfojtik commented Aug 25, 2016

@smarterclayton https://bugzilla.redhat.com/show_bug.cgi?id=1369769

basically you have Dockerfile with FROM rhel7 and you run new-app locally and expect rhel7 will be resolved properly if Docker runs with -add-registry

@smarterclayton
Copy link
Contributor

First off - 90% of the systems out there will never have those aliases.
The remainder will be inconsistent. Second, if the master needs to know
about this (and for the web console, it would have to), then you'd have to
add this on the master for image import. Third, a much better workaround
is to create a "rhel" image stream in the "openshift" namespace - that
should work to satisfy the from.

Aliasing is a machine level decision, but should be a cluster level
decision. Registry remapping is also somewhat of a code smell - if admins
want to fiddle with DNS names, we have other tools that can help with
this. I'm neutral on aliasing on the master.

On Aug 25, 2016, at 10:15 AM, Michal Fojtik notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton
https://bugzilla.redhat.com/show_bug.cgi?id=1369769

basically you have Dockerfile with FROM rhel7 and you run new-app locally
and expect rhel7 will be resolved properly if Docker runs with -add-registry


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10637 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p3qbUOaQORS5iuoREdKXJAcJPIeoks5qjaNwgaJpZM4Js_5w
.

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 31, 2016

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2016
@soltysh
Copy link
Contributor

soltysh commented Jan 3, 2017

Based on previous comment, I'm closing this as this won't get fixed.

@soltysh soltysh closed this Jan 3, 2017
@mfojtik mfojtik deleted the new-app-image-lookup branch September 5, 2018 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants