[PR #6576] [client] progressive (chunked) network-map apply #26213

Open
opened 2026-08-05 07:07:02 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6576
Author: @riccardomanfrin
Created: 6/28/2026
Status: 🔄 Open

Base: mainHead: netmap_progressive_alignment


📝 Commits (10+)

  • 29dde90 Modifies handleSync to support progressive peers conns convergence
  • 74bb5c6 Allows to specify max batch for tests
  • 5bec1e8 Adds map state manager
  • d3f2ef9 Comment why not serial
  • bc22926 Drop in case of error, will reconcile with next update
  • 297dcb3 Always run onConverged for every map that is processed
  • 0bf964d Do not process intermediate one if new ones are fresher just use the freshest
  • d82d62e Adds explicit merge call for future map updates
  • 3b8fc68 Do the wholesale (firewall/routes/dns) once only
  • 2552830 Prevents skipping of intermediate map updates potentially not applied

📊 Changes

5 files changed (+723 additions, -125 deletions)

View changed files

📝 client/internal/engine.go (+180 -118)
📝 client/internal/engine_privileged_test.go (+4 -4)
📝 client/internal/engine_test.go (+44 -3)
client/internal/mapsync.go (+214 -0)
client/internal/mapsync_test.go (+281 -0)

📄 Description

Describe your changes

Applies the network map in bounded passes instead of one shot, so the global
syncMsgMux is released between batches. At ~3000+ peers the old handleSync held
that lock for the whole O(N) apply, starving other subsystems (mgmt keepalive,
lock/timer housekeeping). Scalability hardening — not a bug fix.

  • Bounded peer apply. removePeers/modifyPeers/addNewPeers take a per-pass cap
    (maxPeersPerSyncPass = 300) and return more; idempotent diffs so cap + re-run
    converges. Each pass = one syncMsgMux lock/unlock.
  • MapStateManager (client/internal/mapsync.go, new). Single hand-off between the
    Sync stream (SetTarget) and a background convergence loop. State = appliedGen == targetGen (internal counter, covers config-only updates that carry no serial).
    Coalesces toward the latest target; intermediate maps skipped from apply (logged).
  • Per-map signal. onConverged fires once per processed map (feeds the existing
    RecordSyncDuration / "sync finished" log); skipped & dropped maps don't.
  • Drop-on-error, not retry (mgmt re-delivers the full map; avoids poison-update
    spin). Clean exit on ctx cancel.
  • Wholesale-once. firewall/ACL, DNS, routes, forward rules → applyWholesale, run
    only on a target's first pass (idempotent, replace-semantics, applied before peers).
    Forward rules stashed in e.forwardingRules for the lazy-exclude finalize.
  • Persist every received update in SetTarget (once per update, not per pass);
    persistSyncResponse self-guards config-only (nil NetworkMap).
  • mergeTarget seam — today replaces, the single entrypoint for future delta squash.

The stream callback is now async (SetTarget returns immediately, convergence runs in
the background); TestEngine_Sync already polls so it tolerates it. New pure unit tests
in client/internal/mapsync_test.go.

This PR is also preparation for handling incremental updates

image

Internal scalability task (NetBird "Network Map Performant" workstream), discussed with
the team — no separate public GitHub issue. Reworks the one-shot apply in
https://github.com/netbirdio/netbird/blob/main/client/internal/engine.go

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md.

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Internal client-side refactor: same end state, only internal timing changes
(progressive/async apply). No CLI flags, gRPC/daemon API, config, or user-visible
behavior change.

Docs PR URL (required if "docs added" is checked)

N/A

Summary by CodeRabbit

Summary

  • New Features

    • Improved management sync now applies network maps in bounded multi-pass convergence for smoother, incremental rollout behavior.
    • Added an internal convergence manager that coalesces incoming sync updates and signals completion only after the targeted state is fully processed.
  • Bug Fixes

    • Deferred lazy exclude updates until convergence completes to avoid partial-store effects.
    • Prevented overwriting the last saved network map with config-only updates when the network map is nil.
  • Tests

    • Added/expanded unit tests for convergence pass chunking, coalescing/merge behavior, persistence rules, and “converges then stops” semantics.

🔄 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/netbirdio/netbird/pull/6576 **Author:** [@riccardomanfrin](https://github.com/riccardomanfrin) **Created:** 6/28/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `netmap_progressive_alignment` --- ### 📝 Commits (10+) - [`29dde90`](https://github.com/netbirdio/netbird/commit/29dde908ae7bbf95fb6df96568859e85efd3dc56) Modifies handleSync to support progressive peers conns convergence - [`74bb5c6`](https://github.com/netbirdio/netbird/commit/74bb5c613e6855d6a4776c31c6e61e59cce2197e) Allows to specify max batch for tests - [`5bec1e8`](https://github.com/netbirdio/netbird/commit/5bec1e8f03947c0c973ae2aadc0e7a533373764a) Adds map state manager - [`d3f2ef9`](https://github.com/netbirdio/netbird/commit/d3f2ef9adb575e34bac5ba4066d258a8e51ac012) Comment why not serial - [`bc22926`](https://github.com/netbirdio/netbird/commit/bc22926fe06085bca82a5c45175403e3233f1fdc) Drop in case of error, will reconcile with next update - [`297dcb3`](https://github.com/netbirdio/netbird/commit/297dcb3e24595f72c8308ad8a672fc799fa75923) Always run onConverged for every map that is processed - [`0bf964d`](https://github.com/netbirdio/netbird/commit/0bf964dad785cb3775ee142769ffbf4dc84ac4a3) Do not process intermediate one if new ones are fresher just use the freshest - [`d82d62e`](https://github.com/netbirdio/netbird/commit/d82d62e81887fd7bd543b9af00d85c66dbe177a2) Adds explicit merge call for future map updates - [`3b8fc68`](https://github.com/netbirdio/netbird/commit/3b8fc688f4ce3ac0e04410646e5c49e7aa797ce8) Do the wholesale (firewall/routes/dns) once only - [`2552830`](https://github.com/netbirdio/netbird/commit/2552830184e0892e068fef3adcc86d9ec7b77fd9) Prevents skipping of intermediate map updates potentially not applied ### 📊 Changes **5 files changed** (+723 additions, -125 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/engine.go` (+180 -118) 📝 `client/internal/engine_privileged_test.go` (+4 -4) 📝 `client/internal/engine_test.go` (+44 -3) ➕ `client/internal/mapsync.go` (+214 -0) ➕ `client/internal/mapsync_test.go` (+281 -0) </details> ### 📄 Description ## Describe your changes Applies the network map in **bounded passes** instead of one shot, so the global `syncMsgMux` is released between batches. At ~3000+ peers the old `handleSync` held that lock for the whole O(N) apply, starving other subsystems (mgmt keepalive, lock/timer housekeeping). Scalability hardening — not a bug fix. - **Bounded peer apply.** `removePeers`/`modifyPeers`/`addNewPeers` take a per-pass cap (`maxPeersPerSyncPass = 300`) and return `more`; idempotent diffs so cap + re-run converges. Each pass = one `syncMsgMux` lock/unlock. - **MapStateManager (`client/internal/mapsync.go`, new).** Single hand-off between the Sync stream (`SetTarget`) and a background convergence loop. State = `appliedGen == targetGen` (internal counter, covers config-only updates that carry no serial). Coalesces toward the latest target; intermediate maps skipped from apply (logged). - **Per-map signal.** `onConverged` fires once per *processed* map (feeds the existing `RecordSyncDuration` / "sync finished" log); skipped & dropped maps don't. - **Drop-on-error**, not retry (mgmt re-delivers the full map; avoids poison-update spin). Clean exit on ctx cancel. - **Wholesale-once.** firewall/ACL, DNS, routes, forward rules → `applyWholesale`, run only on a target's first pass (idempotent, replace-semantics, applied before peers). Forward rules stashed in `e.forwardingRules` for the lazy-exclude finalize. - **Persist every received update** in `SetTarget` (once per update, not per pass); `persistSyncResponse` self-guards config-only (nil NetworkMap). - **`mergeTarget` seam** — today replaces, the single entrypoint for future delta squash. The stream callback is now async (`SetTarget` returns immediately, convergence runs in the background); `TestEngine_Sync` already polls so it tolerates it. New pure unit tests in `client/internal/mapsync_test.go`. This PR is also preparation for handling incremental updates <img width="2028" height="2916" alt="image" src="https://github.com/user-attachments/assets/670c13a9-dd88-4cb8-ab1e-d388c418a13b" /> ## Issue ticket number and link Internal scalability task (NetBird "Network Map Performant" workstream), discussed with the team — no separate public GitHub issue. Reworks the one-shot apply in https://github.com/netbirdio/netbird/blob/main/client/internal/engine.go ## Stack <!-- branch-stack --> ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [x] Is a feature enhancement - [x] It is a refactor - [x] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client-side refactor: same end state, only internal timing changes (progressive/async apply). No CLI flags, gRPC/daemon API, config, or user-visible behavior change. ### Docs PR URL (required if "docs added" is checked) N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary * **New Features** * Improved management sync now applies network maps in bounded multi-pass convergence for smoother, incremental rollout behavior. * Added an internal convergence manager that coalesces incoming sync updates and signals completion only after the targeted state is fully processed. * **Bug Fixes** * Deferred lazy exclude updates until convergence completes to avoid partial-store effects. * Prevented overwriting the last saved network map with config-only updates when the network map is nil. * **Tests** * Added/expanded unit tests for convergence pass chunking, coalescing/merge behavior, persistence rules, and “converges then stops” semantics. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 07:07:02 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#26213