prioritise thunderbolt over WIFI #41 #47

itsknk · 2024-07-20T06:51:47Z

For issue #41, I figured the solution could be quite simple since it already supports the Thunderbolt discovery and needs to be prioritized.

I've updated the grpc_discovery.py file to get this done.

Changes made:

Import netifaces and platform.
Create a get_network_interfaces method that prioritizes Thunderbolt interfaces over WiFi on macOS.
Make the __init__ method to call get_network_interfaces and store the result.
Update task_broadcast_presence to broadcast on all available interfaces, prioritizing Thunderbolt on macOS.
Make task_listen_for_peers to listen on all available interfaces.

AlexCheema · 2024-07-20T07:03:32Z

This is awesome! First proper PR, congrats!

Code looks great, only one very minor thing for consistency across the codebase, instead of platform use psutil. It's the same thing but just more consistent across the codebase.

itsknk · 2024-07-20T07:11:40Z

Will change.

AlexCheema · 2024-07-20T07:14:41Z

Also don't we need to add netifaces as a dependency in setup.py?

itsknk · 2024-07-20T07:18:30Z

My bad! We do! I just installed it locally and forgot about setup.py! Will update the whole thing and give you a better one!

AlexCheema · 2024-07-20T07:33:16Z

exo/networking/grpc/grpc_discovery.py

+
+    def get_network_interfaces(self):
+        interfaces = netifaces.interfaces()
+        if psutil.system() == "Darwin":  # macOS


Make this if psutil.MACOS: and remove the comment

Beat me to it!

AlexCheema · 2024-07-20T07:35:42Z

Just added an integration test for discovery. Already coming in useful, we'll see if it still passes after your changes.

itsknk · 2024-07-20T07:42:09Z

I guess setting up netifaces dependency failed?!

exo/networking/grpc/grpc_discovery.py

AlexCheema · 2024-07-20T23:34:49Z

I just quickly tested this myself locally. For me, the interface name does not begin with thunderbolt.

# before thunderbolt connected
>>> netifaces.interfaces()
['lo0', 'gif0', 'stf0', 'anpi0', 'anpi1', 'anpi2', 'en4', 'en5', 'en6', 'en1', 'en2', 'en3', 'bridge0', 'ap1', 'en0', 'awdl0', 'llw0', 'utun0', 'utun1', 'utun2', 'utun3', 'utun4', 'utun5', 'utun6', 'utun7', 'utun8', 'utun9']
# after thunderbolt connected
>>> netifaces.interfaces()
['lo0', 'gif0', 'stf0', 'anpi0', 'anpi1', 'anpi2', 'en4', 'en5', 'en6', 'en1', 'en2', 'en3', 'bridge0', 'ap1', 'en0', 'awdl0', 'llw0', 'utun0', 'utun1', 'utun2', 'utun3', 'utun4', 'utun5', 'utun6', 'utun7', 'utun8', 'utun9', 'en11', 'en16']

As you can see, en11 and en16 were added after connecting thunderbolt.

AlexCheema · 2024-07-20T23:42:08Z

Eventually we should support automatically upgrading to the best interface available (in this case thunderbolt) - see #51. We can implement this later though, for now this is great.

itsknk · 2024-07-20T23:47:09Z

Eventually we should support automatically upgrading to the best interface available (in this case thunderbolt) - see #51. We can implement this later though, for now this is great.

This makes better sense, @AlexCheema.

AlexCheema · 2024-07-21T00:40:07Z

Just to clarify, this still needs to be fixed / tested: #47 (comment)

itsknk · 2024-07-21T06:43:55Z

Update:

Look for Thunderbolt in both the hardware and type fields, as Thunderbolt interfaces might be identified in either.
Identify WiFi interfaces by checking if the type is 'wi-fi' (case-insensitive).
Categories all other interfaces as 'other'.

@AlexCheema, I wrote a test script and it returned:

Please review this.

AlexCheema · 2024-07-21T07:02:22Z

exo/networking/grpc/grpc_discovery.py

+                print(f"Other interfaces: {other_interfaces}")
+
+            # Prioritize Thunderbolt, then WiFi, then others
+            return thunderbolt_interfaces + wifi_interfaces + other_interfaces


Beautiful code. love this

AlexCheema · 2024-07-21T07:11:08Z

Have you got a thunderbolt cable to test this with? I don't think this works in its current state. This is what system_profiler SPNetworkDataType -json gives me for my MacBook Pro's Thunderbolt interface:

{
      "_name": "Thunderbolt Bridge",
      "Ethernet": {
        "MediaOptions": [],
        "MediaSubType": "autoselect"
      },
      "hardware": "Ethernet",
      "interface": "bridge0",
      "ip_address": [
        "169.254.13.209"
      ],
      "IPv4": {
        "AdditionalRoutes": [
          {
            "DestinationAddress": "169.254.13.209",
            "SubnetMask": "255.255.255.255"
          }
        ],
        "Addresses": [
          "169.254.13.209"
        ],
        "ConfigMethod": "DHCP",
        "ConfirmedInterfaceName": "bridge0",
        "InterfaceName": "bridge0",
        "SubnetMasks": [
          "255.255.0.0"
        ]
      },
      "IPv6": {
        "ConfigMethod": "Automatic"
      },
      "Proxies": {
        "ExceptionsList": [
          "*.local",
          "169.254/16"
        ],
        "FTPPassive": "yes"
      },
      "spnetwork_service_order": 4,
      "type": "Ethernet"
    }

AlexCheema · 2024-07-21T07:13:53Z

I had another look and generally I don't think this is doing what we want it to do.
I'm not even sure the behaviour is different to before the changes. We're broadcasting on all interfaces (was already the case as far as I can tell) and listening on all interfaces (was already the case).

I think what we need to do here:

In the broadcast message, add a priority field which indicates the device's preference to receive connections on this particular interface
the listener should keep track of the current priority for each peer (can be in the same dict as known_peers)
if the listener receives a broadcast with a higher priority than it currently has for a connected peer, it should disconnect on the old interface and reconnect on the new interface

itsknk · 2024-07-21T23:10:21Z

Did:

A priority field is added to the broadcast message.
Modified known_peers to include the priority info.
Implemented reconnection logic in on_listen_message when a higher priority interface is detected.
get_network_interfaces returns interfaces with their priorities.
A reconnect_peer method is added to handle the reconnection.

@AlexCheema, wrote a small test script that simulates receiving a broadcast from a peer. And to see whether it keeps track of priority and change when a better one comes. Here is the output from the test:

AlexCheema · 2024-07-21T23:11:52Z

Did:

A priority field is added to the broadcast message.

Modified known_peers to include the priority info.

Implemented reconnection logic in on_listen_message when a higher priority interface is detected.

get_network_interfaces returns interfaces with their priorities.

A reconnect_peer method is added to handle the reconnection.

@AlexCheema, wrote a small test script that simulates receiving a broadcast from a peer. And to see whether it keeps track of priority and change when a better one comes. Here is the output from the test:

@itsknk can you commit the test too? and add some assertions so it's a proper test would be great

itsknk · 2024-07-21T23:17:44Z

Did:

A priority field is added to the broadcast message.

Modified known_peers to include the priority info.

Implemented reconnection logic in on_listen_message when a higher priority interface is detected.

get_network_interfaces returns interfaces with their priorities.

A reconnect_peer method is added to handle the reconnection.

@AlexCheema, wrote a small test script that simulates receiving a broadcast from a peer. And to see whether it keeps track of priority and change when a better one comes. Here is the output from the test:

@itsknk can you commit the test too? and add some assertions so it's a proper test would be great

Done @AlexCheema

AlexCheema · 2024-07-22T04:10:58Z

exo/networking/grpc/grpc_discovery.py

@@ -30,11 +32,42 @@ def __init__(self, node_id: str, node_port: int, listen_port: int, broadcast_por
        self.listen_port = listen_port
        self.broadcast_port = broadcast_port if broadcast_port is not None else listen_port
        self.broadcast_interval = broadcast_interval
-        self.known_peers: Dict[str, Tuple[GRPCPeerHandle, float, float]] = {}
+        self.known_peers: Dict[str, Tuple[GRPCPeerHandle, float, float, int]] = {}  # Added priority


Why is there a comment here?

AlexCheema · 2024-07-22T04:12:04Z

exo/networking/grpc/grpc_discovery.py

+                if not interface_name:
+                    continue
+
+                priority = self.get_interface_priority(interface_name, interface_type, hardware)


This is a pure function. Move it outside of the class

AlexCheema · 2024-07-22T04:13:44Z

exo/networking/grpc/test_network_interfaces.py

+import json
+import psutil
+import time
+


Once you move this off the class, get rid of the duplicate definition here

AlexCheema · 2024-07-22T04:14:35Z

exo/networking/grpc/grpc_discovery.py

        self.broadcast_task = None
        self.listen_task = None
        self.cleanup_task = None

+    def get_network_interfaces(self):


Same here, this is a pure function so move it outside of the class

AlexCheema · 2024-07-22T19:17:47Z

exo/networking/grpc/grpc_discovery.py

 from typing import List, Dict, Callable, Tuple, Coroutine
 from ..discovery import Discovery
 from ..peer_handle import PeerHandle
 from .grpc_peer_handle import GRPCPeerHandle
 from exo.topology.device_capabilities import DeviceCapabilities, device_capabilities, UNKNOWN_DEVICE_CAPABILITIES
 from exo import DEBUG_DISCOVERY

+def get_interface_priority(interface_name, interface_type='', hardware=''):


types please

AlexCheema · 2024-07-22T19:17:55Z

exo/networking/grpc/grpc_discovery.py

+    else:
+        return 0
+
+def get_network_interfaces():


return type

AlexCheema · 2024-07-22T19:18:25Z

exo/networking/grpc/grpc_discovery.py

-                print(f"Error in broadcast presence: {e}")
-                import traceback
-                print(traceback.format_exc())
+                if DEBUG_DISCOVERY >= 2: print(f"Error updating or accessing interfaces: {e}")


why did the message change?

AlexCheema · 2024-07-22T19:20:56Z

exo/networking/grpc/grpc_discovery.py

+            if peer_id in self.known_peers:
+                _, _, _, current_priority = self.known_peers[peer_id]
+                if new_priority > current_priority:
+                    await self.reconnect_peer(peer_id, peer_host, peer_port, new_priority, device_capabilities)


discovery is not responsible for connecting to peers, it just discovers them. connecting should happen in the standard_node.py

this requires a bit of a refactor to do properly, I'm focusing on linux fixes right now but if you have any questions async, leave a comment on this PR and tag me

itsknk · 2024-07-22T23:26:03Z

Hey @AlexCheema, yeah it does look a little complex. After some understanding, I thought of some steps. Please do verify these, and let me know if I'm on the right track.

Let GRPCDiscovery class to handle discovering peers and maintaining their information, without any connection logic.
The StandardNode class will handle the connection logic, including priority-based reconnections.
The update_peers method in StandardNode can check for new peers or higher priority connections and manage the connections accordingly.
The periodic topology collection remains, ensuring the node regularly updates its peer connections based on the latest discovery information.

AlexCheema · 2024-10-03T21:21:01Z

@itsknk

First of all, thanks a lot for these contributions - experimentation helps a lot to inform future decisions even if things don't get merged. I end up throwing away most the code I write.

There have been quite a few changes since this PR (health checks, broadcast on multiple interfaces, priorities).

There's now a priority that is sent in broadcast messages. Connections for a given node_id are upgraded to the highest priority connection that is healthy.

I think this PR might need to be closed now, but I'm happy to send you a retrospective $100 bounty for your work and it helped inform some of the decisions made in the repo.

If you like, we are currently setting priority to 1 for every kind of interface. If we have a good way to assign meaningful priorities (higher is better) e.g. Thunderbolt > WiFi that would be great.

AlexCheema reviewed Jul 20, 2024

View reviewed changes

itsknk force-pushed the main branch 3 times, most recently from f554ae0 to 0222b2e Compare July 20, 2024 14:40

AlexCheema reviewed Jul 20, 2024

View reviewed changes

exo/networking/grpc/grpc_discovery.py Outdated Show resolved Hide resolved

AlexCheema reviewed Jul 20, 2024

View reviewed changes

exo/networking/grpc/grpc_discovery.py Outdated Show resolved Hide resolved

itsknk force-pushed the main branch from d23ab79 to 95f22ed Compare July 20, 2024 23:20

This was referenced Jul 20, 2024

Add our own runners for CI/CD #50

Open

Switch grpc peer connections over to the best interface available #51

Open

itsknk force-pushed the main branch from 41a9f63 to c31e124 Compare July 21, 2024 06:39

AlexCheema reviewed Jul 21, 2024

View reviewed changes

itsknk force-pushed the main branch from 9d1ef8a to c17cf8d Compare July 21, 2024 23:04

itsknk force-pushed the main branch from 2a0e67e to d6016f9 Compare July 21, 2024 23:17

AlexCheema reviewed Jul 22, 2024

View reviewed changes

itsknk force-pushed the main branch from 50fd6b8 to 3355378 Compare July 22, 2024 05:22

AlexCheema reviewed Jul 22, 2024

View reviewed changes

exo/networking/grpc/grpc_discovery.py Outdated

else:

return 0

def get_network_interfaces():

Copy link

Contributor

AlexCheema Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type

AlexCheema reviewed Jul 22, 2024

View reviewed changes

itsknk closed this Oct 4, 2024

itsknk force-pushed the main branch from 3355378 to 2b9dec2 Compare October 4, 2024 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prioritise thunderbolt over WIFI #41 #47

prioritise thunderbolt over WIFI #41 #47

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema Jul 20, 2024

itsknk Jul 20, 2024

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024 •

edited

Loading

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 21, 2024

itsknk commented Jul 21, 2024

AlexCheema Jul 21, 2024

AlexCheema commented Jul 21, 2024

AlexCheema commented Jul 21, 2024

itsknk commented Jul 21, 2024

AlexCheema commented Jul 21, 2024 •

edited

Loading

itsknk commented Jul 21, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

AlexCheema Jul 22, 2024

itsknk commented Jul 22, 2024

AlexCheema commented Oct 3, 2024

prioritise thunderbolt over WIFI #41 #47

prioritise thunderbolt over WIFI #41 #47

Conversation

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 20, 2024 • edited Loading

AlexCheema commented Jul 20, 2024

itsknk commented Jul 20, 2024

AlexCheema commented Jul 21, 2024

itsknk commented Jul 21, 2024

Choose a reason for hiding this comment

AlexCheema commented Jul 21, 2024

AlexCheema commented Jul 21, 2024

itsknk commented Jul 21, 2024

AlexCheema commented Jul 21, 2024 • edited Loading

itsknk commented Jul 21, 2024

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

itsknk commented Jul 22, 2024

AlexCheema commented Oct 3, 2024

AlexCheema commented Jul 20, 2024 •

edited

Loading

AlexCheema commented Jul 21, 2024 •

edited

Loading