Fix automatic removal of ingress sandbox when last service leaves #36538

ctelfer · 2018-03-08T19:03:22Z

- What I did
Patched libnetwork to avoid automatically deleting ingress networks when the only endpoint remaining in the network was the ingress load balancer IP. Then adjust the moby code so that it explicitly removes this endpoint when explicit ingress removal is requested. The issue this fixes is detailed in moby/libnetwork#2097.

- How I did it
Two separate patches are required. The first is to libnetwork to avoid an issue introduced in moby/libnetwork#2011 that automatically removes any network (including ingress) when the number of endpoints drops to 1 and the network contains a load balancing IP. The patch prevents the removal if the network is marked as ingress. The second patch adds logic to the daemon.releaseIngress() call in moby to remove the ingress load balancing endpoint before invoking libnetwork.network.Delete().

- How to verify it
I added a test to the moby integration tests called TestServiceWithIngressNetwork. This test:

spins up a swarm
waits for the ingress network to "settle"
creates a simple service and waits for it to converge
removes the service
waits and then verifies that the ingress network is still present and is not corrupted

If one applies the test patch (the first in the series) without the other two fix patches, the test will fail noting ingress corruption. If one applies the entire PR set of patches, and runs the test, it will pass. (running the test as TESTFLAGS='-test.run TestServiceWithIngressNetwork' make test-integration)

- Description for the changelog
Prevent implicit removal of the ingress network

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

mavenugo

LGTM

mavenugo · 2018-03-09T18:16:46Z

@ctelfer thanks for also adding the integration-cli test for this tricky test scenario.

ddebroy

LGTM

fcrisciani

LGTM

selansen · 2018-03-09T23:22:41Z

LGTM

codecov · 2018-03-11T21:56:27Z

Codecov Report

❗ No coverage uploaded for pull request base (master@241c904). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #36538   +/-   ##
=========================================
  Coverage          ?   34.72%           
=========================================
  Files             ?      612           
  Lines             ?    45403           
  Branches          ?        0           
=========================================
  Hits              ?    15765           
  Misses            ?    27577           
  Partials          ?     2061

vdemeester

@ctelfer SGTM, but can you reference the upstream PR on libnetwork (we'll need to wait for it to get merged to be able to merge it here)

ctelfer · 2018-03-12T15:17:44Z

@vdemeester Thanks for the review. At the time I filed the PR, the libnetwork change wasn't merged. I've updated the vendoring now so it references the appropriate libnetwork commit directly. CI seems to be in progress. For reference, the libnetwork PR for this fix is moby/libnetwork#2097. Thanks again!

thaJeztah · 2018-03-12T16:00:15Z

@ctelfer looks like something went wrong with vendoring;

15:32:37 The result of vndr differs
15:32:37 
15:32:37  M vendor/github.com/docker/libnetwork/config/config.go
15:32:37  M vendor/github.com/docker/libnetwork/controller.go
15:32:37  M vendor/github.com/docker/libnetwork/drivers_ipam.go
15:32:37  M vendor/github.com/docker/libnetwork/ipams/builtin/builtin_unix.go
15:32:37  M vendor/github.com/docker/libnetwork/ipams/builtin/builtin_windows.go
15:32:37  M vendor/github.com/docker/libnetwork/ipamutils/utils.go
15:32:37  M vendor/github.com/docker/libnetwork/network.go
15:32:37  M vendor/github.com/docker/libnetwork/service_common.go
15:32:37  M vendor/github.com/docker/libnetwork/vendor.conf
15:32:37 
15:32:37 Please vendor your package with github.com/LK4D4/vndr.

ctelfer · 2018-03-12T17:03:14Z

facepalm Updated the vendor.conf but forgot to actually run vndr. :(

The commit moby#35422 had the result of accidentally causing the removal of the ingress network when the last member of a service left the network. This did not appear in swarm instances because the swarm manager would still maintain and return cluster state about the network even though it had removed its sandbox and endpoint. This test verifies that after a service gets added and removed that the ingress sandbox remains in a functional state. Signed-off-by: Chris Telfer <[email protected]>

This PR prevents automatic removal of the load balancing sandbox endpoint when the endpoint is the last one in the network but the network is marked as ingress. Signed-off-by: Chris Telfer <[email protected]>

Ingress networks will no longer automatically remove their load-balancing endpoint (and sandbox) automatically when the network is otherwise upopulated. This is to prevent automatic removal of the ingress networks when all the containers leave them. Therefore explicit removal of an ingress network also requires explicit removal of its load-balancing endpoint. Signed-off-by: Chris Telfer <[email protected]>

ctelfer · 2018-03-13T01:36:15Z

Looks like a test in the experimental tests failed. It appears to be an error in swarm teardown in TestAPISwarmRestartCluster. Is there a way to easily run just this test so I can try to reproduce it?

vdemeester

LGTM 🐅

thaJeztah · 2018-03-13T10:28:50Z

Full diff of libnetwork changes being brought in: moby/libnetwork@ed2130d...3aca383

Which includes:

Allow user to specify default address pools for docker networks libnetwork#2058 Allow user to specify default address pools for docker networks
- libnetwork changes related to Allow user to specify default address pools for docker networks #36396
Add warning message for the failure of deleting link device libnetwork#2085 Add warning message for the failure of deleting link device
Limit max length transient log libnetwork#2088 Limit max length transient log
fix for #1333, calling LinkDel to delete link device when the err is NULL libnetwork#2094 fix for testing, issue #1331: Add registry functional test to docker-ci #1333, calling LinkDel to delete link device when the err is NULL
Fix crash if the len was < maxSetStringLen libnetwork#2096 Fix crash if the len was < maxSetStringLen
Prevent ingress deletion when endpoint count == 1 libnetwork#2101 Prevent ingress deletion when endpoint count == 1
- (the changes related to this PR)

thaJeztah

SGTM, but left some questions/notes for possible follow-ups

thaJeztah · 2018-03-13T10:45:22Z

daemon/network.go

@@ -222,6 +222,8 @@ func (daemon *Daemon) releaseIngress(id string) {
 		return
 	}

+	daemon.deleteLoadBalancerSandbox(n)


Was looking why this didn't return errors, but looks like all errors are only logged as warnings. Perhaps (not for this PR) we should rename that function to tryDeleteLoadBalancerSandbox();

moby/daemon/network.go

Lines 505 to 534 in 0b0af85

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

controller := daemon.netController

//The only endpoint left should be the LB endpoint (nw.Name() + "-endpoint")

endpoints := n.Endpoints()

if len(endpoints) == 1 {

sandboxName := n.Name() + "-sbox"

info := endpoints[0].Info()

if info != nil {

sb := info.Sandbox()

if sb != nil {

if err := sb.DisableService(); err != nil {

logrus.Warnf("Failed to disable service on sandbox %s: %v", sandboxName, err)

//Ignore error and attempt to delete the load balancer endpoint

}

}

}

if err := endpoints[0].Delete(true); err != nil {

logrus.Warnf("Failed to delete endpoint %s (%s) in %s: %v", endpoints[0].Name(), endpoints[0].ID(), sandboxName, err)

//Ignore error and attempt to delete the sandbox.

}

if err := controller.SandboxDestroy(sandboxName); err != nil {

logrus.Warnf("Failed to delete %s sandbox: %v", sandboxName, err)

//Ignore error and attempt to delete the network.

}

}

}

What are the side-effects if deleting fails?

Should that function fail if len(endpoints) > 1 ?

Very fair question and one discussed a bit in moby/libnetwork#2097. Ultimately, this function was copied into libnetwork about 6 months ago but the original never got removed. As it was, this was dead code. There is a function of the same name and behavior in libnetwork and it is not exported. It would be possible to just export said function (changing the libnetwork API, but in a small way). However, given that
1. I'm working on a patch in the near term to refactor of load balancing to improve scalability
2. This refactoring might affect how we expose said function
3. We are trying to get this regression fixed for the immanent release
It seemed more prudent to make the smaller change to fix the regression and then clean up the duplicate code in the larger refactorization.

Thanks; yes it's not new code, so I'm good with keeping it for a follow-up. Just was looking at it, and it felt like a lot of error conditions were ignored, so left that comment for later investigation if needed. 👍

thaJeztah · 2018-03-13T10:55:45Z

integration/network/service_test.go

+	err = client.ServiceRemove(context.Background(), serviceID)
+	require.NoError(t, err)
+
+	poll.WaitOn(t, serviceIsRemoved(client, serviceID), pollSettings)


Not for this PR, but we should update testenv.ProtectAll() and testenv.Clean() to also cleanup services.

moby/internal/test/environment/protect.go

Lines 32 to 43 in 3e0299f

// ProtectAll protects the existing environment (containers, images, networks,

// volumes, and, on Linux, plugins) from being cleaned up at the end of test

// runs

func ProtectAll(t testingT, testEnv *Execution) {

ProtectContainers(t, testEnv)

ProtectImages(t, testEnv)

ProtectNetworks(t, testEnv)

ProtectVolumes(t, testEnv)

if testEnv.OSType == "linux" {

ProtectPlugins(t, testEnv)

}

}

moby/internal/test/environment/clean.go

Lines 25 to 42 in 3e0299f

// Clean the environment, preserving protected objects (images, containers, ...)

// and removing everything else. It's meant to run after any tests so that they don't

// depend on each others.

func (e *Execution) Clean(t testingT) {

client := e.APIClient()

platform := e.OSType

if (platform != "windows") || (platform == "windows" && e.DaemonInfo.Isolation == "hyperv") {

unpauseAllContainers(t, client)

}

deleteAllContainers(t, client, e.protectedElements.containers)

deleteAllImages(t, client, e.protectedElements.images)

deleteAllVolumes(t, client, e.protectedElements.volumes)

deleteAllNetworks(t, client, platform, e.protectedElements.networks)

if platform == "linux" {

deleteAllPlugins(t, client, e.protectedElements.plugins)

}

}

Opened #36581 for that

thaJeztah · 2018-03-13T11:06:08Z

integration/network/service_test.go

+	poll.WaitOn(t, swarmIngressReady(client), pollSettings)
+
+	var instances uint64 = 1
+	serviceName := "TestIngressService"


We should try to get rid of names where possible (at least be sure they're unique (could use t.Name() to make them slightly more unique)).

I see there's a couple of other locations already, so also for a follow-up

Opened #36583 for that ^^

thaJeztah · 2018-03-13T11:09:18Z

integration/network/service_test.go

+
+}
+
+const ingressNet = "ingress"


Do we also need a test with a custom ingress network?

That's a fair question. I'm not aware of any code that tags any other networks besides a swarm-mode network named "ingress" as an ingress network. Neither am I aware of our support for such a construct. However, that may just be my lack of experience w/ the product.

My use of the named constant was more out of a general purpose aversion to magic constants sprinkled through code. (i.e. collecting them to a named token makes refactoring easier and saner as a rule ... but will probably not be needed in this case)

Regarding "any code that tags any network besides swarm-mode network named "ingress" as an ingress network": you can create an overlay network named anything after removing existing ingress as long as it marked with --ingress flag. E.g. (after deleting existing ingress) docker network create --ingress -d overlay testingress works

thaJeztah · 2018-03-13T19:55:42Z

Okay, think this should be ready to go; we can address the other points in follow-ups (as discussed above)

ctelfer · 2018-03-13T20:15:42Z

Thanks for all the help!

ctelfer requested review from dnephin and vdemeester as code owners March 8, 2018 19:03

GordonTheTurtle added the status/0-triage label Mar 8, 2018

ctelfer mentioned this pull request Mar 8, 2018

Prevent ingress deletion when endpoint count == 1 moby/libnetwork#2101

Merged

ctelfer force-pushed the ingress-fix branch from e1d7b19 to 28a2117 Compare March 8, 2018 19:32

vdemeester added status/2-code-review and removed status/0-triage labels Mar 9, 2018

vdemeester requested review from vieux and thaJeztah March 9, 2018 08:22

mavenugo approved these changes Mar 9, 2018

View reviewed changes

ddebroy approved these changes Mar 9, 2018

View reviewed changes

fcrisciani approved these changes Mar 9, 2018

View reviewed changes

ctelfer mentioned this pull request Mar 9, 2018

Publish ports to ingress network after docker swarm init --force-new-cluster #34987

Open

yongtang added the rebuild/powerpc label Mar 11, 2018

GordonTheTurtle removed the rebuild/powerpc label Mar 11, 2018

vdemeester added the status/needs-vendoring label Mar 12, 2018

vdemeester suggested changes Mar 12, 2018

View reviewed changes

ctelfer force-pushed the ingress-fix branch from 28a2117 to caf673c Compare March 12, 2018 15:14

ctelfer added 3 commits March 12, 2018 15:19

Update vendoring for libnetwork PR moby#2097

bebad15

This PR prevents automatic removal of the load balancing sandbox endpoint when the endpoint is the last one in the network but the network is marked as ingress. Signed-off-by: Chris Telfer <[email protected]>

ctelfer force-pushed the ingress-fix branch from caf673c to 3da4ebf Compare March 12, 2018 19:29

vdemeester added the rebuild/experimental label Mar 13, 2018

GordonTheTurtle removed the rebuild/experimental label Mar 13, 2018

vdemeester approved these changes Mar 13, 2018

View reviewed changes

thaJeztah approved these changes Mar 13, 2018

View reviewed changes

thaJeztah added area/networking impact/changelog area/swarm and removed status/needs-vendoring labels Mar 13, 2018

thaJeztah merged commit 514fb6c into moby:master Mar 13, 2018

This was referenced Mar 13, 2018

Integration tests: Update testenv.ProtectAll() to also cleanup Services #36581

Open

Integration tests: don't use names where possible #36583

Open

ctelfer deleted the ingress-fix branch March 13, 2018 20:18

thaJeztah mentioned this pull request Mar 14, 2018

[18.03] Fix automatic removal of ingress sandbox when last service leaves docker-archive/docker-ce#463

Merged

ctelfer mentioned this pull request Mar 14, 2018

Removal of ingress networks when empty of other containers moby/libnetwork#2097

Closed

ctelfer mentioned this pull request Apr 18, 2018

Improve load balancing scalability in swarm mode moby/libnetwork#2138

Merged

thaJeztah mentioned this pull request Jul 2, 2019

Flaky test: TestServiceRemoveKeepsIngressNetwork #39426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix automatic removal of ingress sandbox when last service leaves #36538

Fix automatic removal of ingress sandbox when last service leaves #36538

ctelfer commented Mar 8, 2018

mavenugo left a comment

mavenugo commented Mar 9, 2018

ddebroy left a comment

fcrisciani left a comment

selansen commented Mar 9, 2018

codecov bot commented Mar 11, 2018 •

edited

Loading

vdemeester left a comment

ctelfer commented Mar 12, 2018

thaJeztah commented Mar 12, 2018

ctelfer commented Mar 12, 2018

ctelfer commented Mar 13, 2018

vdemeester left a comment

thaJeztah commented Mar 13, 2018 •

edited

Loading

thaJeztah left a comment

thaJeztah Mar 13, 2018

ctelfer Mar 13, 2018

thaJeztah Mar 13, 2018

thaJeztah Mar 13, 2018

thaJeztah Mar 13, 2018

thaJeztah Mar 13, 2018 •

edited

Loading

thaJeztah Mar 13, 2018

thaJeztah Mar 13, 2018

ctelfer Mar 13, 2018

ddebroy Mar 13, 2018

thaJeztah commented Mar 13, 2018

ctelfer commented Mar 13, 2018

	func (daemon *Daemon) deleteLoadBalancerSandbox(n libnetwork.Network) {
	controller := daemon.netController

	//The only endpoint left should be the LB endpoint (nw.Name() + "-endpoint")
	endpoints := n.Endpoints()
	if len(endpoints) == 1 {
	sandboxName := n.Name() + "-sbox"

	info := endpoints[0].Info()
	if info != nil {
	sb := info.Sandbox()
	if sb != nil {
	if err := sb.DisableService(); err != nil {
	logrus.Warnf("Failed to disable service on sandbox %s: %v", sandboxName, err)
	//Ignore error and attempt to delete the load balancer endpoint
	}
	}
	}

	if err := endpoints[0].Delete(true); err != nil {
	logrus.Warnf("Failed to delete endpoint %s (%s) in %s: %v", endpoints[0].Name(), endpoints[0].ID(), sandboxName, err)
	//Ignore error and attempt to delete the sandbox.
	}

	if err := controller.SandboxDestroy(sandboxName); err != nil {
	logrus.Warnf("Failed to delete %s sandbox: %v", sandboxName, err)
	//Ignore error and attempt to delete the network.
	}
	}
	}

	// ProtectAll protects the existing environment (containers, images, networks,
	// volumes, and, on Linux, plugins) from being cleaned up at the end of test
	// runs
	func ProtectAll(t testingT, testEnv *Execution) {
	ProtectContainers(t, testEnv)
	ProtectImages(t, testEnv)
	ProtectNetworks(t, testEnv)
	ProtectVolumes(t, testEnv)
	if testEnv.OSType == "linux" {
	ProtectPlugins(t, testEnv)
	}
	}

	// Clean the environment, preserving protected objects (images, containers, ...)
	// and removing everything else. It's meant to run after any tests so that they don't
	// depend on each others.
	func (e *Execution) Clean(t testingT) {
	client := e.APIClient()

	platform := e.OSType
	if (platform != "windows") \|\| (platform == "windows" && e.DaemonInfo.Isolation == "hyperv") {
	unpauseAllContainers(t, client)
	}
	deleteAllContainers(t, client, e.protectedElements.containers)
	deleteAllImages(t, client, e.protectedElements.images)
	deleteAllVolumes(t, client, e.protectedElements.volumes)
	deleteAllNetworks(t, client, platform, e.protectedElements.networks)
	if platform == "linux" {
	deleteAllPlugins(t, client, e.protectedElements.plugins)
	}
	}

Fix automatic removal of ingress sandbox when last service leaves #36538

Fix automatic removal of ingress sandbox when last service leaves #36538

Conversation

ctelfer commented Mar 8, 2018

mavenugo left a comment

Choose a reason for hiding this comment

mavenugo commented Mar 9, 2018

ddebroy left a comment

Choose a reason for hiding this comment

fcrisciani left a comment

Choose a reason for hiding this comment

selansen commented Mar 9, 2018

codecov bot commented Mar 11, 2018 • edited Loading

Codecov Report

vdemeester left a comment

Choose a reason for hiding this comment

ctelfer commented Mar 12, 2018

thaJeztah commented Mar 12, 2018

ctelfer commented Mar 12, 2018

ctelfer commented Mar 13, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 13, 2018 • edited Loading

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

thaJeztah Mar 13, 2018 • edited Loading

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 Mar 13, 2018

ctelfer commented Mar 13, 2018

codecov bot commented Mar 11, 2018 •

edited

Loading

thaJeztah commented Mar 13, 2018 •

edited

Loading

thaJeztah Mar 13, 2018 •

edited

Loading