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

Extra Path Added to Docker Image Name After App Creation #6570

Closed
stevenpall opened this issue Jan 7, 2016 · 14 comments · Fixed by #6777
Closed

Extra Path Added to Docker Image Name After App Creation #6570

stevenpall opened this issue Jan 7, 2016 · 14 comments · Fixed by #6777

Comments

@stevenpall
Copy link

After running new-app with a Docker image as the parameter, the app gets created, but deployment fails due to the Docker image name being incorrect. /library is added to the path, changing an image name from my.registry.com/test to my.registry.com/library/test. The deployment fails as a result since the latter image does not exist on the remote registry.

oc new-app my.registry.com/test
--> Found Docker image 386c08c (6 weeks old) from my.registry.com for "my.registry.com/test:latest"
    * This image will be deployed in deployment config "test"
--> Creating resources with label app=test ...
    DeploymentConfig "test" created
    Service "test" created
--> Success

oc status
In project test on server https://10.101.7.110:8443

svc/test - 172.30.100.49
  dc/test deploys my.registry.com/library/test:latest
    #1 deployment running for 18 seconds - 1 pod

I thought this might be the same issue as #6516, but the problem there was that the private registry required authentication (mine does not). I should note that I'm using the integrated Docker image (https://hub.docker.com/r/openshift/origin/) for this testing.

@csrwng
Copy link
Contributor

csrwng commented Jan 7, 2016

@ncdc @pweil- same issue that was brought up elsewhere... when parsing an image name, we're adding a library namespace by default.

@pweil-
Copy link
Contributor

pweil- commented Jan 7, 2016

cc @soltysh @miminar

@bparees
Copy link
Contributor

bparees commented Jan 7, 2016

@csrwng when you say "we", is this common code new-app is calling, or is new-app doing this?

@csrwng
Copy link
Contributor

csrwng commented Jan 7, 2016

common code new-app is calling in the registry client

@stevenpall
Copy link
Author

Has the issue here been identified? This is a fairly big blocker for our testing of Origin.

@bparees
Copy link
Contributor

bparees commented Jan 13, 2016

@stevenpall can you explain why this is a blocker for you? You can work around it by simply adding a namespace to your docker tagging process, which you should probably be doing anyway.

@stevenpall
Copy link
Author

@bparees I suppose that would be possible, though I'm confused as to why a namespace (I'm assuming you mean the /library bit) would get added when a full Docker image name is being supplied to new-app. It's a blocker in that any invocation of new-app will fail (since Docker can't pull the image) until the pod config's image name is edited manually.

I also haven't really seen that convention before; typically an image name would consist of <registry>/<application_name>:<tag>. What purpose would the extra namespace serve in this case?

@ncdc
Copy link
Contributor

ncdc commented Jan 13, 2016

@stevenpall this is because of required behavior when interacting with certain images on the Docker Hub. If you try to do something like docker pull centos, this is actually translated to a pull of library/centos because on the Hub, every image repository (centos) has to live in a namespace (library).

It looks like this maybe should only apply to images coming from the Hub. Perhaps we could update our code to only prefix with library when the registry is the Hub. WDYT @smarterclayton?

@smarterclayton
Copy link
Contributor

There were several changes recently that stopped changing the user provided value (using docker image reference .Exact() instead of String()). I expect most of that will be fixed in 1.1.1.

@bparees
Copy link
Contributor

bparees commented Jan 13, 2016

@smarterclayton where were those changes made? (trying to understand if new-app needs to be changing, or it's going to pick up that change for free)

@smarterclayton
Copy link
Contributor

new-app was what was changed (at some point recently)

On Wed, Jan 13, 2016 at 10:04 AM, Ben Parees notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton where were those
changes made? (trying to understand if new-app needs to be changing, or
it's going to pick up that change for free)


Reply to this email directly or view it on GitHub
#6570 (comment).

@csrwng
Copy link
Contributor

csrwng commented Jan 13, 2016

New-app calls the imapi.ParseDockerImageRef function:
https://github.com/openshift/origin/blob/master/pkg/generate/app/imageref.go#L50
which adds the library namespace:
https://github.com/openshift/origin/blob/master/pkg/image/api/helper.go#L64

@smarterclayton
Copy link
Contributor

Sorry, yes, for that we need to have the fix to stop setting library, and
make sure we serialize that back correctly (using the same rules). Before
we fixed Exact() usage, this didn't matter because library got appended
anyway.

On Wed, Jan 13, 2016 at 10:50 AM, Cesar Wong notifications@github.com
wrote:

New-app calls the imapi.ParseDockerImageRef function:

https://github.com/openshift/origin/blob/master/pkg/generate/app/imageref.go#L50
which adds the library namespace:
https://github.com/openshift/origin/blob/master/pkg/image/api/helper.go#L64


Reply to this email directly or view it on GitHub
#6570 (comment).

@bparees
Copy link
Contributor

bparees commented Jan 13, 2016

@smarterclayton sorry, still not following at which point in the code we want to fix this.

is https://github.com/openshift/origin/blob/master/pkg/image/api/helper.go#L64 going to be updated to not set library? If so, by whom and when?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants