Feat: Enable DHT-based peer discovery and routing for cluster peers #489

hsanjuan · 2018-07-17T13:22:59Z

This uses go-libp2p-kad-dht as routing provider for the Cluster Peers.

This means that:

A cluster peer can discover other Cluster peers even if they are
not in their peerstore file.
We remove a bunch of code sending and receiving peers multiaddresses
when a new peer was added to the Cluster.
PeerAdd now takes an ID and not a multiaddress. We do not need to
ask the new peer which is our external multiaddress nor broadcast
the new multiaddress to everyone. This will fix problems when bootstrapping
a new peer to the Cluster while not all the other peers are online.
Adding a new peer does not mean to open connections to all peers
anymore. The number of connections will be made according to the DHT
parameters (this is good to have for future work)

The that detecting a peer addition in the watchPeers() function does
no longer mean that we have connected to it or that we know its
multiaddresses. Therefore it's no point to save the peerstore in these
events anymore.

Here a question opens, should we save the peerstore at all, and should we
save multiaddresses only for cluster peers, or for everyone known?
Currently, the peerstore is only updated on clean shutdown,
and it is updated with all the multiaddresses known, and not limited to
peer IDs in the cluster, (because, why not).

License: MIT
Signed-off-by: Hector Sanjuan code@hector.link

coveralls · 2018-07-17T13:34:44Z

Coverage decreased (-0.3%) to 67.976% when pulling d3d1f96 on feat/dht into 99aea7b on master.

lanzafame

There should also be an accompanying ipfs-cluster-website PR that updates the bootstrapping documentation.

lanzafame · 2018-07-20T05:16:34Z

cluster.go

@@ -86,6 +89,8 @@ func NewCluster(
 		return nil, errors.New("cluster host is nil")
 	}

+	ctx, cancel := context.WithCancel(context.Background())


Unrelated to this particular PR, but could we have the context.Context be passed into the NewCluster function? That way tracing and metrics can follow the ctx from the initialisation of the cluster in daemon.go all the way through.

lanzafame · 2018-07-23T06:47:10Z

rpc_api.go

@@ -335,24 +334,6 @@ func (rpcapi *RPCAPI) ConsensusPeers(ctx context.Context, in struct{}, out *[]pe
 	return err
 }

-/*


Should we really be 'just' removing these RPC calls? Yes, we are pre-v1 but I feel like we should provide at least some form of depreciation period. Especially, considering we don't know to what extent our users have integrated with the API.

RPC API is internal, so yes, I'm happy 'just' removing.

lanzafame · 2018-07-23T06:51:21Z

cluster.go

 	peerManager := pstoremgr.New(host, cfg.GetPeerstorePath())

-	ctx, cancel := context.WithCancel(context.Background())
+	idht, err := dht.New(ctx, host)


Is this a new cluster DHT or a new connection to the IPFS public DHT?

cluster DHT

hsanjuan · 2018-07-23T09:55:45Z

@lanzafame is it good from code point of view? (i fixed the tests now)

Is it ok to just save the peerstore on shutdown now? And is it ok to include all known addresses in it? Other possibilities are: not saving the peerstore at all (not touching what the user provides), or saving only addresses for current peers.

lanzafame · 2018-07-23T09:58:23Z

@lanzafame is it good from code point of view? (i fixed the tests now)

Yep.

Is it ok to just save the peerstore on shutdown now? And is it ok to include all known addresses in it?

Yes and yes.

This uses go-libp2p-kad-dht as routing provider for the Cluster Peers. This means that: * A cluster peer can discover other Cluster peers even if they are not in their peerstore file. * We remove a bunch of code sending and receiving peers multiaddresses when a new peer was added to the Cluster. * PeerAdd now takes an ID and not a multiaddress. We do not need to ask the new peer which is our external multiaddress nor broadcast the new multiaddress to everyone. This will fix problems when bootstrapping a new peer to the Cluster while not all the other peers are online. * Adding a new peer does not mean to open connections to all peers anymore. The number of connections will be made according to the DHT parameters (this is good to have for future work) The that detecting a peer addition in the watchPeers() function does no longer mean that we have connected to it or that we know its multiaddresses. Therefore it's no point to save the peerstore in these events anymore. Here a question opens, should we save the peerstore at all, and should we save multiaddresses only for cluster peers, or for everyone known? Currently, the peerstore is only updated on clean shutdown, and it is updated with all the multiaddresses known, and not limited to peer IDs in the cluster, (because, why not). License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2018-08-13T08:33:18Z

@lanzafame what's missing here? I'm going to look at that failing test, see if I can make it not fail randomly

lanzafame · 2018-08-14T00:07:03Z

@hsanjuan sorry, nothing, it just fell off my radar.

hsanjuan self-assigned this Jul 17, 2018

hsanjuan requested a review from lanzafame July 17, 2018 13:23

ghost added the status/in-progress In progress label Jul 17, 2018

lanzafame suggested changes Jul 23, 2018

View reviewed changes

hsanjuan force-pushed the feat/dht branch 2 times, most recently from 86325de to 1575cbb Compare July 23, 2018 09:45

hsanjuan force-pushed the feat/dht branch from 1575cbb to a70e07a Compare July 23, 2018 10:08

hsanjuan force-pushed the feat/dht branch from a70e07a to d3d1f96 Compare July 24, 2018 13:33

hsanjuan added a commit to ipfs-cluster/ipfs-cluster-website that referenced this pull request Aug 3, 2018

Update docs for DHT-powered peers: ipfs-cluster/ipfs-cluster#489

cfe202b

hsanjuan mentioned this pull request Aug 3, 2018

Update docs for DHT-powered peers ipfs-cluster/ipfs-cluster-website#31

Merged

lanzafame approved these changes Aug 14, 2018

View reviewed changes

hsanjuan merged commit 2675bf8 into master Aug 14, 2018

ghost removed the status/in-progress In progress label Aug 14, 2018

hsanjuan deleted the feat/dht branch August 14, 2018 09:34

hsanjuan mentioned this pull request Aug 19, 2018

Release 0.5.0 #454

Closed

hsanjuan added a commit to ipfs-cluster/ipfs-cluster-website that referenced this pull request Aug 22, 2018

Update docs for DHT-powered peers: ipfs-cluster/ipfs-cluster#489

a4c1d96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Enable DHT-based peer discovery and routing for cluster peers #489

Feat: Enable DHT-based peer discovery and routing for cluster peers #489

hsanjuan commented Jul 17, 2018

coveralls commented Jul 17, 2018 •

edited

Loading

lanzafame left a comment

lanzafame Jul 20, 2018

lanzafame Jul 23, 2018

hsanjuan Jul 23, 2018

lanzafame Jul 23, 2018

lanzafame Jul 23, 2018

hsanjuan Jul 23, 2018

hsanjuan commented Jul 23, 2018

lanzafame commented Jul 23, 2018

hsanjuan commented Aug 13, 2018

lanzafame commented Aug 14, 2018

Feat: Enable DHT-based peer discovery and routing for cluster peers #489

Feat: Enable DHT-based peer discovery and routing for cluster peers #489

Conversation

hsanjuan commented Jul 17, 2018

coveralls commented Jul 17, 2018 • edited Loading

lanzafame left a comment

Choose a reason for hiding this comment

lanzafame Jul 20, 2018

Choose a reason for hiding this comment

lanzafame Jul 23, 2018

Choose a reason for hiding this comment

hsanjuan Jul 23, 2018

Choose a reason for hiding this comment

lanzafame Jul 23, 2018

Choose a reason for hiding this comment

lanzafame Jul 23, 2018

Choose a reason for hiding this comment

hsanjuan Jul 23, 2018

Choose a reason for hiding this comment

hsanjuan commented Jul 23, 2018

lanzafame commented Jul 23, 2018

hsanjuan commented Aug 13, 2018

lanzafame commented Aug 14, 2018

coveralls commented Jul 17, 2018 •

edited

Loading