Netlink timeouts #1976

fcrisciani · 2017-10-10T20:35:36Z

Add netlink timeouts to all the netlink operations
Vendored netlink library to pick the required methods (temporary my branch)

fcrisciani · 2017-10-10T20:36:49Z

@mavenugo can you give a pass to the changes, the risk is that the new vendoring of the netlink is pretty heavy in changes. We should try to reproduce the issue and then apply this patch

codecov-io · 2017-10-10T20:51:53Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #1976   +/-   ##
=========================================
  Coverage          ?   38.94%           
=========================================
  Files             ?      137           
  Lines             ?    27390           
  Branches          ?        0           
=========================================
  Hits              ?    10666           
  Misses            ?    15426           
  Partials          ?     1298

Impacted Files	Coverage Δ
ipvs/netlink.go	`0% <0%> (ø)`
ipvs/ipvs.go	`0% <0%> (ø)`
drivers/overlay/ov_network.go	`2.07% <33.33%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 460ac8f...e78b4ff. Read the comment docs.

andrewhsu · 2017-11-08T23:59:14Z

How do I make sure this PR does the fix? cc @antonybichon17

fcrisciani · 2017-11-09T00:48:38Z

@andrewhsu the deadlock condition triggered on socket closed is tested in the netlink library project. This commit simply uses the same timeout

antonybichon17 · 2017-11-09T16:55:04Z

You are using the timeout in a different part of the code, triggered by different actions.
So from your perspective @fcrisciani no additional tests are needed to cover the changes you made?

fcrisciani · 2017-11-09T22:21:14Z

@antonybichon17 for the validation IMHO we should try to reproduce the customer deadlock that would be the best way to have the validation.
I will try to see if there can an easy way to add a test here to but still would not be the case experienced by the customer, but more the same unit test of the library

fcrisciani · 2017-11-10T06:02:28Z

drivers/overlay/ov_network.go

@@ -713,14 +719,17 @@ func (n *network) watchMiss(nlSock *nl.NetlinkSocket) {
 	t := time.Now()
 	for {
 		msgs, err := nlSock.Receive()
-		if err != nil {
+		// When the receive timeout expires the receive will return EAGAIN
+		if err == syscall.EAGAIN {
 			n.Lock()
 			nlFd := nlSock.GetFd()
 			n.Unlock()
 			if nlFd == -1 {
 				// The netlink socket got closed, simply exit to not leak this goroutine
 				return
 			}


better to add a continue here to avoid spam logs

fcrisciani · 2017-11-17T23:16:56Z

@antonybichon17 test added

vieux

LGTM

thaJeztah · 2017-11-20T14:21:08Z

ping @mavenugo @abhi PTAL

abhi · 2017-11-29T18:26:41Z

drivers/overlay/ov_network.go

@@ -713,14 +719,18 @@ func (n *network) watchMiss(nlSock *nl.NetlinkSocket) {
 	t := time.Now()
 	for {
 		msgs, err := nlSock.Receive()
-		if err != nil {
+		// When the receive timeout expires the receive will return EAGAIN
+		if err == syscall.EAGAIN {


minor nit

if err != nil{ if err == syscall.EAGAIN{ .... } .... }

abhi · 2017-11-29T18:29:50Z

ipvs/ipvs.go

 	"github.com/vishvananda/netlink/nl"
 	"github.com/vishvananda/netns"
 )

+var netlinkSocketsTimeout = 3 * time.Second


For my understanding is this a standard timeout ?

nope is decided from us, the tradeoff is between waking up too ofter or being block too much time. Don't have an opinion on how much is the "right" time here

abhi · 2017-11-29T18:30:02Z

ipvs/ipvs.go

+	if err := sock.SetReceiveTimeout(&tv); err != nil {
+		return nil, err
+	}
+	if err := sock.SetReceiveTimeout(&tv); err != nil {


why is this done twice ?

good catch it should be ReceiveTimeout and SendTimeout

Shouldn't netlinkSendSocketTimeout be netlinkRecvSocketsTimeout here ?

abhi · 2017-11-29T18:30:50Z

ipvs/netlink.go

-	)
-
-	if err := s.Send(req); err != nil {
+	err := s.Send(req)


nit :

if err := s.Send(req);err != nil { ..... }

abhi · 2017-11-29T18:31:31Z

ipvs/netlink.go

@@ -221,6 +218,12 @@ func execute(s *nl.NetlinkSocket, req *nl.NetlinkRequest, resType uint16) ([][]b
 done:
 	for {
 		msgs, err := s.Receive()
+		if err == syscall.EAGAIN {


same as above.

if err != nil { if err == syscall.EAGAIN { .... } .... }

fcrisciani · 2017-11-30T21:01:54Z

ipvs/ipvs.go

 	"github.com/vishvananda/netlink/nl"
 	"github.com/vishvananda/netns"
 )

+const (
+	netlinkRecvSocketsTimeout = 3 * time.Second
+	netlinkSendSocketTimeout  = 30 * time.Second


this is set to ah higher value to avoid timeout error in case the system is busy. I noticed that the error coming from the write failure to IPVS is not percolated up, so if the addLBBackend backend failed in the configuration, the error is not handled at all. I will follow up on this on a different PR though because the change will be potentially non trivial.

mavenugo · 2017-12-04T17:28:09Z

drivers/overlay/ov_network.go

-			n.Unlock()
-			if nlFd == -1 {
-				// The netlink socket got closed, simply exit to not leak this goroutine
-				return


Shouldnt we retain this logic for all cases ?

In case the file descriptor of the netlink socket is closed the recvfrom is not returning. This may create deadlock conditions. The current solution is to make sure that all the netlink socket used have a proper timeout set on them to have the possibility to return Added test to emulate the watchMiss condition Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

- needed the methods to set the proper timeout Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

mavenugo

LGTM

fcrisciani force-pushed the netlink-timeouts branch from bb38615 to 20c86db Compare October 23, 2017 07:47

fcrisciani requested review from abhi, vieux and mavenugo October 23, 2017 08:44

fcrisciani commented Nov 10, 2017

View reviewed changes

fcrisciani mentioned this pull request Nov 16, 2017

Fix leak of watchMiss goroutine #1786

Merged

fcrisciani force-pushed the netlink-timeouts branch 2 times, most recently from a8dd1c0 to e78b4ff Compare November 17, 2017 23:16

fcrisciani requested a review from antonybichon17 November 17, 2017 23:17

vieux approved these changes Nov 17, 2017

View reviewed changes

abhi reviewed Nov 29, 2017

View reviewed changes

fcrisciani force-pushed the netlink-timeouts branch 2 times, most recently from 19f93aa to fc7323d Compare November 30, 2017 20:50

fcrisciani commented Nov 30, 2017

View reviewed changes

fcrisciani mentioned this pull request Dec 4, 2017

Swarm memory issues after lots of one-off containers moby/moby#35261

Closed

mavenugo reviewed Dec 4, 2017

View reviewed changes

Flavio Crisciani added 2 commits December 4, 2017 09:40

Vendor latest netlink library

45ea903

- needed the methods to set the proper timeout Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the netlink-timeouts branch from fc7323d to 45ea903 Compare December 4, 2017 17:40

mavenugo approved these changes Dec 4, 2017

View reviewed changes

mavenugo merged commit 16c634f into moby:master Dec 4, 2017

fcrisciani mentioned this pull request Jan 4, 2018

[Backport 17.06] Netlink socket timeouts #2044

Merged

thaJeztah mentioned this pull request Apr 2, 2018

Netdb debug tool moby/moby#35677

Merged

fcrisciani mentioned this pull request Jun 27, 2018

too many libnetwork-setkey process suspending, create new container timeout due to possible dead lock #2208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netlink timeouts #1976

Netlink timeouts #1976

fcrisciani commented Oct 10, 2017 •

edited

Loading

fcrisciani commented Oct 10, 2017

codecov-io commented Oct 10, 2017 •

edited

Loading

andrewhsu commented Nov 8, 2017

fcrisciani commented Nov 9, 2017

antonybichon17 commented Nov 9, 2017

fcrisciani commented Nov 9, 2017

fcrisciani Nov 10, 2017

fcrisciani commented Nov 17, 2017

vieux left a comment

thaJeztah commented Nov 20, 2017

abhi Nov 29, 2017

abhi Nov 29, 2017

fcrisciani Nov 29, 2017

abhi Nov 29, 2017

fcrisciani Nov 29, 2017

mavenugo Dec 4, 2017

abhi Nov 29, 2017

abhi Nov 29, 2017

fcrisciani Nov 30, 2017

mavenugo Dec 4, 2017

mavenugo left a comment

Netlink timeouts #1976

Netlink timeouts #1976

Conversation

fcrisciani commented Oct 10, 2017 • edited Loading

fcrisciani commented Oct 10, 2017

codecov-io commented Oct 10, 2017 • edited Loading

Codecov Report

andrewhsu commented Nov 8, 2017

fcrisciani commented Nov 9, 2017

antonybichon17 commented Nov 9, 2017

fcrisciani commented Nov 9, 2017

Choose a reason for hiding this comment

fcrisciani commented Nov 17, 2017

vieux left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 20, 2017

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

mavenugo left a comment

Choose a reason for hiding this comment

fcrisciani commented Oct 10, 2017 •

edited

Loading

codecov-io commented Oct 10, 2017 •

edited

Loading