Support health aware load balancing #27279

dongluochen · 2016-10-11T08:21:10Z

This change support health aware load balancing and DNS records. Close #24092.

- What I did

A container is added to service records when it starts (without healthcheck config), or when healthcheck passes.

- How I did it

Cherrypick @sanimej's service enable API. Add service activate API. Add instruments to container start and shutdown to activate/deactivate service records.

- How to verify it

Here are the steps to verify the change.

#on one of your node poll HTTP endpoint
while true; do curl -s http://127.0.0.1:8001 | head -n 1; sleep 0.1; done
#run a service with published port
docker service create -p 8001:5000 --replicas 1 --name test dongluochen/simpleweb:0.2
#from HTTP poll validate the container is added to load balancer only when it becomes healthy
#scale your service 
docker service scale test=10
#validate the new containers are added only when they become healthy
#downsize your service 
docker service scale test=1
#validate the containers are removed quickly

Docker/docker integration test doesn't support this type of testing. No integration test is added.

- Description for the changelog

This PR cherrypicks @sanimej's change. It cannot be merged as it is. After review, the cherrypick should be replaced by change from docker/libnetwork.

Ping @sanimej @mrjana .

aaronlehmann · 2016-10-11T16:53:23Z

daemon/network.go

@@ -178,6 +178,10 @@ func (daemon *Daemon) SetupIngress(create clustertypes.NetworkCreateRequest, nod
 		if err := ep.Join(sb, nil); err != nil {
 			logrus.Errorf("Failed joining ingress sandbox to ingress endpoint: %v", err)
 		}
+
+		if err := sb.EnableService(); err != nil {
+			logrus.Errorf("Failed enabling service for ingress sandbox: %v", err)


logrus.WithError(err).Errorf?

WithError(err) is not used here to be consistent with other logs in this file.

@dongluochen Let's not be consistently bad. Please use the logging package correctly.

xiaods · 2016-10-11T23:53:10Z

vendor/src/github.com/docker/libnetwork/networkdb/networkdb.go

 func (nDB *NetworkDB) CreateEntry(tname, nid, key string, value []byte) error {
-	if _, err := nDB.GetEntry(tname, nid, key); err == nil {
+	e, _ := nDB.getEntry(tname, nid, key)


func (nDB *NetworkDB) GetEntry(tname, nid, key string) ([]byte, error) {

i feel the e is not in correct position.

Also, please check the error.

Ping @sanimej.

i feel the e is not in correct position.

e is not the error here. Its the entry returned by getEntry. Yes, the error check can be added.

@dongluochen I will make a note of this and correct it when pushing the change again in libnetwork.

thanks for your clarify, got it.

stevvooe · 2016-10-12T00:35:58Z

daemon/cluster/executor/container/controller.go

@@ -183,6 +183,10 @@ func (r *controller) Start(ctx context.Context) error {

 	// no health check
 	if ctnr.Config == nil || ctnr.Config.Healthcheck == nil {
+		if err := r.adapter.backend.ActivateContainerServiceBinding(r.adapter.container.name()); err != nil {


Make a method on adapter to capture all this. Should just be r.adapter.activateServiceBinding().

stevvooe · 2016-10-12T00:36:47Z

vendor/src/github.com/docker/libnetwork/networkdb/networkdb.go

 func (nDB *NetworkDB) CreateEntry(tname, nid, key string, value []byte) error {
-	if _, err := nDB.GetEntry(tname, nid, key); err == nil {
+	e, _ := nDB.getEntry(tname, nid, key)


Also, please check the error.

stevvooe · 2016-10-19T17:29:46Z

vendor/src/github.com/docker/libnetwork/endpoint.go

+	return false
+}
+
+func (ep *endpoint) setServiceEnabled(state bool) {


enableService.

stevvooe · 2016-10-19T17:30:44Z

vendor/src/github.com/docker/libnetwork/endpoint.go

@@ -303,6 +304,24 @@ func (ep *endpoint) isAnonymous() bool {
 	return ep.anonymous
 }

+// CompareAndSwap ep's serviceEnabled. If its in oldState, set it to newState
+// and return true.  If its not in oldState return false
+func (ep *endpoint) casServiceEnabled(oldState, newState bool) bool {


This is kind of a bazaar API. I'm not sure that CAS works unless you know the last observed revision, not just value. For example, below, the value is not actually observed. It seems like you just need to know if the action succeeded or not.

I'm not sure that CAS works unless you know the last observed revision, not just value - See the usages of this API. Its intent is to allow EnableService or DisableService only once per sandbox. The underlying type being worked on is a boolean. Its a low level primitive. Its not a CAS operation on a type that can take multiple values where you need a revision number per operation (ie: typical CAS semantics provided by Consul etc.)

@sanimej Both uses hard-code oldValue. I don't see how it does that.

Let's say you have a the set of values for the boolean:

true, false, false, true, true

The serializability of the result depends on the most recent previous value. If I just know that it was true last, I can't tell if I am setting it based on stale information or the most recent true value.

If we want to support "only once", use a sync.Once for enable/disable but I am surprised we have that limitation.

Usage of this API is to enable the service when the image's healthcheck passes and disable the service first when its being shutdown. Both EnableService and DisableService are idempotent. In both the cases the oldValue and newValue are fixed. Its not dependent on what the current state is. But a continuous flip flop between multiple true & false states is a bug and its something the caller has to deal with.

I considered using sync.Once. It would need two Once variables and we still need a bool to track the current state. The current approach achieves the same result with one bool.

@sanimej It's weird code and doesn't really achieve what you think it achieves. You get the same exact result if you have a single argument, since both use the complement as the oldValue. I have no clue why you would ever make them idempotent. Enable/Disable being a one way operation is an implementation detail of swarmkit. Users of the API should be able to add and remove the service at their leisure. Enforcing it at this level is error prone, buggy and creates weird code like this.

@stevvooe
You get the same exact result if you have a single argument, since both use the complement as the oldValue. I would rather be explicit by passing both old and new values.

{Enable|Disable}Service is libnetwork API. Its not an implementation detail of swarmkit.

By idempotent what I meant here is one can call EnableService on a sandbox when the service is already enabled. If you are thinking about concurrent calls to EnableService & DisableService calls that means the healtcheck for some reason is flipping between healthy and unhealthy and has to be handled by the caller.

@sanimej I really think that this code isn't really doing anything of value. Looks like it could be accomplished like this:

func (ep *endpoint) enableService(enable bool) bool { ep.mu.Lock() defer ep.mu.Lock() if ep.serviceEnabled != enable { ep.serviceEnabled = enable return true } return false }

@stevvooe Yes, since the type here is boolean the oldValue argument is optional. If you think this is better for readability, ie: just calling it enableService with one argument, I can change it. But what it is doing is a CAS operation.

Since its a libnetwork change I will modify it in the libnetwork PR.

@sanimej Yes, this only needs one method that sets the value.

Complement is implied with a boolean in CAS for this usage:

current old new set? used

true true true false no

true true false true yes

true false false false no

true false true false yes

false false false false no

false false true true yes

false true true false no

false true false false yes

Pulling out the lines in the above table that represent the usage of these methods, we have the following:

current old new set? used

true true false true yes

true false true false yes

false false true true yes

false true false false yes

Per inspection, we can see that the return value, set? is independent of old, meaning that because you always use the complement for old, CAS is independent of last observed.

stevvooe · 2016-10-19T23:11:43Z

vendor/src/github.com/docker/libnetwork/sandbox.go

+func (sb *sandbox) EnableService() error {
+	for _, ep := range sb.getConnectedEndpoints() {
+		if ep.casServiceEnabled(false, true) {
+			if e := ep.addToCluster(); e != nil {


Please use err here.

thaJeztah · 2016-10-20T20:48:13Z

@stevvooe Is this ready to go to code review? I see no decision was made on design 😇

stevvooe · 2016-10-20T22:12:07Z

@thaJeztah Moved to code review.

Which aspects of the design were under review?

thaJeztah · 2016-10-21T00:58:10Z

@stevvooe design review can be either UX, or design of the feature in general. When in doubt, we usually put it in design review, but feel free to move PR's to "code review" if no design review is required, or if it was already approved

aluzzardi · 2016-10-31T21:39:14Z

LGTM on the docker/docker (controller/adapter) part. @stevvooe ?

stevvooe · 2016-10-31T21:53:31Z

LGTM

thaJeztah · 2016-10-31T22:15:39Z

moving to docs review

thaJeztah · 2016-10-31T22:19:08Z

hold on; needs vendoring, and CI didn't run 😢

thaJeztah · 2016-10-31T22:21:22Z

discussed with @dongluochen, and this is basically a bug fix, no user-facing changes (other than everything working even better), so docs LGTM

vdemeester · 2016-11-01T03:39:07Z

@dongluochen you need to vendor it though 👼

sanimej · 2016-11-03T22:02:49Z

@dongluochen libnetwork API change has been merged. You can go ahead with vendoring of libnetwork sha a98901aebe7ce920b6fbf02ebe5c3afc9ca975b8 along with the docker changes.

LK4D4 · 2016-11-04T01:34:59Z

@dongluochen note that vendoring process is changed to using github.com/LK4D4/vndr.

mavenugo · 2016-11-04T16:41:01Z

@dongluochen could you pls fix the libnetwork vendoring as suggested by @sanimej ? this is blocking the upcoming vendoring for windows and solaris overlay support.

thaJeztah

thanks @dongluochen

LGTM (if green)

Signed-off-by: Dong Chen <dongluo.chen@docker.com>

mavenugo · 2016-11-05T16:14:10Z

daemon/container_operations.go

@@ -719,6 +719,13 @@ func (daemon *Daemon) connectToNetwork(container *container.Container, idOrName
 		return err
 	}

+	if !container.Managed {


@sanimej @dongluochen won't this impact all the containers in all the networks (swarm-mode or otherwise) including docker0 , host, null networks ? Will sb.EnableService protect us from any regressions to other use-cases that is using other networks ?

@mavenugo I think this change does not change the current behavior that each endpoint calls addToCluster only once. Here when an unmanaged container joins a network, sb.EnableService would enumerate every endpoint but only the new endpoint would call addToCluster. Is this your concern?

+func (sb *sandbox) EnableService() error { + for _, ep := range sb.getConnectedEndpoints() { + if ep.enableService(true) { + if err := ep.addToCluster(); err != nil { + ep.enableService(false) + return fmt.Errorf("could not update state for endpoint %s into cluster: %v", ep.Name(), err) + } + } + } + return nil +}

@dongluochen thats not my concern. But I went through the libnetwork code to understand the behavior and I think that is fine. The question I raised wont be impacted here since addToCluster will not fail for endpoints connected to networks that !isClusterEligible().

@mavenugo yes, !isClusterEligible() will make it a noop for non-swarm networks. The same check was relied on earlier (before the addition of EnableService() API) when addToCluster was getting called unconditionally.

mavenugo · 2016-11-05T16:14:18Z

the experimental CI failure doesnt seem related to this change. cc @tiborvass (for the failure in DockerSuite.TestPluginActive).

mavenugo · 2016-11-07T10:42:45Z

#28115 & #28116 addresses the experimental failures. This PR is good to go.

anghelutar · 2017-09-02T11:54:44Z

We are trying to accomplish health-aware DNS load-balancing and according to this conversation it should be possible from docker 13 on. However, it does not seem to work on a setup using docker 13.1. We are not using docker swarm, we start services via docker-compose and use an external overlay network. A ping to a service started twice (first being healthy and second unhealthy) resolves to both ips, while we would expect to receive only the healthy one. Why isn't the unhealthy one removed from the DNS?

koxu1996 · 2018-07-24T17:49:43Z

@anghelutar I am experiencing the same problem. Did you find solution?

anghelutar · 2018-07-25T06:10:08Z

Hello @koxu1996,

I did not find a solution, I was adviced to use docker swarm. See:

https://forums.docker.com/t/health-aware-dns-based-load-balancing/37566

roxana

GordonTheTurtle added the status/0-triage label Oct 11, 2016

aaronlehmann reviewed Oct 11, 2016

View reviewed changes

xiaods reviewed Oct 11, 2016

View reviewed changes

stevvooe requested changes Oct 12, 2016

View reviewed changes

vdemeester added status/1-design-review and removed status/0-triage labels Oct 17, 2016

dongluochen force-pushed the lbapi_integration branch from 121671b to 8e1b2bb Compare October 19, 2016 01:55

dongluochen changed the title ~~[WIP] Support health aware load balancing~~ Support health aware load balancing Oct 19, 2016

stevvooe reviewed Oct 19, 2016

View reviewed changes

thaJeztah assigned stevvooe Oct 20, 2016

stevvooe added status/2-code-review and removed status/1-design-review labels Oct 20, 2016

dongluochen force-pushed the lbapi_integration branch from 8e1b2bb to 54f5245 Compare October 21, 2016 18:04

sanimej mentioned this pull request Oct 24, 2016

Add sandbox API for task insertion to service LB and service discovery moby/libnetwork#1519

Merged

dongluochen mentioned this pull request Oct 27, 2016

How is discovery supposed to work? docker-archive/classicswarm#1902

Closed

thaJeztah added this to the 1.13.0 milestone Oct 29, 2016

stevvooe approved these changes Oct 31, 2016

View reviewed changes

thaJeztah added status/3-docs-review and removed status/2-code-review labels Oct 31, 2016

thaJeztah added the status/needs-vendoring label Oct 31, 2016

dongluochen force-pushed the lbapi_integration branch from 54f5245 to da3c5a1 Compare November 4, 2016 17:26

thaJeztah approved these changes Nov 4, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/3-docs-review status/needs-vendoring labels Nov 4, 2016

dongluochen force-pushed the lbapi_integration branch from da3c5a1 to 92bdd75 Compare November 4, 2016 17:44

dongluochen added 2 commits November 4, 2016 21:50

dynamic service binding.

ca81f6e

Signed-off-by: Dong Chen <dongluo.chen@docker.com>

Vendor libnetwork a98901aebe7ce920b6fbf02ebe5c3afc9ca975b8.

aba4190

Signed-off-by: Dong Chen <dongluo.chen@docker.com>

dongluochen force-pushed the lbapi_integration branch from 92bdd75 to aba4190 Compare November 5, 2016 04:51

mavenugo reviewed Nov 5, 2016

View reviewed changes

mavenugo approved these changes Nov 7, 2016

View reviewed changes

mavenugo merged commit bee2bee into moby:master Nov 7, 2016

thaJeztah added the impact/changelog label Nov 7, 2016

This was referenced Nov 25, 2016

container can accept requests when became unhealthy in swarm mode #28741

Closed

swarm: block controller.Start until container is healthy #24545

Merged

thaJeztah mentioned this pull request Dec 6, 2016

Docker swarm DNS and load balancing behaviour #27837

Closed

EvgenyOrekhov mentioned this pull request Jan 19, 2017

Add the HEALTHCHECK instruction to the relevant dockerfiles docker-library/php#366

Closed

current	old	new	set?	used
`true`	`true`	`true`	`false`	no
`true`	`true`	`false`	`true`	yes
`true`	`false`	`false`	`false`	no
`true`	`false`	`true`	`false`	yes
`false`	`false`	`false`	`false`	no
`false`	`false`	`true`	`true`	yes
`false`	`true`	`true`	`false`	no
`false`	`true`	`false`	`false`	yes

Support health aware load balancing #27279

Support health aware load balancing #27279

Conversation

dongluochen commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanimej Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Oct 20, 2016

stevvooe commented Oct 20, 2016

thaJeztah commented Oct 21, 2016

aluzzardi commented Oct 31, 2016

stevvooe commented Oct 31, 2016

thaJeztah commented Oct 31, 2016

thaJeztah commented Oct 31, 2016

thaJeztah commented Oct 31, 2016

vdemeester commented Nov 1, 2016

sanimej commented Nov 3, 2016

LK4D4 commented Nov 4, 2016

mavenugo commented Nov 4, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Nov 5, 2016

mavenugo commented Nov 7, 2016

anghelutar commented Sep 2, 2017 • edited

koxu1996 commented Jul 24, 2018

anghelutar commented Jul 25, 2018

sanimej Oct 19, 2016 •

edited

anghelutar commented Sep 2, 2017 •

edited