Add builtin nodes discovery #16229

icecrime · 2015-09-10T23:26:14Z

Use pkg/discovery to provide nodes discovery between daemon instances.

The functionality is driven by two different command-line flags:
--discovery-backend and --discovery-address. It can be used in two
ways by interested components:

Externally by calling the /info API and examining the discovery
backend. The pkg/discovery package can then be used to hit the same
endpoint and watch for appearing or disappearing nodes. That is the
method that will for example be used by Swarm.
Internally by using the Daemon.discoveryWatcher instance. That is
the method that will for example be used by libnetwork.

Ping @abronan @mavenugo @vieux.

duglin · 2015-09-10T23:48:24Z

@icecrime is there a design doc that describes how this works? In particular, what is relationship with swarm? I wasn't aware that we were headed towards a single daemon knowing about other daemons.

aaronlehmann · 2015-09-11T00:15:37Z

pkg/discovery/file/file.go

+}
+
+// Watch is exported
+func (s *Discovery) Watch(stopCh <-chan struct{}) (<-chan discovery.Entries, <-chan error) {


Don't know if it's overengineering for this case, but we could use inotify to avoid polling the file (at least on Linux).

https://godoc.org/golang.org/x/exp/inotify

Or fsnotify which is cross-platform and already vendored... but still need to fallback to polling as I've seen multiple people with issues on docker logs -f which is using inotify.

Good point, but can be done in a separate PR as the engine won't use file (only swarm uses it)

aaronlehmann · 2015-09-11T00:20:00Z

pkg/discovery/generator.go

+		return []string{pattern}
+	}
+
+	template := re.ReplaceAllString(pattern, "%d")


It looks like this will break with a pattern that matches the regexp multiple times. It will also behave strangely if the pattern includes a % character or a printf directive. I feel it's better to avoid using Sprintf with a user-controlled format string.

icecrime · 2015-09-11T00:23:26Z

@duglin Unfortunately, I don't have a design doc: let me try to summarize in a few sentences.

Both Swarm and mutihost networking require nodes discovery. On the Swarm side, this is nicely done with a well delimited discovery package. The idea here is to take in this package at the Engine level, thus providing a single nodes discovery mechanism to rely on:

Swarm can query the Engine on the /info endpoint and learn about the discovery backend in use. By instantiating a discovery itself, it can then watch for nodes membership changes.
Libnetwork can use the daemon.discoveryWatcher field and watch for nodes membership changes.

This is an quick and easy way to provide such facility in the Engine. In the future, we could imagine exposing nodes membership changes as part of the /events API, or provide a new endpoint to get the list of known neighbor nodes.

icecrime · 2015-09-11T01:23:40Z

This is broken because ./hack/vendor.sh magic is removing the stretchr/testify dependency. Go figure. (ping @jfrazelle)

jessfraz · 2015-09-11T01:30:43Z

I know how to fix PR will come when I get home

On Thursday, September 10, 2015, Arnaud Porterie notifications@github.com
wrote:

This is broken because ./hack/vendor.sh magic is removing the
stretchr/testify dependency. Go figure. (ping @jfrazelle
https://github.com/jfrazelle)

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

LK4D4 · 2015-09-11T02:29:02Z

Is pkg/discovery copy of some package or what? Why it uses testify at first place?

vieux · 2015-09-11T04:20:24Z

@LK4D4 this package is in swarm, we use testify in swarm.
As the ownership of this pkg is going to shift toward the engine, I'm fine with rewriting the test to look like the other docker/docker/pkg

As soon as it's moved, we are going to remove it from swarm and vendor this one.

LK4D4 · 2015-09-11T05:08:45Z

Maybe we should look to releasing some cool libs under docker organization. Because as I see pkg is still unnoticed by community. Same for distribution/context btw.

cpuguy83 · 2015-09-11T11:37:54Z

+1 for under a separate repo.

duglin · 2015-09-11T12:54:28Z

@LK4D4 +1 to a spot for generic packages

icecrime · 2015-09-11T22:50:50Z

Agree we need a better place for generic packages (I think @duglin had ideas about that).
I took the Swarm discovery package as it is with a few interface changes: the file, node, and token strategies are actually not used by the Engine. Do we have to fix as part of this PR?

PR is update and rebased to fix the broken tests (thanks @jfrazelle)!

abronan · 2015-09-11T23:08:19Z

As part of the concern on keeping this under pkg, we can still incubate this here and move this in a reasonable time frame under its own repository. We did this for swarm and libkv and it worked pretty well. This lets the time to migrate the tests and see how it works for both swarm and libnetwork.

icecrime · 2015-09-13T16:53:41Z

+1 @abronan

Tests are still broken, probably related to vendor pruning (reping @jfrazelle, sorry).

mavenugo · 2015-09-14T01:09:04Z

+1 @abronan.

mavenugo · 2015-09-25T02:00:01Z

@icecrime the test failures are due to the invalid kv-store configuration outside of the experimental. this requires a fix from libnetwork to be vendored-in. I will push a PR to vendor-in latest libnetwork and that will unblock this PR.

Absorb Swarm's discovery package in order to provide a common node discovery mechanism to be used by both Swarm and networking code. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>

icecrime · 2015-09-25T20:36:12Z

Updated to take last discussions into account. Also rebased: with latest libkv/libnetwork change the tests should hopefully be green.

dhiltgen · 2015-09-25T20:52:06Z

pkg/discovery/kv/kv.go

+
+	// Creates a new store, will ignore options given
+	// if not supported by the chosen store
+	s.store, err = libkv.NewStore(s.backend, addrs, nil)


My hunch is this will probably have to come in a subsequent change, but it would be nice to support configuration settings for libkv. In particular, in multi-host environments, being able to set up ClientTLS configuration based on docker/libkv@e7f836c would be beneficial.

@dhiltgen yes. this will come as a subsequent PR. also, we need to work with Darren on the PR to make this a 1-time dynamic configuration.

Use `pkg/discovery` to provide nodes discovery between daemon instances. The functionality is driven by two different command-line flags: the experimental `--cluster-store` (previously `--kv-store`) and `--cluster-advertise`. It can be used in two ways by interested components: 1. Externally by calling the `/info` API and examining the cluster store field. The `pkg/discovery` package can then be used to hit the same endpoint and watch for appearing or disappearing nodes. That is the method that will for example be used by Swarm. 2. Internally by using the `Daemon.discoveryWatcher` instance. That is the method that will for example be used by libnetwork. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>

abronan · 2015-09-26T02:02:29Z

LGTM, let's address enhancements and documentation in other PRs (like for the /join and /leave endpoints).

mavenugo · 2015-09-26T02:21:00Z

Thanks @icecrime. LGTM.

mavenugo · 2015-09-26T04:24:58Z

@aluzzardi if you agree, we should merge this and that will unblock the dependent PRs. As discussed, the backend updates can be taken up in subsequent PRs.

vieux · 2015-09-26T05:35:19Z

LGTM as well

aluzzardi · 2015-09-28T01:04:06Z

LGTM

Add builtin nodes discovery

vieux · 2015-09-28T01:56:54Z

\o/

stevvooe · 2015-09-28T10:15:17Z

daemon/config.go

+	// ClusterStore is the storage backend used for the cluster information. It is used by both
+	// multihost networking (to store networks and endpoints information) and by the node discovery
+	// mechanism.
+	ClusterStore string


Typically, the convention in Go is to identify stringly-typed network endpoint fields with Addr. This should be ClusterStoreAddr.

bash completion for #16229

GordonTheTurtle added the status/0-triage label Sep 10, 2015

icecrime added this to the 1.9.0 milestone Sep 10, 2015

icecrime added status/1-design-review and removed status/0-triage labels Sep 10, 2015

aaronlehmann reviewed Sep 11, 2015
View reviewed changes

icecrime force-pushed the discovery branch 2 times, most recently from ff3626d to 1454028 Compare September 11, 2015 00:19

aaronlehmann reviewed Sep 11, 2015
View reviewed changes

icecrime force-pushed the discovery branch 2 times, most recently from 7bf0ad4 to b60e7ad Compare September 11, 2015 01:22

icecrime force-pushed the discovery branch from b60e7ad to 5aee252 Compare September 11, 2015 01:26

jessfraz mentioned this pull request Sep 11, 2015

allow vendoring of test packages #16231

Merged

icecrime force-pushed the discovery branch from 5aee252 to bb85557 Compare September 11, 2015 22:50

jimmyxian mentioned this pull request Sep 14, 2015

Use discovery from docker/docker docker-archive/classicswarm#1211

Closed

icecrime force-pushed the discovery branch from bb85557 to a8efa2a Compare September 14, 2015 20:37

jessfraz mentioned this pull request Sep 15, 2015

cleanup vendoring of test deps #16294

Merged

icecrime force-pushed the discovery branch from 5fa75d2 to 578d74c Compare September 25, 2015 00:27

Add pkg/discovery for nodes discovery

166e082

Absorb Swarm's discovery package in order to provide a common node discovery mechanism to be used by both Swarm and networking code. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>

icecrime force-pushed the discovery branch from 578d74c to 846c7b7 Compare September 25, 2015 20:35

dhiltgen reviewed Sep 25, 2015
View reviewed changes

icecrime force-pushed the discovery branch from 846c7b7 to d67ccdd Compare September 25, 2015 21:51

icecrime force-pushed the discovery branch from d67ccdd to 7d193ef Compare September 25, 2015 21:52

tiborvass added status/2-code-review and removed status/1-design-review labels Sep 26, 2015

aluzzardi added a commit that referenced this pull request Sep 28, 2015

Merge pull request #16229 from icecrime/discovery

9324b15

Add builtin nodes discovery

aluzzardi merged commit 9324b15 into moby:master Sep 28, 2015

abronan mentioned this pull request Sep 28, 2015

Fix discovery package README with the right --cluster* flags #16618

Merged

albers mentioned this pull request Sep 28, 2015

bash completion for #16229 #16622

Merged

stevvooe reviewed Sep 28, 2015
View reviewed changes

sdurrheimer mentioned this pull request Sep 28, 2015

Add zsh completion for --cluster-store and --cluster-advertise #16629

Merged

coolljt0725 mentioned this pull request Sep 28, 2015

Add cluster advertise address to host address as default #16630

Closed

vdemeester added a commit that referenced this pull request Sep 30, 2015

Merge pull request #16622 from albers/completion-cluster

f22674a

bash completion for #16229

thaJeztah added impact/api impact/changelog impact/cli labels Oct 1, 2015

icecrime deleted the discovery branch February 27, 2016 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builtin nodes discovery #16229

Add builtin nodes discovery #16229

icecrime commented Sep 10, 2015

duglin commented Sep 10, 2015

aaronlehmann Sep 11, 2015

cpuguy83 Sep 11, 2015

vieux Sep 11, 2015

aaronlehmann Sep 11, 2015

icecrime commented Sep 11, 2015

icecrime commented Sep 11, 2015

jessfraz commented Sep 11, 2015

LK4D4 commented Sep 11, 2015

vieux commented Sep 11, 2015

LK4D4 commented Sep 11, 2015

cpuguy83 commented Sep 11, 2015

duglin commented Sep 11, 2015

icecrime commented Sep 11, 2015

abronan commented Sep 11, 2015

icecrime commented Sep 13, 2015

mavenugo commented Sep 14, 2015

mavenugo commented Sep 25, 2015

icecrime commented Sep 25, 2015

dhiltgen Sep 25, 2015

mavenugo Sep 25, 2015

abronan commented Sep 26, 2015

mavenugo commented Sep 26, 2015

mavenugo commented Sep 26, 2015

vieux commented Sep 26, 2015

aluzzardi commented Sep 28, 2015

vieux commented Sep 28, 2015

stevvooe Sep 28, 2015

Add builtin nodes discovery #16229

Add builtin nodes discovery #16229

Conversation

icecrime commented Sep 10, 2015

duglin commented Sep 10, 2015

aaronlehmann Sep 11, 2015

Choose a reason for hiding this comment

cpuguy83 Sep 11, 2015

Choose a reason for hiding this comment

vieux Sep 11, 2015

Choose a reason for hiding this comment

aaronlehmann Sep 11, 2015

Choose a reason for hiding this comment

icecrime commented Sep 11, 2015

icecrime commented Sep 11, 2015

jessfraz commented Sep 11, 2015

LK4D4 commented Sep 11, 2015

vieux commented Sep 11, 2015

LK4D4 commented Sep 11, 2015

cpuguy83 commented Sep 11, 2015

duglin commented Sep 11, 2015

icecrime commented Sep 11, 2015

abronan commented Sep 11, 2015

icecrime commented Sep 13, 2015

mavenugo commented Sep 14, 2015

mavenugo commented Sep 25, 2015

icecrime commented Sep 25, 2015

dhiltgen Sep 25, 2015

Choose a reason for hiding this comment

mavenugo Sep 25, 2015

Choose a reason for hiding this comment

abronan commented Sep 26, 2015

mavenugo commented Sep 26, 2015

mavenugo commented Sep 26, 2015

vieux commented Sep 26, 2015

aluzzardi commented Sep 28, 2015

vieux commented Sep 28, 2015

stevvooe Sep 28, 2015

Choose a reason for hiding this comment