V2 mirror support #13374

RichardScothern · 2015-05-21T00:55:00Z

The v2 registry will act as a pull-through cache, and needs to be
handled differently by the client to the v1 registry mirror.

See distribution/distribution#459 for details

Configuration

Only one v2 registry can be configured as a mirror. Acceptable configurations
in this chanage are: 0...n v1 mirrors or 1 v2 mirror. A mixture of v1 and v2
mirrors is considered an error.

Pull

If a v2 mirror is configured, all pulls are redirected to that mirror. The
mirror will serve the content locally or attempt a pull from the upstream mirror,
cache it locally, and then serve to the client.

Push

If an image is tagged to a mirror, it will be pushed to the mirror and be
stored locally there. Otherwise, images are pushed to the hub. This is
unchanged behavior.

This change should not affect the v1 codepath.

closes distribution/distribution#84
Connects to distribution/distribution#19, distribution/distribution#459

stevenschlansker · 2015-05-21T18:21:34Z

I strongly disagree with "Only one v2 registry can be configured as a mirror."
Having multiple fallback mirrors is incredibly useful, as it allows much easier HA without complicated server configuration. It also allows multi-datacenter availability of registries which is even harder to configure on the server side.

Compare configuring your daemon as --add-mirror mycorp-mirror-uswest.mycorp.com --add-mirror mycorp-mirror-useast.mycorp.com versus attempting to create a highly available server side mirror spanning the US west and east coasts.

This was originally filed in #9161

RichardScothern · 2015-05-21T19:10:58Z

@stevenschlansker . I appreciate your point, but you can get the same effect with a load balancer pointing to your two mirror instances without too much complication.

Putting load balancing code in the client which deals with all edge cases would complicate this feature and is outside the scope of this change.

dmp42 · 2015-05-21T19:11:54Z

@RichardScothern agreed.

Thanks @stevenschlansker
Like I said in the other ticket, it's certainly a great idea for the future, and I think it can be implemented as a compatible extension later on, on top of this.

It's still going to make the code more complex, and needs to be thought through (specifically with regard to delegated delivery of layers and failure handling at that level).

jessfraz · 2015-05-21T22:38:52Z

is this dependent on #13375

stevvooe · 2015-05-22T01:32:56Z

This is looking good. We may need a few tests.

dmcgowan · 2015-05-22T20:28:54Z

graph/pull.go

+		return v2MirrorEndpoint, v2MirrorRepoInfo, nil
+	}
+	if v2MirrorEndpoint != nil && v1MirrorCount > 0 {
+		lastErr = fmt.Errorf("v1 and v2 mirrors configured")


Why is this an error?

Since the behavior's are so different, I don't see how mixing v1 and v2 mirrors would work well. We can avoid this by having backwards compatibility (just v1 mirrors) or new behavior (a v2 mirror)

dmcgowan · 2015-05-22T20:36:58Z

I have some concerns about the change of behavior from v1 mirrors. The v1 mirrors were only intended to be used for mirroring the official index. While this is an absolutely horrendous feature and requirement, it does have some relevance here. Without sending the hostname as part of the repo name, the mirror will not know which registry to fetch the content from. The assumption based on the current flag is that the content would come from the official index. If you try and send the hostname as part of the reponame, I believe the current path regexs will choke.

How would the mirror behave between these two requestss

docker pull myregistry.com/dmcgowan/myapp
docker pull dmcgowan/myapp

Are there any instructions on how to test this feature?

tiborvass · 2015-05-22T20:40:15Z

graph/pull.go

+		registry.DockerHeaders(imagePullConfig.MetaHeaders)...,
+	)
+	client := registry.HTTPClient(tr)
+	mirrorSession, err := registry.NewSession(client, imagePullConfig.AuthConfig, mirrorEndpoint)


@RichardScothern did you verify whether this will send credentials to the mirror?

This will send credentials, this needs to clear out the AuthConfig

RichardScothern · 2015-05-22T21:13:43Z

@dmcgowan : the behavior does change between registry versions. Hence the fallback to v1 behavior with v1 mirrors and the inability to mix v1 and v2. In your scenario the registry would have to deal with a hostname and implement behavior on checking upstream for myregistry.com

Testing this feature is a bit tricky since we don't have the implementation in the v2 registry yet. I was testing with a pre-loaded v2 registry as a mirror.

dmcgowan · 2015-05-23T01:10:48Z

@RichardScothern Created a PR against your fork to make it official only. If you don't have time to merge it I will carry the PR. I cannot sign off on this PR without either limiting it to official only (ugly but current behavior) or making this is a much larger change to support a the full namespace format in the URL generation and support sending the canonical name as the remote name. The first is less controversial and more likely to get merged. The latter we WILL do, but will require more discussion and sign off for the namespace proposals.

The v2 registry will act as a pull-through cache, and needs to be handled differently by the client to the v1 registry mirror. See distribution/distribution#459 for details Configuration Only one v2 registry can be configured as a mirror. Acceptable configurations in this chanage are: 0...n v1 mirrors or 1 v2 mirror. A mixture of v1 and v2 mirrors is considered an error. Pull If a v2 mirror is configured, all pulls are redirected to that mirror. The mirror will serve the content locally or attempt a pull from the upstream mirror, cache it locally, and then serve to the client. Push If an image is tagged to a mirror, it will be pushed to the mirror and be stored locally there. Otherwise, images are pushed to the hub. This is unchanged behavior. Signed-off-by: Richard Scothern <richard.scothern@gmail.com>

Strip authconfig from session to keep credentials from being sent to the mirror. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

RichardScothern · 2015-05-26T22:17:19Z

Closing - this won't make 1.7.0.

This feature will be far more useful with full namespace support.

RichardScothern · 2015-05-27T18:58:01Z

Re-opened. We would like to have feature parity with the v1 registry in 1.7

tiborvass · 2015-05-27T20:54:53Z

design +1, this is only for the official index, and does not send hub creds to the mirror.
It is incompatible with v1 mirrors, and there can only be one v2 mirror set, at least for now in this PR. Rationale: if people want to do some fancy things, they can set up load balancers and set the v2 mirror to that, basically eliminating the need to complexify the client.

tiborvass · 2015-05-27T20:56:50Z

Code LGTM

jessfraz · 2015-05-27T20:59:09Z

graph/pull.go

+	v1MirrorCount := 0
+	var v2MirrorEndpoint *registry.Endpoint
+	var v2MirrorRepoInfo *registry.RepositoryInfo
+	var lastErr error


i just dont really like it... but i guess i see the point

calavera · 2015-05-27T21:00:52Z

it would be cool to have some tests around mirror configuration, because looking at the code I don't really know if it works 😄

calavera · 2015-05-27T21:01:25Z

graph/pull.go

+	if repoInfo.Index.Official {
+		v2mirrorEndpoint, v2mirrorRepoInfo, err := configureV2Mirror(repoInfo, s.registryService)
+		if err != nil {
+			logrus.Errorf("Error configuring mirrors: %s", err)


this log might not be necessary if we're returning the error.

Signed-off-by: Richard Scothern <richard.scothern@gmail.com>

tiborvass · 2015-05-27T22:36:15Z

graph/pull.go

+		}
+
+		if v2mirrorEndpoint != nil {
+			logrus.Debugf("Attempting pull from v2 mirror: %s", v2mirrorEndpoint.URL)


Attempting to pull

dmcgowan · 2015-05-27T22:40:19Z

LGTM

calavera · 2015-05-27T23:01:13Z

@RichardScothern no signature in that latest commit

Please, add some tests 🙏

tiborvass · 2015-05-27T23:02:53Z

graph/pull.go

+
+	if v1MirrorCount == len(mirrors) {
+		// OK, but mirrors are v1
+		return nil, nil, nil


@RichardScothern this is the exit when there are no mirrors at all, we should probably just have a very early return if len(mirrors) == 0 at the beginning of this function, otherwise this is confusing because it just so happens that v1MirrorCount would also be 0.

- Match verbiage with other output - Remove dead code and clearer flow Signed-off-by: Richard Scothern <richard.scothern@gmail.com>

tiborvass · 2015-05-28T00:41:04Z

RE LGTM

jessfraz · 2015-05-28T01:15:05Z

LGtM

V2 mirror support

GordonTheTurtle added status/0-triage dco/no labels May 21, 2015

dmp42 added this to the 1.7.0 milestone May 21, 2015

RichardScothern force-pushed the v2-mirror branch from e2565d3 to 24b6e8d Compare May 21, 2015 00:59

GordonTheTurtle removed the dco/no label May 21, 2015

jessfraz added status/1-design-review and removed status/0-triage labels May 21, 2015

RichardScothern force-pushed the v2-mirror branch from 24b6e8d to 53445f9 Compare May 21, 2015 04:50

RichardScothern changed the title ~~V2 mirror~~ V2 mirror support May 21, 2015

dmcgowan reviewed May 22, 2015
View reviewed changes

tiborvass reviewed May 22, 2015
View reviewed changes

RichardScothern and others added 2 commits May 26, 2015 11:08

Allow mirroring only for the official index

c19962a

Strip authconfig from session to keep credentials from being sent to the mirror. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

RichardScothern force-pushed the v2-mirror branch from 53445f9 to c19962a Compare May 26, 2015 18:09

icecrime removed this from the 1.7.0 milestone May 26, 2015

RichardScothern closed this May 26, 2015

RichardScothern reopened this May 27, 2015

icecrime added status/2-code-review and removed status/1-design-review labels May 27, 2015

jessfraz reviewed May 27, 2015
View reviewed changes

calavera reviewed May 27, 2015
View reviewed changes

Restore the stripped registry version number

f6f7d35

Signed-off-by: Richard Scothern <richard.scothern@gmail.com>

RichardScothern force-pushed the v2-mirror branch from ce57b21 to f6f7d35 Compare May 27, 2015 22:16

tiborvass reviewed May 27, 2015
View reviewed changes

GordonTheTurtle added the dco/no label May 27, 2015

tiborvass reviewed May 27, 2015
View reviewed changes

RichardScothern force-pushed the v2-mirror branch from 2b86c8c to 14fb0b7 Compare May 27, 2015 23:18

GordonTheTurtle removed the dco/no label May 27, 2015

Review feedback:

e817e08

- Match verbiage with other output - Remove dead code and clearer flow Signed-off-by: Richard Scothern <richard.scothern@gmail.com>

RichardScothern force-pushed the v2-mirror branch from 14fb0b7 to e817e08 Compare May 28, 2015 00:29

tiborvass added a commit that referenced this pull request May 28, 2015

Merge pull request #13374 from RichardScothern/v2-mirror

2daede5

V2 mirror support

tiborvass merged commit 2daede5 into moby:master May 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2 mirror support #13374

V2 mirror support #13374

RichardScothern commented May 21, 2015

stevenschlansker commented May 21, 2015

RichardScothern commented May 21, 2015

dmp42 commented May 21, 2015

jessfraz commented May 21, 2015

stevvooe commented May 22, 2015

dmcgowan May 22, 2015

RichardScothern May 22, 2015

dmcgowan commented May 22, 2015

tiborvass May 22, 2015

dmcgowan May 23, 2015

RichardScothern commented May 22, 2015

dmcgowan commented May 23, 2015

RichardScothern commented May 26, 2015

RichardScothern commented May 27, 2015

tiborvass commented May 27, 2015

tiborvass commented May 27, 2015

jessfraz May 27, 2015

tiborvass May 27, 2015

jessfraz May 27, 2015

calavera commented May 27, 2015

calavera May 27, 2015

tiborvass May 27, 2015

dmcgowan commented May 27, 2015

calavera commented May 27, 2015

tiborvass May 27, 2015

RichardScothern May 27, 2015

tiborvass commented May 28, 2015

jessfraz commented May 28, 2015

V2 mirror support #13374

V2 mirror support #13374

Conversation

RichardScothern commented May 21, 2015

stevenschlansker commented May 21, 2015

RichardScothern commented May 21, 2015

dmp42 commented May 21, 2015

jessfraz commented May 21, 2015

stevvooe commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardScothern commented May 22, 2015

dmcgowan commented May 23, 2015

RichardScothern commented May 26, 2015

RichardScothern commented May 27, 2015

tiborvass commented May 27, 2015

tiborvass commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calavera commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 27, 2015

calavera commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented May 28, 2015

jessfraz commented May 28, 2015