Skip to content

Only read relevant nodes from database in PeerChangedResponse #2509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kradalby merged 8 commits into from
Apr 8, 2025
Merged

Only read relevant nodes from database in PeerChangedResponse #2509

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af04eb5
Only read relevant nodes from database in PeerChangedResponse
enkelmann-tng Mar 28, 2025
67bdd19
Rework to ensure transactional consistency in PeerChangedResponse again
enkelmann-tng Mar 28, 2025
619ca9f
An empty nodeIDs list should return an empty nodes list
enkelmann-tng Mar 28, 2025
c1d57c7
Add test to ListNodesSubset
enkelmann-tng Mar 28, 2025
e3d44d3
Link PR in CHANGELOG.md
enkelmann-tng Apr 1, 2025
e0df114
combine ListNodes and ListNodesSubset into one function
enkelmann-tng Apr 1, 2025
cf70643
query for all nodes in ListNodes if no parameter is given
enkelmann-tng Apr 7, 2025
03e892c
also add optional filtering for relevant nodes to ListPeers
enkelmann-tng Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ The new policy can be used by setting the environment variable
[#2493](https://github.com/juanfont/headscale/pull/2493)
- If a OIDC provider doesn't include the `email_verified` claim in its ID
tokens, Headscale will attempt to get it from the UserInfo endpoint.
- Improve performance by only querying relevant nodes from the database for node updates
Copy link
Collaborator

@kradalby kradalby Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please link the pr

Copy link
Contributor Author

@Enkelmann Enkelmann Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


## 0.25.1 (2025-02-25)

Expand Down
34 changes: 25 additions & 9 deletions hscontrol/mapper/mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package mapper
import (
"encoding/binary"
"encoding/json"
"errors"
"fmt"
"gorm.io/gorm"
"io/fs"
"net/url"
"os"
Expand Down Expand Up @@ -257,11 +259,6 @@ func (m *Mapper) PeerChangedResponse(
) ([]byte, error) {
resp := m.baseMapResponse()

peers, err := m.ListPeers(node.ID)
Copy link
Collaborator

@kradalby kradalby Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is not necessary, but instead of listing all, we should just list the one from the change map.

It would make more sense to make ListPeers(node NodeID, peerIDs ...NodeID) and only fetch the ones that are needed here.

Copy link
Contributor Author

@Enkelmann Enkelmann Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted against changing ListPeers for now. It is also used for full map responses, where I think all peers are needed (although I have not looked too deeply into this function yet). But I can of course change the naming scheme if you like something like ListPeersSubset better than ListNodesSubset.

Copy link
Collaborator

@kradalby kradalby Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think modifying both ListNodes and ListPeers to be able to take a optional list arg would be the best.

if err != nil {
return nil, err
}

var removedIDs []tailcfg.NodeID
var changedIDs []types.NodeID
for nodeID, nodeChanged := range changed {
Expand All @@ -273,13 +270,21 @@ func (m *Mapper) PeerChangedResponse(
}

changedNodes := make(types.Nodes, 0, len(changedIDs))
for _, peer := range peers {
if slices.Contains(changedIDs, peer.ID) {
changedNodes = append(changedNodes, peer)
for _, changedID := range changedIDs {
if changedID != node.ID {
changedNode, err := m.GetNode(changedID)
Copy link
Collaborator

@kradalby kradalby Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with doing it this way, and not as mentioned above is that we dont get transactional consistency. In this case, nodes might change in between calls to GetNode. While this might not be an obvious problem, it might lead to incredibly hard to debug problems.

Copy link
Contributor Author

@Enkelmann Enkelmann Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reworked the code to use exactly one database query. I will also add a test to the database query function at least.

if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
continue
} else {
return nil, err
}
}
changedNodes = append(changedNodes, changedNode)
}
}

err = appendPeerChanges(
err := appendPeerChanges(
&resp,
false, // partial change
m.polMan,
Expand Down Expand Up @@ -496,6 +501,17 @@ func (m *Mapper) ListPeers(nodeID types.NodeID) (types.Nodes, error) {
return peers, nil
}

func (m *Mapper) GetNode(nodeID types.NodeID) (*types.Node, error) {
node, err := m.db.GetNodeByID(nodeID)
if err != nil {
return nil, err
}
online := m.notif.IsLikelyConnected(node.ID)
node.IsOnline = &online

return node, nil
}

func nodeMapToList(nodes map[uint64]*types.Node) types.Nodes {
ret := make(types.Nodes, 0)

Expand Down