diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 9572538bb..3bd44f6c7 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -259,6 +259,8 @@ func (s *NodeStore) PutNode(n types.Node) types.NodeView { } // UpdateNodeFunc is a function type that takes a pointer to a [types.Node] and modifies it. +// Referenced data, including Hostinfo and slices, is shared with published +// snapshots. Replace it instead of mutating it in place. type UpdateNodeFunc func(n *types.Node) // UpdateNode applies a function to modify a specific node in the @@ -537,7 +539,7 @@ func (s *NodeStore) applyBatch(batch []work) { } oldGivenName := n.GivenName - pre := n.Clone() + pre := n fn(&n) if n.GivenName != oldGivenName { @@ -550,7 +552,7 @@ func (s *NodeStore) applyBatch(batch []work) { w.pre[id] = pre.View() } - relation, election := updateChanges(pre, &n) + relation, election := updateChanges(&pre, &n) relationChanged = relationChanged || relation electionChanged = electionChanged || election } diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 5e6cb48d2..56c3f47a5 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1682,6 +1682,53 @@ func TestUpdateNodeDiff(t *testing.T) { assert.False(t, before.Valid()) } +func TestUpdateNodeDiffSharesUnchangedRoutes(t *testing.T) { + node := createTestNode(1, 1, "user1", "node1") + peer := createTestNode(2, 2, "user2", "node2") + announced := make([]netip.Prefix, 2048) + + ip := netip.MustParseAddr("10.0.0.0") + for i := range announced { + announced[i] = netip.PrefixFrom(ip, 32) + ip = ip.Next() + } + + node.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: announced} + node.ApprovedRoutes = []netip.Prefix{announced[0]} + lastSeen := time.Now().Add(-time.Minute) + node.LastSeen = &lastSeen + + var peerBuilds atomic.Int64 + + store := NewNodeStore(types.Nodes{&node, &peer}, + func(nodes []types.NodeView) map[types.NodeID][]types.NodeID { + peerBuilds.Add(1) + + return allowAllPeersFunc(nodes) + }, TestBatchSize, TestBatchTimeout) + store.Start() + + defer store.Stop() + + peerBuilds.Store(0) + + published := store.data.Load() + now := time.Now() + before, after, ok := store.UpdateNodeDiff(1, func(n *types.Node) { + n.LastSeen = &now + }) + require.True(t, ok) + assert.Same(t, &before.AnnouncedRoutes()[0], &after.AnnouncedRoutes()[0]) + assert.Same(t, &published.nodesByID[1].Hostinfo.RoutableIPs[0], + &after.AnnouncedRoutes()[0]) + assert.Equal(t, lastSeen, before.LastSeen().Get()) + assert.Equal(t, lastSeen, *published.nodesByID[1].LastSeen) + assert.Equal(t, now, after.LastSeen().Get()) + assert.Zero(t, peerBuilds.Load()) + assert.Same(t, &published.peersByNode[1][0], &store.data.Load().peersByNode[1][0]) + assert.Equal(t, now, store.ListPeers(2).At(0).LastSeen().Get()) +} + // TestListPeersExcludesSelf proves a node is never returned among its own // peers, on both the snapshot path and the explicit peer-ID path. // @@ -2032,15 +2079,20 @@ func benchNodes(n int) ([]types.User, types.Nodes) { // (lastseen) should cost far fewer builds than a relation-changing one // (route, tag). func BenchmarkNodeStoreWrite(b *testing.B) { - users, nodes := benchNodes(617) - for _, kind := range []struct { - name string - mutate func(i int, n *types.Node) + name string + announced int + mutate func(i int, n *types.Node) }{ {name: "lastseen", mutate: func(_ int, n *types.Node) { n.LastSeen = new(time.Now()) }}, + { + name: "lastseen-many-routes", announced: 4096, + mutate: func(_ int, n *types.Node) { + n.LastSeen = new(time.Now()) + }, + }, {name: "route", mutate: func(i int, n *types.Node) { subnet := netip.PrefixFrom(netip.AddrFrom4([4]byte{10, byte(200 + i%50), 0, 0}), 24) //nolint:gosec n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} @@ -2056,6 +2108,17 @@ func BenchmarkNodeStoreWrite(b *testing.B) { n.UserID, n.User = nil, nil }}, } { + users, nodes := benchNodes(617) + if kind.announced > 0 { + nodes[0].Hostinfo.RoutableIPs = make([]netip.Prefix, kind.announced) + + ip := netip.MustParseAddr("10.0.0.0") + for i := range kind.announced { + nodes[0].Hostinfo.RoutableIPs[i] = netip.PrefixFrom(ip, 32) + ip = ip.Next() + } + } + b.Run(fmt.Sprintf("%s/n=%d", kind.name, len(nodes)), func(b *testing.B) { var calls atomic.Int64 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 7c3a850c5..22df631fe 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3876,7 +3876,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // update that shrinks the announced set can drop the node // out of HA candidacy without touching ApprovedRoutes. // Clear any stale Unhealthy bit in that case. - if len(currentNode.AllApprovedRoutes()) == 0 { + if currentNode.Unhealthy && len(currentNode.AllApprovedRoutes()) == 0 { currentNode.Unhealthy = false } }) diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index b01c82d32..3293daedd 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -1222,6 +1222,15 @@ func equalUnordered[E comparable](a, b []E, cmp func(E, E) int) bool { return slices.Equal(ac, bc) } +// Unchanged snapshots share immutable slices, so pointer equality avoids a scan. +func sameSlice[E comparable](a, b []E) bool { + if len(a) != len(b) { + return false + } + + return len(a) == 0 || &a[0] == &b[0] || slices.Equal(a, b) +} + // HasPolicyChange reports whether the node has changes that affect // policy evaluation. Includes approved subnet routes because they act // as source identity in [Node.CanAccess] for subnet-to-subnet ACLs, @@ -1247,6 +1256,11 @@ func (nv NodeView) HasPolicyChange(other NodeView) bool { return true } + if sameSlice(nv.AnnouncedRoutes(), other.AnnouncedRoutes()) && + sameSlice(nv.ж.ApprovedRoutes, other.ж.ApprovedRoutes) { + return false + } + if !equalPrefixesUnordered(nv.SubnetRoutes(), other.SubnetRoutes()) { return true } diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 30291fbf1..3a561dd6f 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -1076,6 +1076,54 @@ func TestNodeIsExpiredAt(t *testing.T) { }) } +func TestHasPolicyChangeSharedRouteInputs(t *testing.T) { + subnet := netip.MustParsePrefix("10.0.0.0/24") + exit := netip.MustParsePrefix("::/0") + base := Node{ + Hostinfo: &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet, exit}}, + ApprovedRoutes: []netip.Prefix{subnet, exit}, + } + + for _, tt := range []struct { + name string + mutate func(*Node) + want bool + }{ + {name: "shared slices", mutate: func(*Node) {}}, + {name: "equal slices", mutate: func(n *Node) { + n.Hostinfo = n.Hostinfo.Clone() + n.ApprovedRoutes = []netip.Prefix{subnet, exit} + }}, + {name: "reordered slices", mutate: func(n *Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{exit, subnet}} + n.ApprovedRoutes = []netip.Prefix{exit, subnet} + }}, + {name: "approval removed", mutate: func(n *Node) { + n.ApprovedRoutes = []netip.Prefix{subnet} + }, want: true}, + {name: "announcement removed", mutate: func(n *Node) { + n.Hostinfo = &tailcfg.Hostinfo{RoutableIPs: []netip.Prefix{subnet}} + }, want: true}, + } { + t.Run(tt.name, func(t *testing.T) { + other := base + tt.mutate(&other) + require.Equal(t, tt.want, base.View().HasPolicyChange(other.View())) + }) + } + + shared := base + + var changed bool + + allocs := testing.AllocsPerRun(10, func() { + changed = base.View().HasPolicyChange(shared.View()) + }) + + require.False(t, changed) + require.Zero(t, allocs, "unchanged route inputs should not derive route lists") +} + func TestValidateHostinfo(t *testing.T) { t.Parallel()