From 676b5b79469bf247be2deb632d3de7ed87ebf012 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Mon, 11 Jan 2021 11:38:49 -0800
Subject: [PATCH] net/netcheck: improve the preferred DERP hysteresis

Users in Amsterdam (as one example) were flipping back and forth
between equidistant London & Frankfurt relays too much.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 net/netcheck/netcheck.go      | 30 ++++++++++++++++++++++++------
 net/netcheck/netcheck_test.go | 18 ++++++++++++++++++
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go
index 27d2cedc1..1b0194830 100644
--- a/net/netcheck/netcheck.go
+++ b/net/netcheck/netcheck.go
@@ -1102,6 +1102,10 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
+	var prevDERP int
+	if c.last != nil {
+		prevDERP = c.last.PreferredDERP
+	}
 	if c.prev == nil {
 		c.prev = map[time.Time]*Report{}
 	}
@@ -1119,9 +1123,9 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) {
 			delete(c.prev, t)
 			continue
 		}
-		for hp, d := range pr.RegionLatency {
-			if bd, ok := bestRecent[hp]; !ok || d < bd {
-				bestRecent[hp] = d
+		for regionID, d := range pr.RegionLatency {
+			if bd, ok := bestRecent[regionID]; !ok || d < bd {
+				bestRecent[regionID] = d
 			}
 		}
 	}
@@ -1129,13 +1133,27 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) {
 	// Then, pick which currently-alive DERP server from the
 	// current report has the best latency over the past maxAge.
 	var bestAny time.Duration
-	for hp := range r.RegionLatency {
-		best := bestRecent[hp]
+	var oldRegionCurLatency time.Duration
+	for regionID, d := range r.RegionLatency {
+		if regionID == prevDERP {
+			oldRegionCurLatency = d
+		}
+		best := bestRecent[regionID]
 		if r.PreferredDERP == 0 || best < bestAny {
 			bestAny = best
-			r.PreferredDERP = hp
+			r.PreferredDERP = regionID
 		}
 	}
+
+	// If we're changing our preferred DERP but the old one's still
+	// accessible and the new one's not much better, just stick with
+	// where we are.
+	if prevDERP != 0 &&
+		r.PreferredDERP != prevDERP &&
+		oldRegionCurLatency != 0 &&
+		bestAny > oldRegionCurLatency/3*2 {
+		r.PreferredDERP = prevDERP
+	}
 }
 
 func updateLatency(m map[int]time.Duration, regionID int, d time.Duration) {
diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go
index da229ee4c..94a5b369b 100644
--- a/net/netcheck/netcheck_test.go
+++ b/net/netcheck/netcheck_test.go
@@ -195,6 +195,24 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) {
 			wantPrevLen: 1, // t=[0123]s all gone. (too old, older than 10 min)
 			wantDERP:    3, // only option
 		},
+		{
+			name: "preferred_derp_hysteresis_no_switch",
+			steps: []step{
+				{0 * time.Second, report("d1", 4, "d2", 5)},
+				{1 * time.Second, report("d1", 4, "d2", 3)},
+			},
+			wantPrevLen: 2,
+			wantDERP:    1, // 2 didn't get fast enough
+		},
+		{
+			name: "preferred_derp_hysteresis_do_switch",
+			steps: []step{
+				{0 * time.Second, report("d1", 4, "d2", 5)},
+				{1 * time.Second, report("d1", 4, "d2", 1)},
+			},
+			wantPrevLen: 2,
+			wantDERP:    2, // 2 got fast enough
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {