Skip to content

ringhash: move out of the xds package #8249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 18, 2025
Merged

ringhash: move out of the xds package #8249

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5dc5c02
ringhash: move out of the xds package
atollena Apr 14, 2025
1473b59
move XDSRequestHash/SetXDSRequestHash to internal/xds
atollena Apr 14, 2025
b1112ce
unexport LBConfig
atollena Apr 14, 2025
5d1e357
comments from doug
atollena Apr 17, 2025
9b037a8
Merge branch 'master' into issue-8213
atollena Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions xds/internal/balancer/ringhash/config.go → balancer/ringhash/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,17 @@ import (

"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/metadata"
"google.golang.org/grpc/serviceconfig"
iringhash "google.golang.org/grpc/internal/ringhash"
)

// LBConfig is the balancer config for ring_hash balancer.
type LBConfig struct {
serviceconfig.LoadBalancingConfig `json:"-"`

MinRingSize uint64 `json:"minRingSize,omitempty"`
MaxRingSize uint64 `json:"maxRingSize,omitempty"`
RequestHashHeader string `json:"requestHashHeader,omitempty"`
}

const (
defaultMinSize = 1024
defaultMaxSize = 4096
ringHashSizeUpperBound = 8 * 1024 * 1024 // 8M
)

func parseConfig(c json.RawMessage) (*LBConfig, error) {
var cfg LBConfig
func parseConfig(c json.RawMessage) (*iringhash.LBConfig, error) {
var cfg iringhash.LBConfig
if err := json.Unmarshal(c, &cfg); err != nil {
return nil, err
}
Expand Down
24 changes: 13 additions & 11 deletions ...internal/balancer/ringhash/config_test.go → balancer/ringhash/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ import (
"testing"

"github.com/google/go-cmp/cmp"

"google.golang.org/grpc/internal/envconfig"
iringhash "google.golang.org/grpc/internal/ringhash"
"google.golang.org/grpc/internal/testutils"
)

Expand All @@ -33,26 +35,26 @@ func (s) TestParseConfig(t *testing.T) {
js string
envConfigCap uint64
requestHeaderEnvVar bool
want *LBConfig
want *iringhash.LBConfig
wantErr bool
}{
{
name: "OK",
js: `{"minRingSize": 1, "maxRingSize": 2}`,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 1, MaxRingSize: 2},
want: &iringhash.LBConfig{MinRingSize: 1, MaxRingSize: 2},
},
{
name: "OK with default min",
js: `{"maxRingSize": 2000}`,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: defaultMinSize, MaxRingSize: 2000},
want: &iringhash.LBConfig{MinRingSize: defaultMinSize, MaxRingSize: 2000},
},
{
name: "OK with default max",
js: `{"minRingSize": 2000}`,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 2000, MaxRingSize: defaultMaxSize},
want: &iringhash.LBConfig{MinRingSize: 2000, MaxRingSize: defaultMaxSize},
},
{
name: "min greater than max",
Expand All @@ -72,27 +74,27 @@ func (s) TestParseConfig(t *testing.T) {
name: "max greater than global limit",
js: `{"minRingSize": 1, "maxRingSize": 6000}`,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 1, MaxRingSize: 4096},
want: &iringhash.LBConfig{MinRingSize: 1, MaxRingSize: 4096},
},
{
name: "min and max greater than global limit",
js: `{"minRingSize": 5000, "maxRingSize": 6000}`,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 4096, MaxRingSize: 4096},
want: &iringhash.LBConfig{MinRingSize: 4096, MaxRingSize: 4096},
},
{
name: "min and max less than raised global limit",
js: `{"minRingSize": 5000, "maxRingSize": 6000}`,
envConfigCap: 8000,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 5000, MaxRingSize: 6000},
want: &iringhash.LBConfig{MinRingSize: 5000, MaxRingSize: 6000},
},
{
name: "min and max greater than raised global limit",
js: `{"minRingSize": 10000, "maxRingSize": 10000}`,
envConfigCap: 8000,
requestHeaderEnvVar: true,
want: &LBConfig{MinRingSize: 8000, MaxRingSize: 8000},
want: &iringhash.LBConfig{MinRingSize: 8000, MaxRingSize: 8000},
},
{
name: "min greater than upper bound",
Expand All @@ -112,7 +114,7 @@ func (s) TestParseConfig(t *testing.T) {
name: "request metadata key set",
js: `{"requestHashHeader": "x-foo"}`,
requestHeaderEnvVar: true,
want: &LBConfig{
want: &iringhash.LBConfig{
MinRingSize: defaultMinSize,
MaxRingSize: defaultMaxSize,
RequestHashHeader: "x-foo",
Expand All @@ -122,7 +124,7 @@ func (s) TestParseConfig(t *testing.T) {
name: "request metadata key set with uppercase letters",
js: `{"requestHashHeader": "x-FOO"}`,
requestHeaderEnvVar: true,
want: &LBConfig{
want: &iringhash.LBConfig{
MinRingSize: defaultMinSize,
MaxRingSize: defaultMaxSize,
RequestHashHeader: "x-foo",
Expand All @@ -146,7 +148,7 @@ func (s) TestParseConfig(t *testing.T) {
name: "request hash header cleared when RingHashSetRequestHashKey env var is false",
js: `{"requestHashHeader": "x-foo"}`,
requestHeaderEnvVar: false,
want: &LBConfig{
want: &iringhash.LBConfig{
MinRingSize: defaultMinSize,
MaxRingSize: defaultMaxSize,
},
Expand Down
4 changes: 2 additions & 2 deletions ...er/ringhash/e2e/ringhash_balancer_test.go → ...er/ringhash/e2e/ringhash_balancer_test.go
View file
Open in desktop
Copy link
Member

@dfawley dfawley Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this package under an internal directory. Or can we move it into the parent directory and just put it in the ringhash_test package?

Copy link
Collaborator Author

@atollena atollena Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpctest"
iringhash "google.golang.org/grpc/internal/ringhash"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/internal/testutils/xds/e2e"
Expand All @@ -50,7 +51,6 @@ import (
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
"google.golang.org/grpc/status"
"google.golang.org/grpc/xds/internal/balancer/ringhash"

v3clusterpb "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
Expand Down Expand Up @@ -129,7 +129,7 @@ func (s) TestRingHash_ReconnectToMoveOutOfTransientFailure(t *testing.T) {
r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}})

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
ctx = ringhash.SetXDSRequestHash(ctx, 0)
ctx = iringhash.SetXDSRequestHash(ctx, 0)
defer cancel()
client := testgrpc.NewTestServiceClient(cc)
if _, err := client.EmptyCall(ctx, &testpb.Empty{}); err != nil {
Expand Down
0 xds/internal/balancer/ringhash/logging.go → balancer/ringhash/logging.go
View file
Open in desktop
File renamed without changes.
4 changes: 3 additions & 1 deletion xds/internal/balancer/ringhash/picker.go → balancer/ringhash/picker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ import (
"strings"

xxhash "github.com/cespare/xxhash/v2"

"google.golang.org/grpc/balancer"
"google.golang.org/grpc/connectivity"
iringhash "google.golang.org/grpc/internal/ringhash"
"google.golang.org/grpc/metadata"
)

Expand Down Expand Up @@ -55,7 +57,7 @@ func (p *picker) Pick(info balancer.PickInfo) (balancer.PickResult, error) {
var requestHash uint64
if p.requestHashHeader == "" {
var ok bool
if requestHash, ok = XDSRequestHash(info.Ctx); !ok {
if requestHash, ok = iringhash.XDSRequestHash(info.Ctx); !ok {
return balancer.PickResult{}, fmt.Errorf("ringhash: expected xDS config selector to set the request hash")
}
} else {
Expand Down
3 changes: 2 additions & 1 deletion ...internal/balancer/ringhash/picker_test.go → balancer/ringhash/picker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"google.golang.org/grpc/balancer"
"google.golang.org/grpc/connectivity"
iringhash "google.golang.org/grpc/internal/ringhash"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/metadata"
)
Expand Down Expand Up @@ -156,7 +157,7 @@ func (s) TestPickerPickFirstTwo(t *testing.T) {
endpointStates: epStates,
}
got, err := p.Pick(balancer.PickInfo{
Ctx: SetXDSRequestHash(ctx, 0), // always pick the first endpoint on the ring.
Ctx: iringhash.SetXDSRequestHash(ctx, 0), // always pick the first endpoint on the ring.
})
if (err != nil || tt.wantErr != nil) && !errors.Is(err, tt.wantErr) {
t.Errorf("Pick() error = %v, wantErr %v", err, tt.wantErr)
Expand Down
0 xds/internal/balancer/ringhash/ring.go → balancer/ringhash/ring.go
View file
Open in desktop
File renamed without changes.
3 changes: 1 addition & 2 deletions xds/internal/balancer/ringhash/ring_test.go → balancer/ringhash/ring_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@ import (
"math"
"testing"

xxhash "github.com/cespare/xxhash/v2"
"google.golang.org/grpc/internal/balancer/weight"
"google.golang.org/grpc/resolver"

xxhash "github.com/cespare/xxhash/v2"
)

var testEndpoints []resolver.Endpoint
Expand Down
5 changes: 3 additions & 2 deletions xds/internal/balancer/ringhash/ringhash.go → balancer/ringhash/ringhash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"google.golang.org/grpc/internal/balancer/weight"
"google.golang.org/grpc/internal/grpclog"
"google.golang.org/grpc/internal/pretty"
iringhash "google.golang.org/grpc/internal/ringhash"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/ringhash"
"google.golang.org/grpc/serviceconfig"
Expand Down Expand Up @@ -85,7 +86,7 @@ type ringhashBalancer struct {
child balancer.Balancer

mu sync.Mutex
config *LBConfig
config *iringhash.LBConfig
inhibitChildUpdates bool
shouldRegenerateRing bool
endpointStates *resolver.EndpointMap[*endpointState]
Expand Down Expand Up @@ -173,7 +174,7 @@ func (b *ringhashBalancer) UpdateClientConnState(ccs balancer.ClientConnState) e
b.logger.Infof("Received update from resolver, balancer config: %+v", pretty.ToJSON(ccs.BalancerConfig))
}

newConfig, ok := ccs.BalancerConfig.(*LBConfig)
newConfig, ok := ccs.BalancerConfig.(*iringhash.LBConfig)
if !ok {
return fmt.Errorf("unexpected balancer config with type: %T", ccs.BalancerConfig)
}
Expand Down
Loading
Loading