Make redis client pool size configurable via GNMI_REDIS_POOL_SIZE#706
Open
make1980 wants to merge 2 commits into
Open
Make redis client pool size configurable via GNMI_REDIS_POOL_SIZE#706make1980 wants to merge 2 commits into
make1980 wants to merge 2 commits into
Conversation
After the go-redis v9 migration (sonic-net#528), each redis client defaults its PoolSize to 10*GOMAXPROCS, which scales with logical CPUs. GNMI creates one client per DB per namespace, so telemetry RSS grows with CPU count (sonic-net/sonic-buildimage#26825). Add dbconfig.ApplyRedisPoolSize() which sets PoolSize from the GNMI_REDIS_POOL_SIZE env var. When the var is unset or invalid, the default pool size is left unchanged. Applied to all production redis client creations. Signed-off-by: Michael Ma <kema2@microsoft.com>
|
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mechanism to cap go-redis connection pool sizes (via GNMI_REDIS_POOL_SIZE) to reduce telemetry RSS growth on high-CPU systems after the go-redis v9 migration, and applies it consistently across the various Redis client creation sites in the codebase.
Changes:
- Introduce
dbconfig.ApplyRedisPoolSize(*redis.Options)to optionally overrideredis.Options.PoolSizefromGNMI_REDIS_POOL_SIZE. - Update Redis client initialization sites to construct
redis.Options, apply the helper, then create clients. - Extend the change to additional packages (e.g., bypass path and DPU proxy adapter) to keep Redis client behavior consistent.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
sonic_db_config/redis_pool.go |
Adds env-driven helper to optionally override go-redis PoolSize. |
sonic_data_client/mixed_db_client.go |
Applies pool-size helper for mixed DB Redis clients. |
sonic_data_client/events_client.go |
Applies pool-size helper for COUNTERS_DB Redis client used by event stats updater. |
sonic_data_client/db_client.go |
Applies pool-size helper for TCP/unix Redis clients created for targets/namespaces. |
pkg/interceptors/dpuproxy/redis.go |
Applies pool-size helper to DPU proxy Redis client creation. |
pkg/bypass/bypass.go |
Applies pool-size helper to bypass path CONFIG_DB client creation. |
gnmi_server/connection_manager.go |
Applies pool-size helper to connection manager’s STATE_DB Redis client creation. |
dialout/dialout_client/dialout_client.go |
Applies pool-size helper to dial-out CONFIG_DB Redis client creation (unix/tcp). |
common_utils/notification_producer.go |
Applies pool-size helper to notification producer’s STATE_DB Redis client creation. |
Comment on lines
+1
to
+2
| // Package dbconfig redis_pool.go provides optional, environment-driven tuning | ||
| // of the redis client connection pool used by gNMI's redis clients. |
Comment on lines
+35
to
+40
| n, err := strconv.Atoi(v) | ||
| if err != nil || n <= 0 { | ||
| log.Warningf("Ignoring invalid %s=%q (must be a positive integer): %v", | ||
| RedisPoolSizeEnvVar, v, err) | ||
| return | ||
| } |
Comment on lines
+18
to
+26
| // ApplyRedisPoolSize sets opts.PoolSize from the GNMI_REDIS_POOL_SIZE | ||
| // environment variable. | ||
| // | ||
| // If the variable is unset, empty, or not a positive integer, opts is left | ||
| // unchanged so go-redis keeps its default pool size (10 * runtime.GOMAXPROCS), | ||
| // which scales with the number of logical CPUs. Setting the variable lets | ||
| // operators cap per-client connections (and their buffers) on high-CPU | ||
| // systems to reduce memory consumption. | ||
| func ApplyRedisPoolSize(opts *redis.Options) { |
Comment on lines
39
to
42
| rclient := redis.NewClient(opts) | ||
| if rclient == nil { | ||
| return nil, fmt.Errorf("Cannot create redis client.") | ||
| } |
Comment on lines
+52
to
55
| rclient = redis.NewClient(opts) | ||
|
|
||
| res, _ := rclient.HGetAll(context.Background(), "TELEMETRY_CONNECTIONS").Result() | ||
|
|
- redis_pool.go: drop package-doc form on second file; separate parse-error from non-positive value in the warning; add unit tests - notification_producer.go: remove dead redis.NewClient nil check and now-unused fmt import - connection_manager.go: use table constant and handle HGetAll error Signed-off-by: Michael Ma <kema2@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hdwhdw
self-requested a review
July 7, 2026 03:08
hdwhdw
requested changes
Jul 13, 2026
| "github.com/golang/glog" | ||
| gnmipb "github.com/openconfig/gnmi/proto/gnmi" | ||
| "github.com/redis/go-redis/v9" | ||
| sdcfg "github.com/sonic-net/sonic-gnmi/sonic_db_config" |
Contributor
There was a problem hiding this comment.
This import breaks the pure build because sonic_db_config also pulls in swsscommon. Could we move the existing implementation and test to a pure package like:
// internal/redisopts/redis_pool.go
package redisopts
const PoolSizeEnvVar = "GNMI_REDIS_POOL_SIZE"
func ApplyPoolSize(opts *redis.Options) {
// Existing parsing and validation logic.
}Then add internal/redisopts to PURE_PACKAGES in pure.mk and update the call sites to:
import "github.com/sonic-net/sonic-gnmi/internal/redisopts"
redisopts.ApplyPoolSize(opts)| // RedisPoolSizeEnvVar is the environment variable used to override the redis | ||
| // client connection pool size. When it is not set, callers must leave the | ||
| // go-redis default pool size unchanged. | ||
| const RedisPoolSizeEnvVar = "GNMI_REDIS_POOL_SIZE" |
Contributor
There was a problem hiding this comment.
Is there a sonic-buildimage PR associated with this change?
|
@make1980 Can you please provide ETA to address the comments? I assume you need to create another PR in sonic-buildimage to fix the bug. Can you let us know ETA for that as well? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
After the go-redis v9 migration (#528), each
redis.Clientdefaults its connectionPoolSizeto10 * runtime.GOMAXPROCS, which scales with the number of logical CPUs. gNMI creates one redis client per DB per namespace (plus a few others), so the telemetry process RSS grows with CPU count, as reported in sonic-net/sonic-buildimage#26825 (e.g. ~78 MB on a 2-CPU switch vs ~104 MB on a 16-CPU switch while idle). The earlierdefer Close()fix (commit d297045) addressed an unrelated connection leak and did not change this scaling.How I did it
dbconfig.ApplyRedisPoolSize(opts *redis.Options)(new filesonic_db_config/redis_pool.go). It reads theGNMI_REDIS_POOL_SIZEenvironment variable and setsopts.PoolSizeonly when the variable is set to a positive integer.sonic_data_client/db_client.go(initRedisDbClients,useRedisTcpClient)sonic_data_client/events_client.gosonic_data_client/mixed_db_client.gognmi_server/connection_manager.gocommon_utils/notification_producer.godialout/dialout_client/dialout_client.gopkg/bypass/bypass.gopkg/interceptors/dpuproxy/redis.goHow to verify it
GNMI_REDIS_POOL_SIZEis unset.GNMI_REDIS_POOL_SIZE=2for the telemetry process and confirm reduced/stable RSS on high-CPU systems:echo "RSS=$(grep VmRSS /proc/$(pidof telemetry)/status | awk '{print $2,$3}')"~#clients * PoolSizeand no longer scale with CPU count.Fixes sonic-net/sonic-buildimage#26825