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

support registry mirror config reload #29650

Merged
merged 1 commit into from Jan 4, 2017

Conversation

Projects
None yet
5 participants
@allencloud
Contributor

allencloud commented Dec 22, 2016

Signed-off-by: allencloud allen.sun@daocloud.io

fixes #29594

this PR makes docker daemon support reload config registry mirror.

- What I did

  1. add registry-mirror enable reload
  2. fix a bug that duplicated mirrors will appear in the daemon and docker info
  3. add a test case for that

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Show outdated Hide outdated daemon/daemon.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 22, 2016

Member

Feature-wise, I'm ok with adding this. I do think that the whole configuration (re)load functionality needs a massive overhaul / refactor though; I think the code is getting quite messy, and a lot of duplication

Member

thaJeztah commented Dec 22, 2016

Feature-wise, I'm ok with adding this. I do think that the whole configuration (re)load functionality needs a massive overhaul / refactor though; I think the code is getting quite messy, and a lot of duplication

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 22, 2016

Member

Design-wise we're okay with this

@aaronlehmann do you see any possible issues with live-reloading this list?

Member

thaJeztah commented Dec 22, 2016

Design-wise we're okay with this

@aaronlehmann do you see any possible issues with live-reloading this list?

@@ -663,7 +663,7 @@ func TestMirrorEndpointLookup(t *testing.T) {
}
return false
}
s := DefaultService{config: makeServiceConfig([]string{"my.mirror"}, nil)}
s := DefaultService{config: makeServiceConfig([]string{"https://my.mirror"}, nil)}

This comment has been minimized.

@thaJeztah

thaJeztah Dec 23, 2016

Member

Why did you change this test?

@thaJeztah

thaJeztah Dec 23, 2016

Member

Why did you change this test?

This comment has been minimized.

@allencloud

allencloud Dec 23, 2016

Contributor

since it fails in the test, when validateMirror.
Mirror address must have scheme. In all the test file, mirror has scheme, while this has no.

In addition, I need more investigation.

@allencloud

allencloud Dec 23, 2016

Contributor

since it fails in the test, when validateMirror.
Mirror address must have scheme. In all the test file, mirror has scheme, while this has no.

In addition, I need more investigation.

Show outdated Hide outdated registry/config.go Outdated
@thaJeztah

@allencloud I left some comments, will have another look after it's updated 😊

Show outdated Hide outdated registry/config.go Outdated
Show outdated Hide outdated registry/config.go Outdated
Show outdated Hide outdated registry/config.go Outdated
Show outdated Hide outdated registry/config.go Outdated
Show outdated Hide outdated registry/config.go Outdated
@allencloud

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Dec 24, 2016

Contributor

Thanks a lot. Really helpful comments. I updated this PR. PTAL

Contributor

allencloud commented Dec 24, 2016

Thanks a lot. Really helpful comments. I updated this PR. PTAL

Show outdated Hide outdated registry/config.go Outdated
@thaJeztah

oops, looks like I had a review "pending" here; see my comment

Show outdated Hide outdated registry/config.go Outdated
"http://mirror-1.com/?q=foo",
"http://mirror-1.com/v1/",
"http://mirror-1.com/v1/?q=foo",
"http://mirror-1.com/v1/?q=foo#frag",
"http://mirror-1.com?q=foo",
"https://mirror-1.com#frag",
"https://mirror-1.com/",
"https://mirror-1.com/#frag",

This comment has been minimized.

@thaJeztah

thaJeztah Dec 27, 2016

Member

can you add a http://foo:bar@mirror-1.com/ to the test?

@thaJeztah

thaJeztah Dec 27, 2016

Member

can you add a http://foo:bar@mirror-1.com/ to the test?

@@ -341,6 +341,106 @@ func TestDaemonReloadLabels(t *testing.T) {
}
}
func TestDaemonReloadMirrors(t *testing.T) {

This comment has been minimized.

@thaJeztah

thaJeztah Dec 27, 2016

Member

I wonder if we should simplify this test, and have a single slice of test-cases, each with "before", "new", and "expected", and a property that indicates if it should fail.

@thaJeztah

thaJeztah Dec 27, 2016

Member

I wonder if we should simplify this test, and have a single slice of test-cases, each with "before", "new", and "expected", and a property that indicates if it should fail.

This comment has been minimized.

@allencloud

allencloud Dec 27, 2016

Contributor

That is great. I will try that. Thanks a lot.

@allencloud

allencloud Dec 27, 2016

Contributor

That is great. I will try that. Thanks a lot.

This comment has been minimized.

@allencloud

allencloud Dec 29, 2016

Contributor

Updated with clearer test. PTAL 😄

@allencloud

allencloud Dec 29, 2016

Contributor

Updated with clearer test. PTAL 😄

Show outdated Hide outdated daemon/daemon.go Outdated
Show outdated Hide outdated registry/config.go Outdated
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 3, 2017

Contributor

Code LGTM

Contributor

aaronlehmann commented Jan 3, 2017

Code LGTM

support registry mirror config reload
Signed-off-by: allencloud <allen.sun@daocloud.io>
@thaJeztah

LGTM, thanks!

@thaJeztah thaJeztah merged commit d3f30d6 into moby:master Jan 4, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29151 has succeeded
Details
janky Jenkins build Docker-PRs 37744 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 8809 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 4, 2017

@allencloud allencloud deleted the allencloud:support-registry-mirror-config-reload branch Jan 4, 2017

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