diff --git a/client/internal/routemanager/client.go b/client/internal/routemanager/client.go index 38cf4bf65..370ad5cf4 100644 --- a/client/internal/routemanager/client.go +++ b/client/internal/routemanager/client.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/netip" + "time" log "github.com/sirupsen/logrus" @@ -18,6 +19,7 @@ type routerPeerStatus struct { connected bool relayed bool direct bool + latency time.Duration } type routesUpdate struct { @@ -68,6 +70,7 @@ func (c *clientNetwork) getRouterPeerStatuses() map[string]routerPeerStatus { connected: peerStatus.ConnStatus == peer.StatusConnected, relayed: peerStatus.Relayed, direct: peerStatus.Direct, + latency: peerStatus.Latency, } } return routePeerStatuses @@ -83,11 +86,13 @@ func (c *clientNetwork) getRouterPeerStatuses() map[string]routerPeerStatus { // * Non-relayed: Routes without relays are preferred. // * Direct connections: Routes with direct peer connections are favored. // * Stability: In case of equal scores, the currently active route (if any) is maintained. +// * Latency: Routes with lower latency are prioritized. // // It returns the ID of the selected optimal route. func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]routerPeerStatus) string { chosen := "" - chosenScore := 0 + chosenScore := float64(0) + currScore := float64(0) currID := "" if c.chosenRoute != nil { @@ -95,7 +100,7 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro } for _, r := range c.routes { - tempScore := 0 + tempScore := float64(0) peerStatus, found := routePeerStatuses[r.ID] if !found || !peerStatus.connected { continue @@ -103,9 +108,18 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro if r.Metric < route.MaxMetric { metricDiff := route.MaxMetric - r.Metric - tempScore = metricDiff * 10 + tempScore = float64(metricDiff) * 10 } + // in some temporal cases, latency can be 0, so we set it to 1s to not block but try to avoid this route + latency := time.Second + if peerStatus.latency != 0 { + latency = peerStatus.latency + } else { + log.Warnf("peer %s has 0 latency", r.Peer) + } + tempScore += 1 - latency.Seconds() + if !peerStatus.relayed { tempScore++ } @@ -114,7 +128,7 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro tempScore++ } - if tempScore > chosenScore || (tempScore == chosenScore && r.ID == currID) { + if tempScore > chosenScore || (tempScore == chosenScore && chosen == "") { chosen = r.ID chosenScore = tempScore } @@ -123,18 +137,26 @@ func (c *clientNetwork) getBestRouteFromStatuses(routePeerStatuses map[string]ro chosen = r.ID chosenScore = tempScore } + + if r.ID == currID { + currScore = tempScore + } } - if chosen == "" { + switch { + case chosen == "": var peers []string for _, r := range c.routes { peers = append(peers, r.Peer) } log.Warnf("the network %s has not been assigned a routing peer as no peers from the list %s are currently connected", c.network, peers) - - } else if chosen != currID { - log.Infof("new chosen route is %s with peer %s with score %d for network %s", chosen, c.routes[chosen].Peer, chosenScore, c.network) + case chosen != currID: + if currScore != 0 && currScore < chosenScore+0.1 { + return currID + } else { + log.Infof("new chosen route is %s with peer %s with score %f for network %s", chosen, c.routes[chosen].Peer, chosenScore, c.network) + } } return chosen diff --git a/client/internal/routemanager/client_test.go b/client/internal/routemanager/client_test.go index 3700d72ec..d24d42b8e 100644 --- a/client/internal/routemanager/client_test.go +++ b/client/internal/routemanager/client_test.go @@ -3,6 +3,7 @@ package routemanager import ( "net/netip" "testing" + "time" "github.com/netbirdio/netbird/route" ) @@ -13,7 +14,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { name string statuses map[string]routerPeerStatus expectedRouteID string - currentRoute *route.Route + currentRoute string existingRoutes map[string]*route.Route }{ { @@ -32,7 +33,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer1", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, { @@ -51,7 +52,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer1", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, { @@ -70,7 +71,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer1", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, { @@ -89,7 +90,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer1", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "", }, { @@ -118,7 +119,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer2", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, { @@ -147,7 +148,7 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer2", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, { @@ -176,18 +177,141 @@ func TestGetBestrouteFromStatuses(t *testing.T) { Peer: "peer2", }, }, - currentRoute: nil, + currentRoute: "", expectedRouteID: "route1", }, + { + name: "multiple connected peers with different latencies", + statuses: map[string]routerPeerStatus{ + "route1": { + connected: true, + latency: 300 * time.Millisecond, + }, + "route2": { + connected: true, + latency: 10 * time.Millisecond, + }, + }, + existingRoutes: map[string]*route.Route{ + "route1": { + ID: "route1", + Metric: route.MaxMetric, + Peer: "peer1", + }, + "route2": { + ID: "route2", + Metric: route.MaxMetric, + Peer: "peer2", + }, + }, + currentRoute: "", + expectedRouteID: "route2", + }, + { + name: "should ignore routes with latency 0", + statuses: map[string]routerPeerStatus{ + "route1": { + connected: true, + latency: 0 * time.Millisecond, + }, + "route2": { + connected: true, + latency: 10 * time.Millisecond, + }, + }, + existingRoutes: map[string]*route.Route{ + "route1": { + ID: "route1", + Metric: route.MaxMetric, + Peer: "peer1", + }, + "route2": { + ID: "route2", + Metric: route.MaxMetric, + Peer: "peer2", + }, + }, + currentRoute: "", + expectedRouteID: "route2", + }, + { + name: "current route with similar score and similar but slightly worse latency should not change", + statuses: map[string]routerPeerStatus{ + "route1": { + connected: true, + relayed: false, + direct: true, + latency: 12 * time.Millisecond, + }, + "route2": { + connected: true, + relayed: false, + direct: true, + latency: 10 * time.Millisecond, + }, + }, + existingRoutes: map[string]*route.Route{ + "route1": { + ID: "route1", + Metric: route.MaxMetric, + Peer: "peer1", + }, + "route2": { + ID: "route2", + Metric: route.MaxMetric, + Peer: "peer2", + }, + }, + currentRoute: "route1", + expectedRouteID: "route1", + }, + { + name: "current chosen route doesn't exist anymore", + statuses: map[string]routerPeerStatus{ + "route1": { + connected: true, + relayed: false, + direct: true, + latency: 20 * time.Millisecond, + }, + "route2": { + connected: true, + relayed: false, + direct: true, + latency: 10 * time.Millisecond, + }, + }, + existingRoutes: map[string]*route.Route{ + "route1": { + ID: "route1", + Metric: route.MaxMetric, + Peer: "peer1", + }, + "route2": { + ID: "route2", + Metric: route.MaxMetric, + Peer: "peer2", + }, + }, + currentRoute: "routeDoesntExistAnymore", + expectedRouteID: "route2", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + currentRoute := &route.Route{ + ID: "routeDoesntExistAnymore", + } + if tc.currentRoute != "" { + currentRoute = tc.existingRoutes[tc.currentRoute] + } + // create new clientNetwork client := &clientNetwork{ network: netip.MustParsePrefix("192.168.0.0/24"), routes: tc.existingRoutes, - chosenRoute: tc.currentRoute, + chosenRoute: currentRoute, } chosenRoute := client.getBestRouteFromStatuses(tc.statuses)