xds: Implement circuit breaking support. #4050

GarrettGutierrez1 · 2020-11-19T02:51:58Z

xds: Implement circuit breaking support.

menghanl · 2020-11-20T18:50:38Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

@@ -285,6 +285,15 @@ func (b *cdsBalancer) handleSecurityConfig(config *xdsclient.SecurityConfig) err
 	return nil
 }

+func getCircuitBreaking(update xdsclient.ClusterUpdate) (circuitBreaking bool, maxRequests uint32) {
+	for _, threshold := range update.Thresholds {


We can move this for loop into xds client where we read the CDS response. So in ClusterUpdate, we only need the MaxRequests int.

I don't think we have plans to support the other priorities.

Implemented this change. This logic was moved into the client package, when it creates the ClusterUpdate from the CDS response.

menghanl · 2020-11-20T18:57:21Z

xds/internal/balancer/edsbalancer/config.go

@@ -42,6 +42,10 @@ type EDSConfig struct {
 	// will be disabled.  If set to the empty string, load reporting will
 	// be sent to the same server that we obtained CDS data from.
 	LrsLoadReportingServerName *string
+	// If circuit breaking is enabled.
+	CircuitBreaking bool


We don't need this bool.

You can make MaxRequests a pointer, and nil would mean circuit breaking is unset.

Got rid of this bool, MaxRequests is a pointer now.

menghanl · 2020-11-20T19:01:13Z

xds/internal/balancer/edsbalancer/eds_impl.go

@@ -389,6 +391,18 @@ func (edsImpl *edsBalancerImpl) handleSubConnStateChange(sc balancer.SubConn, s
 	}
 }

+// updateConfig handles changes to the circuit breaking configuration.
+func (edsImpl *edsBalancerImpl) updateConfig(edsConfig *EDSConfig) {
+	if edsImpl.counter == nil || edsImpl.counter.ServiceName != edsConfig.EDSServiceName {


Let's protect this with an env variable GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING. So if this is set to false, edsImpl.counter is never set (is always nil).

Add the env variable like this: https://github.com/grpc/grpc-go/blob/master/xds/internal/env/env.go#L41

Created the environment variable.

menghanl · 2020-11-20T19:02:31Z

xds/internal/balancer/edsbalancer/eds_impl.go

+	if edsImpl.counter == nil || edsImpl.counter.ServiceName != edsConfig.EDSServiceName {
+		edsImpl.counter = &client.ServiceRequestsCounter{ServiceName: edsConfig.EDSServiceName}
+	}
+	edsImpl.counter.UpdateService(edsConfig.CircuitBreaking, edsConfig.MaxRequests)


You shouldn't update the service of this count. You should update the counter.

There should be a map[string]counter in the client package. And you set edsImpl.counter to the corresponding value. When service name changes, you read the new value, and set edsImpl.counter.

Made this change.

xds/internal/balancer/edsbalancer/eds_impl.go

menghanl · 2020-11-20T19:46:49Z

xds/internal/balancer/edsbalancer/config.go

+	// If circuit breaking is enabled.
+	CircuitBreaking bool
+	// Max requests when circuit breaking.
+	MaxRequests uint32


With a second thought, we might not need this field.

The reason we need this is to update the upper bound in the counter.
There are two approaches:

keep this field, and store it inside the eds.dropPicker. So the global counter doesn't keep the upper bound, but each picker keeps it. And we update the picker when this value changes.

remove this field (so there's no change to the EDSConfig). Inside the CDS balancer, when it receives the ClusterUpdate, it updates the upper of the corresponding counter.

Now I'm a bit in favor of 2. Maybe give it a try. (Keep them as separate commits though, in case we regret later.)

I implemented 1 in this branch and pushed it. I'll implement 2 in a separate branch.

https://github.com/GarrettGutierrez1/grpc-go/tree/xds-circuit-breaking-no-edsconfig-change

This branch implements 2. I do think it is simpler (less files changed).

I switched it up, this branch now implements 2, and the following link implements 1.

https://github.com/GarrettGutierrez1/grpc-go/tree/xds-circuit-breaking-moved-max-requests-tracking

menghanl · 2020-11-20T19:50:28Z

xds/internal/client/client_requests_counter.go

+
+type servicesRequestsCounter struct {
+	mu       sync.Mutex
+	services map[string]serviceInfo


A global map[string]counter would work. Each dropPicker would only need the counter, not the map.

Made this change, now the dropPicker gets the counter.

menghanl · 2020-11-20T19:51:31Z

xds/internal/client/client_requests_counter.go

+}
+
+type serviceInfo struct {
+	circuitBreaking bool


No need for this bool.
If circuit breaking is not enabled, this struct wouldn't exist, the drop picker will see a nil counter.

Removed this bool.

menghanl · 2020-11-20T19:52:04Z

xds/internal/client/client_requests_counter.go

+	if !ok {
+		return fmt.Errorf("service name %v not identified", c.ServiceName)
+	}
+	sInfo.numRequests++


Try to not lock and ++. Use atomic instead.
Lock adds too much contention.

There is a check of numRequests (to make sure it isn't > maxRequests) and then after that it is updated using ++, can I still use an atomic if I am doing 2 dependent operations on it?

menghanl · 2020-11-20T19:53:10Z

xds/internal/client/client_xds.go

+// circuitBreakersFromCluster extracts the circuit breakers configuration from
+// the received cluster resource. Returns nil if no CircuitBreakers or no
+// Thresholds in CircuitBreakers.
+func circuitBreakersFromCluster(cluster *v3clusterpb.Cluster) ([]CircuitBreakerThreshold, error) {


This doesn't need to return error, right. It's always nil.

Made this change.

menghanl · 2020-12-04T22:34:03Z

xds/internal/balancer/edsbalancer/eds.go

@@ -113,6 +113,8 @@ type edsBalancerImplInterface interface {
 	handleSubConnStateChange(sc balancer.SubConn, state connectivity.State)
 	// updateState handle a balancer state update from the priority.
 	updateState(priority priorityType, s balancer.State)
+	// updateServiceRequestsCounter handles an update to the service name.


updates the service requests counter to the one for the given service name.

Made this change.

menghanl · 2020-12-04T22:38:17Z

xds/internal/balancer/edsbalancer/eds_test.go

@@ -131,6 +132,10 @@ func (f *fakeEDSBalancer) handleEDSResponse(edsResp xdsclient.EndpointsUpdate) {

 func (f *fakeEDSBalancer) updateState(priority priorityType, s balancer.State) {}

+func (f *fakeEDSBalancer) updateServiceRequestsCounter(serviceName string) {
+	f.serviceName.Send(serviceName)


Where should that be verified? I'm guessing added to one of the waitFor functions below?

Yes. And call it to verify in the test.

Made this change. Wasn't 100% sure what test you are talking about, added the check to TestConfigChildPolicyUpdate because it is the only test that invokes the policy update and has a fakeEDSBalancer.

menghanl · 2020-12-04T22:40:36Z

xds/internal/client/client_requests_counter.go

+	defer src.mu.Unlock()
+	c, ok := src.services[serviceName]
+	if !ok {
+		c = &ServiceRequestsCounter{ServiceName: serviceName, maxRequests: 1024}


Define a const for 1024, and use the const here and following.

Made this change.

menghanl · 2020-12-04T22:42:38Z

xds/internal/client/client_requests_counter_test.go

+		name:          "does-not-exceed-max-requests",
+		maxRequests:   1024,
+		numRequests:   1024,
+		errorExpected: false,


Replace errorExpected with expectedSuccesses and expectedErrors.

Made this change.

menghanl · 2020-12-04T22:44:15Z

xds/internal/client/client_requests_counter_test.go

+	},
+}
+
+func counterTestInit() {


Why do you need this? Is this to reset the counters? If so, call it resetServiceRequestsCounter?

Made this change.

xds/internal/client/client_requests_counter_test.go

menghanl · 2020-12-04T22:49:35Z

xds/internal/client/client_requests_counter.go

+}
+
+// SetMaxRequests updates the max requests for a service's counter.
+func SetMaxRequests(serviceName string, maxRequests *uint32) *ServiceRequestsCounter {


This function doesn't need the return value.

Made this change.

menghanl

LGTM. 2 nits.

xds/internal/balancer/edsbalancer/eds_test.go

menghanl · 2020-12-07T19:27:58Z

xds/internal/client/client_requests_counter_test.go

+		t.Error("no error when error expected")
+	}
+	if loadedError != nil {
+		fmt.Println(loadedError.(error))


Delete this?

Made this change.

GarrettGutierrez1 added 12 commits November 5, 2020 12:38

1

f002b7e

implementation

81840c4

Refactored to counter struct

027f3b9

Added ClusterUpdate test

45960e9

testing counter

7ce9958

Testing edsconfig

6efeb41

added marshall test

4021d5d

Added picker test

a60e3b6

fixed conflict

a53d4b4

working this time

ceb64d4

added comment

8bc9b02

removed rand

63ca25b

GarrettGutierrez1 added this to the 1.34 Release milestone Nov 19, 2020

GarrettGutierrez1 added the Type: Feature New features or improvements in behavior label Nov 19, 2020

using waitgroups in counter test

23e96d4

GarrettGutierrez1 requested a review from menghanl November 19, 2020 18:04

GarrettGutierrez1 assigned menghanl Nov 19, 2020

menghanl closed this Nov 20, 2020

menghanl reopened this Nov 20, 2020

menghanl suggested changes Nov 20, 2020

View reviewed changes

menghanl assigned GarrettGutierrez1 and unassigned menghanl Nov 20, 2020

GarrettGutierrez1 added 7 commits November 30, 2020 13:15

Using *uint32 as max requests in various confis

f3beb84

Removed error return value

eff8dfa

fixed error

4cd2b4f

Reworked counter

3308919

vet fix

13c8c41

Merge branch 'master' into xds-circuit-breaking

b74f32e

Fixed error from merge master

96282cb

GarrettGutierrez1 assigned menghanl Dec 2, 2020

GarrettGutierrez1 assigned menghanl and unassigned GarrettGutierrez1 Dec 4, 2020

GarrettGutierrez1 added 3 commits December 4, 2020 13:12

merge fix

fad40e7

merge fix 2

67e9563

test fix

3406b64

menghanl suggested changes Dec 4, 2020

View reviewed changes

menghanl assigned GarrettGutierrez1 and unassigned menghanl Dec 4, 2020

Reworking testing

1c16e68

GarrettGutierrez1 assigned menghanl and unassigned GarrettGutierrez1 Dec 5, 2020

menghanl reviewed Dec 7, 2020

View reviewed changes

menghanl assigned GarrettGutierrez1 and unassigned menghanl Dec 7, 2020

Added counter update test

3d4130d

GarrettGutierrez1 assigned menghanl and unassigned GarrettGutierrez1 Dec 8, 2020

menghanl approved these changes Dec 8, 2020

View reviewed changes

menghanl assigned GarrettGutierrez1 and unassigned menghanl Dec 8, 2020

Merge branch 'master' into xds-circuit-breaking

8cbb4f6

GarrettGutierrez1 merged commit ff16195 into grpc:master Dec 8, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2021

xds: Implement circuit breaking support. #4050

xds: Implement circuit breaking support. #4050

Conversation

GarrettGutierrez1 commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment