Maid-1977 churn test with tunnel #1396

maqi · 2017-03-17T15:50:25Z

No description provided.

…l tunnel nodes

maidsafe-highfive · 2017-03-17T15:50:34Z

(maidsafe_highfive has picked a reviewer for you, use r? to override)

Fraser999 · 2017-03-17T16:20:52Z

src/peer_manager.rs

-                peer.peer_id.map_or(None, |peer_id| Some((*peer.name(), peer_id)))
-            } else {
-                None
+    /// Returns the closest section to the peer and the closest section to own.


It only returns direct-connected peers. I wouldn't say "closest" either - just our section and the peer's section.

Fraser999 · 2017-03-17T16:24:38Z

src/peer_manager.rs

+        self.routing_table
+            .our_section()
+            .iter()
+            .chain(self.routing_table.closest_section(name).1.iter())


I think we should use self.routing_table.get_section(name) here to ensure we return the right section (this will return a different section if the one for name is still empty) and to avoid exposing another RT function.

but get_section will return None if the section for name is still empty.
I think here we are more interested in ensure the coverage, instead of section accurate

@maqi if it returns None, that should be fine right? cos that means we dont have anyone else "yet" from the target section, which means we'll soon be having some people added and at that point we'll query them if they can tunnel to this person anyways from the add_to_routing_table msg flow. This here is going to get us someone who we may loose a lot earlier due to further future splits and that can be prevented by using the other option maybe.

Fraser999 · 2017-03-17T16:25:09Z

src/peer_manager.rs

+            .our_section()
+            .iter()
+            .chain(self.routing_table.closest_section(name).1.iter())
+            .sorted_by(|name0, name1| name.cmp_distance(name0, name1))


Do we need to sort now?

if not sort, the result pattern will be based on the pattern of RT, which will be consistent across the nodes in the same section, hence making certain node overloaded for the test.
the sort is just trying to share the load meanwhile maintain a deterministic behavior.

i tested without sort (50 iterations of aggressive churn) and seems the new approach (our section + peer's section) already spread load enough and won't fail the test due to overloaded node.
I will remove the sort part then

Fraser999 · 2017-03-17T16:26:28Z

src/routing_table/mod.rs

@@ -989,7 +989,7 @@ impl<T: Binary + Clone + Copy + Debug + Default + Hash + Xorable> RoutingTable<T

    /// Returns the prefix of the closest non-empty section to `name`, regardless of whether `name`
    /// belongs in that section or not, and the section itself.
-    fn closest_section(&self, name: &T) -> (&Prefix<T>, &BTreeSet<T>) {
+    pub fn closest_section(&self, name: &T) -> (&Prefix<T>, &BTreeSet<T>) {


I don't think this change is required.

as the comment above, get_section may return None, I think this expose is required.

Fraser999 · 2017-03-17T16:34:37Z

src/states/node.rs

-            for dst_id in peers_needing_tunnel {
+        for (dst_id, peer_name) in self.peer_mgr.peers_needing_tunnel() {
+            if self.peer_mgr
+                .potential_tunnel_nodes(&peer_name)


The call to self.peer_mgr.potential_tunnel_nodes().contains(...) used to be outside the loop and done only if peers_needing_tunnel was non-empty to avoid repeating the relatively costly call. Not a big deal I guess.

but now we have to re-calculate for each peer now. :(
the good thing is now we only looking for section, so hope it won't be too costly.

But instead of computing the set of all potential tunnel nodes, we could just:

(self.peer_mgr.routing_table().our_section().contains(&peer_name) || self.peer_mgr.routing_table().get_section(public_id.name()).map_or(false, |section| section.contains(&peer_name))) && self.peer_mgr.get_state_by_name(public_id.name()).map_or(false, PeerState::can_tunnel_for)

(Or, probably cleaner, move that check into the peer manager.)

fizyk20 · 2017-03-17T16:45:38Z

src/peer_manager.rs

+                }
+                self.peer_map.get_by_name(name).map_or(None,
+                                                       |peer| if peer.state.can_tunnel_for() {
+                                                           peer.peer_id().map_or(None, |peer_id| {


and_then() looks like a more natural choice here than map_or().

fizyk20 · 2017-03-17T16:46:33Z

src/peer_manager.rs

        self.peer_map
            .peers()
            .filter_map(|peer| match peer.state {
-                PeerState::SearchingForTunnel => peer.peer_id,
+                PeerState::SearchingForTunnel => {
+                    peer.peer_id.map_or(None, |peer_id| Some((peer_id, *peer.name())))


Again - and_then() seems better.

fizyk20 · 2017-03-17T16:49:42Z

tests/mock_crust/churn.rs

        let index = rng.gen_range(1, len + 1);
+
+        if nodes.len() > 16 {


Do we want this constant 16 here? Or was it supposed to be 2 * min_section_size like somewhere above?

fizyk20 · 2017-03-17T16:49:59Z

tests/mock_crust/churn.rs

        let config = Config::with_contacts(&[nodes[proxy].handle.endpoint()]);

        nodes.insert(index, TestNode::builder(network).config(config).create());
+
+        if nodes.len() > 16 {


Like above.

fizyk20 · 2017-03-17T16:51:09Z

tests/mock_crust/churn.rs

+        if nodes.len() > 16 {
+            if index <= proxy {
+                proxy += 1;
+            }


What's the purpose of this? Some comment would be useful here.

new node may got inserted into a position before the proxy, hence push proxy_node's index by one.

Oh, I get it! Makes sense 👍 But a comment would still be nice ;)

fizyk20 · 2017-03-17T16:54:49Z

tests/mock_crust/churn.rs

@@ -213,6 +253,7 @@ impl ExpectedGets {
                }
            }
        }
+        assert!(unexpected_receive <= self.sections.len());


Why do we allow unexpected messages now and why this number?

this is due to the scenario :
1, A and B are tunnelling via C
2, A being the leader of a group message M (B is supposed to receive M)
3, A lost C (connection lost or C got removed), hence remove B
4, A collected enough signatures from section, hence sending M out according to its RT
the sending list will contain an out-of-range node D
5, D also lost tunnel to a close-range node due to the removal of the tunnel_node
6, D will then receive that msg and handle it as from its perspective, it is within close-range.

we cannot resolve it from routing perspective only.
And such unexpected_receive normally will be just one for each message, if happens.

Since it's at most one for each message, let's not just count, but instead record the messages itself and assert that each of them was received in at most one unexpected node.

afck · 2017-03-18T11:10:00Z

src/states/node.rs

-            for dst_id in peers_needing_tunnel {
+        for (dst_id, peer_name) in self.peer_mgr.peers_needing_tunnel() {
+            if self.peer_mgr
+                .potential_tunnel_nodes(&peer_name)


But instead of computing the set of all potential tunnel nodes, we could just:

(self.peer_mgr.routing_table().our_section().contains(&peer_name) || self.peer_mgr.routing_table().get_section(public_id.name()).map_or(false, |section| section.contains(&peer_name))) && self.peer_mgr.get_state_by_name(public_id.name()).map_or(false, PeerState::can_tunnel_for)

(Or, probably cleaner, move that check into the peer manager.)

afck · 2017-03-18T11:13:26Z

tests/mock_crust/churn.rs

+        let mut block_peer = gen_range_except(rng, 0, nodes.len(), Some(new_node));
+        while block_peer == proxy {
+            block_peer = gen_range_except(rng, 0, nodes.len(), Some(new_node));
+        }


You can avoid the potentially unlimited number of random number generations:

let mut block_peer = gen_range_except(rng, 0, nodes.len() - 1, Some(new_node)); if block_peer >= proxy { block_peer += 1; }

(Also below.)

afck · 2017-03-18T11:20:30Z

tests/mock_crust/churn.rs

+                                   node.name(),
+                                   key,
+                                   self.sections);
+                            unexpected_receive += 1;


For Section and PrefixSection destinations, this should still be impossible, shouldn't it? In that case, let's assert that dst is in fact a group authority here.

afck · 2017-03-18T11:21:48Z

tests/mock_crust/churn.rs

@@ -213,6 +253,7 @@ impl ExpectedGets {
                }
            }
        }
+        assert!(unexpected_receive <= self.sections.len());


Since it's at most one for each message, let's not just count, but instead record the messages itself and assert that each of them was received in at most one unexpected node.

Fraser999 · 2017-03-19T13:01:00Z

src/peer_manager.rs

            .collect()
    }

+    /// Returns true if peer is direct-connected and in our section or in tunnel_client's section.
+    pub fn is_potential_tunnel_node(&self, peer: &PublicId, tunnel_client: &XorName) -> bool {
+        (self.routing_table.our_section().contains(peer.name()) ||


Maybe overkill, but just to err on the side of safety we could also return false if peer is ourself.

this function is only being called in node::add_to_routing_table
if peer is ourself, we shall already return at https://github.com/maidsafe/routing/blob/master/src/states/node.rs#L1404-L1410, because routing_table::add will return an error for that case https://github.com/maidsafe/routing/blob/master/src/routing_table/mod.rs#L502-L504 anyway (also I think peer_mgr won't have peer info for self, right?)

so, I think we are safe to not carry out an additional check here?

Fraser999 · 2017-03-19T15:05:10Z

src/peer_manager.rs

+        (self.routing_table.our_section().contains(peer.name()) ||
+         self.routing_table
+            .get_section(peer.name())
+            .map_or(false, |section| section.contains(tunnel_client))) &&


If we're calling this function, it's implied that we don't already have a connection to tunnel_client, so section.contains(tunnel_client) should always return false, right? Should we just check that get_section() is the same for peer and tunnel_client and not None?

afck · 2017-03-20T08:53:51Z

tests/mock_crust/churn.rs

-        while block_peer == proxy {
-            block_peer = gen_range_except(rng, 0, nodes.len(), Some(new_node));
+        let mut block_peer = gen_range_except(rng, 0, nodes.len() - 1, Some(new_node));
+        if block_peer == proxy {


That should be >= proxy. (Also below.)

afck · 2017-03-20T13:48:56Z

tests/mock_crust/utils.rs

@@ -40,18 +40,15 @@ const BALANCED_POLLING: bool = true;
 pub fn gen_range_except<T: Rng>(rng: &mut T,
                                low: usize,
                                high: usize,
-                                exclude: Option<usize>)
+                                exclude: BTreeSet<usize>)


This could be exclude: &BTreeSet<usize> instead, then we wouldn't need to clone the set.

…or exclude: I with I: IntoIterator<usize>, then it would accept vectors and None and Some etc.

…nnel_node

…tion merge or split

maqi added 2 commits March 16, 2017 14:32

test/churn: churn test with disrupted and blocked connections

f8c00de

fix/peer_manager: use both closest section to peer and us as potentia…

600b014

…l tunnel nodes

maidsafe-highfive assigned afck Mar 17, 2017

Fraser999 reviewed Mar 17, 2017

View reviewed changes

fizyk20 reviewed Mar 17, 2017

View reviewed changes

maqi force-pushed the MAID-1977 branch from e152635 to 4f7b000 Compare March 18, 2017 10:00

afck reviewed Mar 18, 2017

View reviewed changes

maqi force-pushed the MAID-1977 branch 4 times, most recently from ed095ea to 40449ba Compare March 19, 2017 07:34

Fraser999 reviewed Mar 19, 2017

View reviewed changes

maqi force-pushed the MAID-1977 branch from 40449ba to bfaf7b3 Compare March 20, 2017 01:51

fix/churn_test_with_tunnel: address review comments

1f5e7ee

maqi force-pushed the MAID-1977 branch from bfaf7b3 to 1f5e7ee Compare March 20, 2017 03:55

afck reviewed Mar 20, 2017

View reviewed changes

afck approved these changes Mar 20, 2017

View reviewed changes

update gen_range_except function; additional check in is_potential_tu…

7baed61

…nnel_node

maqi force-pushed the MAID-1977 branch from 1695674 to 7baed61 Compare March 20, 2017 14:15

maqi added 3 commits March 20, 2017 22:15

Merge branch 'master' into MAID-1977

cd45b15

remove un-necessary BTreeSet type specification

4b5d65b

Additional check that we are not sending SectionUpdate during own_sec…

8ab4c5b

…tion merge or split

Viv-Rajkumar merged commit 89aa9ad into maidsafe:master Mar 20, 2017

maqi deleted the MAID-1977 branch August 1, 2017 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maid-1977 churn test with tunnel #1396

Maid-1977 churn test with tunnel #1396

maqi commented Mar 17, 2017

maidsafe-highfive commented Mar 17, 2017

Fraser999 Mar 17, 2017

Fraser999 Mar 17, 2017

maqi Mar 17, 2017

Viv-Rajkumar Mar 17, 2017

Fraser999 Mar 17, 2017

maqi Mar 17, 2017 •

edited

maqi Mar 18, 2017 •

edited

Fraser999 Mar 17, 2017

maqi Mar 17, 2017

Fraser999 Mar 17, 2017

maqi Mar 17, 2017

afck Mar 18, 2017

fizyk20 Mar 17, 2017

fizyk20 Mar 17, 2017

fizyk20 Mar 17, 2017

fizyk20 Mar 17, 2017

fizyk20 Mar 17, 2017

maqi Mar 17, 2017

fizyk20 Mar 17, 2017 •

edited

fizyk20 Mar 17, 2017

maqi Mar 17, 2017 •

edited

afck Mar 18, 2017

afck Mar 18, 2017

afck Mar 18, 2017

afck Mar 18, 2017

afck Mar 18, 2017

Fraser999 Mar 19, 2017

maqi Mar 20, 2017 •

edited

Fraser999 Mar 19, 2017

afck Mar 20, 2017

afck Mar 20, 2017 •

edited

afck Mar 20, 2017

Maid-1977 churn test with tunnel #1396

Maid-1977 churn test with tunnel #1396

Conversation

maqi commented Mar 17, 2017

maidsafe-highfive commented Mar 17, 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

maqi Mar 17, 2017 • edited

Choose a reason for hiding this comment

maqi Mar 18, 2017 • edited

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

fizyk20 Mar 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqi Mar 17, 2017 • edited

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

maqi Mar 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck Mar 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqi Mar 17, 2017 •

edited

maqi Mar 18, 2017 •

edited

fizyk20 Mar 17, 2017 •

edited

maqi Mar 17, 2017 •

edited

maqi Mar 20, 2017 •

edited

afck Mar 20, 2017 •

edited