-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Connection Manager #4288
WIP: Connection Manager #4288
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,21 +45,17 @@ import ( | |
yamux "gx/ipfs/QmNWCEvi7bPRcvqAV8AKLGVNoQdArWi7NJayka2SM4XtRe/go-smux-yamux" | ||
cid "gx/ipfs/QmNp85zy9RLrQ5oQD4hPyS39ezrrXpcaa7R4Y9kxdWQLLQ/go-cid" | ||
mplex "gx/ipfs/QmP81tTizXSjKTLWhjty1rabPQHe1YPMj6Bq5gR5fWZakn/go-smux-multiplex" | ||
connmgr "gx/ipfs/QmPErLx83hgF5XKqjeYaLWXJSFon4mH7YPzoftUvfrPgQG/go-libp2p-connmgr" | ||
floodsub "gx/ipfs/QmPGT8xqmDnbCbc5HCpjSCzPw7HcsSKuzVAWGjAtF3oftm/floodsub" | ||
routing "gx/ipfs/QmPR2JzfKd9poHx9XBhzoFeBBC31ZM3W5iUPKJZWyaoZZm/go-libp2p-routing" | ||
pstore "gx/ipfs/QmPgDWmTmuzvP7QE5zwo1TmjbJme9pmZHNujB2453jkCTr/go-libp2p-peerstore" | ||
mafilter "gx/ipfs/QmQBB2dQLmQHJgs2gqZ3iqL2XiuCtUCvXzWt5kMXDf5Zcr/go-maddr-filter" | ||
metrics "gx/ipfs/QmQbh3Rb7KM37As3vkHYnEFnzkVXNCP8EYGtHz6g2fXk14/go-libp2p-metrics" | ||
ipnet "gx/ipfs/QmQq9YzmdFdWNTDdArueGyD7L5yyiRQigrRHJnTGkxcEjT/go-libp2p-interface-pnet" | ||
discovery "gx/ipfs/QmRQ76P5dgvxTujhfPsCRAG83rC15jgb1G9bKLuomuC6dQ/go-libp2p/p2p/discovery" | ||
p2pbhost "gx/ipfs/QmRQ76P5dgvxTujhfPsCRAG83rC15jgb1G9bKLuomuC6dQ/go-libp2p/p2p/host/basic" | ||
rhost "gx/ipfs/QmRQ76P5dgvxTujhfPsCRAG83rC15jgb1G9bKLuomuC6dQ/go-libp2p/p2p/host/routed" | ||
identify "gx/ipfs/QmRQ76P5dgvxTujhfPsCRAG83rC15jgb1G9bKLuomuC6dQ/go-libp2p/p2p/protocol/identify" | ||
ping "gx/ipfs/QmRQ76P5dgvxTujhfPsCRAG83rC15jgb1G9bKLuomuC6dQ/go-libp2p/p2p/protocol/ping" | ||
goprocess "gx/ipfs/QmSF8fPo3jgVBAy8fpdjjYqgG87dkJgUprRBHRd2tmfgpP/goprocess" | ||
mamask "gx/ipfs/QmSMZwvs3n4GBikZ7hKzT17c3bk65FmyZo2JqtJ16swqCv/multiaddr-filter" | ||
u "gx/ipfs/QmSU6eubNdhXjFBJBSksTp8kv8YRub8mGAPv8tVJHmL2EU/go-ipfs-util" | ||
logging "gx/ipfs/QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52/go-log" | ||
dht "gx/ipfs/QmT7PnPxYkeKPCG8pAnucfcjrXc15Q7FgvFv7YC24EPrw8/go-libp2p-kad-dht" | ||
b58 "gx/ipfs/QmT8rehPR3F6bmwL6zjUN8XpiDBFFpMP2myPdC6ApsWfJf/go-base58" | ||
addrutil "gx/ipfs/QmVJGsPeK3vwtEyyTxpCs47yjBYMmYsAhEouPDF3Gb2eK3/go-addr-util" | ||
ds "gx/ipfs/QmVSase1JP7cq9QkPT46oNwdp9pT6kBkG3oqS14y3QcZjG/go-datastore" | ||
|
@@ -68,11 +64,17 @@ import ( | |
ma "gx/ipfs/QmXY77cVe7rVRQXZZQRioukUM7aRW3BTcAgJe12MCtb3Ji/go-multiaddr" | ||
peer "gx/ipfs/QmXYjuNuxVzXKJCfWasQk1RqkhVLDM9jtUKhqc2WPQmFSB/go-libp2p-peer" | ||
smux "gx/ipfs/QmY9JXR3FupnYAYJWK9aMr9bCpqWKcToQ1tz8DVGTrHpHw/go-stream-muxer" | ||
dht "gx/ipfs/QmYi2NvTAiv2xTNJNcnuz3iXDDT1ViBwLFXmDb2g7NogAD/go-libp2p-kad-dht" | ||
ifconnmgr "gx/ipfs/QmYkCrTwivapqdB3JbwvwvxymseahVkcm46ThRMAA24zCr/go-libp2p-interface-connmgr" | ||
ic "gx/ipfs/QmaPbCnUMBohSGo3KnxEa2bHqyJVVeEEcwtqJAYxerieBo/go-libp2p-crypto" | ||
p2phost "gx/ipfs/QmaSxYRuMq4pkpBBG2CYaRrPx2z7NmMVEs34b9g61biQA6/go-libp2p-host" | ||
circuit "gx/ipfs/QmbMNjK69isbpzVGKKrsnM7Sqyh3TVKAphRn5WuUhwTFbW/go-libp2p-circuit" | ||
discovery "gx/ipfs/Qmbgce14YTWE2qhE49JVvTBPaHTyz3FaFmqQPyuZAz6C28/go-libp2p/p2p/discovery" | ||
p2pbhost "gx/ipfs/Qmbgce14YTWE2qhE49JVvTBPaHTyz3FaFmqQPyuZAz6C28/go-libp2p/p2p/host/basic" | ||
rhost "gx/ipfs/Qmbgce14YTWE2qhE49JVvTBPaHTyz3FaFmqQPyuZAz6C28/go-libp2p/p2p/host/routed" | ||
identify "gx/ipfs/Qmbgce14YTWE2qhE49JVvTBPaHTyz3FaFmqQPyuZAz6C28/go-libp2p/p2p/protocol/identify" | ||
ping "gx/ipfs/Qmbgce14YTWE2qhE49JVvTBPaHTyz3FaFmqQPyuZAz6C28/go-libp2p/p2p/protocol/ping" | ||
p2phost "gx/ipfs/Qmc1XhrFEiSeBNn3mpfg6gEuYCt5im2gYmNVmncsvmpeAk/go-libp2p-host" | ||
pnet "gx/ipfs/QmcWmYQEQCrezztaQ81nXzMx2jaAEow17wdesDAjjR769r/go-libp2p-pnet" | ||
floodsub "gx/ipfs/Qmdnza7rLi7CMNNwNhNkcs9piX5sf6rxE8FrCsPzYtUEUi/floodsub" | ||
circuit "gx/ipfs/QmfHWhmJSJD9RjogJdPsb7wzJbUkxpZkctHvAfvJCTAP6X/go-libp2p-circuit" | ||
) | ||
|
||
const IpnsValidatorTag = "ipns" | ||
|
@@ -219,11 +221,17 @@ func (n *IpfsNode) startOnlineServices(ctx context.Context, routingOption Routin | |
return err | ||
} | ||
|
||
connmgr, err := constructConnMgr(cfg.Swarm.ConnMgr) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
hostopts := &ConstructPeerHostOpts{ | ||
AddrsFactory: addrsFactory, | ||
DisableNatPortMap: cfg.Swarm.DisableNatPortMap, | ||
DisableRelay: cfg.Swarm.DisableRelay, | ||
EnableRelayHop: cfg.Swarm.EnableRelayHop, | ||
ConnectionManager: connmgr, | ||
} | ||
peerhost, err := hostOption(ctx, n.Identity, n.Peerstore, n.Reporter, | ||
addrfilter, tpt, protec, hostopts) | ||
|
@@ -261,6 +269,22 @@ func (n *IpfsNode) startOnlineServices(ctx context.Context, routingOption Routin | |
return n.Bootstrap(DefaultBootstrapConfig) | ||
} | ||
|
||
func constructConnMgr(cfg config.ConnMgr) (ifconnmgr.ConnManager, error) { | ||
switch cfg.Type { | ||
case "", "none": | ||
return nil, nil | ||
case "basic": | ||
grace, err := time.ParseDuration(cfg.GracePeriod) | ||
if err != nil { | ||
return nil, fmt.Errorf("parsing Swarm.ConnMgr.GracePeriod: %s", err) | ||
} | ||
|
||
return connmgr.NewConnManager(cfg.LowWater, cfg.HighWater, grace), nil | ||
default: | ||
return nil, fmt.Errorf("unrecognized ConnMgr.Type: %q", cfg.Type) | ||
} | ||
} | ||
|
||
func (n *IpfsNode) startLateOnlineServices(ctx context.Context) error { | ||
cfg, err := n.Repo.Config() | ||
if err != nil { | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change related to this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it just happened to change in a dependency that i needed to update here |
||
} | ||
} | ||
return nil | ||
|
@@ -789,6 +813,7 @@ type ConstructPeerHostOpts struct { | |
DisableNatPortMap bool | ||
DisableRelay bool | ||
EnableRelayHop bool | ||
ConnectionManager ifconnmgr.ConnManager | ||
} | ||
|
||
type HostOption func(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protc ipnet.Protector, opts *ConstructPeerHostOpts) (p2phost.Host, error) | ||
|
@@ -814,6 +839,9 @@ func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr | |
if !opts.DisableNatPortMap { | ||
hostOpts = append(hostOpts, p2pbhost.NATPortMap) | ||
} | ||
if opts.ConnectionManager != nil { | ||
hostOpts = append(hostOpts, opts.ConnectionManager) | ||
} | ||
|
||
addrsFactory := opts.AddrsFactory | ||
if !opts.DisableRelay { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -250,7 +250,7 @@ Options for configuring the swarm. | |
|
||
- `AddrFilters` | ||
An array of address filters (multiaddr netmasks) to filter dials to. | ||
See https://github.com/ipfs/go-ipfs/issues/1226#issuecomment-120494604 for more | ||
See [this issue](https://github.com/ipfs/go-ipfs/issues/1226#issuecomment-120494604) for more | ||
information. | ||
|
||
- `DisableBandwidthMetrics` | ||
|
@@ -267,3 +267,17 @@ Disables the p2p-circuit relay transport. | |
- `EnableRelayHop` | ||
Enables HOP relay for the node. If this is enabled, the node will act as | ||
an intermediate (Hop Relay) node in relay circuits for connected peers. | ||
|
||
### `ConnMgr` | ||
Connection manager configuration. | ||
|
||
- `Type` | ||
Sets the type of connection manager to use, options are: `"none"` and `"basic"`. | ||
|
||
- `LowWater` | ||
LowWater is the minimum number of connections to maintain. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
- `HighWater` | ||
HighWater is the number of connections that, when exceeded, will trigger a connection GC operation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really like these names. Wouldn't simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I'd avoid min/low as they imply that we'll actively seek peers up to this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So... are we in favor of still using I agree that "min" would imply we actively open connections until we have that many, but I think "low" avoids this connotation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
- `GracePeriod` | ||
GracePeriod is a time duration that new connections are immune from being closed by the connection manager. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless I'm mistaken, this can trivially be used to DOS a node. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, connections in the grace period are basically not counted, see here: https://github.com/libp2p/go-libp2p-connmgr/blob/master/connmgr.go#L61 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, so we literally just skip them. Nevermind, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.