Skip to content
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

Jump to bottom

xds: support pick_first custom load balancing policy (A62) #6314

Merged
merged 7 commits into from May 30, 2023
Merged

xds: support pick_first custom load balancing policy (A62) #6314

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e13e8c5
xds: support pick_first custom load balancing policy
dfawley May 24, 2023
1f3f8d6
envconfig knob
dfawley May 24, 2023
6c64630
go mod tidy everywhere
dfawley May 24, 2023
8d2f69a
review comments
dfawley May 25, 2023
57a3f29
small extra cleanup
dfawley May 26, 2023
54330ac
Merge master branch
dfawley May 26, 2023
90ca8aa
re-update go-control-plane to repair after merge
dfawley May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 8 additions & 22 deletions test/pickfirst_test.go
View file
Open in desktop
Expand Up @@ -382,6 +382,7 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) {
wg.Wait()
}

// Tests the PF LB policy with shuffling enabled.
func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Top level comment about setup and expectation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not be detailed and explain the whole test setup and everything in the comment. The name is very self-explanatory, but I added a short comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I mean. I just feel like it should be briefly explain stuff like configures pick_first as top level lb of the channel, sends it address list before knob and connection isn't random, after knob it is random (with our determinisitic randomness algorithm plumbed). See

grpc-go/test/pickfirst_test.go

Line 239 in 8d2f69a

// TestPickFirst_NewAddressWhileBlocking tests the case where pick_first is
for an example of what I'm going for.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is unnecessarily detailed and a recipe for the test & comment diverging when someone decides to change the test to add an extra check or do things in a different order because of a race, etc. I'd rather not over-document our tests, and would definitely not require detailed documentation anywhere. It's actually pretty rare to have any comments at the top of a test.

E.g. https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/net/http/client_test.go;l=550

defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
zasweq marked this conversation as resolved.
Show resolved Hide resolved
envconfig.PickFirstLBConfig = true
Expand All @@ -393,6 +394,7 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
grpcrand.Shuffle = func(n int, f func(int, int)) {
if n != 2 {
t.Errorf("Shuffle called with n=%v; want 2", n)
return
}
f(0, 1) // reverse the two addresses
}
Expand Down Expand Up @@ -435,6 +437,8 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) {
}
}

// Tests the PF LB policy with the environment variable support of address list
// shuffling disabled.
func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Please add top level comment with setup and expectation.

Copy link
Member Author

Choose a reason for hiding this comment

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

As above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig)
zasweq marked this conversation as resolved.
Show resolved Hide resolved
envconfig.PickFirstLBConfig = false
Expand All @@ -446,6 +450,7 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
grpcrand.Shuffle = func(n int, f func(int, int)) {
if n != 2 {
t.Errorf("Shuffle called with n=%v; want 2", n)
zasweq marked this conversation as resolved.
Show resolved Hide resolved
return
}
f(0, 1) // reverse the two addresses
}
Expand All @@ -457,15 +462,10 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

// Push an update with both addresses and shuffling disabled. We should
// connect to backend 0.
r.UpdateState(resolver.State{Addresses: []resolver.Address{addrs[0], addrs[1]}})
if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
t.Fatal(err)
}

// Send a config with shuffling enabled. This will reverse the addresses,
// but the channel should still be connected to backend 0.
// so we should connect to backend 1 if shuffling is supported. However
// with it disabled at the start of the test, we will connect to backend 0
// instead.
shufState := resolver.State{
ServiceConfig: parseServiceConfig(t, r, serviceConfig),
Addresses: []resolver.Address{addrs[0], addrs[1]},
Expand All @@ -474,18 +474,4 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) {
if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
t.Fatal(err)
}

// Send a resolver update with no addresses. This should push the channel
// into TransientFailure.
r.UpdateState(resolver.State{})
awaitState(ctx, t, cc, connectivity.TransientFailure)

// Send the same config as last time with shuffling enabled. Since we are
// not connected to backend 0, we should connect to backend 1 if shuffling
// is supported. However with it disabled at the start of the test, we
// will connect to backend 0 instead.
r.UpdateState(shufState)
if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil {
t.Fatal(err)
}
}
6 changes: 4 additions & 2 deletions xds/internal/xdsclient/xdslbregistry/converter/converter.go
View file
Open in desktop
Expand Up @@ -28,7 +28,9 @@ import (
"strings"

"github.com/golang/protobuf/proto"
"google.golang.org/grpc"
"google.golang.org/grpc/balancer"
"google.golang.org/grpc/balancer/roundrobin"
"google.golang.org/grpc/balancer/weightedroundrobin"
"google.golang.org/grpc/internal/envconfig"
internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
Expand Down Expand Up @@ -110,11 +112,11 @@ func convertPickFirstProtoToServiceConfig(rawProto []byte, _ int) (json.RawMessa
if err != nil {
return nil, fmt.Errorf("error marshaling JSON for type %T: %v", pfCfg, err)
}
return makeBalancerConfigJSON("pick_first", js), nil
return makeBalancerConfigJSON(grpc.PickFirstBalancerName, js), nil
}

func convertRoundRobinProtoToServiceConfig([]byte, int) (json.RawMessage, error) {
return makeBalancerConfigJSON("round_robin", json.RawMessage("{}")), nil
return makeBalancerConfigJSON(roundrobin.Name, json.RawMessage("{}")), nil
}

type wrrLocalityLBConfig struct {
Expand Down
17 changes: 16 additions & 1 deletion xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go
View file
Open in desktop
Expand Up @@ -106,7 +106,7 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
wantConfig: `[{"ring_hash_experimental": { "minRingSize": 10, "maxRingSize": 100 }}]`,
},
{
name: "pick_first",
name: "pick_first_shuffle",
policy: &v3clusterpb.LoadBalancingPolicy{
Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{
{
Expand All @@ -120,6 +120,19 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
},
wantConfig: `[{"pick_first": { "shuffleAddressList": true }}]`,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: can you add a test case for it being unset and it creating false? I guess that is specific and might be more appropriate (unset and also set to false -> false as per my comment on the main code) in ParseConfig test in pick_first rather than scope it to this test here which tests the registry. Actually since the registry does incorporate this proto -> json conversion, maybe we should add tests here. Or maybe that would scale up the codebase too much but I'm now mindful of those 3 buckets (not set, set and zero, set and non zero) in both the xDS flow through proto and the user writing json itself in the file system e.g. and how those 3 buckets map to our balancer's eventual internal configuration type returned from ParseConfig().

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no "unset" in the PF proto input. This is a boolean field. If it's missing from the struct, it defaults to its zero value of false. That said, testing that it outputs false when the input is false/absent-so-zero is worthwhile, so done.

Copy link
Contributor

@zasweq zasweq May 26, 2023 •
edited

Choose a reason for hiding this comment

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

Oh. I've seen the proto bools be wrapped in a proto pointer type. Right, now if it's not explicitly set it gets it's zero (false, not nil because it's a bool value type), and it should make a false config in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen the proto bools be wrapped in a proto pointer type.

There are wrapper types for use cases where presence is important. That's not in use here, because presence is not important for this field (missing=default=zero=false).

{
name: "pick_first",
policy: &v3clusterpb.LoadBalancingPolicy{
Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{
{
TypedExtensionConfig: &v3corepb.TypedExtensionConfig{
TypedConfig: testutils.MarshalAny(&v3pickfirstpb.PickFirst{}),
},
},
},
},
wantConfig: `[{"pick_first": { "shuffleAddressList": false }}]`,
},
{
name: "round_robin",
policy: &v3clusterpb.LoadBalancingPolicy{
Expand Down Expand Up @@ -301,6 +314,8 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) {
if err != nil {
t.Fatalf("ConvertToServiceConfig(%s) failed: %v", pretty.ToJSON(test.policy), err)
}
// got and want must be unmarshalled since JSON strings shouldn't
// generally be directly compared.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: briefly mention why. E.g. removes whitespaces/nondeterministic creation/functionality equiavlent JSON even with whitespaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can assume a programmer understands why JSON strings can't be directly compared.

var got []map[string]interface{}
if err := json.Unmarshal(rawJSON, &got); err != nil {
t.Fatalf("Error unmarshalling rawJSON (%q): %v", rawJSON, err)
Expand Down