From c87d58063a554763b05dc6de24e0059cab693001 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 5 Nov 2023 07:31:06 -0800 Subject: [PATCH] control/controlclient: move lastPrintMap field from Direct to mapSession It was a really a mutable field owned by mapSession that we didn't move in earlier commits. Once moved, it's then possible to de-func-ify the code and turn it into a regular method rather than an installed optional hook. Noticed while working to move map session lifetimes out of Direct.sendMapRequest's single-HTTP-connection scope. Updates #7175 Updates #cleanup Change-Id: I6446b15793953d88d1cabf94b5943bb3ccac3ad9 Signed-off-by: Brad Fitzpatrick --- control/controlclient/auto.go | 2 +- control/controlclient/direct.go | 16 +--------------- control/controlclient/map.go | 34 +++++++++++++++++++++------------ 3 files changed, 24 insertions(+), 28 deletions(-) diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index fa5e2e106..1dc903376 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -481,7 +481,7 @@ func (mrs mapRoutineState) UpdateNetmapDelta(muts []netmap.NodeMutation) bool { // control server, and keeping the netmap up to date. func (c *Auto) mapRoutine() { defer close(c.mapDone) - mrs := &mapRoutineState{ + mrs := mapRoutineState{ c: c, bo: backoff.NewBackoff("mapRoutine", c.logf, 30*time.Second), } diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 80f6e919b..31481d50c 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -67,7 +67,6 @@ type Direct struct { controlKnobs *controlknobs.Knobs // always non-nil serverURL string // URL of the tailcontrol server clock tstime.Clock - lastPrintMap time.Time logf logger.Logf netMon *netmon.Monitor // or nil discoPubKey key.DiscoPublic @@ -969,8 +968,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap return nil } - var mapResIdx int // 0 for first message, then 1+ for deltas - sess := newMapSession(persist.PrivateNodeKey(), nu, c.controlKnobs) defer sess.Close() sess.cancel = cancel @@ -979,17 +976,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap sess.altClock = c.clock sess.machinePubKey = machinePubKey sess.onDebug = c.handleDebugMessage - sess.onConciseNetMapSummary = func(summary string) { - // Occasionally print the netmap header. - // This is handy for debugging, and our logs processing - // pipeline depends on it. (TODO: Remove this dependency.) - now := c.clock.Now() - if now.Sub(c.lastPrintMap) < 5*time.Minute { - return - } - c.lastPrintMap = now - c.logf("[v1] new network map[%d]:\n%s", mapResIdx, summary) - } sess.onSelfNodeChanged = func(nm *netmap.NetworkMap) { c.mu.Lock() defer c.mu.Unlock() @@ -1019,7 +1005,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap // the same format before just closing the connection. // We can use this same read loop either way. var msg []byte - for ; mapResIdx == 0 || isStreaming; mapResIdx++ { + for mapResIdx := 0; mapResIdx == 0 || isStreaming; mapResIdx++ { vlogf("netmap: starting size read after %v (poll %v)", time.Since(t0).Round(time.Millisecond), mapResIdx) var siz [4]byte if _, err := io.ReadFull(res.Body, siz[:]); err != nil { diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 8623208b7..6c0e006a5 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -64,15 +64,12 @@ type mapSession struct { // to request that the long poll activity watchdog timeout be reset. onDebug func(_ context.Context, _ *tailcfg.Debug, watchdogReset chan<- struct{}) error - // onConciseNetMapSummary, if non-nil, is called with the Netmap.VeryConcise summary - // whenever a map response is received. - onConciseNetMapSummary func(string) - // onSelfNodeChanged is called before the NetmapUpdater if the self node was // changed. onSelfNodeChanged func(*netmap.NetworkMap) // Fields storing state over the course of multiple MapResponses. + lastPrintMap time.Time lastNode tailcfg.NodeView peers map[tailcfg.NodeID]*tailcfg.NodeView // pointer to view (oddly). same pointers as sortedPeers. sortedPeers []*tailcfg.NodeView // same pointers as peers, but sorted by Node.ID @@ -108,17 +105,30 @@ func newMapSession(privateNodeKey key.NodePrivate, nu NetmapUpdater, controlKnob watchdogReset: make(chan struct{}), // Non-nil no-op defaults, to be optionally overridden by the caller. - logf: logger.Discard, - vlogf: logger.Discard, - cancel: func() {}, - onDebug: func(context.Context, *tailcfg.Debug, chan<- struct{}) error { return nil }, - onConciseNetMapSummary: func(string) {}, - onSelfNodeChanged: func(*netmap.NetworkMap) {}, + logf: logger.Discard, + vlogf: logger.Discard, + cancel: func() {}, + onDebug: func(context.Context, *tailcfg.Debug, chan<- struct{}) error { return nil }, + onSelfNodeChanged: func(*netmap.NetworkMap) {}, } ms.sessionAliveCtx, ms.sessionAliveCtxClose = context.WithCancel(context.Background()) return ms } +// occasionallyPrintSummary logs summary at most once very 5 minutes. The +// summary is the Netmap.VeryConcise result from the last received map response. +func (ms *mapSession) occasionallyPrintSummary(summary string) { + // Occasionally print the netmap header. + // This is handy for debugging, and our logs processing + // pipeline depends on it. (TODO: Remove this dependency.) + now := ms.clock().Now() + if now.Sub(ms.lastPrintMap) < 5*time.Minute { + return + } + ms.lastPrintMap = now + ms.logf("[v1] new network map (periodic):\n%s", summary) +} + func (ms *mapSession) clock() tstime.Clock { return cmpx.Or[tstime.Clock](ms.altClock, tstime.StdClock{}) } @@ -200,7 +210,7 @@ func (ms *mapSession) HandleNonKeepAliveMapResponse(ctx context.Context, resp *t ms.updateStateFromResponse(resp) if ms.tryHandleIncrementally(resp) { - ms.onConciseNetMapSummary(ms.lastNetmapSummary) // every 5s log + ms.occasionallyPrintSummary(ms.lastNetmapSummary) return nil } @@ -210,7 +220,7 @@ func (ms *mapSession) HandleNonKeepAliveMapResponse(ctx context.Context, resp *t nm := ms.netmap() ms.lastNetmapSummary = nm.VeryConcise() - ms.onConciseNetMapSummary(ms.lastNetmapSummary) + ms.occasionallyPrintSummary(ms.lastNetmapSummary) // If the self node changed, we might need to update persist. if resp.Node != nil {