From 9089efea06200e6c9ddb323a21308ba076fab1e2 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 23 Aug 2023 10:05:21 -0700 Subject: [PATCH] net/netmon: make ChangeFunc's signature take new ChangeDelta, not bool Updates #9040 Change-Id: Ia43752064a1a6ecefc8802b58d6eaa0b71cf1f84 Signed-off-by: Brad Fitzpatrick --- cmd/tailscaled/debug.go | 8 ++--- ipn/ipnlocal/local.go | 7 +++-- logtail/logtail.go | 5 ++- net/netmon/netmon.go | 56 ++++++++++++++++++++++++++------- net/netmon/netmon_test.go | 8 ++--- net/sockstats/sockstats_tsgo.go | 42 ++++++++++++++----------- net/tsdial/tsdial.go | 5 ++- wgengine/userspace.go | 9 +++--- 8 files changed, 87 insertions(+), 53 deletions(-) diff --git a/cmd/tailscaled/debug.go b/cmd/tailscaled/debug.go index b6a95ba49..4d81894bb 100644 --- a/cmd/tailscaled/debug.go +++ b/cmd/tailscaled/debug.go @@ -82,13 +82,13 @@ func runMonitor(ctx context.Context, loop bool) error { } defer mon.Close() - mon.RegisterChangeCallback(func(changed bool, st *interfaces.State) { - if !changed { - log.Printf("Network monitor fired; no change") + mon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) { + if !delta.Major { + log.Printf("Network monitor fired; not a major change") return } log.Printf("Network monitor fired. New state:") - dump(st) + dump(delta.New) }) if loop { log.Printf("Starting link change monitor; initial state:") diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 8ef5b2be8..c0dd70528 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -50,6 +50,7 @@ import ( "tailscale.com/net/dnscache" "tailscale.com/net/dnsfallback" "tailscale.com/net/interfaces" + "tailscale.com/net/netmon" "tailscale.com/net/netns" "tailscale.com/net/netutil" "tailscale.com/net/tsaddr" @@ -342,7 +343,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo b.prevIfState = netMon.InterfaceState() // Call our linkChange code once with the current state, and // then also whenever it changes: - b.linkChange(false, netMon.InterfaceState()) + b.linkChange(&netmon.ChangeDelta{New: netMon.InterfaceState()}) b.unregisterNetMon = netMon.RegisterChangeCallback(b.linkChange) b.unregisterHealthWatch = health.RegisterWatcher(b.onHealthChange) @@ -508,11 +509,11 @@ func (b *LocalBackend) pauseOrResumeControlClientLocked() { } // linkChange is our network monitor callback, called whenever the network changes. -// major is whether ifst is different than earlier. -func (b *LocalBackend) linkChange(major bool, ifst *interfaces.State) { +func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) { b.mu.Lock() defer b.mu.Unlock() + ifst := delta.New hadPAC := b.prevIfState.HasPAC() b.prevIfState = ifst b.pauseOrResumeControlClientLocked() diff --git a/logtail/logtail.go b/logtail/logtail.go index 096f96422..4544af9d7 100644 --- a/logtail/logtail.go +++ b/logtail/logtail.go @@ -22,7 +22,6 @@ import ( "time" "tailscale.com/envknob" - "tailscale.com/net/interfaces" "tailscale.com/net/netmon" "tailscale.com/net/sockstats" "tailscale.com/tstime" @@ -427,8 +426,8 @@ func (l *Logger) internetUp() bool { func (l *Logger) awaitInternetUp(ctx context.Context) { upc := make(chan bool, 1) - defer l.netMonitor.RegisterChangeCallback(func(changed bool, st *interfaces.State) { - if st.AnyInterfaceUp() { + defer l.netMonitor.RegisterChangeCallback(func(delta *netmon.ChangeDelta) { + if delta.New.AnyInterfaceUp() { select { case upc <- true: default: diff --git a/net/netmon/netmon.go b/net/netmon/netmon.go index 4de8a47f4..cb79d27c0 100644 --- a/net/netmon/netmon.go +++ b/net/netmon/netmon.go @@ -71,9 +71,37 @@ type Monitor struct { } // ChangeFunc is a callback function registered with Monitor that's called when the -// network changed. The changed parameter is whether the network changed -// enough for State to have changed since the last callback. -type ChangeFunc func(changed bool, state *interfaces.State) +// network changed. +type ChangeFunc func(*ChangeDelta) + +// ChangeDelta describes the difference between two network states. +type ChangeDelta struct { + // Old is the old interface state, if known. + // It's nil if the old state is unknown. + // Do not mutate it. + Old *interfaces.State + + // New is the new network state. + // It is always non-nil. + // Do not mutate it. + New *interfaces.State + + // Major is our legacy boolean of whether the network changed in some major + // way. + // + // Deprecated: do not remove. As of 2023-08-23 we're in a renewed effort to + // remove it and ask specific qustions of ChangeDelta instead. Look at Old + // and New (or add methods to ChangeDelta) instead of using Major. + Major bool + + // TimeJumped is whether there was a big jump in wall time since the last + // time we checked. This is a hint that a mobile sleeping device might have + // come out of sleep. + TimeJumped bool + + // TODO(bradfitz): add some lazy cached fields here as needed with methods + // on *ChangeDelta to let callers ask specific questions +} // New instantiates and starts a monitoring instance. // The returned monitor is inactive until it's started by the Start method. @@ -299,29 +327,33 @@ func (m *Monitor) debounce() { } else { m.mu.Lock() - oldState := m.ifState - changed := !curState.EqualFiltered(oldState, m.isInterestingInterface, interfaces.UseInterestingIPs) - if changed { + delta := &ChangeDelta{ + Old: m.ifState, + New: curState, + } + delta.Major = !delta.New.EqualFiltered(delta.Old, m.isInterestingInterface, interfaces.UseInterestingIPs) + if delta.Major { m.gwValid = false m.ifState = curState - if s1, s2 := oldState.String(), curState.String(); s1 == s2 { + if s1, s2 := delta.Old.String(), delta.New.String(); s1 == s2 { m.logf("[unexpected] network state changed, but stringification didn't: %v", s1) - m.logf("[unexpected] old: %s", jsonSummary(oldState)) - m.logf("[unexpected] new: %s", jsonSummary(curState)) + m.logf("[unexpected] old: %s", jsonSummary(delta.Old)) + m.logf("[unexpected] new: %s", jsonSummary(delta.New)) } } // See if we have a queued or new time jump signal. if shouldMonitorTimeJump && m.checkWallTimeAdvanceLocked() { m.resetTimeJumpedLocked() - if !changed { + delta.TimeJumped = true + if !delta.Major { // Only log if it wasn't an interesting change. m.logf("time jumped (probably wake from sleep); synthesizing major change event") - changed = true + delta.Major = true } } for _, cb := range m.cbs { - go cb(changed, m.ifState) + go cb(delta) } m.mu.Unlock() } diff --git a/net/netmon/netmon_test.go b/net/netmon/netmon_test.go index 7d2516404..b209fd730 100644 --- a/net/netmon/netmon_test.go +++ b/net/netmon/netmon_test.go @@ -8,8 +8,6 @@ import ( "sync/atomic" "testing" "time" - - "tailscale.com/net/interfaces" ) func TestMonitorStartClose(t *testing.T) { @@ -40,7 +38,7 @@ func TestMonitorInjectEvent(t *testing.T) { } defer mon.Close() got := make(chan bool, 1) - mon.RegisterChangeCallback(func(changed bool, state *interfaces.State) { + mon.RegisterChangeCallback(func(*ChangeDelta) { select { case got <- true: default: @@ -101,9 +99,9 @@ func TestMonitorMode(t *testing.T) { done = t.C } n := 0 - mon.RegisterChangeCallback(func(changed bool, st *interfaces.State) { + mon.RegisterChangeCallback(func(d *ChangeDelta) { n++ - t.Logf("cb: changed=%v, ifSt=%v", changed, st) + t.Logf("cb: changed=%v, ifSt=%v", d.Major, d.New) }) mon.Start() <-done diff --git a/net/sockstats/sockstats_tsgo.go b/net/sockstats/sockstats_tsgo.go index 37edddddf..d94f279ad 100644 --- a/net/sockstats/sockstats_tsgo.go +++ b/net/sockstats/sockstats_tsgo.go @@ -266,25 +266,29 @@ func setNetMon(netMon *netmon.Monitor) { sockStats.usedInterfaces[ifIndex] = 1 } - netMon.RegisterChangeCallback(func(changed bool, state *interfaces.State) { - if changed { - if ifName := state.DefaultRouteInterface; ifName != "" { - ifIndex := state.Interface[ifName].Index - sockStats.mu.Lock() - defer sockStats.mu.Unlock() - // Ignore changes to unknown interfaces -- it would require - // updating the tx/rxBytesByInterface maps and thus - // additional locking for every read/write. Most of the time - // the set of interfaces is static. - if _, ok := sockStats.knownInterfaces[ifIndex]; ok { - sockStats.currentInterface.Store(uint32(ifIndex)) - sockStats.usedInterfaces[ifIndex] = 1 - sockStats.currentInterfaceCellular.Store(isLikelyCellularInterface(ifName)) - } else { - sockStats.currentInterface.Store(0) - sockStats.currentInterfaceCellular.Store(false) - } - } + netMon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) { + if !delta.Major { + return + } + state := delta.New + ifName := state.DefaultRouteInterface + if ifName == "" { + return + } + ifIndex := state.Interface[ifName].Index + sockStats.mu.Lock() + defer sockStats.mu.Unlock() + // Ignore changes to unknown interfaces -- it would require + // updating the tx/rxBytesByInterface maps and thus + // additional locking for every read/write. Most of the time + // the set of interfaces is static. + if _, ok := sockStats.knownInterfaces[ifIndex]; ok { + sockStats.currentInterface.Store(uint32(ifIndex)) + sockStats.usedInterfaces[ifIndex] = 1 + sockStats.currentInterfaceCellular.Store(isLikelyCellularInterface(ifName)) + } else { + sockStats.currentInterface.Store(0) + sockStats.currentInterfaceCellular.Store(false) } }) } diff --git a/net/tsdial/tsdial.go b/net/tsdial/tsdial.go index 8312c4e81..6a35b18fc 100644 --- a/net/tsdial/tsdial.go +++ b/net/tsdial/tsdial.go @@ -18,7 +18,6 @@ import ( "time" "tailscale.com/net/dnscache" - "tailscale.com/net/interfaces" "tailscale.com/net/netknob" "tailscale.com/net/netmon" "tailscale.com/net/netns" @@ -139,8 +138,8 @@ func (d *Dialer) SetNetMon(netMon *netmon.Monitor) { d.netMonUnregister = d.netMon.RegisterChangeCallback(d.linkChanged) } -func (d *Dialer) linkChanged(major bool, state *interfaces.State) { - if !major { +func (d *Dialer) linkChanged(delta *netmon.ChangeDelta) { + if !delta.Major { return } d.mu.Lock() diff --git a/wgengine/userspace.go b/wgengine/userspace.go index ce7815177..0fcf01324 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -25,7 +25,6 @@ import ( "tailscale.com/ipn/ipnstate" "tailscale.com/net/dns" "tailscale.com/net/flowtrack" - "tailscale.com/net/interfaces" "tailscale.com/net/netmon" "tailscale.com/net/packet" "tailscale.com/net/sockstats" @@ -304,9 +303,9 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) logf("link state: %+v", e.netMon.InterfaceState()) - unregisterMonWatch := e.netMon.RegisterChangeCallback(func(changed bool, st *interfaces.State) { + unregisterMonWatch := e.netMon.RegisterChangeCallback(func(delta *netmon.ChangeDelta) { tshttpproxy.InvalidateCache() - e.linkChange(changed, st) + e.linkChange(delta) }) closePool.addFunc(unregisterMonWatch) e.netMonUnregister = unregisterMonWatch @@ -1099,7 +1098,9 @@ func (e *userspaceEngine) LinkChange(_ bool) { e.netMon.InjectEvent() } -func (e *userspaceEngine) linkChange(changed bool, cur *interfaces.State) { +func (e *userspaceEngine) linkChange(delta *netmon.ChangeDelta) { + changed := delta.Major // TODO(bradfitz): ask more specific questions? + cur := delta.New up := cur.AnyInterfaceUp() if !up { e.logf("LinkChange: all links down; pausing: %v", cur)