Skip to content

Vendor distribution client v2#13375

Merged
calavera merged 3 commits intomoby:masterfrom
tiborvass:distribution-refactor
Jul 16, 2015
Merged

Vendor distribution client v2#13375
calavera merged 3 commits intomoby:masterfrom
tiborvass:distribution-refactor

Conversation

@tiborvass
Copy link
Contributor

@tiborvass tiborvass commented May 21, 2015

This patch vendors the v2 distribution registry client and uses it instead of the v2 codepath in graph/ and registry/ folders. It should not affect the v1 codepath.

Praise @dmcgowan for the good, blame me for the bad :)

Some clarifications on the value of this PR to help it get merged:

  • gets rid of the v2 distribution client code from engine. This alone is HUGE, because it means the distribution team can iterate faster on improvements on both the client and the server. It also means we will have much higher test coverage, so higher quality, less risk.
  • Abstracted v1 and v2 behind a Pusher and Puller interface in graph/ to show that those are two completely separate codepaths waiting to be merged and abstracted at a lower level (on the registry client level).
  • rewrote ping and fallback logic. This had to be redone, because of how to bootstrap the new v2 glue code. Currently, master has a ping logic that would, on EVERY USER/API REQUEST, ping a v2 endpoint, then ping a v1 endpoint if it failed, and this is what would determine what version of the registry Docker is talking to. Now with this PR, we assume v2 hence skipping the pinging v2 part, we have a (new) notion of endpoints (called APIEndpoint in the code) that we try one after the other, if the error returned by a registry API call satisfies a typed-error fallback logic detailed in registry.WrapError: so not all errors will fallback to v1. This part will be improved in the future, for instance: allow registry endpoints to determine their fallback policies.
  • Towards having one TLS logic: this code got rid of setting up certs, not on every user/api request, but on EVERY REQUEST to the registry (one user request can result in multiple registry requests). @diogomonica @NathanMcCauley please review that part, we added more constraints on ciphers. The idea for the future is to have one package that prepares ready-to-use TLSConfigs for Docker.
  • As a result of the previous two, an APIEndpoint is no longer "secure" or "insecure". It's HTTP, or HTTPS with an InsecureSkipVerify flag set to true or false. Having the HTTP endpoint be a different one than the HTTPS endpoint is cleaner in the code and easier to reason about: if HTTPS fails, try HTTP (which was added to the list of endpoints, only because the registry was marked as insecure).

Big future direction: the brightest day will be when we can get rid of the v1 code. In order to get there, we will have to refactor some more. Most importantly, we will make what many hoped this PR would do: have the code in graph talk only one API, which gets dispatched to the appropriate API version codepath. We will also get rid of much of the old code in registry/, things like Endpoint will retire.

@tiborvass tiborvass force-pushed the distribution-refactor branch from 48856bf to fabe2ec Compare May 21, 2015 01:27
@tiborvass tiborvass force-pushed the distribution-refactor branch from fabe2ec to 002e1d1 Compare May 21, 2015 01:29
@tiborvass tiborvass removed the dco/no label May 21, 2015
@tiborvass tiborvass force-pushed the distribution-refactor branch 3 times, most recently from 4b8e6e3 to 43e8c7e Compare May 21, 2015 01:43
@cpuguy83
Copy link
Member

cpuguy83 commented May 21, 2015

Test failures in /registry.

@tiborvass
Copy link
Contributor Author

tiborvass commented May 21, 2015

@cpuguy83 yeah taking care of it

@tiborvass tiborvass force-pushed the distribution-refactor branch 3 times, most recently from b08d191 to 03a1391 Compare May 21, 2015 17:32
@jessfraz
Copy link
Contributor

jessfraz commented May 21, 2015

So tbh I am kinda disappointed, I thought there was going to be this magical interface for push pull with v1 and v2 and I guess it is just not there yet. I am wondering does this add any new features that we absolutely need for 1.7, is this like a step towards the magical clean interface. what do we gain here other than vendoring registry but using the same V2 functions like we had before.

@tiborvass
Copy link
Contributor Author

tiborvass commented May 21, 2015

@jfrazelle I'm sorry to have disappointed you, we originally wanted it to have one interface for both but it turned out to be more complex than what we wanted, plus imagine how difficult it would have been to review, if here we're "just" getting rid of v2 code in engine.

So the next steps are to continue the cleanup, to eventually get to the promised land :) With promised land++ being no v1 code.

@jessfraz jessfraz mentioned this pull request May 21, 2015
Merged
@jessfraz
Copy link
Contributor

jessfraz commented May 21, 2015

no worries totally understandable

On Thu, May 21, 2015 at 3:37 PM, Tibor Vass [email protected]
wrote:

@jfrazelle https://github.com/jfrazelle I'm sorry to have disappointed
you, we originally wanted it to have one interface for both but it turned
out to be more complex than what we wanted, plus imagine how difficult it
would have been to review, if here we're "just" getting rid of v2 code in
engine.

So the next steps are to continue the cleanup, to eventually get to the
promised land :) With promised land++ being no v1 code.

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

@dmcgowan
Copy link
Member

dmcgowan commented May 21, 2015

@jfrazelle: @tiborvass and I both took a crack at implementing the v1 code using the magical v2 interface. We were not happy with the results. @tiborvass was working on an intermediate solution that would provide a uniform interface for both v1 and v2. We agreed the code change was too large to be considered a refactor and would require more testing than could be provided for the freeze date. I agree with Tibor we should phase this in and the next steps will be better.

This not only provides vendoring of distribution client and getting rid of the horrendous (and was supposed to be temporary) v2 code from registry, but also takes a step toward getting rid of the ping logic and opening the door for true separation of name and transport. This will enable more advanced use cases for mirroring and allow integration of the namespace work being done in distribution.

@tiborvass tiborvass force-pushed the distribution-refactor branch from 03a1391 to df83a22 Compare May 22, 2015 01:21
@tiborvass tiborvass added this to the 1.7.0 milestone May 22, 2015
@nakedible
Copy link
Contributor

nakedible commented May 22, 2015

This branch needs a test that forbids falling back to V1 registry for pulls with digest reference: #12879. Pull #12881 is fixing that, but this rewrites the code entirely. Also, I could not find the code path which is verifying manifest digests at all (since manifest.loadDigest is no longer used and is dead code), but I probably just missed something.

@tiborvass
Copy link
Contributor Author

tiborvass commented May 22, 2015

Thanks @nakedible will update the code

@tiborvass tiborvass force-pushed the distribution-refactor branch 4 times, most recently from 0eaa3b2 to bd8787b Compare May 22, 2015 18:00
dmcgowan
dmcgowan reviewed Jul 15, 2015
View reviewed changes
graph/push_v2.go Outdated
Copy link
Member

@dmcgowan dmcgowan Jul 15, 2015

Choose a reason for hiding this comment

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

A BlobWriter should always be closed regardless of whether Commit or Cancel is called. This comment can be removed.

Copy link
Contributor Author

@tiborvass tiborvass Jul 15, 2015

Choose a reason for hiding this comment

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

ok thanks!

@tiborvass tiborvass force-pushed the distribution-refactor branch from 204626f to cf87231 Compare July 15, 2015 22:19
@tiborvass
Copy link
Contributor Author

tiborvass commented Jul 15, 2015

Addressed comments

@dmcgowan
Copy link
Member

dmcgowan commented Jul 15, 2015

Investigating bug with auth token

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 15, 2015
@tiborvass tiborvass force-pushed the distribution-refactor branch from 26b60c4 to d2568ec Compare July 15, 2015 22:43
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 15, 2015
@tiborvass
Copy link
Contributor Author

tiborvass commented Jul 15, 2015

Added @dmcgowan's fix commit, fingers crossed.

@tiborvass tiborvass force-pushed the distribution-refactor branch 2 times, most recently from 49679c5 to 6685991 Compare July 15, 2015 23:00
@tiborvass tiborvass force-pushed the distribution-refactor branch from 6685991 to 19515a7 Compare July 16, 2015 17:13
@dmcgowan
Copy link
Member

dmcgowan commented Jul 16, 2015

LGTM

calavera added a commit that referenced this pull request Jul 16, 2015
@calavera
Merge pull request #13375 from tiborvass/distribution-refactor
5365e6b 
Vendor distribution client v2
@calavera calavera merged commit 5365e6b into moby:master Jul 16, 2015
@thaJeztah thaJeztah mentioned this pull request Jul 16, 2015
Closed
@stevvooe stevvooe mentioned this pull request Jul 16, 2015
Closed
@tiborvass tiborvass mentioned this pull request Jul 21, 2015
Merged
@willmtemple willmtemple mentioned this pull request Jul 22, 2015
Closed
@tiborvass tiborvass mentioned this pull request Jul 24, 2015
Closed
@tiborvass tiborvass deleted the distribution-refactor branch July 17, 2019 00:34
This was referenced Nov 30, 2024
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Dec 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@stevvooe stevvooe

Labels

area/distribution Image Distribution status/2-code-review

Projects

None yet

Milestone

1.8.0

Development

Successfully merging this pull request may close these issues.