xds: Outlier Detection configuration in Cluster Resolver Balancer #5371

zasweq · 2022-05-19T20:57:02Z

This PR adds branching logic for constructing the Priority LB's config. In the case that Outlier Detection is configured on with the corresponding Environment variable, the top level balancer's configuration for each priority will be of type Outlier Detection with a Cluster Impl child rather than a Cluster Impl. This had to include the Outlier Detection's balancer to pass Cluster Resolver tests, as the Outlier Detection policy needed to be registered.

Branched off #5370.

RELEASE NOTES: None

dfawley · 2022-05-31T20:03:24Z

Please resolve the conflict and reassign, thanks!

…uilder

dfawley · 2022-06-03T20:28:33Z

xds/internal/balancer/outlierdetection/balancer.go

+type outlierDetectionBalancer struct {
+}
+
+func (b *outlierDetectionBalancer) UpdateClientConnState(s balancer.ClientConnState) error {


Just return nil from Build instead of having this type.

Or don't implement balancer.Builder fully yet and call bb.ParseConfig directly in tests.

I tried the second option, it broke since this needs to be registered in config validation at a higher level. Thus, I kept implementing balancer.Builder and returned nil from build instead of the type, and deleted this type.

dfawley · 2022-06-03T20:54:39Z

xds/internal/balancer/outlierdetection/balancer_test.go

+	bb := balancer.Get(Name)
+	if bb == nil {
+		t.Fatalf("balancer.Get(%q) returned nil", Name)
+	}
+	parser, ok := bb.(balancer.ConfigParser)
+	if !ok {
+		t.Fatalf("balancer %q does not implement the ConfigParser interface", Name)
+	}


Related to the above: parser := bb{}?

Ah, good point. Switched.

dfawley · 2022-06-03T20:55:40Z

xds/internal/balancer/outlierdetection/balancer_test.go

+
+	tests := []struct {
+		name    string
+		input   json.RawMessage


Call this a string so you can remove all the json.RawMessage literals (cast before passing to ParseConfig).

dfawley · 2022-06-03T20:56:47Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		name    string
+		input   json.RawMessage
+		wantCfg serviceconfig.LoadBalancingConfig
+		wantErr bool


Instead of a boolean, let's check for a string to be a substring of the error message.

Good point. This actually lead me to discover that I was actually getting a parsing error not a validation error (see the 100 below, it shouldn't error - and it's also the wrong field name haha).

Also, another thing I noticed is that I never check if there is a child/the child is a clusterimpl/the child config is valid. These are assumptions I made for my od balancer, which if broken would cause nil panic. What should I do about this? Should I add a validation? Defensively program in my Outlier Detection PR?

It won't ever be called otherwise since we prepare the config ourselves but still.

Discussed offline

Added check for child being there (validating child was already there implicitly from unmarshalJSON if present)

dfawley · 2022-06-03T20:59:13Z

xds/internal/balancer/clusterresolver/configbuilder.go

 			retConfig.Priorities = append(retConfig.Priorities, names...)
+			retAddrs = append(retAddrs, addrs...)


Can these be moved up above the if envconfig, and the two if envconfigs merged?

Ah, great catch. Switched.

dfawley · 2022-06-03T20:59:24Z

xds/internal/balancer/clusterresolver/configbuilder.go

 			retConfig.Priorities = append(retConfig.Priorities, name)
+			retAddrs = append(retAddrs, addrs...)


Switched to one if as well.

dfawley · 2022-06-03T21:00:05Z

xds/internal/balancer/clusterresolver/configbuilder.go

 		}
 	}
 	return retConfig, retAddrs, nil
 }

+func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig {
+	odCfgs := make(map[string]*outlierdetection.LBConfig)


make(..., len(ciCfgs))?

Switched. Why is this faster/better?

Pre-allocating is faster so it doesn't need to grow as you add entries if you go over the default initial size. Or if you have fewer entries than the default initial size, then you save memory.

dfawley · 2022-06-03T21:00:50Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := odCfg
+	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.


Pass *clusterimpl.LBConfig then?

I don't get how whether clusterimpl.LBConfig relates to whether this panics or not. It panics on odCfgReturn.ChildPolicy. I needed to make the function arguments value types to make new memory and not write over heap memory used elsewhere.

Discussed offline

Oh, I see how it panics, callsite at 161. Pointer to nil.

Fixed it. Let me know if you like solution.

zasweq

Thanks for the comments boss man :D!

zasweq · 2022-06-04T01:26:21Z

xds/internal/balancer/clusterresolver/configbuilder.go

 			retConfig.Priorities = append(retConfig.Priorities, names...)
+			retAddrs = append(retAddrs, addrs...)


Ah, great catch. Switched.

zasweq · 2022-06-04T01:26:29Z

xds/internal/balancer/clusterresolver/configbuilder.go

 			retConfig.Priorities = append(retConfig.Priorities, name)
+			retAddrs = append(retAddrs, addrs...)


Switched to one if as well.

zasweq · 2022-06-04T01:27:57Z

xds/internal/balancer/clusterresolver/configbuilder.go

 		}
 	}
 	return retConfig, retAddrs, nil
 }

+func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig {
+	odCfgs := make(map[string]*outlierdetection.LBConfig)


Switched. Why is this faster/better?

zasweq · 2022-06-04T01:33:59Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := odCfg
+	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.


I don't get how whether clusterimpl.LBConfig relates to whether this panics or not. It panics on odCfgReturn.ChildPolicy. I needed to make the function arguments value types to make new memory and not write over heap memory used elsewhere.

zasweq · 2022-06-04T01:56:43Z

xds/internal/balancer/outlierdetection/balancer_test.go

+
+	tests := []struct {
+		name    string
+		input   json.RawMessage


zasweq · 2022-06-04T02:33:40Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		name    string
+		input   json.RawMessage
+		wantCfg serviceconfig.LoadBalancingConfig
+		wantErr bool


Good point. This actually lead me to discover that I was actually getting a parsing error not a validation error (see the 100 below, it shouldn't error - and it's also the wrong field name haha).

zasweq · 2022-06-04T02:44:36Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		name    string
+		input   json.RawMessage
+		wantCfg serviceconfig.LoadBalancingConfig
+		wantErr bool


Also, another thing I noticed is that I never check if there is a child/the child is a clusterimpl/the child config is valid. These are assumptions I made for my od balancer, which if broken would cause nil panic. What should I do about this? Should I add a validation? Defensively program in my Outlier Detection PR?

zasweq · 2022-06-04T02:49:56Z

xds/internal/balancer/outlierdetection/balancer_test.go

+	bb := balancer.Get(Name)
+	if bb == nil {
+		t.Fatalf("balancer.Get(%q) returned nil", Name)
+	}
+	parser, ok := bb.(balancer.ConfigParser)
+	if !ok {
+		t.Fatalf("balancer %q does not implement the ConfigParser interface", Name)
+	}


Ah, good point. Switched.

zasweq · 2022-06-04T02:52:12Z

xds/internal/balancer/outlierdetection/balancer.go

+type outlierDetectionBalancer struct {
+}
+
+func (b *outlierDetectionBalancer) UpdateClientConnState(s balancer.ClientConnState) error {


I tried the second option, it broke since this needs to be registered in config validation at a higher level. Thus, I kept implementing balancer.Builder and returned nil from build instead of the type, and deleted this type.

zasweq · 2022-06-04T02:53:10Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		name    string
+		input   json.RawMessage
+		wantCfg serviceconfig.LoadBalancingConfig
+		wantErr bool


It won't ever be called otherwise since we prepare the config ourselves but still.

zasweq

Thanks for the comments :D!

zasweq · 2022-06-07T20:53:58Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := odCfg
+	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.


Oh, I see how it panics, callsite at 161. Pointer to nil.

zasweq · 2022-06-07T21:11:48Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := odCfg
+	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.


Fixed it. Let me know if you like solution.

zasweq · 2022-06-07T21:12:39Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		name    string
+		input   json.RawMessage
+		wantCfg serviceconfig.LoadBalancingConfig
+		wantErr bool


Added check for child being there (validating child was already there implicitly from unmarshalJSON if present)

dfawley · 2022-06-08T15:29:02Z

xds/internal/balancer/clusterresolver/configbuilder.go

-	odCfgRet := odCfg
-	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.
-	return &odCfgRet
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {


Why not pass ciCfg by pointer? Generally structs should be passed by pointer unless there's a good reason not to.

See: go/go-style/decisions#pass-values

Switched to passing by pointer.

dfawley · 2022-06-08T15:32:34Z

xds/internal/balancer/outlierdetection/balancer.go

@@ -90,6 +91,11 @@ func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, err
 	if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100 {
 		return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage)
 	}
+
+	if lbCfg.ChildPolicy == nil {
+		return nil, errors.New("LBConfig.ChildPolicy needs to be present")


s/needs to/must/?

dfawley · 2022-06-08T15:37:23Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := &outlierdetection.LBConfig{}
+	if odCfg != nil {


Is it valid for this to be nil? Why? For the JSON config, can we make the field a value instead of a pointer so it's never nil? Then pass by value to avoid the explicit copy.

It's not valid. This config is prepared by the cluster resolver, so we're guaranteed for this not to be nil, but this handles the nil panic I discussed earlier with you.

Had the discovery mechanism hold onto a value, and pass it by value around. I kept this function returning a pointer though, as that is the value of internalserviceconfig.BalancerConfig that I populate.

xds/internal/balancer/outlierdetection/balancer.go

xds/internal/balancer/outlierdetection/balancer_test.go

easwars · 2022-06-08T19:01:46Z

xds/internal/balancer/clusterresolver/configbuilder.go

-	odCfgRet := odCfg
-	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.
-	return &odCfgRet
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {


See: go/go-style/decisions#pass-values

xds/internal/balancer/clusterresolver/configbuilder_test.go

xds/internal/balancer/outlierdetection/balancer_test.go

zasweq

Thanks for the comments! I'll wait to rebase my full PR since I don't want to force push in case one of you is in the middle of a review on the full one. Maybe I should wait until this is merged to rebase. The balancer is all orthogonal.

xds/internal/balancer/clusterresolver/configbuilder_test.go

xds/internal/balancer/outlierdetection/balancer.go

zasweq · 2022-06-08T20:19:41Z

xds/internal/balancer/outlierdetection/balancer.go

@@ -90,6 +91,11 @@ func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, err
 	if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100 {
 		return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage)
 	}
+
+	if lbCfg.ChildPolicy == nil {
+		return nil, errors.New("LBConfig.ChildPolicy needs to be present")


xds/internal/balancer/outlierdetection/balancer.go

xds/internal/balancer/outlierdetection/balancer_test.go

xds/internal/balancer/outlierdetection/balancer.go

zasweq · 2022-06-09T00:14:09Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := &outlierdetection.LBConfig{}
+	if odCfg != nil {


It's not valid. This config is prepared by the cluster resolver, so we're guaranteed for this not to be nil, but this handles the nil panic I discussed earlier with you.

zasweq · 2022-06-09T01:04:22Z

xds/internal/balancer/clusterresolver/configbuilder.go

-	odCfgRet := odCfg
-	odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want.
-	return &odCfgRet
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {


Switched to passing by pointer.

zasweq · 2022-06-09T01:05:35Z

xds/internal/balancer/clusterresolver/configbuilder.go

+
+func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig {
+	odCfgRet := &outlierdetection.LBConfig{}
+	if odCfg != nil {


Had the discovery mechanism hold onto a value, and pass it by value around. I kept this function returning a pointer though, as that is the value of internalserviceconfig.BalancerConfig that I populate.

easwars · 2022-06-15T23:30:05Z

xds/internal/balancer/clusterresolver/clusterresolver_test.go

+		return err
+	}
+	gotCCS := ccs.(balancer.ClientConnState)
+	if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(resolver.State{}, "Addresses", "ServiceConfig", "Attributes")); diff != "" {


Would this work instead?

if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(balancer.ClientConnState{}, "ResolverState")); diff != "" { ... }

Also, why do we have to ignore the resolver state part of the update? Just so that we dont have to specify it in the wantCCS?

Yup, that works :). I just commented this out to test since I forgot since it's been so long, and yeah super verbose resolver state I would have to match in wantCCS.

zasweq

Thanks for the comment :D!

zasweq · 2022-06-17T01:11:51Z

xds/internal/balancer/clusterresolver/clusterresolver_test.go

+		return err
+	}
+	gotCCS := ccs.(balancer.ClientConnState)
+	if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(resolver.State{}, "Addresses", "ServiceConfig", "Attributes")); diff != "" {


Yup, that works :). I just commented this out to test since I forgot since it's been so long, and yeah super verbose resolver state I would have to match in wantCCS.

zasweq added the Type: Feature label May 19, 2022

zasweq added this to the 1.47 Release milestone May 19, 2022

zasweq force-pushed the od-config-cluster-resolver branch from 9c7e762 to f01bc93 Compare May 23, 2022 19:24

zasweq modified the milestones: 1.47 Release, 1.48 Release May 23, 2022

zasweq requested a review from dfawley May 24, 2022 17:32

zasweq assigned dfawley May 24, 2022

dfawley assigned zasweq and unassigned dfawley May 31, 2022

Added branching logic for Outlier Detection configuration in config b…

72138c7

…uilder

zasweq force-pushed the od-config-cluster-resolver branch from f01bc93 to 72138c7 Compare May 31, 2022 21:39

dfawley reviewed Jun 3, 2022

View reviewed changes

zasweq commented Jun 4, 2022

View reviewed changes

Responded to Doug's comments

0f84478

zasweq assigned dfawley and unassigned zasweq Jun 6, 2022

zasweq mentioned this pull request Jun 6, 2022

xds: Added Outlier Detection Balancer #5403

Closed

dfawley assigned zasweq and unassigned dfawley Jun 7, 2022

zasweq commented Jun 7, 2022

View reviewed changes

Responded to Doug's comments

770161c

zasweq assigned dfawley and unassigned zasweq Jun 7, 2022

zasweq mentioned this pull request Jun 7, 2022

xds: add Outlier Detection Balancer #5413

Closed

dfawley reviewed Jun 8, 2022

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jun 8, 2022

easwars reviewed Jun 8, 2022

View reviewed changes

zasweq added 2 commits June 8, 2022 20:21

Addressed comments outside the ones in config builder

2e16f0a

Responded to comments in config builder

6e8454e

zasweq commented Jun 9, 2022

View reviewed changes

zasweq assigned easwars and dfawley and unassigned zasweq Jun 9, 2022

easwars reviewed Jun 15, 2022

View reviewed changes

easwars assigned zasweq and unassigned easwars and dfawley Jun 15, 2022

zasweq commented Jun 17, 2022

View reviewed changes

Responded to Easwar's comment

4572880

zasweq mentioned this pull request Jun 17, 2022

xds: add Outlier Detection Balancer #5435

Merged

easwars approved these changes Jun 17, 2022

View reviewed changes

zasweq merged commit 29d9970 into grpc:master Jun 17, 2022

github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2022

		retConfig.Priorities = append(retConfig.Priorities, names...)
		retAddrs = append(retAddrs, addrs...)

xds: Outlier Detection configuration in Cluster Resolver Balancer #5371

xds: Outlier Detection configuration in Cluster Resolver Balancer #5371

Conversation

zasweq commented May 19, 2022 • edited

dfawley commented May 31, 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

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

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

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

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

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

zasweq commented May 19, 2022 •

edited