xds: Add Outlier Detection configuration and CDS handling #5299

zasweq · 2022-04-06T20:42:20Z

This PR adds Outlier Detection configuration, with a placeholder OutlierDetectionBalancer that will be implemented in the future, and also the CDS handling of this (conversion from xdsclient representation of Outlier Detection data -> LB Configuration) as described in A50.
The next steps involve the Cluster Resolver handling of this configuration (Outlier Detection as top level configuration for the priority, current cluster_impl LB Configuration as child of this configuration). After Cluster Resolver handling comes the actual Outlier Detection LB Policy implementation. We won't be able to merge the Cluster Resolver changes until the Outlier Detection LB Policy is merged.

RELEASE NOTES: None

…ection configuration

menghanl · 2022-04-07T20:27:35Z

xds/internal/balancer/outlierdetection/config.go

+	// when an outlier status is detected through success rate statistics. This
+	// setting can be used to disable ejection or to ramp it up slowly. Defaults
+	// to 100.
+	EnforcingSuccessRate uint32 `json:"enforcingSuccessRate,omitempty"`


This field name in the gRFC is enforcement_percentage.

Yeah, moving target :/ from when I first implemented. Switched. (I will double check again)

menghanl · 2022-04-07T20:28:10Z

xds/internal/balancer/outlierdetection/config.go

+	// ejected when an outlier status is detected through failure percentage
+	// statistics. This setting can be used to disable ejection or to ramp it up
+	// slowly. Defaults to 0.
+	EnforcingFailurePercentage uint32 `json:"enforcingFailurePercentage,omitempty"`


This is also enforcement_percentage in the gRFC

menghanl · 2022-04-07T20:30:37Z

xds/internal/balancer/outlierdetection/config.go

+	// algorithm. If set, failure rate ejections will be performed.
+	FailurePercentageEjection *FailurePercentageEjection `json:"failurePercentageEjection,omitempty"`
+	// ChildPolicy is the config for the child policy.
+	ChildPolicy/*[] why is this repeated in design?*/ *internalserviceconfig.BalancerConfig `json:"childPolicy,omitempty"`


It is the common practice to specify a list, and we will use the first valid one.
The BalancerConfig type you use here handles the list, so this field doesn't need to be a go slice: https://github.com/grpc/grpc-go/blob/master/internal/serviceconfig/serviceconfig.go#L65

Ah, noted. I remember Doug mentioning that now. Why is it designed as such?

menghanl · 2022-04-07T20:40:16Z

xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go

@@ -224,11 +225,14 @@ func makeNewSubConn(ctx context.Context, edsCC balancer.ClientConn, parentCC *te
 // the address attributes added as part of the intercepted NewSubConn() method
 // indicate the use of fallback credentials.
 func (s) TestSecurityConfigWithoutXDSCreds(t *testing.T) {
+	oldOutlierDetection := envconfig.XDSOutlierDetection


Can this be moved to the setup functions? Like setupWithWatch?

And why do you need to make the new env var true for all the tests?
I was expecting one set of new tests that set this to true, but the others are untouched.

Yeah, fair enough. I added it to everything to have the no-op Outlier Detection LB config be generated. I'll leave the env var off for all these tests, and then send nil LB Config.

menghanl · 2022-04-07T20:52:30Z

xds/internal/balancer/outlierdetection/balancer.go

+// Name is the name of the outlier detection balancer.
+const Name = "outlier_detection_experimental"
+
+func init() {


I think you probably don't need this balancer (or register()) for this PR.
Only the config type should be good enough (and you will need to move that test function to a config_test.go file maybe).

Sounds good. Deleted.

zasweq

Thanks for the comments :D!

zasweq · 2022-04-07T22:14:58Z

xds/internal/balancer/outlierdetection/config.go

+	// when an outlier status is detected through success rate statistics. This
+	// setting can be used to disable ejection or to ramp it up slowly. Defaults
+	// to 100.
+	EnforcingSuccessRate uint32 `json:"enforcingSuccessRate,omitempty"`


Yeah, moving target :/ from when I first implemented. Switched. (I will double check again)

zasweq · 2022-04-07T22:15:05Z

xds/internal/balancer/outlierdetection/config.go

+	// ejected when an outlier status is detected through failure percentage
+	// statistics. This setting can be used to disable ejection or to ramp it up
+	// slowly. Defaults to 0.
+	EnforcingFailurePercentage uint32 `json:"enforcingFailurePercentage,omitempty"`


zasweq · 2022-04-07T22:15:46Z

xds/internal/balancer/outlierdetection/config.go

+	// algorithm. If set, failure rate ejections will be performed.
+	FailurePercentageEjection *FailurePercentageEjection `json:"failurePercentageEjection,omitempty"`
+	// ChildPolicy is the config for the child policy.
+	ChildPolicy/*[] why is this repeated in design?*/ *internalserviceconfig.BalancerConfig `json:"childPolicy,omitempty"`


Ah, noted. I remember Doug mentioning that now. Why is it designed as such?

zasweq · 2022-04-07T22:16:55Z

xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go

@@ -224,11 +225,14 @@ func makeNewSubConn(ctx context.Context, edsCC balancer.ClientConn, parentCC *te
 // the address attributes added as part of the intercepted NewSubConn() method
 // indicate the use of fallback credentials.
 func (s) TestSecurityConfigWithoutXDSCreds(t *testing.T) {
+	oldOutlierDetection := envconfig.XDSOutlierDetection


Yeah, fair enough. I added it to everything to have the no-op Outlier Detection LB config be generated. I'll leave the env var off for all these tests, and then send nil LB Config.

zasweq · 2022-04-07T22:45:58Z

xds/internal/balancer/outlierdetection/balancer.go

+// Name is the name of the outlier detection balancer.
+const Name = "outlier_detection_experimental"
+
+func init() {


Sounds good. Deleted.

menghanl · 2022-04-08T00:53:36Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+	if od == nil {
+		// "If the outlier_detection field is not set in the Cluster message, a
+		// "no-op" outlier_detection config will be generated, with interval set
+		// to the maximum possible value and all other fields unset." - A50
+		return &outlierdetection.LBConfig{
+			Interval: 1<<63 - 1,
+		}
+	}


Does this mean we will always have a outlier detection balancer, even when the xDS response doesn't specify the config? And the outlier detection balancer will always keep the counts and everything?

Yes, IIUC. (another reason I asked about increasing complexity of system on Monday). The no-op balancer takes away the functionality of the Outlier Detection balancer, and simply acts as a forwarder.

The no-op balancer

But is it a no-op balancer? Are you going to treat Interval:maxint differently?
Otherwise it's going to do all the counting, just that it will never use the numbers to eject any connections.

True. I think I'll treat Interval:maxint the same, almost logically a no-op, but not quite. WDYT?

menghanl

Two minor comments. LGTM otherwise

menghanl · 2022-05-09T18:17:43Z

xds/internal/balancer/cdsbalancer/cdsbalancer_test.go

@@ -654,15 +700,17 @@ func (s) TestClose(t *testing.T) {
 	// xdsClient, and makes sure that the CDS balancer registers a watch on the
 	// provided xdsClient.
 	xdsC, cdsB, edsB, _, cancel := setupWithWatch(t)
-	defer cancel()
+	defer func() {


Revert this?

menghanl · 2022-05-09T18:18:59Z

xds/internal/balancer/cdsbalancer/cdsbalancer_test.go

+	oldOutlierDetection := envconfig.XDSOutlierDetection
+	envconfig.XDSOutlierDetection = true
+	defer func() {
+		envconfig.XDSOutlierDetection = oldOutlierDetection
+	}()


You don't need to set env var for this test, right?

zasweq · 2022-05-09T19:25:51Z

Thanks for the comments :D!

zasweq added 3 commits April 1, 2022 16:36

Added Outlier Detection configuration and CDS handling of Outlier Det…

973e524

…ection configuration

Added Env Var

f8a29a7

Added ParseConfig and iterated on outlier detection

195d274

zasweq requested a review from menghanl April 6, 2022 20:42

zasweq assigned menghanl Apr 6, 2022

zasweq added the Type: Feature New features or improvements in behavior label Apr 6, 2022

zasweq added this to the 1.47 Release milestone Apr 6, 2022

menghanl suggested changes Apr 7, 2022

View reviewed changes

menghanl assigned zasweq and unassigned menghanl Apr 7, 2022

zasweq added 2 commits April 7, 2022 18:42

Save progress before deleting Outlier Detection Balancer

e8276cb

Deleted Outlier Detection Balancer

67c524b

zasweq commented Apr 7, 2022

View reviewed changes

menghanl reviewed Apr 8, 2022

View reviewed changes

zasweq assigned menghanl and unassigned zasweq Apr 8, 2022

zasweq mentioned this pull request Apr 8, 2022

xds: Outlier Detection configuration in Cluster Resolver Balancer #5304

Closed

menghanl reviewed May 9, 2022

View reviewed changes

menghanl assigned zasweq and unassigned menghanl May 9, 2022

Responded to Menghan's comments

857ef81

zasweq assigned menghanl and unassigned zasweq May 9, 2022

menghanl approved these changes May 9, 2022

View reviewed changes

menghanl assigned zasweq and unassigned menghanl May 9, 2022

zasweq merged commit 462d867 into grpc:master May 9, 2022

zasweq mentioned this pull request May 13, 2022

cmp should not be used outside testing packages #5353

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: Add Outlier Detection configuration and CDS handling #5299

xds: Add Outlier Detection configuration and CDS handling #5299

zasweq commented Apr 6, 2022

menghanl Apr 7, 2022

zasweq Apr 7, 2022

menghanl Apr 7, 2022

zasweq Apr 7, 2022

menghanl Apr 7, 2022

zasweq Apr 7, 2022

menghanl Apr 7, 2022

menghanl Apr 7, 2022

zasweq Apr 7, 2022

menghanl Apr 7, 2022

zasweq Apr 7, 2022

zasweq left a comment

zasweq Apr 7, 2022

zasweq Apr 7, 2022

zasweq Apr 7, 2022

zasweq Apr 7, 2022

zasweq Apr 7, 2022

menghanl Apr 8, 2022 •

edited

Loading

zasweq Apr 8, 2022

menghanl Apr 8, 2022 •

edited

Loading

zasweq Apr 8, 2022

menghanl left a comment

menghanl May 9, 2022

zasweq May 9, 2022

menghanl May 9, 2022

zasweq May 9, 2022

zasweq commented May 9, 2022

xds: Add Outlier Detection configuration and CDS handling #5299

xds: Add Outlier Detection configuration and CDS handling #5299

Conversation

zasweq commented Apr 6, 2022

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

zasweq left a 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 Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl Apr 8, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented May 9, 2022

menghanl Apr 8, 2022 •

edited

Loading

menghanl Apr 8, 2022 •

edited

Loading