From 91b1c1400c18ab4e33ee091d2967c5144b8b1afc Mon Sep 17 00:00:00 2001 From: Michael Ma Date: Mon, 22 Jun 2026 10:58:01 -0700 Subject: [PATCH 1/2] Make redis client pool size configurable via GNMI_REDIS_POOL_SIZE After the go-redis v9 migration (#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 --- common_utils/notification_producer.go | 6 ++-- dialout/dialout_client/dialout_client.go | 12 ++++--- gnmi_server/connection_manager.go | 6 ++-- pkg/bypass/bypass.go | 7 ++-- pkg/interceptors/dpuproxy/redis.go | 7 ++-- sonic_data_client/db_client.go | 12 ++++--- sonic_data_client/events_client.go | 6 ++-- sonic_data_client/mixed_db_client.go | 6 ++-- sonic_db_config/redis_pool.go | 42 ++++++++++++++++++++++++ 9 files changed, 84 insertions(+), 20 deletions(-) create mode 100644 sonic_db_config/redis_pool.go diff --git a/common_utils/notification_producer.go b/common_utils/notification_producer.go index bfaa42e37..56ba5cb37 100644 --- a/common_utils/notification_producer.go +++ b/common_utils/notification_producer.go @@ -28,13 +28,15 @@ func GetRedisDBClient() (*redis.Client, error) { log.Errorf("DB err: %v", err) return nil, err } - rclient := redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "tcp", Addr: addr, Password: "", // no password set DB: db, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + rclient := redis.NewClient(opts) if rclient == nil { return nil, fmt.Errorf("Cannot create redis client.") } diff --git a/dialout/dialout_client/dialout_client.go b/dialout/dialout_client/dialout_client.go index d5aa17983..51af76ed2 100644 --- a/dialout/dialout_client/dialout_client.go +++ b/dialout/dialout_client/dialout_client.go @@ -658,25 +658,29 @@ func DialOutRun(ctx context.Context, ccfg *ClientConfig) error { if err != nil { return err } - redisDb = redis.NewClient(&redis.Options{ + optsUnix := &redis.Options{ Network: "unix", Addr: addr, Password: "", // no password set DB: dbn, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(optsUnix) + redisDb = redis.NewClient(optsUnix) } else { addr, err := sdcfg.GetDbTcpAddr("CONFIG_DB", ns) if err != nil { return err } - redisDb = redis.NewClient(&redis.Options{ + optsTcp := &redis.Options{ Network: "tcp", Addr: addr, Password: "", // no password set DB: dbn, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(optsTcp) + redisDb = redis.NewClient(optsTcp) } separator, err := sdc.GetTableKeySeparator("CONFIG_DB", ns) diff --git a/gnmi_server/connection_manager.go b/gnmi_server/connection_manager.go index 4c0a9a07f..ddd75b1b1 100644 --- a/gnmi_server/connection_manager.go +++ b/gnmi_server/connection_manager.go @@ -41,13 +41,15 @@ func (cm *ConnectionManager) PrepareRedis() { log.Errorf("DB err: %v", err) return } - rclient = redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "tcp", Addr: addr, Password: "", DB: db, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + rclient = redis.NewClient(opts) res, _ := rclient.HGetAll(context.Background(), "TELEMETRY_CONNECTIONS").Result() diff --git a/pkg/bypass/bypass.go b/pkg/bypass/bypass.go index bd162de6d..dbb067ceb 100644 --- a/pkg/bypass/bypass.go +++ b/pkg/bypass/bypass.go @@ -12,6 +12,7 @@ import ( "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" "google.golang.org/grpc/metadata" ) @@ -72,13 +73,15 @@ func getConfigDbClientDefault() (*redis.Client, error) { } } - client := redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: network, Addr: addr, Password: "", DB: configDbId, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + client := redis.NewClient(opts) return client, nil } diff --git a/pkg/interceptors/dpuproxy/redis.go b/pkg/interceptors/dpuproxy/redis.go index f6e86688b..5e90d8d63 100644 --- a/pkg/interceptors/dpuproxy/redis.go +++ b/pkg/interceptors/dpuproxy/redis.go @@ -4,6 +4,7 @@ import ( "context" "github.com/redis/go-redis/v9" + sdcfg "github.com/sonic-net/sonic-gnmi/sonic_db_config" ) // RedisClient defines the interface for Redis operations needed by DPU resolver. @@ -31,10 +32,12 @@ func (a *GoRedisAdapter) HGetAll(ctx context.Context, key string) (map[string]st // NewRedisClient creates a new Redis client connected to SONiC's Redis instance. // It connects via Unix socket to the specified database. func NewRedisClient(socketPath string, db int) *redis.Client { - return redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "unix", Addr: socketPath, Password: "", // SONiC Redis has no password DB: db, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + return redis.NewClient(opts) } diff --git a/sonic_data_client/db_client.go b/sonic_data_client/db_client.go index 58629d35e..a42c761c7 100644 --- a/sonic_data_client/db_client.go +++ b/sonic_data_client/db_client.go @@ -554,13 +554,15 @@ func useRedisTcpClient() error { continue } // DB connector for direct redis operation - redisDb := redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "tcp", Addr: addr, Password: "", // no password set DB: int(dbn), DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + redisDb := redis.NewClient(opts) Target2RedisDb[dbNamespace][dbName] = redisDb } } @@ -589,13 +591,15 @@ func initRedisDbClients() { continue } // DB connector for direct redis operation - redisDb := redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "unix", Addr: addr, Password: "", // no password set DB: int(dbn), DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + redisDb := redis.NewClient(opts) Target2RedisDb[dbNamespace][dbName] = redisDb } } diff --git a/sonic_data_client/events_client.go b/sonic_data_client/events_client.go index d1ea0a4fb..19194e3de 100644 --- a/sonic_data_client/events_client.go +++ b/sonic_data_client/events_client.go @@ -236,13 +236,15 @@ func update_stats(evtc *EventClient) { return } - rclient = redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "tcp", Addr: addr, Password: "", // no password set, DB: dbId, DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + rclient = redis.NewClient(opts) // Init current values for cumulative keys and clear for absolute for _, key := range STATS_CUMULATIVE_KEYS { diff --git a/sonic_data_client/mixed_db_client.go b/sonic_data_client/mixed_db_client.go index 8318f1989..1e4cffd08 100644 --- a/sonic_data_client/mixed_db_client.go +++ b/sonic_data_client/mixed_db_client.go @@ -635,13 +635,15 @@ func initRedisDbMap() { return } // DB connector for direct redis operation - redisDb := redis.NewClient(&redis.Options{ + opts := &redis.Options{ Network: "unix", Addr: addr, Password: "", // no password set DB: int(dbn), DialTimeout: 0, - }) + } + sdcfg.ApplyRedisPoolSize(opts) + redisDb := redis.NewClient(opts) RedisDbMap[ns+":"+container+":"+dbName] = redisDb } } diff --git a/sonic_db_config/redis_pool.go b/sonic_db_config/redis_pool.go new file mode 100644 index 000000000..a0e47b8d8 --- /dev/null +++ b/sonic_db_config/redis_pool.go @@ -0,0 +1,42 @@ +// Package dbconfig redis_pool.go provides optional, environment-driven tuning +// of the redis client connection pool used by gNMI's redis clients. +package dbconfig + +import ( + "os" + "strconv" + + log "github.com/golang/glog" + "github.com/redis/go-redis/v9" +) + +// 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" + +// 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) { + if opts == nil { + return + } + v, ok := os.LookupEnv(RedisPoolSizeEnvVar) + if !ok || v == "" { + // Not set: do not modify the default pool size. + return + } + 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 + } + opts.PoolSize = n +} From 1b6bdae41ebad55f673aa64da0d17bec0d5c2ef4 Mon Sep 17 00:00:00 2001 From: Michael Ma Date: Mon, 22 Jun 2026 11:34:28 -0700 Subject: [PATCH 2/2] Address review comments - 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 --- common_utils/notification_producer.go | 4 --- gnmi_server/connection_manager.go | 6 +++- sonic_db_config/redis_pool.go | 12 +++++-- sonic_db_config/redis_pool_test.go | 45 +++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 8 deletions(-) create mode 100644 sonic_db_config/redis_pool_test.go diff --git a/common_utils/notification_producer.go b/common_utils/notification_producer.go index 56ba5cb37..7fa750260 100644 --- a/common_utils/notification_producer.go +++ b/common_utils/notification_producer.go @@ -3,7 +3,6 @@ package common_utils import ( "context" "encoding/json" - "fmt" "strings" sdcfg "github.com/sonic-net/sonic-gnmi/sonic_db_config" @@ -37,9 +36,6 @@ func GetRedisDBClient() (*redis.Client, error) { } sdcfg.ApplyRedisPoolSize(opts) rclient := redis.NewClient(opts) - if rclient == nil { - return nil, fmt.Errorf("Cannot create redis client.") - } if _, err := rclient.Ping(context.Background()).Result(); err != nil { return nil, err } diff --git a/gnmi_server/connection_manager.go b/gnmi_server/connection_manager.go index ddd75b1b1..4cdc5a0eb 100644 --- a/gnmi_server/connection_manager.go +++ b/gnmi_server/connection_manager.go @@ -51,7 +51,11 @@ func (cm *ConnectionManager) PrepareRedis() { sdcfg.ApplyRedisPoolSize(opts) rclient = redis.NewClient(opts) - res, _ := rclient.HGetAll(context.Background(), "TELEMETRY_CONNECTIONS").Result() + res, err := rclient.HGetAll(context.Background(), table).Result() + if err != nil { + log.Errorf("Failed to read %s for cleanup: %v", table, err) + return + } if res == nil { return diff --git a/sonic_db_config/redis_pool.go b/sonic_db_config/redis_pool.go index a0e47b8d8..584b8a045 100644 --- a/sonic_db_config/redis_pool.go +++ b/sonic_db_config/redis_pool.go @@ -1,5 +1,6 @@ -// Package dbconfig redis_pool.go provides optional, environment-driven tuning -// of the redis client connection pool used by gNMI's redis clients. +// redis_pool.go provides optional, environment-driven tuning of the redis +// client connection pool used by gNMI's redis clients. + package dbconfig import ( @@ -33,10 +34,15 @@ func ApplyRedisPoolSize(opts *redis.Options) { return } n, err := strconv.Atoi(v) - if err != nil || n <= 0 { + if err != nil { log.Warningf("Ignoring invalid %s=%q (must be a positive integer): %v", RedisPoolSizeEnvVar, v, err) return } + if n <= 0 { + log.Warningf("Ignoring non-positive %s=%q (must be a positive integer)", + RedisPoolSizeEnvVar, v) + return + } opts.PoolSize = n } diff --git a/sonic_db_config/redis_pool_test.go b/sonic_db_config/redis_pool_test.go new file mode 100644 index 000000000..af9f48bb4 --- /dev/null +++ b/sonic_db_config/redis_pool_test.go @@ -0,0 +1,45 @@ +package dbconfig + +import ( + "os" + "testing" + + "github.com/redis/go-redis/v9" +) + +func TestApplyRedisPoolSize(t *testing.T) { + tests := []struct { + name string + set bool + value string + expected int // expected PoolSize after apply; 0 means left at default + }{ + {name: "unset leaves default", set: false, expected: 0}, + {name: "empty leaves default", set: true, value: "", expected: 0}, + {name: "valid positive value applied", set: true, value: "5", expected: 5}, + {name: "zero leaves default", set: true, value: "0", expected: 0}, + {name: "negative leaves default", set: true, value: "-3", expected: 0}, + {name: "non-integer leaves default", set: true, value: "abc", expected: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.set { + t.Setenv(RedisPoolSizeEnvVar, tt.value) + } else { + os.Unsetenv(RedisPoolSizeEnvVar) + } + + opts := &redis.Options{} + ApplyRedisPoolSize(opts) + if opts.PoolSize != tt.expected { + t.Errorf("PoolSize = %d, want %d", opts.PoolSize, tt.expected) + } + }) + } +} + +func TestApplyRedisPoolSizeNilOptions(t *testing.T) { + // Must not panic on nil options. + ApplyRedisPoolSize(nil) +}