ipn, tstime : add opt in rate limiting for netmap updates on the IPN bus by barnstar · Pull Request #14119 · tailscale/tailscale

barnstar · 2024-11-15T20:13:19Z

updates tailscale/corp#24553

Adds opt-in rate limiting to limit netmap updates to, at most, one every 3 seconds when the client includes the NotifyRateLimitNetmaps option in the ipn bus watcher opts. This should mitigate issues with excessive
memory and CPU usage in clients on large, busy tailnets.

This is not a complete of comprehensive fix, but it requires the addition of only a single flag by clients and should mitigate the issue of runaway memory/CPU consumption while we rethink how we handle of netmap updates on the ipn bus more generally.

updates tailscale/corp#24553 Adds opt-in rate limiting to limit netmap updates to, at most, one every 3 seconds when the client includes the NotifyRateLimitNetmaps option in the ipn bus watcher opts. This should mitigate issues with excessive memory and CPU usage in clients on large, busy tailnets. Signed-off-by: Jonathan Nobels <jonathan@tailscale.com>

bradfitz

I like where this is going but I think this a bit more complicated than it needs to be, and I can't convince myself it's correct as a result.

bradfitz · 2024-11-15T20:21:00Z

ipn/ipnlocal/local.go

+	if mask&ipn.NotifyRateLimitNetmaps != 0 {
+		b.setNetmapRateLimit(ipn.DefaultNetmapRateLimit)
+	} else {
+		b.setNetmapRateLimit(0)
+	}
+


this is kinda weird making a per-session WatchOpt bit affect the LocalBackend globally.

What about something like #14120 ?

Ack.. Yeah, I'll toss this and run some tests using your patch. You're correct on the out-of-order comment below. I just spotted a couple of cases where this gives odd results.

bradfitz · 2024-11-15T20:21:13Z

ipn/ipnlocal/local.go

+// or not the netmap was sent or cancelled respectively.  A nil return value indicates that the netmap
+// was sent immediately.  The returned value is primarily useful for testing and you can safely ignore
+// it and just call this method at will.
+func (b *LocalBackend) sendNetmap(nm *netmap.NetworkMap) chan bool {


nothing uses this returned channel except for tests?

bradfitz · 2024-11-15T20:21:32Z

ipn/ipnlocal/local.go

+	// deferredNetmapCancel is used to cancel deferred netmap updates which
+	// were initially blocked due to rate limiting.  We always attempt to send the latest
+	// netmap once the rate limiter allows it, discarding any pending netmaps.
+	deferredNetmapCancel context.CancelFunc


bradfitz · 2024-11-15T20:21:46Z

ipn/ipnlocal/local.go

+// b.mu must be held
+func (b *LocalBackend) setNetmapRateLimit(interval time.Duration) {


add "Locked" suffix to the method name

bradfitz · 2024-11-15T20:21:52Z

ipn/ipnlocal/local.go


+// setNetmapRateLimit Sets the minimum interval between netmap updates on the IPN Bus (in seconds)
+// If interval is 0 or negative, the rate limiter is disabled.  Netmap rate limiting is
+// disabled by default


trailing period

bradfitz · 2024-11-15T20:23:01Z

ipn/ipnlocal/local.go

+		case <-ctx.Done():
+			c <- false
+		}
+		close(c)


there's only one value ever sent, right? you don't need to close channels. a close is just a special type of send saying it's all done. But if the contract is there's only one value, you can just omit this

bradfitz · 2024-11-15T20:23:20Z

ipn/ipnlocal/local.go

+
 var _ controlclient.NetmapDeltaUpdater = (*LocalBackend)(nil)

-// UpdateNetmapDelta implements controlclient.NetmapDeltaUpdater.


restore this?

bradfitz · 2024-11-15T20:25:47Z

ipn/ipnlocal/local.go

+	go func() {
+		select {
+		case <-time.After(delay):
+			b.send(notify)


I can't convince myself that this goroutine's send can't get scheduled out of order with another send, and then push down IPN bus updates out of order.

bradfitz · 2024-11-15T20:27:11Z

ipn/ipnlocal/local.go

+		if b.deferredNetmapCancel != nil {
+			b.deferredNetmapCancel()
+		}


I think all state changes need to flush any pending message and then send the State update.

bradfitz · 2024-11-15T20:29:47Z

ipn/ipnlocal/local.go

+		b.deferredNetmapCancel()
+	}


you need to wait for it to be done after this.

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's #14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

barnstar · 2024-11-15T21:55:39Z

Closing in favour of #14120

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's #14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's #14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's #14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's #14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

@barnstar

Limit spamming GUIs with boring updates to once in 3 seconds, unless the notification is relatively interesting and the GUI should update immediately. This is basically @barnstar's tailscale#14119 but with the logic moved to be per-watch-session (since the bit is per session), rather than globally. And this distinguishes notable Notify messages (such as state changes) and makes them send immediately. Updates tailscale/corp#24553 Change-Id: I79cac52cce85280ce351e65e76ea11e107b00b49 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

barnstar requested review from agottardo, bradfitz and nickoneill November 15, 2024 20:13

barnstar force-pushed the jonathan/netmap_ratelimiter branch from 3aa809a to b24b36f Compare November 15, 2024 20:14

bradfitz reviewed Nov 15, 2024

View reviewed changes

bradfitz mentioned this pull request Nov 15, 2024

ipn/ipnlocal: add IPN Bus NotifyRateLimit watch bit NotifyRateLimit #14120

Merged

barnstar closed this Nov 15, 2024

barnstar deleted the jonathan/netmap_ratelimiter branch April 1, 2025 13:15

		// b.mu must be held
		func (b *LocalBackend) setNetmapRateLimit(interval time.Duration) {


		var _ controlclient.NetmapDeltaUpdater = (*LocalBackend)(nil)

		// UpdateNetmapDelta implements controlclient.NetmapDeltaUpdater.

Conversation

barnstar commented Nov 15, 2024

Uh oh!

bradfitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barnstar commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants