xds: Add support for Custom LB Policies by zasweq · Pull Request #6224 · grpc/grpc-go

zasweq · 2023-04-26T19:16:04Z

This PR adds support for Custom LB Policies, configured through xDS. This implements the full functionality defined in https://github.com/grpc/proposal/blob/master/A52-xds-custom-lb-policies.md.

Summary of changes:

Delete the old LBPolicy field and switch it to new one in the xDS Client -> cds_balancer -> cluster_resolver flow
In the cluster resolver, just send this config downward, rather than have a branch of different logic for ring hash and round robin. Consolidate and add logic to addresses passed down. Previously it was different for round robin vs. ring hash. Add locality weight as an attribute, and now always set ew to lw * ew, and set the full hierarchy path regardless.
Complete the implementation for wrr_locality balancer, which only function is to prepare weighted_target configuration for it’s child. Synchronization was trivial here, as not a Client Conn, and balancer.Balancer operations are guaranteed to be called synchronously. Child balancer built at build time, since never expected to change, and wrr_locality is simply a wrapper on the child balancer.
Force a state update (which will be Transient Failure) from the Weighted Target balancer if passed in a config with no targets. This fixes a bug, where previously Transient Failure was not being reported in this case and failover was not happening correctly.
Improved String() method on resolver.Address. Prints only the non deprecated fields, and has support for printing Attributes now (in order to trigger, the key or value of a specific attribute has to implement the fmt.Stringer interface). This was added for easier debuggability for failing cluster_resolver tests.

RELEASE NOTES:

xds: Add support for Custom LB Policies as per gRFC A52

easwars

Small pass. Figured that I'm going to be able to get to everything in one pass. So, this is going to involve a bit of back and forth. Sorry about that.

zasweq · 2023-05-05T19:00:26Z

Got to all the comments except trailing comma. I'll get to that after doing a pass on Doug's PR. Should be ready for another pass though :D.

easwars · 2023-05-08T18:43:28Z

+	"fmt"
+	"testing"
+
+	v3 "github.com/cncf/xds/go/xds/type/v3"


This needs to match the string used in other imports (and specified in the copybara config).

easwars · 2023-05-08T18:59:39Z

+// be passed 5 ports, and the first two ports will be put in the first locality,
+// and the last three will be put in the second locality. It also configures the
+// proto message passed in as the Locality + Endpoint picking policy in CDS.
+func clientResourcesNewFieldSpecifiedAndPortsInMultipleLocalities2(params e2e.ResourceParams, ports []uint32, m proto.Message) e2e.UpdateOptions {


If we do enhance EndpointOptions to contain LocalityOptions, we wouldn't need this helper function. This can be written as part of the test, so that the reader will clearly be able to see which ports belong to which localities.

Deleted this helper. Thanks.

Actually, I think you can delete the endpoint flow/helpers to build the EDS configuration, but this overall flow is much cleaner with a documented function (about the 5 ports passed in, proto.Message that gets plumbed as the Locality + Endpoint picking policy in CDS).

I still think getting rid of the helper would be cleaner since the reader of the test will know exactly what is happening by reading the body of the test instead of having to navigate to this helper, and read its comment and then figure out what is happening.

Fine. I don't agree, but switched.

easwars · 2023-05-08T18:59:52Z

+
+	managementServer, nodeID, _, r, cleanup := e2e.SetupManagementServer(t, e2e.ManagementServerOptions{})
+	defer cleanup()
+	backend1 := stubserver.StartTestService(t, nil)


For loop instead?

I prefer it this way to hold ref to backend to grab backend.Address, and also backend.Port which I pass to helper you commented about above. If you feel strongly about this I'm willing to change it.

You can still hold the reference to the address and the port in a slice. But if you dont want to do it, then thats fine. But do add a new line after this block and before the test table starts.

Yeah, forgot to mention would require creating two length 5 slices which I don't like.

easwars · 2023-05-09T00:03:40Z

 		return nil, fmt.Errorf("xds: invalid LBConfig for wrrlocality: %s, error: %v", string(s), err)
 	}
 	if lbCfg == nil || lbCfg.ChildPolicy == nil {
-		return nil, errors.New("xds: invalidw LBConfig for wrrlocality: child policy field must be set")
+		return nil, errors.New("xds: invalid LBConfig for wrrlocality: child policy field must be set")


Do we want the xds prefix here?

Changed to same prefix as the errors returned from UpdateClientConnState xds_wrr_locality:

easwars · 2023-05-09T00:04:30Z

+		}
+		ai, ok := getAddrInfo(addr)
+		if !ok {
+			return fmt.Errorf("xds_wrr_locality: addr: %v is misisng locality weight information", addr)


Could this be instead?
return fmt.Errorf("xds_wrr_locality: misisng locality weight information in address %q", addr)

You kept my missing typo in your suggestion :). Switched, without the typo :).

easwars · 2023-05-09T00:05:55Z

+	wtCfgJSON, err := json.Marshal(wtCfg)
+	if err != nil {
+		// Shouldn't happen.
+		return fmt.Errorf("xds_wrr_locality: error marshalling prepared wtCfg: %v", wtCfg)


Please make this error message more readable:
return fmt.Errorf("xds_wrr_locality: error marshalling prepared config: %v", wtCfg)

And I remember having a big discussion between marshalling vs marshaling with Doug :). Maybe you should rekindle it.

Ummmmmmm yeah I thought it was marshaling. What's difference? Switched to what you had.

easwars · 2023-05-09T00:24:03Z

+	}
+	var sc serviceconfig.LoadBalancingConfig
+	if sc, err = b.childParser.ParseConfig(wtCfgJSON); err != nil {
+		return fmt.Errorf("xds_wrr_locality: config generated %v by wrr_locality_experimental is invalid: %v", wtCfgJSON, err)


there is wrr_locality twice in this error string

Deleted "by wrr_locality_experimental."

easwars

Some minor comments remaining. LGTM.

easwars · 2023-05-09T00:27:38Z

+	"google.golang.org/grpc/xds/internal"
+)
+
+var (


easwars · 2023-05-09T00:31:33Z

+// be passed 5 ports, and the first two ports will be put in the first locality,
+// and the last three will be put in the second locality. It also configures the
+// proto message passed in as the Locality + Endpoint picking policy in CDS.
+func clientResourcesNewFieldSpecifiedAndPortsInMultipleLocalities2(params e2e.ResourceParams, ports []uint32, m proto.Message) e2e.UpdateOptions {


I still think getting rid of the helper would be cleaner since the reader of the test will know exactly what is happening by reading the body of the test instead of having to navigate to this helper, and read its comment and then figure out what is happening.

zasweq requested a review from easwars April 26, 2023 19:16

zasweq assigned easwars Apr 26, 2023

zasweq added the Type: Feature New features or improvements in behavior label Apr 26, 2023

zasweq added this to the 1.55 Release milestone Apr 26, 2023

zasweq force-pushed the add-custom-lb-support branch from 7decbac to 928d0ca Compare April 26, 2023 20:10

zasweq added 3 commits May 4, 2023 15:12

Add support for Custom LB Policies

36ae60b

Fix failover problem by having weighted_target report TF

fc6e6a8

Fix attribute get and call ParseConfig on child

119986e

zasweq force-pushed the add-custom-lb-support branch from 4f07f87 to 119986e Compare May 4, 2023 19:12

easwars reviewed May 4, 2023

View reviewed changes

Comment thread attributes/attributes.go

Comment thread balancer/weightedtarget/weightedaggregator/aggregator.go

Comment thread balancer/weightedtarget/weightedtarget.go Outdated

Comment thread balancer/weightedtarget/weightedtarget.go

easwars assigned zasweq and unassigned easwars May 4, 2023

easwars modified the milestones: 1.55 Release, 1.56 Release May 4, 2023

Responded to Easwar's comments

f2f940b

zasweq assigned easwars and unassigned zasweq May 4, 2023

Vet

2de1e5c

easwars reviewed May 5, 2023

View reviewed changes

easwars assigned zasweq and unassigned easwars May 5, 2023

Responded to Easwar's comments

ce00b1c

zasweq assigned easwars and unassigned zasweq May 5, 2023

Log first attribute to get rid of comma and space on last

2e482b7

easwars reviewed May 5, 2023

View reviewed changes

easwars assigned zasweq and unassigned easwars May 5, 2023

Responded to Easwar's comments

98d0934

zasweq assigned easwars and unassigned zasweq May 8, 2023

easwars reviewed May 8, 2023

View reviewed changes

easwars assigned zasweq and unassigned easwars May 8, 2023

easwars reviewed May 8, 2023

View reviewed changes

Comment thread xds/internal/balancer/wrrlocality/balancer_test.go

easwars reviewed May 8, 2023

View reviewed changes

Responded to Easwar's comments

08e421c

zasweq assigned easwars and unassigned zasweq May 8, 2023

Implemented Easwar's attributes String() solution

01bdd53

easwars reviewed May 9, 2023

View reviewed changes

easwars approved these changes May 9, 2023

View reviewed changes

easwars assigned zasweq and unassigned easwars May 9, 2023

Responded to Easwar's comments

51cc74a

zasweq merged commit 5e58734 into grpc:master May 9, 2023

dfawley mentioned this pull request Jul 24, 2023

xDS: support for custom LB config #5658

Closed

searKing mentioned this pull request Aug 23, 2023

attributes: print typed nil values instead of panic #6574

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Nov 5, 2023

Conversation

zasweq commented Apr 26, 2023 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zasweq commented May 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zasweq May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zasweq commented Apr 26, 2023 •

edited by easwars

Loading

zasweq May 8, 2023 •

edited

Loading

zasweq May 9, 2023 •

edited

Loading