Only read relevant nodes from database in PeerChangedResponse #2509

Enkelmann · 2025-03-28T11:11:46Z

Querying the whole database becomes a performance issue if a lot of nodes exist in it, even if all those nodes are offline. In PeerChangedResponse we actually know which nodes need to be queried, so we can restrict the database queries to these nodes.

In my tests, PeerChangedResponse usually needed to query less than 5 nodes, so the change resulted in an order of magnitude better performance for a database with several hundred nodes (most of them offline).

have read the CONTRIBUTING.md file
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

I can also write a test if desired. The PR should not change any behavior, so I wanted to base tests on existing tests for PeerChangedResponse. However, I did not find any existing tests for this function, so I am happy for pointers where to start for testing PeerChangedResponse.

kradalby

I think the general change makes sense, in general a change request is (in my observation) not more than a subset of nodes, like you call out.

I am up for this, but it needs to be fetched as ones.

kradalby · 2025-03-28T11:38:49Z

hscontrol/mapper/mapper.go

 ) ([]byte, error) {
 	resp := m.baseMapResponse()

-	peers, err := m.ListPeers(node.ID)


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.

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.

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

kradalby · 2025-03-28T11:40:06Z

hscontrol/mapper/mapper.go

-			changedNodes = append(changedNodes, peer)
+	for _, changedID := range changedIDs {
+		if changedID != node.ID {
+			changedNode, err := m.GetNode(changedID)


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.

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

Enkelmann · 2025-03-28T13:49:43Z

The code is reworked to use exactly one database query and I also added a unit test. I have some concern that my unit test does not cover the case of a NodeID being zero, although the code to add nodes in the test seem to suggest that this case cannot happen anyway.

kradalby

Much better, couple of comments to simplify and keep around lest functions/code.

kradalby · 2025-03-31T14:02:20Z

CHANGELOG.md

  [#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


please link the pr

kradalby · 2025-03-31T14:03:58Z

hscontrol/db/node.go

 	return nodes, nil
 }

+func (hsdb *HSDatabase) ListNodesSubset(nodeIDs types.NodeIDs) (types.Nodes, error) {


Suggested change

func (hsdb *HSDatabase) ListNodesSubset(nodeIDs types.NodeIDs) (types.Nodes, error) {

func (hsdb *HSDatabase) ListNodes(nodeIDs ...types.NodeID) (types.Nodes, error) {

I would prefer to just have the same function for this, but use the list expansion argument like this so it remains optional for getting all.

Is implemented.

kradalby · 2025-03-31T14:05:54Z

hscontrol/mapper/mapper.go

 ) ([]byte, error) {
 	resp := m.baseMapResponse()

-	peers, err := m.ListPeers(node.ID)


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

kradalby · 2025-04-01T08:13:02Z

hscontrol/mapper/mapper.go

 	return peers, nil
 }

+func (m *Mapper) ListNodes(nodeIDs ...types.NodeIDs) (types.Nodes, error) {


You need to pass it as types.NodeID (without the s), now you are passing it as a list of lists

That is by design. When using an optional argument we face the problem of having two different types of empty lists, both of which are needed:

The empty list of not using the optional argument. We want to return all nodes in this case.

The empty list of using the optional argument, but with a list where all nodes have been filtered out. We want to return an empty list here and not all nodes.

One could use the nil and []type.NodeID{} variants of empty lists to distinguish these two cases. But in my experience, code depending on different behavior between these two empty list variants is a future bug waiting to happen. I know that my design also has a hidden footgun in that one could pass more than one list of nodes as parameter to the function. But I would prefer having two ListNodes functions before changing the parameter type to ...types.NodeID.

Side remark: I will wait until we have settled on a design for ListNodes before I take a second look at ListPeers.

The empty list of using the optional argument, but with a list where all nodes have been filtered out. We want to return an empty list here and not all nodes.

This scenario does not make any sense to me, in the case where all nodes has been filtered out, you would have an empty list of IDs and you would instead not call for the database and return early.

Well, the proposed code does check it and returns early right before the database access would otherwise occur. In my opinion it is better than relying on the caller to know that he has to check it before calling ListNodes.

As context: In my load tests this case did actually happen and there was no check in PeerChangedResponse before querying the database for zero nodes.

I think it will be fine with a docstring explaining the filter. Current behaviour is not that ergonomic and I would want it to be.

Although I do not like it from a coding-style perspective, I implemented it now.

ghost · 2025-04-07T08:53:06Z

Pull Request Revisions

Revision	Description
r2	Added peer filtering to ListPeers Expanded `ListPeers` method to support optional peer ID filtering in database and mapper layers
r1	Optimize node database query performance Enhanced `ListNodes` to support filtering by node IDs, improving database query efficiency and reducing unnecessary data retrieval

✅ AI review completed for r2

Enkelmann · 2025-04-07T09:03:34Z

Ok, ListNodes now returns all nodes when no parameter is given and the behavior is documented in a docstring. If you like this version, I can take a look at ListPeers.

kradalby · 2025-04-07T09:24:24Z

Looks great, thank you!

Enkelmann · 2025-04-08T09:32:51Z

I added the same functionality to ListPeers (currently not used anywhere).
As far as I can tell the integration test failures have nothing to do with the PR, so I have not done anything about them.

kradalby

Great! thank you!

Only read relevant nodes from database in PeerChangedResponse

af04eb5

Enkelmann requested review from juanfont, kradalby, nblock and ohdearaugustin as code owners March 28, 2025 11:11

kradalby requested changes Mar 28, 2025

View reviewed changes

enkelmann-tng added 3 commits March 28, 2025 13:48

Rework to ensure transactional consistency in PeerChangedResponse again

67bdd19

An empty nodeIDs list should return an empty nodes list

619ca9f

Add test to ListNodesSubset

c1d57c7

kradalby requested changes Mar 31, 2025

View reviewed changes

enkelmann-tng added 2 commits April 1, 2025 07:58

Link PR in CHANGELOG.md

e3d44d3

combine ListNodes and ListNodesSubset into one function

e0df114

kradalby reviewed Apr 1, 2025

View reviewed changes

query for all nodes in ListNodes if no parameter is given

cf70643

ghost approved these changes Apr 7, 2025

View reviewed changes

also add optional filtering for relevant nodes to ListPeers

03e892c

ghost approved these changes Apr 8, 2025

View reviewed changes

kradalby approved these changes Apr 8, 2025

View reviewed changes

kradalby merged commit 0d31347 into juanfont:main Apr 8, 2025
271 of 276 checks passed

kradalby mentioned this pull request Apr 14, 2025

[Feature] Replacing encoding/json with https://github.com/bytedance/sonic #2513

Closed

2 tasks

	func (hsdb *HSDatabase) ListNodesSubset(nodeIDs types.NodeIDs) (types.Nodes, error) {
	func (hsdb *HSDatabase) ListNodes(nodeIDs ...types.NodeID) (types.Nodes, error) {

Uh oh!

Only read relevant nodes from database in PeerChangedResponse #2509

Only read relevant nodes from database in PeerChangedResponse #2509

Uh oh!

Conversation

Enkelmann commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kradalby 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!

Enkelmann commented Mar 28, 2025

Uh oh!

kradalby 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!

Enkelmann Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ghost commented Apr 7, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Revisions

Uh oh!

Enkelmann commented Apr 7, 2025

Uh oh!

kradalby commented Apr 7, 2025

Uh oh!

Enkelmann commented Apr 8, 2025

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enkelmann commented Mar 28, 2025 •

edited

Loading

Enkelmann Apr 3, 2025 •

edited

Loading

ghost commented Apr 7, 2025 •

edited by ghost

Loading