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

Use api for pulling images instead of shelling out #351

Merged
merged 2 commits into from Jul 8, 2014

Conversation

discordianfish
Copy link
Contributor

I've just copied the functions to parse and validate the repo name from Docker because otherwise this would require including the complete Docker sources.

/cc @brendandburns

@thockin
Copy link
Member

thockin commented Jul 3, 2014

I have two questions

  1. Does the API correctly handle pulling an image that is already being pulled? The CLI recognizes this and blocks the caller until it is done, which is simple to handle.

  2. Why is this better than shelling out? Other than cleanliness, shelling out has the nice property that it is less code. I have this same question in general - why bother with the API and the library deps to use it, when I can just assemble a commandline and exec it?

@brendandburns
Copy link
Contributor

Personally I like using the api, since it provides a structured response,
and higher granularity input.

It also provides better future proofing. APIs have deprecation policies,
etc. CLI flags and options, not so much.

I haven't looked at the PR (on my phone) but can we make certain that the
bits of code derived from the docker code, are clearly marked and isolated
in a separate file, with a clear copyright header?

Thanks
Brendan
On Jul 3, 2014 8:27 AM, "Tim Hockin" notifications@github.com wrote:

I have two questions

  1. Does the API correctly handle pulling an image that is already being
    pulled? The CLI recognizes this and blocks the caller until it is done,
    which is simple to handle.

  2. Why is this better than shelling out? Other than cleanliness, shelling
    out has the nice property that it is less code. I have this same question
    in general - why bother with the API and the library deps to use it, when I
    can just assemble a commandline and exec it?


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

@smarterclayton
Copy link
Contributor

Actually, with Docker the CLI deprecation policy has been much stronger than the API deprecation policy. With 1.0 in place the API now has a much stronger change policy, but I would say they're equally stable now.

@discordianfish
Copy link
Contributor Author

Ok, I discussed that parsing in #docker-dev and it seems it's not necessary to do that because the 'fromImage' parameter (which is called, a bit misfortune, 'Repository' in go-dockerclient) gets parsed server side for repo and so on. So I've removed the parsing.

The docker API now is stable, nobody should run docker < 1.0 already because the security fixes in 1.0.
Regarding the parallel pulls, I will look into that tomorrow. I also need to verify that the fromImage parameter can include tags or whether we need to parse the tag from the name first. So hold off with merging for now.

@discordianfish
Copy link
Contributor Author

So the parallel pulls should behave the same: It blocks until the pull finished, then returns:
https://github.com/dotcloud/docker/blob/master/server/server.go#L1365

But the tags need to provided explicitly. So if Pull(image string) should support tags (user/image:tag), we need to either parse the tag or extend kubernetes container structs to support a tags attribute.

Parsing the tag is rather ugly because we basically need to parse everything which, as you saw earlier, is pretty complex.

So to me the right way to solve this is either:

  1. Make Docker parse the tag from the string
  2. Add tag to kubernetes container representation

What do you think?

@thockin
Copy link
Member

thockin commented Jul 5, 2014

Where do you see tag in the docker API? Maybe I'm looking in the wrong
place - I don't see them mentioned at all..

I'm in favor of (1) - this is not something the client should be expected
to parse, I think.

On Fri, Jul 4, 2014 at 5:55 AM, discordianfish notifications@github.com
wrote:

So the parallel pulls should behave the same: It blocks until the pull
finished, then returns:
https://github.com/dotcloud/docker/blob/master/server/server.go#L1365

But the tags need to provided explicitly. So if Pull(image string) should
support tags (user/image:tag), we need to either parse the tag or extend
kubernetes container structs to support a tags attribute.

Parsing the tag is rather ugly because we basically need to parse
everything which, as you saw earlier, is pretty complex.

So to me the right way to solve this is either:

  1. Make Docker parse the tag from the string
  2. Add tag to kubernetes container representation

What do you think?

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

@discordianfish
Copy link
Contributor Author

@thockin See "Create an image" here: http://docs.docker.com/reference/api/docker_remote_api_v1.12/#22-images
But the API docs are a bit misleading in that one, so I've created moby/moby#6837

And to me it also looks like the server should parse that, given it parses all other parameters from the name as well. But maybe there are reasons for not doing it. I'll discuss that, but if this gets changed kubernetes would require at least some future version of Docker. Would that be okay? Or should this be rather backward compatible? In that case we still need to parse the tag to support older docker daemons.

@discordianfish
Copy link
Contributor Author

@thockin I've opened moby/moby#6876 to track adding parsing of the tag to the daemon. But since it probably make sense to support already existing Docker versions, I've added the parsing to kubernetes for :). See updated code.

@thockin
Copy link
Member

thockin commented Jul 7, 2014

I find no "Creating" anywhere on the page at
http://docs.docker.com/reference/api/docker_remote_api_v1.12/#22-images

?

On Mon, Jul 7, 2014 at 3:26 AM, discordianfish notifications@github.com
wrote:

@thockin https://github.com/thockin See "Create an image" here:
http://docs.docker.com/reference/api/docker_remote_api_v1.12/#22-images
But the API docs are a bit misleading in that one, so I've created
moby/moby#6837 moby/moby#6837

And to me it also looks like the server should parse that, given it parses
all other parameters from the name as well. But maybe there are reasons for
not doing it. I'll discuss that, but if this gets changed kubernetes would
require at least some future version of Docker. Would that be okay? Or
should this be rather backward compatible? In that case we still need to
parse the tag to support older docker daemons.

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

@discordianfish
Copy link
Contributor Author

@thockin Not "Creating", but "Create" :) But yes it's rather hard to navigate the docs..

@@ -335,6 +330,26 @@ func makePortsAndBindings(container *api.Container) (map[docker.Port]struct{}, m
return exposedPorts, portBindings
}

// Parses image name including an tag and returns image name and tag .
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a "TODO" comment explaining that we don'tr really want to be doing this and pointing at the docker issues?

thockin added a commit that referenced this pull request Jul 8, 2014
Use api for pulling images instead of shelling out
@thockin thockin merged commit 92cf666 into kubernetes:master Jul 8, 2014
@discordianfish discordianfish deleted the use-api-for-pull branch July 8, 2014 17:00
vishh pushed a commit to vishh/kubernetes that referenced this pull request Apr 6, 2016
Add a validation handler to check cAdvisor's underlying dependencies.
keontang pushed a commit to keontang/kubernetes that referenced this pull request May 14, 2016
Change apiserver setup for baremetal cluster.
keontang pushed a commit to keontang/kubernetes that referenced this pull request Jul 1, 2016
Change apiserver setup for baremetal cluster.
harryge00 pushed a commit to harryge00/kubernetes that referenced this pull request Aug 11, 2016
Change apiserver setup for baremetal cluster.
mqliang pushed a commit to mqliang/kubernetes that referenced this pull request Dec 8, 2016
Change apiserver setup for baremetal cluster.
mqliang pushed a commit to mqliang/kubernetes that referenced this pull request Mar 3, 2017
Change apiserver setup for baremetal cluster.
wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020
Add testing script to invoke example testing
marun pushed a commit to marun/kubernetes that referenced this pull request Sep 17, 2020
b3atlesfan pushed a commit to b3atlesfan/kubernetes that referenced this pull request Feb 5, 2021
*: add license header test + travis badge
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.

None yet

4 participants