Routing Table Refresh manager #601

aarshkshah1992 · 2020-04-22T07:37:51Z

Routing Table PR at libp2p/go-libp2p-kbucket#77.

It's time we did this. Also incorporates changes for #550 and #555 in meta-issue #556.

Once we have the new SearchByKadId RPC implemented, switching to it should just be a config change for the RefreshManager.

willscott · 2020-04-22T14:27:25Z

dht.go

+	// To grok the Math Wizardy that produced these exact equations, please be patient as a document explaining it will
+	// be published soon.
+	l1 := math.Log(float64(1) / float64(defaultBucketSize))                              //(Log(1/K))
+	l2 := math.Log(float64(1) - (float64(cfg.concurrency) / float64(defaultBucketSize))) // Log(1 - (alpha / K))


cfg.bucketSize?

yes. We also should have a check that alpha < K. If alpha >= K the math here falls apart and so maxLastSuccessfulOutboundThreshold should just be cfg.routingTable.refreshInterval

This is done.

willscott · 2020-04-23T19:42:51Z

rtrefresh/rt_refresh_manager.go

+	rfnc := func(c int) (err error) {
+		cpl := uint(c)


Suggested change

rfnc := func(c int) (err error) {

cpl := uint(c)

rfnc := func(cpl uint) (err error) {

This is done.

willscott · 2020-04-23T19:44:22Z

rtrefresh/rt_refresh_manager.go


-		var err error
+	rfnc := func(c int) (err error) {


may be worth making forceRefresh / refreshCpls paramters, and making this a method, rather than an anonymous function

@willscott

Given that this anon func is ONLY used here and you'd have to pass it multiple params, I'm not sure we need the new method/abstraction. It's just a simple 4 line func.

aschmahmann · 2020-04-22T15:25:07Z

dht_bootstrap_test.go

 	require.Equal(t, connectedTo.self, d1.routingTable.ListPeers()[0])

 	// now emit the address change event
 	em, err := d1.host.EventBus().Emitter(&event.EvtLocalAddressesUpdated{})
 	require.NoError(t, err)
 	require.NoError(t, em.Emit(event.EvtLocalAddressesUpdated{}))
-	waitForWellFormedTables(t, []*IpfsDHT{d1}, 2, 2, 2*time.Second)
+	require.True(t, waitForWellFormedTables(t, []*IpfsDHT{d1}, 2, 2, 10*time.Second))


Was this flaky before, or did some change in this PR necessitate switching from 2 to 10 seconds here?

@aschmahmann This looks like an accidental change. I might have made it to test something. I've reverted it.

dht_test.go

aschmahmann · 2020-04-22T15:30:06Z

dht_test.go

@@ -829,6 +827,19 @@ func TestRefreshBelowMinRTThreshold(t *testing.T) {
 	assert.Equal(t, dhtE.self, dhtA.routingTable.Find(dhtE.self), "A's routing table should have peer E!")
 }

+func TestQueryWithEmptyRTShouldNotPanic(t *testing.T) {


I think we should add checks for at least SearchValues and GetPublicKey, since they should be easy enough and could end up with slightly different code paths. Could also add in the other functions from the routing interface too (Provide + PutValue).

@aschmahmann

We now ALSO cover GetValue, SearchValues and Provide.

Looks like GetPublicKey goes to the other node which involves some wire communication so not testing that for now.

Ok, it's a bit weird that GetPublicKey seems difficult to test. However, given that it's basically unused I'm ok not blocking on this for now and coming back to it later.

aschmahmann · 2020-05-08T17:31:44Z

dht.go

+	// To grok the Math Wizardy that produced these exact equations, please be patient as a document explaining it will
+	// be published soon.
+	l1 := math.Log(float64(1) / float64(defaultBucketSize))                              //(Log(1/K))
+	l2 := math.Log(float64(1) - (float64(cfg.concurrency) / float64(defaultBucketSize))) // Log(1 - (alpha / K))


yes. We also should have a check that alpha < K. If alpha >= K the math here falls apart and so maxLastSuccessfulOutboundThreshold should just be cfg.routingTable.refreshInterval

aschmahmann · 2020-05-08T17:52:15Z

rtrefresh/rt_refresh_manager.go

+	refreshQueryTimeout time.Duration                               // timeout for one refresh query
+
+	// interval between two periodic refreshes.
+	// also, a cpl wont be refreshed it the time since it was last refreshed


Suggested change

// also, a cpl wont be refreshed it the time since it was last refreshed

// also, a cpl wont be refreshed if the time since it was last refreshed

This is done.

aschmahmann · 2020-05-08T17:59:08Z

rtrefresh/rt_refresh_manager.go

+
+		// EXECUTE the refresh
+
+		// ping Routing Table peers that haven't been hear of/from in the interval they should have been.


Suggested change

// ping Routing Table peers that haven't been hear of/from in the interval they should have been.

// ping Routing Table peers that haven't been heard of/from in the interval they should have been.

This is done.

aschmahmann · 2020-05-08T18:09:55Z

rtrefresh/rt_refresh_manager.go

+					livelinessCtx, cancel := context.WithTimeout(r.ctx, peerPingTimeout)
+					if err := r.h.Connect(livelinessCtx, peer.AddrInfo{ID: ps.Id}); err != nil {
+						logger.Debugw("evicting peer after failed ping", "peer", ps.Id, "error", err)
+						r.rt.RemovePeer(ps.Id)


Do you think it's worth throwing in a comment here about if they don't support the DHT protocol upon reconnect that it should be caught by the IdentifyCompleted events? Would adding the comment be too confusing and/or prone to becoming outdated?

@aschmahmann Yes, I don't think that documentation belongs to the refresh manager at all. This guy's job is simply to remove peers that aren't alive irrespective of whether the support the protocol or not.

aschmahmann · 2020-05-08T18:40:54Z

rtrefresh/rt_refresh_manager.go

+			// had paid more attention in the Math classes at university.
+			// So, please be patient and a doc explaining it will be published soon.
+			if r.rt.NPeersForCpl(cpl) == 0 {
+				lastCpl := min(2*c, len(refreshCpls)-1)


I think there's an off by one error here. i.e. lastCpl := min(2*(c+1), len(refreshCpls)-1).

@petar @Stebalien can you double check me here?

It's ok: Substitute "len(refreshCpls)-1" for "lastCpl" in the "<" condition of the for loop. You get:
i < len(refreshCpls)-1+1
That's what you want.

@aarshkshah1992 I talked this over with @petar and where we got was that there's still some formalization to be figured out about how we want to deal with gaps. However, using lastCpl := min(2*c, len(refreshCpls)-1) isn't necessarily great as is because if c=0 then we stop refreshing which seems bad (although it's highly unlikely in a large network).

Given that the pathological case where the first couple buckets are empty should be quite rare, it probably doesn't matter how many more buckets we refresh beyond 2*c (e.g. instead of the 2c in the min statement use 2c+1, 2(c+1), or min(2c, 5)). For now I'd just go with lastCpl := min(2*(c+1), len(refreshCpls)-1).

rtrefresh/rt_refresh_manager_test.go

aarshkshah1992 · 2020-05-11T11:26:58Z

@aschmahmann I believe I've address all your concerns. Please take a look when you can.

aschmahmann

Just a few small changes and I think we're good to go!

aschmahmann · 2020-05-12T16:22:08Z

go.mod

@@ -18,7 +18,7 @@ require (
 	github.com/libp2p/go-eventbus v0.1.0
 	github.com/libp2p/go-libp2p v0.8.2
 	github.com/libp2p/go-libp2p-core v0.5.3
-	github.com/libp2p/go-libp2p-kbucket v0.4.1
+	github.com/libp2p/go-libp2p-kbucket v0.4.2-0.20200511065848-eb2adab8eaba


Any reason we shouldn't do a kbucket release and upgrade this?

This is done.

aschmahmann · 2020-05-12T16:27:14Z

rtrefresh/rt_refresh_manager.go

+var logger = logging.Logger("dht/RtRefreshManager")
+
+const (
+	peerPingTimeout = 10 * time.Second


Why is this a constant instead of a RefresherManager variable? Is it implicitly relying on other constants (e.g. we're using 2x the libp2p/go-tcp-transport's default 5 second timeout), or just a default that seems reasonable?

@aschmahmann Just the default seems reasonable. This is a simple dial timeout.

aschmahmann · 2020-05-12T18:05:22Z

rtrefresh/rt_refresh_manager.go

+			// had paid more attention in the Math classes at university.
+			// So, please be patient and a doc explaining it will be published soon.
+			if r.rt.NPeersForCpl(cpl) == 0 {
+				lastCpl := min(2*c, len(refreshCpls)-1)


@aarshkshah1992 I talked this over with @petar and where we got was that there's still some formalization to be figured out about how we want to deal with gaps. However, using lastCpl := min(2*c, len(refreshCpls)-1) isn't necessarily great as is because if c=0 then we stop refreshing which seems bad (although it's highly unlikely in a large network).

Given that the pathological case where the first couple buckets are empty should be quite rare, it probably doesn't matter how many more buckets we refresh beyond 2*c (e.g. instead of the 2c in the min statement use 2c+1, 2(c+1), or min(2c, 5)). For now I'd just go with lastCpl := min(2*(c+1), len(refreshCpls)-1).

aschmahmann · 2020-05-13T08:11:33Z

dht_test.go

@@ -829,6 +827,19 @@ func TestRefreshBelowMinRTThreshold(t *testing.T) {
 	assert.Equal(t, dhtE.self, dhtA.routingTable.Find(dhtE.self), "A's routing table should have peer E!")
 }

+func TestQueryWithEmptyRTShouldNotPanic(t *testing.T) {


Ok, it's a bit weird that GetPublicKey seems difficult to test. However, given that it's basically unused I'm ok not blocking on this for now and coming back to it later.

aarshkshah1992 · 2020-05-14T10:33:45Z

@aschmahmann Thanks for the great catch of the pathological condition. Have made the changes . Let's ship it !

aschmahmann

@aarshkshah1992 Left one nit that you are free to ignore if you want.

LGTM. Squash + merge when ready.

aschmahmann · 2020-05-19T15:04:40Z

dht_bootstrap.go

-	// FIXME: this can block. Ideally, we'd return a channel without blocking.
-	// https://github.com/libp2p/go-libp2p-kad-dht/issues/609


nit: I don't have strong feelings about having FIXMEs like this in the code. However, this issue still remains so I'd leave the comment. Your call though

I've added this back.

rt refresh refactor

63e5381

aarshkshah1992 requested review from Stebalien, willscott and aschmahmann April 22, 2020 07:37

aarshkshah1992 mentioned this pull request Apr 22, 2020

Fix Routing Table Refresh after we have a SearchByKadId RPC #556

Open

7 tasks

test query with empty RT

b44ddf8

aschmahmann mentioned this pull request Apr 22, 2020

NPeersForCpl and collapse empty buckets libp2p/go-libp2p-kbucket#77

Merged

aarshkshah1992 added 2 commits April 23, 2020 22:30

dont refresh all buckets

f41fb77

kbucket changes

e4ecc48

willscott reviewed Apr 23, 2020

View reviewed changes

aschmahmann requested changes May 8, 2020

View reviewed changes

aarshkshah1992 added 2 commits May 11, 2020 15:38

Merge branch 'master' into feat/refresh-manager

4e3e8ee

better docs

86b915e

lint and fmt

88f59b3

aschmahmann requested changes May 13, 2020

View reviewed changes

changes as per review

404c8b3

aarshkshah1992 added 4 commits May 14, 2020 16:15

kbucket version bump

a5fcbe7

Merge branch 'master' into feat/refresh-manager

ecac82c

import time

4993f69

fix linting

f278333

aschmahmann approved these changes May 19, 2020

View reviewed changes

add a FIXME

19737c5

aarshkshah1992 merged commit 2851c88 into master May 19, 2020

aarshkshah1992 deleted the feat/refresh-manager branch May 19, 2020 16:29

aschmahmann mentioned this pull request May 21, 2020

Stop refreshing once we stop filling buckets #550

Closed

Stebalien mentioned this pull request May 26, 2020

Release 0.6.0 ipfs/kubo#7347

Closed

77 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routing Table Refresh manager #601

Routing Table Refresh manager #601

aarshkshah1992 commented Apr 22, 2020 •

edited

Loading

willscott Apr 22, 2020

aschmahmann May 8, 2020

aarshkshah1992 May 11, 2020

willscott Apr 23, 2020

aarshkshah1992 May 11, 2020

willscott Apr 23, 2020

aarshkshah1992 May 11, 2020 •

edited

Loading

aschmahmann Apr 22, 2020

aarshkshah1992 May 11, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 May 11, 2020

aschmahmann May 13, 2020

aschmahmann May 8, 2020

aschmahmann May 8, 2020

aarshkshah1992 May 11, 2020

aschmahmann May 8, 2020

aarshkshah1992 May 11, 2020

aschmahmann May 8, 2020

aarshkshah1992 May 11, 2020

aschmahmann May 8, 2020

petar May 10, 2020

aschmahmann May 12, 2020

aarshkshah1992 commented May 11, 2020

aschmahmann left a comment

aschmahmann May 12, 2020

aarshkshah1992 May 14, 2020

aschmahmann May 12, 2020

aarshkshah1992 May 14, 2020

aschmahmann May 12, 2020

aschmahmann May 13, 2020

aarshkshah1992 commented May 14, 2020

aschmahmann left a comment

aschmahmann May 19, 2020

aarshkshah1992 May 19, 2020

	rfnc := func(c int) (err error) {
	cpl := uint(c)
	rfnc := func(cpl uint) (err error) {

	// also, a cpl wont be refreshed it the time since it was last refreshed
	// also, a cpl wont be refreshed if the time since it was last refreshed


		// EXECUTE the refresh

		// ping Routing Table peers that haven't been hear of/from in the interval they should have been.

		// FIXME: this can block. Ideally, we'd return a channel without blocking.
		// https://github.com/libp2p/go-libp2p-kad-dht/issues/609

Routing Table Refresh manager #601

Routing Table Refresh manager #601

Conversation

aarshkshah1992 commented Apr 22, 2020 • edited Loading

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

aarshkshah1992 May 11, 2020 • edited Loading

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

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

aarshkshah1992 commented May 11, 2020

aschmahmann 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

aarshkshah1992 commented May 14, 2020

aschmahmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Apr 22, 2020 •

edited

Loading

aarshkshah1992 May 11, 2020 •

edited

Loading