WIP: Connection Manager #4288

whyrusleeping · 2017-10-06T16:06:37Z

This is the first WIP branch of the connection manager. It allows you to set up a ConnMgr that tries to keep the number of open connections your node has within certain bounds.

Theres still a lot to do here.

whyrusleeping · 2017-10-08T17:05:42Z

supplementary: libp2p/go-libp2p-kad-dht#95

whyrusleeping · 2017-10-08T17:16:06Z

also: libp2p/go-libp2p-circuit#20

whyrusleeping · 2017-10-08T17:19:33Z

proposal:

Merge this roughly as is (without connection tagging logic) so that we can start using it in 'dumb' mode asap. Then, PR the different connection tagging things in individually.

whyrusleeping · 2017-10-08T17:31:47Z

note: disabled by default, you will need to set up the configuration as defined in the config.md doc

magik6k

Looks mostly good here. I still need to dig more into the libp2p side of things.

magik6k · 2017-10-08T18:04:54Z

core/core.go

@@ -814,6 +839,10 @@ func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr
 	if !opts.DisableNatPortMap {
 		hostOpts = append(hostOpts, p2pbhost.NATPortMap)
 	}
+	if opts.ConnectionManager != nil {
+		fmt.Println("adding conn manager to host opts")


Looks like debugging leftover.

magik6k · 2017-10-08T18:05:25Z

core/core.go

+	switch cfg.Type {
+	case "", "none":
+		return nil, nil
+	default:


This should probably be on the bottom of the switch

magik6k · 2017-10-08T18:13:03Z

docs/config.md

+LowWater is the minimum number of connections to maintain.
+
+- `HighWater`
+HighWater is the number of connections that, when exceeded, will trigger a connection GC operation.


I don't really like these names. Wouldn't simple Low/High be enough?

Hrm... I'd something a little more descriptive than that. I've seen these terms used in this context before. But i'm open to suggestions

How about a simple Min and Max or maybe MinConn or MaxConn?

Watermark is something that is familiar to me in terms of setting upper and lower bounds to something in programming. No special feelings attached though.

Note: I'd avoid min/low as they imply that we'll actively seek peers up to this point.

So... are we in favor of still using HighWater and LowWater ? I'm getting mixed signals (especially from @Stebalien who thumbs upped davids comment, but suggested changing things).

I agree that "min" would imply we actively open connections until we have that many, but I think "low" avoids this connotation.

(GC)Treshold(Conn/Peers), (GC)Target(Conn/Peers)? Water may sound confusing for some even if it's technically correct.

magik6k · 2017-10-08T18:19:19Z

Note: connections used in ipfs p2p may need tagging

whyrusleeping · 2017-10-08T20:20:29Z

Maybe we should actually have this enabled by default, limit people to 900-1000 connections or so. Even if it does cause potentially strange behaviour, its probably better than endless "too many open file descriptor" errors...

whyrusleeping · 2017-10-09T13:14:16Z

For reference, the connection closing logic is here: https://github.com/libp2p/go-libp2p-connmgr

whyrusleeping · 2017-10-09T13:23:03Z

To set the config, run:

ipfs config --json Swarm.ConnMgr '{"GracePeriod": "30s","HighWater": 1000,"LowWater": 900,"Type": "basic"}'

daviddias · 2017-10-09T13:19:04Z

core/core.go

@@ -380,7 +404,7 @@ func setupDiscoveryOption(d config.Discovery) DiscoveryOption {
 			if d.MDNS.Interval == 0 {
 				d.MDNS.Interval = 5
 			}
-			return discovery.NewMdnsService(ctx, h, time.Duration(d.MDNS.Interval)*time.Second)
+			return discovery.NewMdnsService(ctx, h, time.Duration(d.MDNS.Interval)*time.Second, discovery.ServiceTag)


Is this change related to this PR?

No, it just happened to change in a dependency that i needed to update here

daviddias · 2017-10-09T13:21:42Z

core/core.go

 	hostopts := &ConstructPeerHostOpts{
 		AddrsFactory:      addrsFactory,
 		DisableNatPortMap: cfg.Swarm.DisableNatPortMap,
 		DisableRelay:      cfg.Swarm.DisableRelay,
 		EnableRelayHop:    cfg.Swarm.EnableRelayHop,
+		ConnectionManager: connmgr,


Do we need to instantiate the ConnManager here? As a user of libp2p, I would assume that ConnManager is just a thing that exists and that all I need to do is pass an option to select the strategy, saving users from yet importing another package.

It might be nice to have a libp2p-bundle package that imports everything all together, but for now the tying together of all the pieces is done here in the core constructor. Importing things at the top level as much as possible makes dependency tree modifications much cheaper. If this were done in a lower package, then every time i make a change to the connection manager logic, i have to update that package (or packages) and then update those into here.

daviddias · 2017-10-09T13:23:21Z

docs/config.md

+Sets the type of connection manager to use, options are: `"none"` and `"basic"`.
+
+- `LowWater`
+LowWater is the minimum number of connections to maintain.


How is lowWater enforced? Does ConnManager run Discovery Mechanisms if we are poorly connected? (Didn't see this in the go-libp2p-connmgr code)

No, its not a 'minimum number of connections' its just the point at which we stop closing connections. So if LowWater is 900 and HighWater is 1000, we will trigger a Trim once we have over 1000, and then close connections until we have 900 or less

daviddias · 2017-10-09T13:24:19Z

docs/config.md

+LowWater is the minimum number of connections to maintain.
+
+- `HighWater`
+HighWater is the number of connections that, when exceeded, will trigger a connection GC operation.


Watermark is something that is familiar to me in terms of setting upper and lower bounds to something in programming. No special feelings attached though.

daviddias · 2017-10-09T13:25:12Z

docs/config.md

+- `HighWater`
+HighWater is the number of connections that, when exceeded, will trigger a connection GC operation.
+- `GracePeriod`
+GracePeriod is the length of time that new connections are immune from being closed by the connection manager.


I assume this is in seconds? Can get that written down?

Its a time duration (like all the other time related things). 30s == 30 seconds, 1h == one hour, 4m == 4 minutes, etc

daviddias · 2017-10-09T13:27:03Z

p2p/p2p.go

@@ -11,7 +11,7 @@ import (
 	ma "gx/ipfs/QmXY77cVe7rVRQXZZQRioukUM7aRW3BTcAgJe12MCtb3Ji/go-multiaddr"
 	peer "gx/ipfs/QmXYjuNuxVzXKJCfWasQk1RqkhVLDM9jtUKhqc2WPQmFSB/go-libp2p-peer"
 	pro "gx/ipfs/QmZNkThpqfVXs9GNbexPrfBbXSLNYeKrE7jwFM2oqHbyqN/go-libp2p-protocol"
-	p2phost "gx/ipfs/QmaSxYRuMq4pkpBBG2CYaRrPx2z7NmMVEs34b9g61biQA6/go-libp2p-host"
+	p2phost "gx/ipfs/Qmc1XhrFEiSeBNn3mpfg6gEuYCt5im2gYmNVmncsvmpeAk/go-libp2p-host"


Curiosity, in go, interface packages don't get an interface name?

No, they do. But this is where we're instantiating the concrete type.

daviddias · 2017-10-09T13:29:39Z

I see that https://github.com/libp2p/go-libp2p-connmgr/blob/master/connmgr.go keeps track of the peers to whom we opened connections. I guess in go memory is very cheap, in our JS implementation I would/will implement this ConnManager as just checking everything that is on PeerBook (your PeerStore) and using the .isConnected a method in each PeerInfo instance to check that, just to save allocating more memory to store the same info and avoiding having to update in 2 places.

whyrusleeping · 2017-10-09T13:32:48Z

I see that https://github.com/libp2p/go-libp2p-connmgr/blob/master/connmgr.go keeps track of the peers to whom we opened connections.

Primarily, it keeps track of tags on each connection.

Stebalien

So, it's probably not too hard for a determined attacker to DoS an IPFS node but the grace period seems a bit too easy to exploit.

In the future, we can use some form of simple proof of work to ensure peers actually want to talk to us (under high loads) but, for now, the simplest solution I can think of is to limit the number of nodes in the grace period set (note: connections from our node should always have a grace period). It's still possible to make it hard to establish new inbound connections but that can already be an issue.

Stebalien · 2017-10-09T23:54:33Z

docs/config.md

+LowWater is the minimum number of connections to maintain.
+
+- `HighWater`
+HighWater is the number of connections that, when exceeded, will trigger a connection GC operation.


Note: I'd avoid min/low as they imply that we'll actively seek peers up to this point.

Stebalien · 2017-10-09T23:55:38Z

docs/config.md

+- `HighWater`
+HighWater is the number of connections that, when exceeded, will trigger a connection GC operation.
+- `GracePeriod`
+GracePeriod is a time duration that new connections are immune from being closed by the connection manager.


Unless I'm mistaken, this can trivially be used to DOS a node.

I wouldnt call it trivially, but yes, you can use this to exceed a nodes maximum connection count and make a connection closing sweep not reach the low water mark.

Sorry, for some reason, I assumed we had a hard "don't open new connections past the high water mark" limit (I was a bit sleep deprived due to jet lag...). That would have effectively taken the node offline.

However, still worried that this will be a way to monopolize a peer by tying up their connection quota with connections in the grace period. Basically, you can ensure that no connection lasts more than 10 seconds. Maybe don't count connections in the grace period?

Yeah, connections in the grace period are basically not counted, see here: https://github.com/libp2p/go-libp2p-connmgr/blob/master/connmgr.go#L61

I see, so we literally just skip them. Nevermind, LGTM.

dignifiedquire

Interface looks good to me. I trust all of you to make the right decision on figuring out the names for the marks ;)

Kubuxu · 2017-10-11T16:59:10Z

Observations from some of our nodes, connection closing was run

pluto, which is one of bootstrap nodes: has started maxing out its CPU when connection closing - it was almost maxing out its CPU when it was not closing too. Fetching files from pluto is almost impossible but it was hard before that.
biham, which is one of the storage hosts: I have observed significant reduction in CPU, FDs and RAM usage and it is still responsive in regards of fetching files. Connection closing for it is great addition.

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping · 2017-10-14T15:30:44Z

ping @diasdavid @lgierth @Kubuxu for review (or response to previous review follow up)

ghost · 2017-10-14T19:50:05Z

package.json

      "name": "go-testutil",
-      "version": "1.1.12"
+      "version": "1.1.11"


Is go-testutil being downgraded?

hrm... thats probably the fault of a rebase gone rogue. I'll take care of it.

Ah, its because we accidentally updated go-testutil in master without bubbling the changes from the rest of the deps at the same time. I think this change is safe for now, we can bubble up those deps and fix things later.

ghost

LGTM 👏 👍

One tiny comment about the go-testutil downgrade

Kubuxu

This part LGTM, I am still not done with reviewing some of the dep changes but as far as I got 👍

Kubuxu · 2017-10-14T22:20:19Z

Only comment I have is that it would be good to have info about this feature returned somewhere from the API and have a test that config is being loaded correctly.

whyrusleeping · 2017-10-14T22:29:49Z

@Kubuxu

Only comment I have is that it would be good to have info about this feature returned somewhere from the API and have a test that config is being loaded correctly.

Any suggestions here?

ghost · 2017-10-16T13:38:45Z

Any suggestions here?

The network section in ipfs diag sys would do

whyrusleeping · 2017-10-16T16:03:48Z

@Kubuxu i think we should merge this now, and do the api introspection as a separate PR. Theres a fair amount of discussion to be had around that (i realize as i write it).

whyrusleeping · 2017-10-16T16:43:06Z

see #4308

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

Kubuxu · 2017-10-16T16:56:26Z

I am not blocking this PR. I just raised a note that we should have it.

whyrusleeping · 2017-10-16T17:36:59Z

Choo Choo!

Kubuxu · 2017-10-16T18:40:03Z

https://www.youtube.com/watch?v=PJqVaWREzk0

skorokithakis · 2017-10-30T22:17:24Z

Semi-relatedly, and because I want to activate this feature and see what difference it makes, is there a command that will show me on the CLI the (number of) active connections so I can tune the high/low water marks sensibly? Is ipfs swarm peers what I'm looking for?

whyrusleeping · 2017-10-30T22:43:57Z

@skorokithakis ipfs swarm peers lists all active connections. I'm also working on a subcommand for the connection manager which should allow some more fine grained information

Implement Connection Manager

whyrusleeping requested review from Stebalien, vyzo, Kubuxu, magik6k, a user, dignifiedquire and daviddias October 8, 2017 17:05

whyrusleeping force-pushed the feat/connmgr branch 2 times, most recently from 33d901c to 1f3decc Compare October 8, 2017 17:29

magik6k reviewed Oct 8, 2017

View reviewed changes

whyrusleeping force-pushed the feat/connmgr branch from 1f3decc to a5367a3 Compare October 9, 2017 13:05

daviddias requested changes Oct 9, 2017

View reviewed changes

whyrusleeping force-pushed the feat/connmgr branch from a5367a3 to 76c4b04 Compare October 9, 2017 13:37

Stebalien requested changes Oct 10, 2017

View reviewed changes

ghost mentioned this pull request Oct 10, 2017

ipfs: run connection closing experiment ipfs/infra#330

Merged

dignifiedquire approved these changes Oct 11, 2017

View reviewed changes

Stebalien approved these changes Oct 11, 2017

View reviewed changes

update deps for new connmgr code

94ec788

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping force-pushed the feat/connmgr branch from 76c4b04 to 3b8eed8 Compare October 14, 2017 12:58

ghost assigned whyrusleeping Oct 14, 2017

ghost added the status/in-progress In progress label Oct 14, 2017

ghost reviewed Oct 14, 2017

View reviewed changes

ghost approved these changes Oct 14, 2017

View reviewed changes

Kubuxu approved these changes Oct 14, 2017

View reviewed changes

Integrate connection manager

62d5217

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping force-pushed the feat/connmgr branch from 3b8eed8 to 62d5217 Compare October 16, 2017 16:46

whyrusleeping merged commit 6beab66 into master Oct 16, 2017

whyrusleeping removed the status/in-progress In progress label Oct 17, 2017

whyrusleeping deleted the feat/connmgr branch October 30, 2017 22:43

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022

Merge pull request ipfs#4288 from ipfs/feat/connmgr

4e0e4ca

Implement Connection Manager

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022

go-ipfs-config: Merge pull request ipfs#4288 from ipfs/feat/connmgr

d663b31

Implement Connection Manager

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022

go-ipfs-config: Merge pull request ipfs#4288 from ipfs/feat/connmgr

d214226

Implement Connection Manager

WIP: Connection Manager #4288

WIP: Connection Manager #4288

Conversation

whyrusleeping commented Oct 6, 2017 • edited

whyrusleeping commented Oct 8, 2017

whyrusleeping commented Oct 8, 2017

whyrusleeping commented Oct 8, 2017

whyrusleeping commented Oct 8, 2017

magik6k 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

magik6k commented Oct 8, 2017

whyrusleeping commented Oct 8, 2017

whyrusleeping commented Oct 9, 2017

whyrusleeping commented Oct 9, 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

Choose a reason for hiding this comment

daviddias commented Oct 9, 2017

whyrusleeping commented Oct 9, 2017

Stebalien 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

dignifiedquire left a comment

Choose a reason for hiding this comment

Kubuxu commented Oct 11, 2017

whyrusleeping commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

Kubuxu commented Oct 14, 2017

whyrusleeping commented Oct 14, 2017

ghost commented Oct 16, 2017

whyrusleeping commented Oct 16, 2017

whyrusleeping commented Oct 16, 2017

Kubuxu commented Oct 16, 2017

whyrusleeping commented Oct 16, 2017

Kubuxu commented Oct 16, 2017

skorokithakis commented Oct 30, 2017 • edited

whyrusleeping commented Oct 30, 2017

whyrusleeping commented Oct 6, 2017 •

edited

skorokithakis commented Oct 30, 2017 •

edited