From a75ba12203d85e39f128cb62d4e12f7372c7570f Mon Sep 17 00:00:00 2001 From: Flavio Crisciani Date: Thu, 12 Oct 2017 21:41:29 -0700 Subject: [PATCH 1/2] Handle cleanup DNS for attachable container Attachable containers they are tasks with no service associated their cleanup was not done properly so it was possible to have a leak of their name resolution if that was the last container on the network. Cleanupservicebindings was not able to do the cleanup because there is no service, while also the notification of the delete arrives after that the network is already being cleaned Signed-off-by: Flavio Crisciani (cherry picked from commit 1c04e1980dfd6debab6e43d9db672c0f01528868) Conflicts: service_common.go Signed-off-by: Andrew Hsu --- controller.go | 1 + network.go | 4 ++++ networkdb/networkdb.go | 5 ++++- service_common.go | 22 +++++++++++++--------- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/controller.go b/controller.go index 95b46bcdd0..e3b6f77754 100644 --- a/controller.go +++ b/controller.go @@ -336,6 +336,7 @@ func (c *controller) clusterAgentInit() { // should still be present when cleaning up // service bindings c.agentClose() + c.cleanupServiceDiscovery("") c.cleanupServiceBindings("") c.clearIngress(true) diff --git a/network.go b/network.go index 998df6072d..7394b9ec48 100644 --- a/network.go +++ b/network.go @@ -830,6 +830,10 @@ func (n *network) delete(force bool) error { logrus.Errorf("Failed leaving network %s from the agent cluster: %v", n.Name(), err) } + // Cleanup the service discovery for this network + c.cleanupServiceDiscovery(n.ID()) + + // Cleanup the load balancer c.cleanupServiceBindings(n.ID()) // deleteFromStore performs an atomic delete operation and the diff --git a/networkdb/networkdb.go b/networkdb/networkdb.go index 24ea31526f..1b1b25eb8b 100644 --- a/networkdb/networkdb.go +++ b/networkdb/networkdb.go @@ -490,7 +490,10 @@ func (nDB *NetworkDB) deleteNodeNetworkEntries(nid, node string) { // without doing a delete of all the objects entry.ltime++ } - nDB.createOrUpdateEntry(nid, tname, key, entry) + + if !oldEntry.deleting { + nDB.createOrUpdateEntry(nid, tname, key, entry) + } } else { // the local node is leaving the network, all the entries of remote nodes can be safely removed nDB.deleteEntry(nid, tname, key) diff --git a/service_common.go b/service_common.go index c8155ca236..d6aca20936 100644 --- a/service_common.go +++ b/service_common.go @@ -141,6 +141,19 @@ func newService(name string, id string, ingressPorts []*PortConfig, serviceAlias } } +// cleanupServiceDiscovery when the network is being deleted, erase all the associated service discovery records +func (c *controller) cleanupServiceDiscovery(cleanupNID string) { + c.Lock() + defer c.Unlock() + if cleanupNID == "" { + logrus.Debugf("cleanupServiceDiscovery for all networks") + c.svcRecords = make(map[string]svcInfo) + return + } + logrus.Debugf("cleanupServiceDiscovery for network:%s", cleanupNID) + delete(c.svcRecords, cleanupNID) +} + func (c *controller) cleanupServiceBindings(cleanupNID string) { var cleanupFuncs []func() @@ -164,15 +177,6 @@ func (c *controller) cleanupServiceBindings(cleanupNID string) { continue } - // The network is being deleted, erase all the associated service discovery records - // TODO(fcrisciani) separate the Load Balancer from the Service discovery, this operation - // can be done safely here, but the rmServiceBinding is still keeping consistency in the - // data structures that are tracking the endpoint to IP mapping. - c.Lock() - logrus.Debugf("cleanupServiceBindings erasing the svcRecords for %s", nid) - delete(c.svcRecords, nid) - c.Unlock() - for eid, ip := range lb.backEnds { epID := eid epIP := ip From b2109eec7458130cac2b31134a5f1ce3050f8eec Mon Sep 17 00:00:00 2001 From: Flavio Crisciani Date: Thu, 19 Oct 2017 13:36:29 +0200 Subject: [PATCH 2/2] Add test for cleanupServiceDiscovery Unit test for the cleanupServiceDiscovery, follow up of PR: #1985 Signed-off-by: Flavio Crisciani (cherry picked from commit 52a9ab55ce92a541798d130a87a84ca34f0b6178) Signed-off-by: Andrew Hsu --- service_common_test.go | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 service_common_test.go diff --git a/service_common_test.go b/service_common_test.go new file mode 100644 index 0000000000..3a0c2f1807 --- /dev/null +++ b/service_common_test.go @@ -0,0 +1,45 @@ +package libnetwork + +import ( + "net" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCleanupServiceDiscovery(t *testing.T) { + c, err := New() + require.NoError(t, err) + defer c.Stop() + + n1, err := c.NewNetwork("bridge", "net1", "", nil) + require.NoError(t, err) + defer n1.Delete() + + n2, err := c.NewNetwork("bridge", "net2", "", nil) + require.NoError(t, err) + defer n2.Delete() + + n1.(*network).addSvcRecords("N1ep1", "service_test", "serviceID1", net.ParseIP("192.168.0.1"), net.IP{}, true, "test") + n1.(*network).addSvcRecords("N2ep2", "service_test", "serviceID2", net.ParseIP("192.168.0.2"), net.IP{}, true, "test") + + n2.(*network).addSvcRecords("N2ep1", "service_test", "serviceID1", net.ParseIP("192.168.1.1"), net.IP{}, true, "test") + n2.(*network).addSvcRecords("N2ep2", "service_test", "serviceID2", net.ParseIP("192.168.1.2"), net.IP{}, true, "test") + + if len(c.(*controller).svcRecords) != 2 { + t.Fatalf("Service record not added correctly:%v", c.(*controller).svcRecords) + } + + // cleanup net1 + c.(*controller).cleanupServiceDiscovery(n1.ID()) + + if len(c.(*controller).svcRecords) != 1 { + t.Fatalf("Service record not cleaned correctly:%v", c.(*controller).svcRecords) + } + + c.(*controller).cleanupServiceDiscovery("") + + if len(c.(*controller).svcRecords) != 0 { + t.Fatalf("Service record not cleaned correctly:%v", c.(*controller).svcRecords) + } +}