starred/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-01-11 11:50:30 +01:00
Code Issues 98 Packages Projects Releases 10 Wiki Activity

[PR #2509] [MERGED] Only read relevant nodes from database in PeerChangedResponse #2699

New Issue
Closed
opened 2025-12-29 03:22:24 +01:00 by adam · 0 comments
adam commented 2025-12-29 03:22:24 +01:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/juanfont/headscale/pull/2509
Author: @Enkelmann
Created: 3/28/2025
Status: ✅ Merged
Merged: 4/8/2025
Merged by: @kradalby

Base: main ← Head: main

📝 Commits (8)

  • af04eb5 Only read relevant nodes from database in PeerChangedResponse
  • 67bdd19 Rework to ensure transactional consistency in PeerChangedResponse again
  • 619ca9f An empty nodeIDs list should return an empty nodes list
  • c1d57c7 Add test to ListNodesSubset
  • e3d44d3 Link PR in CHANGELOG.md
  • e0df114 combine ListNodes and ListNodesSubset into one function
  • cf70643 query for all nodes in ListNodes if no parameter is given
  • 03e892c also add optional filtering for relevant nodes to ListPeers

📊 Changes

4 files changed (+221 additions, -23 deletions)

View changed files

📝 CHANGELOG.md (+1 -0)
📝 hscontrol/db/node.go (+19 -10)
📝 hscontrol/db/node_test.go (+171 -0)
📝 hscontrol/mapper/mapper.go (+30 -13)

📄 Description

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.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/juanfont/headscale/pull/2509 **Author:** [@Enkelmann](https://github.com/Enkelmann) **Created:** 3/28/2025 **Status:** ✅ Merged **Merged:** 4/8/2025 **Merged by:** [@kradalby](https://github.com/kradalby) **Base:** `main` ← **Head:** `main` --- ### 📝 Commits (8) - [`af04eb5`](https://github.com/juanfont/headscale/commit/af04eb5ffdcdc7060df0bb4190b27effc7bc7a3f) Only read relevant nodes from database in PeerChangedResponse - [`67bdd19`](https://github.com/juanfont/headscale/commit/67bdd195600cde0512bbbbaab853ef3b4d0fb9a4) Rework to ensure transactional consistency in PeerChangedResponse again - [`619ca9f`](https://github.com/juanfont/headscale/commit/619ca9faac8658989f75722cc899d74661f23912) An empty nodeIDs list should return an empty nodes list - [`c1d57c7`](https://github.com/juanfont/headscale/commit/c1d57c7c5f8978998f1e16f5b3940733d9c384f0) Add test to ListNodesSubset - [`e3d44d3`](https://github.com/juanfont/headscale/commit/e3d44d3b445437459c4bd0bb12954c75a7a1a801) Link PR in CHANGELOG.md - [`e0df114`](https://github.com/juanfont/headscale/commit/e0df114d5ab169c6d1f7c23e769837a639a3f5af) combine ListNodes and ListNodesSubset into one function - [`cf70643`](https://github.com/juanfont/headscale/commit/cf70643818c9a71d83907cce361a6b89f1285a26) query for all nodes in ListNodes if no parameter is given - [`03e892c`](https://github.com/juanfont/headscale/commit/03e892cfb8fdd18290b03180cf3a5b579f21b99d) also add optional filtering for relevant nodes to ListPeers ### 📊 Changes **4 files changed** (+221 additions, -23 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+1 -0) 📝 `hscontrol/db/node.go` (+19 -10) 📝 `hscontrol/db/node_test.go` (+171 -0) 📝 `hscontrol/mapper/mapper.go` (+30 -13) </details> ### 📄 Description 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). - [x] have read the [CONTRIBUTING.md](./CONTRIBUTING.md) file - [ ] raised a GitHub issue or discussed it on the projects chat beforehand - [x] added unit tests - [ ] added integration tests - [ ] updated documentation if needed - [x] 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`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
adam added the pull-request label 2025-12-29 03:22:24 +01:00
adam closed this issue 2025-12-29 03:22:24 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
CLI
DERP
DNS
Nix
OIDC
SSH
bug
database
documentation
duplicate
enhancement
faq
good first issue
grants
help wanted
might-come
needs design doc
needs investigation
no-stale-bot
out of scope
performance
policy 📝
pull-request
Mirrored from GitHub Pull Request
 question
regression
routes
stale
tags
tailscale-feature-gap
well described ❤️
wontfix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/headscale#2699
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?