Implements a simple tree networking topology #38

nkcr · 2020-04-22T14:20:10Z

As discussed, each node gets the list of addresses and computes in a deterministic manner the tree topology.

Tests will follow.

As discussed, each node gets the list of addresses and computes in a deterministic manner the tree topology.

Gilthoniel

Very nice!

mino/minogrpc/overlay.go

mino/minogrpc/overlay_test.go

mino/minogrpc/routing.go

Gilthoniel · 2020-04-27T09:44:12Z

We can have a student project next semester to implement a more resilient tree.

nkcr · 2020-04-29T07:19:23Z

We can have a student project next semester to implement a more resilient tree.

Yes, that would be a well-defined and interesting project. In that sense I replaced the treeRouting by an interface.

I also updated the test to check if the mesh is fully connected. For the record, here is the complete ordered traffic when 9 nodes relay a packet in a ring fashion with the following sequence client -> N1 -> N2 -> N3 ... N9 -> client

nkcr · 2020-05-04T08:49:05Z

@Gilthoniel ready for a second review. I still have some deadlocks when I play with >15 nodes with DKG, but I'm waiting to merge this PR and will continue investigating in #41 . But maybe you'll already discover something suspicious there. Another thing: can you have a particular look at how contexts are used? I realised I was misusing them but I don't know if its 100% ok now.

Gilthoniel

It's hard to say what's wrong from that PR. I'll need at some point to make a in-depth review of the whole package. The only suspicious thing is the wait on the ctx.Done() which looks wrong to me.

mino/minogrpc/mod.go

Gilthoniel · 2020-05-04T13:53:41Z

mino/minogrpc/mod_test.go

 		&OverlayMsg{},
 		&Envelope{},


Can you remind me why you use two kinds of messages ?

This is something I should merge, but right now it would just add a new layer of uncertainty. I'm adding it to my dodo list

Gilthoniel · 2020-05-04T14:06:12Z

mino/minogrpc/overlay.go

+
+	addrs := make([]mino.Address, len(routingMsg.Addrs))
+	for i, addrStr := range routingMsg.Addrs {
+		addrs[i] = address{addrStr}


That's a good place to use an address factory: If you change the structure, you won't have to come back here.

Gilthoniel · 2020-05-04T14:08:08Z

mino/minogrpc/overlay.go

+	}
+
+	routing, err := o.routingFactory.FromAddrs(addrs, map[string]interface{}{
+		TreeRoutingOpts.Addr: o.addr, "treeHeight": treeHeight,


Gilthoniel · 2020-05-04T14:15:00Z

mino/minogrpc/overlay.go

+	})
+	if err != nil {
+		return xerrors.Errorf("failed to create routing struct: %v", err)
+	}

 	// For the moment this sender can only receive messages to itself
 	// TODO: find a way to know the other nodes.


That's probably done ;)

Gilthoniel · 2020-05-04T14:35:44Z

mino/minogrpc/routing.go

+	Childs    []*treeNode
+}
+
+func (n treeNode) String() string {


One line for the string function for the logs, Display for more complex text.

Gilthoniel · 2020-05-04T14:42:20Z

mino/minogrpc/server.go

+
+	// warning: this call will shuffle addrs
+
+	routing, err := rpc.routingFactory.FromAddrs(addrs, map[string]interface{}{


You could add a FromIterator function to make use of the iterator.

Gilthoniel · 2020-05-04T14:45:00Z

mino/minogrpc/server.go

 				if err == io.EOF {
+					<-ctx.Done()


same remark as above

Gilthoniel · 2020-05-04T15:08:29Z

mino/minogrpc/server_test.go

+
+// Sleep random
+
+func rsleep() {


I'm not a big fan of the random sleep. You expect from a unit test to make sure a particular scenario will end as expected. You manually send messages in different order to test that they will be handled as expected. Random sleep only provokes random errors on Travis.

Yes I agree. I would however find interesting at some point to have some sort of fuzzing tests with different timing to stress the system. But of course not in this context.

Gilthoniel · 2020-05-04T15:10:19Z

mino/minogrpc/traffic.go

 	"go.dedis.ch/fabric/mino"
 )

 var eachLine = regexp.MustCompile(`(?m)^(.+)$`)

+var counter = atomicCounter{}
+
 // traffic is used to keep track of packets traffic in a server
 type traffic struct {


Can you either test this file, or move it to an internal/testing package ?

Yes testing is planed. Added in the TODO because for now this is still very prone to change.

and addresses Gaylor's comments

nkcr · 2020-05-05T16:57:46Z

Thanks @Gilthoniel for the review. I addressed your comments. The only thing that would need a second pass would be the refactor of the routing interface, which is now in minogrpc/routing. (Tests for this package will be added later.)

Gilthoniel

Small comment otherwise it looks good!

Gilthoniel · 2020-05-06T13:05:01Z

mino/minogrpc/routing/routing.go

+	sort.Stable(&addrsBuf)
+
+	// We will use the hash of the addresses to set the random seed.
+	hash := sha256.New()


the two functions have a lot of code repetition that could into a private function.

Implements a simple tree networking topology

Implements a simple tree networking topology

6261861

As discussed, each node gets the list of addresses and computes in a deterministic manner the tree topology.

nkcr added enhancement New feature or request mod/mino About the Mino module labels Apr 22, 2020

nkcr self-assigned this Apr 22, 2020

nkcr requested a review from Gilthoniel April 22, 2020 14:25

Gilthoniel reviewed Apr 27, 2020

View reviewed changes

mino/minogrpc/overlay.go Outdated Show resolved Hide resolved

mino/minogrpc/overlay_test.go Show resolved Hide resolved

mino/minogrpc/routing.go Outdated Show resolved Hide resolved

mino/minogrpc/routing.go Outdated Show resolved Hide resolved

nkcr added 5 commits April 28, 2020 13:44

Addresses Gaylor's comments

1f54d38

Adds test to ckeck sending message to neighbours

d649cc4

Implements a Routing interface

1c1d8e5

Updates the test to check a full-connected mesh

b4a89b2

Adds a graphviz representation of the routing

01ea309

nkcr added 7 commits April 29, 2020 11:41

Refactors addrBufSorter to a simpler type

6c6c53d

Comments out GenerateGraphviz in test

bc2aff7

Updates test

dc855a4

Updates minogrpc

63e7b14

Removes prints

fda5e6b

Removes safeStream

dbb09a3

Cleans the code before review

ecbdfad

nkcr requested a review from Gilthoniel May 4, 2020 08:44

Gilthoniel reviewed May 4, 2020

View reviewed changes

nkcr added 2 commits May 5, 2020 18:43

Refactors Routing interface

2612f64

and addresses Gaylor's comments

Removes unused variable

67b5128

nkcr and others added 3 commits May 5, 2020 19:05

Updates fake

e1a37ea

Fix fake iterators

d3fcabe

Merge branch 'master' into topology-passing

de208a1

Gilthoniel reviewed May 6, 2020

View reviewed changes

nkcr added 2 commits May 6, 2020 18:04

Adds a TODO note about Gaylor's comment

0384ba8

Refactors fromAny and fromIterator to make the code DRY

23b23ec

nkcr requested a review from Gilthoniel May 7, 2020 08:41

Gilthoniel approved these changes May 7, 2020

View reviewed changes

Gilthoniel merged commit 5d4e608 into master May 7, 2020

Gilthoniel deleted the topology-passing branch May 7, 2020 08:59

bbjubjub2494 pushed a commit to bbjubjub2494/dela that referenced this pull request Aug 3, 2024

Merge pull request dedis#38 from dedis/topology-passing

9b1b811

Implements a simple tree networking topology

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements a simple tree networking topology #38

Implements a simple tree networking topology #38

nkcr commented Apr 22, 2020

Gilthoniel left a comment

Gilthoniel commented Apr 27, 2020

nkcr commented Apr 29, 2020

nkcr commented May 4, 2020

Gilthoniel left a comment

Gilthoniel May 4, 2020

nkcr May 5, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

Gilthoniel May 4, 2020

nkcr May 5, 2020

Gilthoniel May 4, 2020

nkcr May 5, 2020

nkcr commented May 5, 2020

Gilthoniel left a comment

Gilthoniel May 6, 2020

nkcr May 7, 2020


		// warning: this call will shuffle addrs

		routing, err := rpc.routingFactory.FromAddrs(addrs, map[string]interface{}{

Implements a simple tree networking topology #38

Implements a simple tree networking topology #38

Conversation

nkcr commented Apr 22, 2020

Gilthoniel left a comment

Choose a reason for hiding this comment

Gilthoniel commented Apr 27, 2020

nkcr commented Apr 29, 2020

nkcr commented May 4, 2020

Gilthoniel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkcr commented May 5, 2020

Gilthoniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment