[PR #6421] [client] use dedicated IsHealthy RPC for management health check instead of GetServerKey #28189

Closed
opened 2026-08-05 08:05:56 -04:00 by saavagebueno · 0 comments
Owner

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

State: closed
Merged: Yes


Describe your changes

The management connection health check in GrpcClient.IsHealthy probed
liveness by calling the GetServerKey RPC and discarding the returned key
(_, err := c.realClient.GetServerKey(...)). That handler does real
per-call work server-side: it reads the WireGuard secret via
secretsManager.GetWGKey(), formats the peer IP, emits a trace log,
increments the CountGetKeyRequest metric, and allocates a response — all
thrown away by the caller.

This switches the probe to the dedicated IsHealthy RPC, a no-op handler
that returns an empty response, so the health check stops re-fetching a
static key it never uses. Liveness semantics are unchanged.

The probe is throttled to ~1 call / 5s per daemon (probeThreshold), so a
management server with N peers running the desktop UI continuously serves
~N/5 of these RPCs per second. For large fleets that is meaningful, fully
avoidable load for what is only a liveness ping. GetServerKey is untouched
for its real purpose (fetching the server public key before encrypted
requests); only the health-check path changes.

The server-side IsHealthy handler already existed
(https://github.com/netbirdio/netbird/blob/main/management/internals/shared/grpc/server.go#L981)
but had no callers; this makes it the canonical management health-check path.

Note for reviewers: after this change the CountGetKeyRequest metric no
longer counts health-check traffic, so any dashboard/alert using it as a
proxy for active clients should be updated.

No public issue. Internal optimization of the management health-check
transport. Pre-change call site on main:
https://github.com/netbirdio/netbird/blob/main/shared/management/client/grpc.go#L527

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)

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 health-check transport detail. No user-facing CLI/API/config
surface changes; the client still reports management connectivity exactly
as before, it just probes via a cheaper RPC.

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

Paste the PR link from https://github.com/netbirdio/docs here:

N/A

Summary by CodeRabbit

  • Bug Fixes
    • Improved server connection health-check mechanism to ensure more reliable detection of connection status and client disconnection states.
**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6421 **State:** closed **Merged:** Yes --- ## Describe your changes The management connection health check in `GrpcClient.IsHealthy` probed liveness by calling the `GetServerKey` RPC and discarding the returned key (`_, err := c.realClient.GetServerKey(...)`). That handler does real per-call work server-side: it reads the WireGuard secret via `secretsManager.GetWGKey()`, formats the peer IP, emits a trace log, increments the `CountGetKeyRequest` metric, and allocates a response — all thrown away by the caller. This switches the probe to the dedicated `IsHealthy` RPC, a no-op handler that returns an empty response, so the health check stops re-fetching a static key it never uses. Liveness semantics are unchanged. The probe is throttled to ~1 call / 5s per daemon (`probeThreshold`), so a management server with N peers running the desktop UI continuously serves ~N/5 of these RPCs per second. For large fleets that is meaningful, fully avoidable load for what is only a liveness ping. `GetServerKey` is untouched for its real purpose (fetching the server public key before encrypted requests); only the health-check path changes. The server-side `IsHealthy` handler already existed (https://github.com/netbirdio/netbird/blob/main/management/internals/shared/grpc/server.go#L981) but had no callers; this makes it the canonical management health-check path. Note for reviewers: after this change the `CountGetKeyRequest` metric no longer counts health-check traffic, so any dashboard/alert using it as a proxy for active clients should be updated. ## Issue ticket number and link No public issue. Internal optimization of the management health-check transport. Pre-change call site on main: https://github.com/netbirdio/netbird/blob/main/shared/management/client/grpc.go#L527 ## Stack <!-- branch-stack --> ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [x] It is a refactor - [ ] Created tests that fail without the change (if possible) > 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 health-check transport detail. No user-facing CLI/API/config surface changes; the client still reports management connectivity exactly as before, it just probes via a cheaper RPC. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved server connection health-check mechanism to ensure more reliable detection of connection status and client disconnection states. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
saavagebueno added the pull-request label 2026-08-05 08:05:56 -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#28189