From d53b900b38c387517eee1436a29c0c5623cacc3d Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 21 Dec 2022 10:16:09 -0800 Subject: [PATCH 1/4] ringhash: allow overriding max ringhash size via environment variable --- internal/envconfig/envconfig.go | 15 ++++ internal/envconfig/envconfig_test.go | 74 +++++++++++++++++++ internal/envconfig/xds.go | 6 ++ xds/internal/balancer/ringhash/config.go | 15 ++-- xds/internal/balancer/ringhash/config_test.go | 43 ++++++++++- 5 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 internal/envconfig/envconfig_test.go diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 7edd196bd3d..794c1f02a8a 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -21,6 +21,7 @@ package envconfig import ( "os" + "strconv" "strings" ) @@ -37,3 +38,17 @@ var ( // ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false"). AdvertiseCompressors = !strings.EqualFold(os.Getenv(advertiseCompressorsStr), "false") ) + +func uint64FromEnv(envVar string, def, min, max uint64) uint64 { + v, err := strconv.ParseUint(os.Getenv(envVar), 10, 64) + if err != nil { + return def + } + if v < min { + return min + } + if v > max { + return max + } + return v +} diff --git a/internal/envconfig/envconfig_test.go b/internal/envconfig/envconfig_test.go new file mode 100644 index 00000000000..a719d8f4b6f --- /dev/null +++ b/internal/envconfig/envconfig_test.go @@ -0,0 +1,74 @@ +/* + * + * Copyright 2022 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package envconfig + +import ( + "os" + "testing" + + "google.golang.org/grpc/internal/grpctest" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +func (s) TestUint64FromEnv(t *testing.T) { + + var testCases = []struct { + name string + val string + def, min, max uint64 + want uint64 + }{ + { + name: "error parsing; want default", + val: "asdf", def: 5, want: 5, + }, { + name: "unset; want default", + val: "", def: 5, want: 5, + }, { + name: "too low; want min", + val: "5", min: 10, want: 10, + }, { + name: "too high; want max", + val: "5", max: 2, want: 2, + }, { + name: "in range; good", + val: "17391", def: 13000, min: 12000, max: 18000, want: 17391, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + const testVar = "testvar" + if tc.val == "" { + os.Unsetenv(testVar) + } else { + os.Setenv(testVar, tc.val) + } + if got := uint64FromEnv(testVar, tc.def, tc.min, tc.max); got != tc.want { + t.Errorf("uint64FromEnv(%q(=%q), %v, %v, %v) = %v; want %v", testVar, tc.val, tc.def, tc.min, tc.max, got, tc.want) + } + }) + } +} diff --git a/internal/envconfig/xds.go b/internal/envconfig/xds.go index af09711a3e8..e1f89d8828d 100644 --- a/internal/envconfig/xds.go +++ b/internal/envconfig/xds.go @@ -99,3 +99,9 @@ var ( // C2PResolverTestOnlyTrafficDirectorURI is the TD URI for testing. C2PResolverTestOnlyTrafficDirectorURI = os.Getenv(c2pResolverTestOnlyTrafficDirectorURIEnv) ) + +// XDSRingHashLimit indicates the maximum ring size which defaults to 4096 but +// may be overridden by setting the environment variable +// "GRPC_XDS_RING_HASH_LIMIT". A hard maximum limit of 8MB and minimum limit +// of 1 is also enforced. +var XDSRingHashLimit = uint64FromEnv("GRPC_XDS_RING_HASH_LIMIT", 4096, 1, 8*1024*1024) diff --git a/xds/internal/balancer/ringhash/config.go b/xds/internal/balancer/ringhash/config.go index 4278b0636c7..287419d0d33 100644 --- a/xds/internal/balancer/ringhash/config.go +++ b/xds/internal/balancer/ringhash/config.go @@ -22,6 +22,7 @@ import ( "encoding/json" "fmt" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/serviceconfig" ) @@ -36,8 +37,6 @@ type LBConfig struct { const ( defaultMinSize = 1024 defaultMaxSize = 4096 - // TODO(apolcyn): make makeRingSizeCap configurable, with either a dial option or global setting - maxRingSizeCap = 4096 ) func parseConfig(c json.RawMessage) (*LBConfig, error) { @@ -51,14 +50,14 @@ func parseConfig(c json.RawMessage) (*LBConfig, error) { if cfg.MaxRingSize == 0 { cfg.MaxRingSize = defaultMaxSize } - if cfg.MinRingSize > maxRingSizeCap { - cfg.MinRingSize = maxRingSizeCap - } - if cfg.MaxRingSize > maxRingSizeCap { - cfg.MaxRingSize = maxRingSizeCap - } if cfg.MinRingSize > cfg.MaxRingSize { return nil, fmt.Errorf("min %v is greater than max %v", cfg.MinRingSize, cfg.MaxRingSize) } + if cfg.MinRingSize > envconfig.XDSRingHashLimit { + cfg.MinRingSize = envconfig.XDSRingHashLimit + } + if cfg.MaxRingSize > envconfig.XDSRingHashLimit { + cfg.MaxRingSize = envconfig.XDSRingHashLimit + } return &cfg, nil } diff --git a/xds/internal/balancer/ringhash/config_test.go b/xds/internal/balancer/ringhash/config_test.go index 175301981ef..1e690338f9a 100644 --- a/xds/internal/balancer/ringhash/config_test.go +++ b/xds/internal/balancer/ringhash/config_test.go @@ -22,14 +22,16 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/internal/envconfig" ) func (s) TestParseConfig(t *testing.T) { tests := []struct { - name string - js string - want *LBConfig - wantErr bool + name string + js string + envConfigLimit uint64 + want *LBConfig + wantErr bool }{ { name: "OK", @@ -52,9 +54,42 @@ func (s) TestParseConfig(t *testing.T) { want: nil, wantErr: true, }, + { + name: "min greater than max greater than global limit", + js: `{"minRingSize": 6000, "maxRingSize": 5000}`, + want: nil, + wantErr: true, + }, + { + name: "max greater than global limit", + js: `{"minRingSize": 1, "maxRingSize": 6000}`, + want: &LBConfig{MinRingSize: 1, MaxRingSize: 4096}, + }, + { + name: "min and max greater than global limit", + js: `{"minRingSize": 5000, "maxRingSize": 6000}`, + want: &LBConfig{MinRingSize: 4096, MaxRingSize: 4096}, + }, + { + name: "min and max less than raised global limit", + js: `{"minRingSize": 5000, "maxRingSize": 6000}`, + envConfigLimit: 8000, + want: &LBConfig{MinRingSize: 5000, MaxRingSize: 6000}, + }, + { + name: "min and max greater than raised global limit", + js: `{"minRingSize": 10000, "maxRingSize": 10000}`, + envConfigLimit: 8000, + want: &LBConfig{MinRingSize: 8000, MaxRingSize: 8000}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.envConfigLimit != 0 { + old := envconfig.XDSRingHashLimit + defer func() { envconfig.XDSRingHashLimit = old }() + envconfig.XDSRingHashLimit = tt.envConfigLimit + } got, err := parseConfig([]byte(tt.js)) if (err != nil) != tt.wantErr { t.Errorf("parseConfig() error = %v, wantErr %v", err, tt.wantErr) From e8eaed38b72c55c610fb8661a2feb8ed7dfe7f01 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 21 Dec 2022 12:54:54 -0800 Subject: [PATCH 2/4] s/limit/cap/ --- internal/envconfig/xds.go | 10 +++--- xds/internal/balancer/ringhash/config.go | 8 ++--- xds/internal/balancer/ringhash/config_test.go | 34 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/envconfig/xds.go b/internal/envconfig/xds.go index e1f89d8828d..5549867843f 100644 --- a/internal/envconfig/xds.go +++ b/internal/envconfig/xds.go @@ -100,8 +100,8 @@ var ( C2PResolverTestOnlyTrafficDirectorURI = os.Getenv(c2pResolverTestOnlyTrafficDirectorURIEnv) ) -// XDSRingHashLimit indicates the maximum ring size which defaults to 4096 but -// may be overridden by setting the environment variable -// "GRPC_XDS_RING_HASH_LIMIT". A hard maximum limit of 8MB and minimum limit -// of 1 is also enforced. -var XDSRingHashLimit = uint64FromEnv("GRPC_XDS_RING_HASH_LIMIT", 4096, 1, 8*1024*1024) +// XDSRingHashCap indicates the maximum ring size which defaults to 4096 +// entries but may be overridden by setting the environment variable +// "GRPC_XDS_RING_HASH_CAP". This does not override the default bounds +// checking which NACKs configs specifying ring sizes > 8*1024*1024 (~8M). +var XDSRingHashCap = uint64FromEnv("GRPC_XDS_RING_HASH_CAP", 4096, 1, 8*1024*1024) diff --git a/xds/internal/balancer/ringhash/config.go b/xds/internal/balancer/ringhash/config.go index 287419d0d33..915674858a1 100644 --- a/xds/internal/balancer/ringhash/config.go +++ b/xds/internal/balancer/ringhash/config.go @@ -53,11 +53,11 @@ func parseConfig(c json.RawMessage) (*LBConfig, error) { if cfg.MinRingSize > cfg.MaxRingSize { return nil, fmt.Errorf("min %v is greater than max %v", cfg.MinRingSize, cfg.MaxRingSize) } - if cfg.MinRingSize > envconfig.XDSRingHashLimit { - cfg.MinRingSize = envconfig.XDSRingHashLimit + if cfg.MinRingSize > envconfig.XDSRingHashCap { + cfg.MinRingSize = envconfig.XDSRingHashCap } - if cfg.MaxRingSize > envconfig.XDSRingHashLimit { - cfg.MaxRingSize = envconfig.XDSRingHashLimit + if cfg.MaxRingSize > envconfig.XDSRingHashCap { + cfg.MaxRingSize = envconfig.XDSRingHashCap } return &cfg, nil } diff --git a/xds/internal/balancer/ringhash/config_test.go b/xds/internal/balancer/ringhash/config_test.go index 1e690338f9a..242e3d34bff 100644 --- a/xds/internal/balancer/ringhash/config_test.go +++ b/xds/internal/balancer/ringhash/config_test.go @@ -27,11 +27,11 @@ import ( func (s) TestParseConfig(t *testing.T) { tests := []struct { - name string - js string - envConfigLimit uint64 - want *LBConfig - wantErr bool + name string + js string + envConfigCap uint64 + want *LBConfig + wantErr bool }{ { name: "OK", @@ -71,24 +71,24 @@ func (s) TestParseConfig(t *testing.T) { want: &LBConfig{MinRingSize: 4096, MaxRingSize: 4096}, }, { - name: "min and max less than raised global limit", - js: `{"minRingSize": 5000, "maxRingSize": 6000}`, - envConfigLimit: 8000, - want: &LBConfig{MinRingSize: 5000, MaxRingSize: 6000}, + name: "min and max less than raised global limit", + js: `{"minRingSize": 5000, "maxRingSize": 6000}`, + envConfigCap: 8000, + want: &LBConfig{MinRingSize: 5000, MaxRingSize: 6000}, }, { - name: "min and max greater than raised global limit", - js: `{"minRingSize": 10000, "maxRingSize": 10000}`, - envConfigLimit: 8000, - want: &LBConfig{MinRingSize: 8000, MaxRingSize: 8000}, + name: "min and max greater than raised global limit", + js: `{"minRingSize": 10000, "maxRingSize": 10000}`, + envConfigCap: 8000, + want: &LBConfig{MinRingSize: 8000, MaxRingSize: 8000}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.envConfigLimit != 0 { - old := envconfig.XDSRingHashLimit - defer func() { envconfig.XDSRingHashLimit = old }() - envconfig.XDSRingHashLimit = tt.envConfigLimit + if tt.envConfigCap != 0 { + old := envconfig.XDSRingHashCap + defer func() { envconfig.XDSRingHashCap = old }() + envconfig.XDSRingHashCap = tt.envConfigCap } got, err := parseConfig([]byte(tt.js)) if (err != nil) != tt.wantErr { From aaa8ed59bcf4f05d1464e6ca5296d0b2c4587de8 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 21 Dec 2022 12:59:11 -0800 Subject: [PATCH 3/4] s/xds// --- internal/envconfig/envconfig.go | 5 +++++ internal/envconfig/xds.go | 6 ------ xds/internal/balancer/ringhash/config.go | 8 ++++---- xds/internal/balancer/ringhash/config_test.go | 6 +++--- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 794c1f02a8a..26228d78b69 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -37,6 +37,11 @@ var ( // AdvertiseCompressors is set if registered compressor should be advertised // ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false"). AdvertiseCompressors = !strings.EqualFold(os.Getenv(advertiseCompressorsStr), "false") + // XDSRingHashCap indicates the maximum ring size which defaults to 4096 + // entries but may be overridden by setting the environment variable + // "GRPC_XDS_RING_HASH_CAP". This does not override the default bounds + // checking which NACKs configs specifying ring sizes > 8*1024*1024 (~8M). + RingHashCap = uint64FromEnv("GRPC_RING_HASH_CAP", 4096, 1, 8*1024*1024) ) func uint64FromEnv(envVar string, def, min, max uint64) uint64 { diff --git a/internal/envconfig/xds.go b/internal/envconfig/xds.go index 5549867843f..af09711a3e8 100644 --- a/internal/envconfig/xds.go +++ b/internal/envconfig/xds.go @@ -99,9 +99,3 @@ var ( // C2PResolverTestOnlyTrafficDirectorURI is the TD URI for testing. C2PResolverTestOnlyTrafficDirectorURI = os.Getenv(c2pResolverTestOnlyTrafficDirectorURIEnv) ) - -// XDSRingHashCap indicates the maximum ring size which defaults to 4096 -// entries but may be overridden by setting the environment variable -// "GRPC_XDS_RING_HASH_CAP". This does not override the default bounds -// checking which NACKs configs specifying ring sizes > 8*1024*1024 (~8M). -var XDSRingHashCap = uint64FromEnv("GRPC_XDS_RING_HASH_CAP", 4096, 1, 8*1024*1024) diff --git a/xds/internal/balancer/ringhash/config.go b/xds/internal/balancer/ringhash/config.go index 915674858a1..4763120fa64 100644 --- a/xds/internal/balancer/ringhash/config.go +++ b/xds/internal/balancer/ringhash/config.go @@ -53,11 +53,11 @@ func parseConfig(c json.RawMessage) (*LBConfig, error) { if cfg.MinRingSize > cfg.MaxRingSize { return nil, fmt.Errorf("min %v is greater than max %v", cfg.MinRingSize, cfg.MaxRingSize) } - if cfg.MinRingSize > envconfig.XDSRingHashCap { - cfg.MinRingSize = envconfig.XDSRingHashCap + if cfg.MinRingSize > envconfig.RingHashCap { + cfg.MinRingSize = envconfig.RingHashCap } - if cfg.MaxRingSize > envconfig.XDSRingHashCap { - cfg.MaxRingSize = envconfig.XDSRingHashCap + if cfg.MaxRingSize > envconfig.RingHashCap { + cfg.MaxRingSize = envconfig.RingHashCap } return &cfg, nil } diff --git a/xds/internal/balancer/ringhash/config_test.go b/xds/internal/balancer/ringhash/config_test.go index 242e3d34bff..d8f9ed30bb6 100644 --- a/xds/internal/balancer/ringhash/config_test.go +++ b/xds/internal/balancer/ringhash/config_test.go @@ -86,9 +86,9 @@ func (s) TestParseConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if tt.envConfigCap != 0 { - old := envconfig.XDSRingHashCap - defer func() { envconfig.XDSRingHashCap = old }() - envconfig.XDSRingHashCap = tt.envConfigCap + old := envconfig.RingHashCap + defer func() { envconfig.RingHashCap = old }() + envconfig.RingHashCap = tt.envConfigCap } got, err := parseConfig([]byte(tt.js)) if (err != nil) != tt.wantErr { From 8e6e58caf6ad77ebe246a101b7231f906e2ac6d4 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 22 Dec 2022 08:14:23 -0800 Subject: [PATCH 4/4] comments --- internal/envconfig/envconfig.go | 4 ++-- internal/envconfig/envconfig_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 26228d78b69..6e6b00ffc1a 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -37,9 +37,9 @@ var ( // AdvertiseCompressors is set if registered compressor should be advertised // ("GRPC_GO_ADVERTISE_COMPRESSORS" is not "false"). AdvertiseCompressors = !strings.EqualFold(os.Getenv(advertiseCompressorsStr), "false") - // XDSRingHashCap indicates the maximum ring size which defaults to 4096 + // RingHashCap indicates the maximum ring size which defaults to 4096 // entries but may be overridden by setting the environment variable - // "GRPC_XDS_RING_HASH_CAP". This does not override the default bounds + // "GRPC_RING_HASH_CAP". This does not override the default bounds // checking which NACKs configs specifying ring sizes > 8*1024*1024 (~8M). RingHashCap = uint64FromEnv("GRPC_RING_HASH_CAP", 4096, 1, 8*1024*1024) ) diff --git a/internal/envconfig/envconfig_test.go b/internal/envconfig/envconfig_test.go index a719d8f4b6f..13923e5bbe6 100644 --- a/internal/envconfig/envconfig_test.go +++ b/internal/envconfig/envconfig_test.go @@ -42,19 +42,19 @@ func (s) TestUint64FromEnv(t *testing.T) { want uint64 }{ { - name: "error parsing; want default", + name: "error parsing", val: "asdf", def: 5, want: 5, }, { - name: "unset; want default", + name: "unset", val: "", def: 5, want: 5, }, { - name: "too low; want min", + name: "too low", val: "5", min: 10, want: 10, }, { - name: "too high; want max", + name: "too high", val: "5", max: 2, want: 2, }, { - name: "in range; good", + name: "in range", val: "17391", def: 13000, min: 12000, max: 18000, want: 17391, }, }