Enabling ILB/ELB on windows using per-node, per-network LB endpoint. #34674

pradipd · 2017-08-29T23:03:40Z

Note: This is 1 of 3 PRs (the other 2 are in libnetwork and swarmkit repos).

Signed-off-by: Pradip Dhara pradipd@microsoft.com

- What I did
Enabled ILB/ELB (routing mesh) functionality for windows.

- How I did it
This change enables ILB/ELB functionality on windows using a per-node, per-network LB endpoint. The per-node, per-network LB endpoint was discussed with docker
as a solution to #30820. Note that only windows is using the LB endpoint. We have not changed the linux networking code to take advantage of the LB endpoint.

- How to verify it
make test on linux (note: I'm still seeing 2 tests occasionally fail. But, I'm fairly certain they are unrelated to my change. I'm sending this out now and will continue to debug).
We have added a bunch of powershell scripts to test out functionality. However, they are not public. We will work with docker folks in figuring out how to make them public. Ideally, we should get the docker_cli_swarm_tests running on windows, but, that is much bigger task and requires changes in windows.

- Description for the changelog
Enabling ILB/ELB on windows using per-node, per-network LB endpoint.

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

pradipd · 2017-08-29T23:04:17Z

libnetwork PR: moby/libnetwork#1925
swarmkit PR: moby/swarmkit#2363

pradipd · 2017-08-29T23:06:19Z

@mavenugo @fcrisciani @msabansal

vieux · 2017-08-30T17:50:04Z

@pradipd can you please fix the tests ?

dnephin · 2017-08-30T17:57:08Z

daemon/network.go

@@ -360,6 +387,34 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string
 	}
 	daemon.LogNetworkEvent(n, "create")

+	if agent && !n.Info().Ingress() && n.Type() == "overlay" {


Please move this code (and the code in deleteNetwork) to a new function. A separate function will make it easier to unit test, and help the reader understand the different steps performed as part of createNetwork and deleteNetwork.

dnephin · 2017-08-30T17:58:20Z

daemon/network.go

+		sb, err := c.NewSandbox(sandboxName)
+		if err != nil {
+			logrus.Errorf("Failed creating %s sandbox: %v", sandboxName, err)
+			return nil, err


I don't think the error needs to be logged. The conventional way of handling errors is to return them with errors.Wrapf(err, "failed creating %s sandbox", sandboxName).

The errors should end up in the logs.

pradipd · 2017-08-31T20:05:11Z

Thanks for the feedback. I'm addressing it now. I had to fix some changes in libnetwork PR (moby/libnetwork#1925) and swarmkit PR (moby/swarmkit#2363)

pradipd · 2017-09-01T17:13:37Z

daemon/network.go

+	endpointName := prefix + "-endpoint"
+	ep, err := n.CreateEndpoint(endpointName, libnetwork.CreateOptionIpam(ip, nil, nil, nil), libnetwork.CreateOptionLoadBalancer())
+	if err != nil {
+		logrus.Errorf("Failed creating %s in sandbox %s: %v", endpointName, sandboxName, err)


Note: I left the logrus error messages here, because they were here before for when we were creating the ingress network.

pradipd · 2017-09-01T17:15:28Z

@vieux In order to get the tests passing I need changes from moby/libnetwork#1925 and moby/swarmkit#2363
Do I have to wait for those 2 pull requests to merge?
Or do i vendor in those changes and point vendor.conf to my github repo?

GordonTheTurtle · 2017-09-07T06:45:30Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "windows_routingmesh" git@github.com:pradipd/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354127368
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

pradipd · 2017-09-12T06:50:03Z

@vieux : Can you help me understand the integration tests?
Originally I added a test to docker_cli_swarm_test.go, but that gave a janky error.
integration-cli is deprecated. Please add an API integration test to
17:19:20 ./integration/COMPONENT/. See ./TESTING.md for more details

So, I moved the test to docker_api_swarm_test.go, but the test is exactly the same. That fixed the Janky error, but, I don't think that was the proper way to fix the test.

I also tried to add the test to integration/service, but, I can't figure out how to run the tests.
How to I run the tests under integration/service?

dnephin

How to I run the tests under integration/service?

The same way you would the cli suite, make test-integration

dnephin · 2017-09-12T15:20:05Z

integration-cli/docker_api_swarm_test.go

+        d := s.AddDaemon(c, true, true)
+
+        // Create a new overlay network
+        out, err := d.Cmd("network", "create", "-d", "overlay", "new-overlay")


The API suite should not use d.Cmd() please use testEnv.APIClient() to get a client, and make calls to the API.

Thanks for the clarification. That is what I thought, but, I saw other tests in the .Cmd() and hence the confusion. I will clean this up.

vieux · 2017-09-15T00:55:11Z

ping @tiborvass @fcrisciani @vdemeester can you take a look ?

thaJeztah

some nits and a question

thaJeztah · 2017-09-15T23:35:06Z

daemon/cluster/executor/container/executor.go

+		return err
+	}
+
+	err = e.backend.AddLBAttachments(lbAttachments)


nit: this can be just

return e.backend.AddLBAttachments(lbAttachments)

Fixed in latest PR.

thaJeztah · 2017-09-15T23:37:20Z

daemon/network.go

-	if err := sb.EnableService(); err != nil {
-		logrus.Errorf("Failed enabling service for ingress sandbox")
-	}
+	daemon.createLoadBalancerSandbox("ingress", create.ID, ip, n, libnetwork.OptionIngress())


Why is the returned error not handled here?

I addressed this in the latest PR

thaJeztah · 2017-09-15T23:40:55Z

daemon/network.go

@@ -235,6 +214,32 @@ func (daemon *Daemon) releaseIngress(id string) {
 	}
 }

+//AddLBAttachments sets the mapping of LB IP address per network
+func (daemon *Daemon) AddLBAttachments(lbAttachments map[string]string) error {
+


Can you remove this empty line here (and other functions you added) to make the coding style match the existing code?

thaJeztah · 2017-09-15T23:49:45Z

daemon/network.go

+func (daemon *Daemon) AddLBAttachments(lbAttachments map[string]string) error {
+
+	for nid, nodeIP := range lbAttachments {
+


Same here, could you remove this empty line?

thaJeztah · 2017-09-16T00:00:54Z

daemon/cluster/executor/container/executor.go

@@ -136,23 +136,34 @@ func (e *executor) Describe(ctx context.Context) (*api.NodeDescription, error) {
 }

 func (e *executor) Configure(ctx context.Context, node *api.Node) error {
-	na := node.Attachment


I'm not too familiar with this, but is node.Attachment no longer needed? (is this now included in node.LbAttachments ?)

Correct. node.Attachment has been deprecated https://github.com/docker/swarmkit/blob/master/api/objects.proto#L64

fcrisciani · 2017-09-17T19:17:16Z

daemon/cluster/executor/container/executor.go

 		e.backend.ReleaseIngress()
+		e.backend.ClearLBAttachments()


is this function clearing the attachments of all the networks or only the Ingress?

It is clearing all the attachments for all networks --- which is wrong.
I fixed this in the latest PR.
The lb attachments are always "reset". I.e. they are cleared and then updated with the latest node.lbattachments passed in from the swarm manager.

fcrisciani · 2017-09-17T19:22:10Z

daemon/cluster/executor/container/executor.go

+		return err
+	}
+
+	err = e.backend.AddLBAttachments(lbAttachments)


The feeling here is that if there is not ingress network there is no lb configured, is it correct or there is a different code path followed?

If it is not ingress network, then we save the lb attachment (actually, only the per network IP address).
We only use the lb IP address when we create a service.

fcrisciani · 2017-09-17T19:36:35Z

daemon/network.go

@@ -496,6 +543,28 @@ func (daemon *Daemon) DeleteNetwork(networkID string) error {
 	return daemon.deleteNetwork(networkID, false)
 }

+func (daemon *Daemon) deleteLoadBalancerSandbox(n libnetwork.Network) {
+


I don't see a disableService here, wondering why there is an asymmetry between the EnableService in the Create and no need for the disableService here

Oops. I think I missed this in the last PR. I'll update it. sorry.

Address in the latest PR.

thaJeztah · 2017-09-18T18:18:26Z

@pradipd vendor check is failing:

18:12:05 The result of vndr differs
18:12:05 
18:12:05  M vendor/github.com/docker/libnetwork/drivers/overlay/peerdb.go
18:12:05 
18:12:05 Please vendor your package with github.com/LK4D4/vndr.

pradipd · 2017-09-18T18:22:21Z

Oops. I shouldn't have done s/sanbox/sandbox/ in the vendored packages.
will fix.

thaJeztah

thanks for addressing my nits; don't forget to squash commits to two commits; 1 for "vendoring" and 1 for the local changes. (We don't have to preserve the "fix vendor" and "address review comments" when merging)

Signed-off-by: Pradip Dhara <pradipd@microsoft.com>

dnephin

Thank you for making those changes to the test case.

I think the test still needs a bit of cleanup. Would you mind opening a PR to address these points?

dnephin · 2017-09-19T16:19:27Z

integration/service/create_test.go

+	poll.WaitOn(t, serviceRunningTasksCount(client, serviceID, instances))
+
+	_, _, err = client.ServiceInspectWithRaw(context.Background(), serviceID, types.ServiceInspectOptions{})
+	require.NoError(t, err)


These 2 lines seem to be unnecessary. We already know the service can be inspected from the poll.WaitOn(). I think these 2 lines need to be removed.

Will remove in new PR.

dnephin · 2017-09-19T16:20:38Z

integration/service/create_test.go

+	require.NoError(t, err)
+	assert.Contains(t, network.Containers, overlayName+"-sbox")
+
+	err = client.ServiceRemove(context.Background(), serviceID)


Everything from this line to the end of the test case seems to be test teardown, which should already be handled by defer setupTest(t)(). Why is all this necessary?

I want to validate that the network is removed cleanly.
I see setupTest() is tearing everything down, but, it is forcefully shutting things down. I want to make sure the typical user pattern works correctly.

dnephin · 2017-09-19T16:22:31Z

integration/service/create_test.go

+	}
+}
+
+func serviceRunningTasksCount(client client.ServiceAPIClient, serviceID string, instances uint64) func(log poll.LogT) poll.Result {


This is a duplicate of serviceContainerCount() in inspect_test.go. I'm fine if you want to rename it, but I don't think we need both. We can just add the state check, which seems to be the only difference.

pradipd · 2017-09-19T17:31:06Z

Will open up a new PR to address these.

pradipd · 2017-09-19T20:55:40Z

#34898 is the PR.

stevvooe · 2017-09-22T02:01:36Z

daemon/cluster/executor/backend.go

@@ -61,4 +62,5 @@ type Backend interface {
 	LookupImage(name string) (*types.ImageInspect, error)
 	PluginManager() *plugin.Manager
 	PluginGetter() *plugin.Store
+	GetLBAttachmentStore() *networkSettings.LBAttachmentStore


GetAttachmentStore

stevvooe · 2017-09-22T02:01:50Z

daemon/daemon.go

@@ -121,6 +122,8 @@ type Daemon struct {
 	pruneRunning     int32
 	hosts            map[string]bool // hosts stores the addresses the daemon is listening on
 	startupDone      chan struct{}
+
+	lbAttachmentStore network.LBAttachmentStore


attachmentStore

stevvooe · 2017-09-22T02:02:21Z

daemon/network/settings.go

@@ -31,3 +34,36 @@ type EndpointSettings struct {
 	*networktypes.EndpointSettings
 	IPAMOperational bool
 }
+
+// LBAttachmentStore stores the load balancer IP address for a network id.
+type LBAttachmentStore struct {


AttachmentStore.

stevvooe · 2017-09-22T02:03:09Z

@pradipd I fixed the naming in swarmkit to remove LB. Could you please do so in moby, as well?

pradipd · 2017-09-22T16:25:33Z

I have made the changes. How do I sync with your changes in swarmkit?
Do I vendor in your branch/commit in vendor.conf?
Or do I wait till you merge your changes into swarmkit, then vendor in the new master commit hash?

stevvooe · 2017-09-22T17:24:31Z

@pradipd Wait till we merge the swarmkit changes and submit a PR with the updates.

pradipd requested review from dnephin and vdemeester as code owners August 29, 2017 23:03

GordonTheTurtle added the status/0-triage label Aug 29, 2017

carlfischer1 mentioned this pull request Aug 30, 2017

Proposal: Extended Service Publishing Support for Windows & Linux #34580

Open

dnephin suggested changes Aug 30, 2017

View reviewed changes

yongtang added status/1-design-review and removed status/0-triage labels Sep 1, 2017

pradipd commented Sep 1, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 7, 2017

dnephin reviewed Sep 12, 2017

View reviewed changes

GordonTheTurtle assigned stevvooe Sep 13, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 13, 2017

pradipd force-pushed the windows_routingmesh branch from 9f7bd27 to a70fad7 Compare September 14, 2017 22:25

pradipd requested review from cpuguy83, dmcgowan, tianon and tonistiigi as code owners September 14, 2017 22:25

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Sep 14, 2017

pradipd force-pushed the windows_routingmesh branch 2 times, most recently from 50f273d to 6fda10b Compare September 14, 2017 22:41

pradipd force-pushed the windows_routingmesh branch from 6fda10b to 704610a Compare September 15, 2017 05:19

vieux added the rebuild/z label Sep 15, 2017

thaJeztah requested changes Sep 16, 2017

View reviewed changes

fcrisciani reviewed Sep 17, 2017

View reviewed changes

thaJeztah approved these changes Sep 18, 2017

View reviewed changes

Enabling ILB/ELB on windows using per-node, per-network LB endpoint.

9bed088

Signed-off-by: Pradip Dhara <pradipd@microsoft.com>

pradipd force-pushed the windows_routingmesh branch from e5e3089 to f314530 Compare September 18, 2017 20:29

vendoring libnetwork and swarmkit

4c1b079

Signed-off-by: Pradip Dhara <pradipd@microsoft.com>

pradipd force-pushed the windows_routingmesh branch from f314530 to 4c1b079 Compare September 18, 2017 20:39

vieux merged commit a2ee40b into moby:master Sep 18, 2017

pradipd deleted the windows_routingmesh branch September 19, 2017 04:54

dnephin reviewed Sep 19, 2017

View reviewed changes

pradipd mentioned this pull request Sep 19, 2017

Addressing some PR feedback on tests #34898

Closed

stevvooe reviewed Sep 22, 2017

View reviewed changes

pradipd mentioned this pull request Sep 22, 2017

api: name attachment field according to surrounding style moby/swarmkit#2385

Merged

andrewhsu mentioned this pull request Sep 29, 2017

Fixing panic when sandbox is nil #35042

Merged

vieux mentioned this pull request Oct 26, 2017

wait not found for raft request id ... #35290

Closed

mavenugo mentioned this pull request Oct 26, 2017

Tasks fail with error "Unable to complete atomic operation, key modified" #35310

Closed

thaJeztah mentioned this pull request Oct 27, 2017

Stack deploy failed with rejected status endpoint create on GW Network failed: endpoint with name gateway_stack_name already exists in network docker_gwbridge #35281

Closed

thaJeztah added area/networking platform/windows labels Nov 8, 2017

thaJeztah mentioned this pull request Nov 8, 2017

Docker daemon crash after joining a swarm cluster #35379

Open

mavenugo mentioned this pull request Nov 29, 2017

Docker Swarm Breaks Private Interface #35515

Closed

		func (daemon *Daemon) AddLBAttachments(lbAttachments map[string]string) error {

		for nid, nodeIP := range lbAttachments {

Enabling ILB/ELB on windows using per-node, per-network LB endpoint. #34674

Enabling ILB/ELB on windows using per-node, per-network LB endpoint. #34674

Conversation

pradipd commented Aug 29, 2017

pradipd commented Aug 29, 2017

pradipd commented Aug 29, 2017

vieux commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradipd commented Aug 31, 2017

Choose a reason for hiding this comment

pradipd commented Sep 1, 2017

GordonTheTurtle commented Sep 7, 2017

pradipd commented Sep 12, 2017

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieux commented Sep 15, 2017

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

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

thaJeztah commented Sep 18, 2017

pradipd commented Sep 18, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

dnephin 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

Choose a reason for hiding this comment

pradipd commented Sep 19, 2017

pradipd commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Sep 22, 2017

pradipd commented Sep 22, 2017

stevvooe commented Sep 22, 2017