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

Open
opened 2026-08-05 08:06:27 -04:00 by saavagebueno · 0 comments
Owner

Original Pull Request: https://github.com/netbirdio/netbird/pull/6576

State: open
Merged: No


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.
**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6576 **State:** open **Merged:** No --- ## 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 -->
saavagebueno added the pull-request label 2026-08-05 08:06:27 -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#28494