Service discovery race on serviceBindings delete. Bug on IP reuse #1808

fcrisciani · 2017-06-16T01:07:20Z

Found an issue where the deletion of the entry from serviceBindings map inside the rmServiceBinding was creating a possible race with a new addServiceBindings.
The deletion from the map was allowing a new go routine to create a completely new service object and race with the final part of the rmServiceBinding.

Another issue identified was the possible reuse of an IP from a service in the svcRecords map. This table is keyed by network ID, so services if they are reusing the same name but different service id cannot be distinguished when they are creating or deleting entries. In a perturbance scenario where a service is updated, notification coming from endpoints that are remote can be delayed and cause state corruption on the new service with local endpoints. To address this issue, move all the map to SetMatrix that will allow to handle gracefully the transition between services.
Added a unit test for this case where 2 services with same name share same IP, the test validates that the deletion of one service does not compromise the service lookup.

fcrisciani · 2017-06-16T01:45:31Z

network.go

@@ -1376,19 +1381,31 @@ func (n *network) getSvcRecords(ep *endpoint) []etchosts.Record {

 	n.ctrlr.Lock()
 	defer n.ctrlr.Unlock()


not sure if makes sense here keep the controller lock only for the resolution, have to check. The SetMatrix is already go routine safe and should be enough

mavenugo

As I take some more time reviewing the changes, pls take a look at the comments.

Also I think @sanimej should take a look at this and comment as well.

mavenugo · 2017-06-17T02:18:35Z

network.go

@@ -1228,75 +1236,76 @@ func (n *network) updateSvcRecord(ep *endpoint, localEps []*endpoint, isAdd bool
 			ipv6 = iface.AddressIPv6().IP
 		}

+		serviceID := ep.svcID


Since this method is a generic method for both containers and services, one cannot assume that serviceID will be a valid one. Hence, I think the code change serviceID = ep.ID() should be brought here if serviceID == "".

will move up

mavenugo · 2017-06-17T02:32:52Z

service_common.go

 	}

 	// Add service name to vip in DNS, if vip is valid. Otherwise resort to DNS RR
 	if len(vip) == 0 {
-		n.(*network).addSvcRecords(eID, svcName, ip, nil, false, method)
+		n.(*network).addSvcRecords(eID, svcName, eID, ip, nil, false, method)


Why is DNS-RR treated as an attachable container use-case, while the svcID can still be valid.

should I add the same logic at the top:
var serviceID string
if svcID == "" {
serviceID = eID
}
WDYT?

mavenugo · 2017-06-17T02:33:14Z

service_common.go

 		}
 	}

 	// If we are doing DNS RR delete the endpoint IP from DNS record right away.
 	if !multipleEntries && len(vip) == 0 {
-		n.(*network).deleteSvcRecords(eID, svcName, ip, nil, false, method)
+		n.(*network).deleteSvcRecords(eID, svcName, eID, ip, nil, false, method)


mavenugo · 2017-06-17T02:34:20Z

network.go

-			Hosts: h,
-			IP:    ip[0].String(),
+			Hosts: k,
+			IP:    mapEntryList[0].(svcMapEntry).ip,
 		})


I have to spend a bit more time reviewing these changes.

Not sure if helps but in words:
line 1389: get all the names in the dns
line 1391: for each name in the dns
line 1396: get all the ip associated to it as a Set
line 1406: Create a record with the first IP that is in the Set

The SetMatrix is a generic data structure, so the description should not be tight to any specific use Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

- Added logic to handle name reuse from different services - Moved the deletion from the serviceBindings map at the end of the rmServiceBindings body to avoid race with new services Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Use the locker to avoid the race between the network deletion and new endpoints being created Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Allow the cleanupServicebindings to take care of the service discovery cleanup. Also avoid to trigger the cleanup for each endpoint from an SD point of view LB and SD will be separated in the future Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

If there is an error locally guarantee that the delete entry on network DB is still honored Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

mavenugo

LGTM

…by#1808) * Correct SetMatrix documentation The SetMatrix is a generic data structure, so the description should not be tight to any specific use Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> * Service Discovery reuse name and serviceBindings deletion - Added logic to handle name reuse from different services - Moved the deletion from the serviceBindings map at the end of the rmServiceBindings body to avoid race with new services Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> * Avoid race on network cleanup Use the locker to avoid the race between the network deletion and new endpoints being created Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> * CleanupServiceBindings to clean the SD records Allow the cleanupServicebindings to take care of the service discovery cleanup. Also avoid to trigger the cleanup for each endpoint from an SD point of view LB and SD will be separated in the future Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> * Addressed comments Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> * NetworkDB deleteEntry has to happen If there is an error locally guarantee that the delete entry on network DB is still honored Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com> (cherry picked from commit 6426d1e)

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

fcrisciani commented Jun 16, 2017

View reviewed changes

This was referenced Jun 16, 2017

Backport sd race network #1809

Closed

Backport sd race network #1812

Merged

mavenugo reviewed Jun 17, 2017

View reviewed changes

Flavio Crisciani added 2 commits June 17, 2017 16:34

Correct SetMatrix documentation

5e91a95

The SetMatrix is a generic data structure, so the description should not be tight to any specific use Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the sd-race-network branch from 5e657f6 to 633e4f4 Compare June 17, 2017 23:49

Flavio Crisciani added 3 commits June 17, 2017 16:58

Avoid race on network cleanup

ee1326a

Use the locker to avoid the race between the network deletion and new endpoints being created Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Addressed comments

5fd5257

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the sd-race-network branch from 633e4f4 to 5fd5257 Compare June 17, 2017 23:59

NetworkDB deleteEntry has to happen

201b241

If there is an error locally guarantee that the delete entry on network DB is still honored Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the sd-race-network branch from 79cdf38 to 201b241 Compare June 18, 2017 05:32

mavenugo approved these changes Jun 18, 2017

View reviewed changes

mavenugo merged commit 6426d1e into moby:master Jun 18, 2017

fcrisciani deleted the sd-race-network branch June 18, 2017 15:11

benturner mentioned this pull request Jun 19, 2017

Support service aliases that differ by container #1814

Open

andrewhsu mentioned this pull request Jun 20, 2017

17.06.0 RC5 tracker docker/for-linux#37

Closed

6 tasks

tiborvass pushed a commit to tiborvass/docker-ce that referenced this pull request Jun 20, 2017

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd

5578ebd

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass mentioned this pull request Jun 20, 2017

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd docker-archive/docker-ce#85

Merged

sanimej mentioned this pull request Jun 23, 2017

Docker swarm DNS periodically fails moby/moby#33721

Closed

silvin-lubecki pushed a commit to silvin-lubecki/docker-ce that referenced this pull request Feb 3, 2020

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd

ba2463a

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

silvin-lubecki pushed a commit to silvin-lubecki/engine-extract that referenced this pull request Feb 3, 2020

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd

a30399b

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

silvin-lubecki pushed a commit to silvin-lubecki/engine-extract that referenced this pull request Mar 10, 2020

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd

2937376

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

silvin-lubecki pushed a commit to silvin-lubecki/engine-extract that referenced this pull request Mar 23, 2020

[engine] Vendor libnetwork to 20bf4e23da1bb3f9d3dc70e4cf531b84c45947bd

5c27bce

Brings in this pull request: - moby/libnetwork#1808 Signed-off-by: Tibor Vass <tibor@docker.com>

thaJeztah mentioned this pull request Jan 30, 2023

libnetwork: fix docker is incredibly slow to start many containers moby/moby#44887

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service discovery race on serviceBindings delete. Bug on IP reuse #1808

Service discovery race on serviceBindings delete. Bug on IP reuse #1808

fcrisciani commented Jun 16, 2017

fcrisciani Jun 16, 2017 •

edited

Loading

mavenugo left a comment

mavenugo Jun 17, 2017

fcrisciani Jun 17, 2017

mavenugo Jun 17, 2017

fcrisciani Jun 17, 2017

mavenugo Jun 17, 2017

fcrisciani Jun 17, 2017

mavenugo Jun 17, 2017

fcrisciani Jun 17, 2017

mavenugo left a comment

		@@ -1376,19 +1381,31 @@ func (n network) getSvcRecords(ep endpoint) []etchosts.Record {

		n.ctrlr.Lock()
		defer n.ctrlr.Unlock()

Service discovery race on serviceBindings delete. Bug on IP reuse #1808

Service discovery race on serviceBindings delete. Bug on IP reuse #1808

Conversation

fcrisciani commented Jun 16, 2017

fcrisciani Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

mavenugo 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

mavenugo left a comment

Choose a reason for hiding this comment

fcrisciani Jun 16, 2017 •

edited

Loading