xds/outlierdetection: fix config handling #6361

zasweq · 2023-06-08T03:35:42Z

This PR fixes Outlier Detection configuration across the xDS System and the Outlier Detection ParseConfig.

This PR touches multiple components:

xDS Client: Switch to emit Outlier Detection configuration into JSON, and also handle xDS Defaults (proto -> layered gRPC JSON structure). The handling of xDS Defaults used to be handled in cds_balancer, and was incorrect, as it did not contain a distinction between unset and set and 0 for fields, causing the first bug listed below.
cds_balancer: Take Outlier Detection JSON and Endpoint + Locality Picking JSON, prepare a Cluster Resolver configuration in JSON. Call ParseConfig() on cluster_resolver child to receive cluster resolver configuration to send downward.
cluster_resolver: Accept Outlier Detection JSON and Endpoint + Locality Picking JSON in it's top level config, ParseConfig() Parses (using gRPC LB Registry) both Outlier Detection JSON and Endpoint + Locality Picking JSON into internal types. Persists these internal types as helpers to build priority configuration, which will eventually get marshaled to JSON, parsed through ParseConfig(), and sent down to priority.
outlier_detection: Set defaults in ParseConfig(). This consists of two layers. Any of the top layers fields will get a default value if unset in JSON. If any second layer fields are present (SuccessRateEjection and FailurePercentage), the fields of the second layer field(s) which are present get a default value if unset in JSON.

This fixes a few bugs:

Previously, in xDS, if an Outlier Detection message was set in proto configuration received in the xDS Client with EnforcingSuccessRate unset, Outlier Detection would not be turned on. However, in this case, the xDS Defaults should cause Outlier Detection to turn on in this scenario, thus enabling Outlier Detection by default in the xDS flow if Outlier Detection proto is present.
cds_balancer was bypassing the cluster_resolver balancer API by not calling ParseConfig on the cluster_resolver balancer, to receive the cluster resolver configuration to pass down, and preparing it's config using it's exported struct directly, and sending that down.
outlier_detection was not setting it's default fields in ParseConfig correctly. Thus, if a user set the Outlier Detection balancer as the top level balancer of the channel using a Service Config string, this would not get populated with the defaults for unset fields. This is an issue because Outlier Detection can be used in both the xDS case and non xDS case.

RELEASE NOTES:

xds/internal/xdsclient/xdsresource: Fix Outlier Detection Config Handling and correctly set xDS Defaults
xds/internal/balancer/outlierdetection: Fix Outlier Detection Config Handling by setting defaults in ParseConfig()

dfawley · 2023-06-09T15:37:50Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+		// Shouldn't happen, registered through imported Cluster Resolver,
+		// defensive programming.
+		logger.Errorf("%q LB policy is needed but not registered", clusterresolver.Name)
+		return nil


It's not actually legal to return nil, though. We should return an LB policy instance that is a nop for everything and provides a TF picker or something in this situation. Maybe add a package to internal/balancer with this trivial implementation.

grpc-go/internal/balancer/gracefulswitch/gracefulswitch.go

Line 123 in 1c0572a

// This is illegal and should never happen; we clear the balancerWrapper

Done. Here and in Cluster Resolver.

dfawley · 2023-06-09T15:37:56Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+	if !ok {
+		// Shouldn't happen, imported Cluster Resolver builder has this method.
+		logger.Errorf("%q LB policy does not implement a config parser", clusterresolver.Name)
+		return nil


Done. Here and in Cluster Resolver.

dfawley · 2023-06-09T15:40:37Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+			// "In the cds LB policy, if the outlier_detection field is not set in
+			// the Cluster resource, a "no-op" outlier_detection config will be
+			// generated in the corresponding DiscoveryMechanism config, with all
+			// fields unset." - A50
+			if odJSON == nil {
+				// This will pick up top level defaults in Cluster Resolver
+				// ParseConfig, but sre and fpe will be nil still so still a
+				// "no-op" config.
+				odJSON = json.RawMessage(`{}`)
+			}


Can this logic be moved to where we produce the JSON OD config from the proto instead? This is part of converting from xds OD config to OD's JSON config.

No, unfortunately not, because the language in the gRFC explicitly states "in the cds lb policy". The issue was I mapped that language to the paragraph following to. We triaged this, and this was the only behavior scoped to the cds lb policy. @murgatroid99

dfawley · 2023-06-09T15:43:26Z

xds/internal/balancer/clusterresolver/clusterresolver.go

+	var cfg *LBConfig
+	if err := json.Unmarshal(j, &cfg); err != nil {


Pointer to a pointer?

Yeah, this is how my wrr_locality balancer works too. https://github.com/grpc/grpc-go/blob/master/xds/internal/balancer/wrrlocality/balancer.go#L93. If I switch it it hangs. I thinkkkk it's because if you declare a value type on the stack, it gets all the zero values.

Of stuff like child config etc.

dfawley · 2023-06-09T15:44:13Z

xds/internal/balancer/clusterresolver/clusterresolver.go

+	for i, dm := range cfg.DiscoveryMechanisms {
+		lbCfg, err := odParser.ParseConfig(dm.OutlierDetection)
+		if err != nil {
+			return nil, fmt.Errorf("error parsing Outlier Detection config: %v", dm.OutlierDetection)


This needs to return err too.

Done. Good catch.

dfawley · 2023-06-09T15:45:53Z

xds/internal/balancer/clusterresolver/clusterresolver.go

+	if envconfig.XDSOutlierDetection {
+		for i, odCfg := range odCfgs {
+			cfg.DiscoveryMechanisms[i].outlierDetection = odCfg
+		}
+	}


Why is this checking the env var, but parsing isn't? That seems wrong. We don't usually validate things we won't use. I think they both should be in this condition and then we don't need two different for loops, right?

Ah right fair point. Changed.

dfawley · 2023-06-09T15:48:21Z

xds/internal/balancer/clusterresolver/configbuilder_test.go

@@ -72,7 +73,10 @@ var (
 	}

 	noopODCfg = outlierdetection.LBConfig{


Why does a noop config have fields set to specific numbers?

Can't do anything about it. The gRFC specifically states "cds generates with all fields unset" for the no-op. Thus, the way our system is set up now, it is literally impossible to uphold the balancer API while keeping all fields unset (will always call ParseConfig() and get defaults). The important part is sre and fpe == nil, which keeps the picker from doing unnecessary counting on the critical rpc path. I talked to Michael about this and he says it's fine. I would like the fields to stay unset, but unfortunately can't uphold API and keep fields unset. Also can't set to zero values because that is different that the language of the gRFC, which states "all fields unset".

Wait how is this config used in these tests, then? It looked like they were inputs, which means they could be completely empty, right? Or are you trying to make the input test data match real-world input data?

The latter.

dfawley · 2023-06-09T15:51:51Z

xds/internal/balancer/outlierdetection/balancer.go

+	lbCfg := &LBConfig{
+		// Default top layer values as documented in A50.
+		Interval:           iserviceconfig.Duration(10 * time.Second),
+		BaseEjectionTime:   iserviceconfig.Duration(30 * time.Second),
+		MaxEjectionTime:    iserviceconfig.Duration(300 * time.Second),
+		MaxEjectionPercent: 10,
+	}


Given the multi-level nature of this...there are now other defaults elsewhere. Can you move these defaults into config.go so they are all together, by adding a LBConfig.UnmarshalJSON method?

Done. Needed to do same trick on that type to prevent infinite recursion and stack overflow.

dfawley · 2023-06-09T15:56:07Z

xds/internal/balancer/outlierdetection/config.go

+type failurePercentageEjection FailurePercentageEjection
+
+// UnmarshalJSON unmarshals JSON into FailurePercentageEjection. If a
+// FailurePercentageEjection field is not set, that field will get it's default


s/it's/its/ - 3x in this PR. Whenever I see "it's" my mind says "it is".

dfawley · 2023-06-09T16:03:51Z

xds/internal/xdsclient/xdsresource/unmarshal_cds.go

+		SuccessRateEjection:       sre,
+		FailurePercentageEjection: fpe,
+	}
+	odLBCfgJSON, err := json.Marshal(odLBCfg)


return json.Marshal(odLBCfg)

Good point. Switched.

zasweq

Thanks for the pass! Got to all comments.

dfawley · 2023-06-09T19:53:44Z

internal/balancer/nop/nop.go

+ *
+ */
+
+// Package nop implements a balancer with all of it's balancer operations as


:/. Haha switched.

dfawley · 2023-06-09T19:54:09Z

internal/balancer/nop/nop.go

+	"google.golang.org/grpc/connectivity"
+)
+
+// Balancer is a balancer with all of it's balancer operations as no-ops, other


:/. Haha switched.

dfawley · 2023-06-09T19:55:08Z

internal/balancer/nop/nop.go

+// and a Connectivity State of TRANSIENT_FAILURE.
+func (b *Balancer) UpdateClientConnState(_ balancer.ClientConnState) error {
+	b.cc.UpdateState(balancer.State{
+		Picker:            base.NewErrPicker(errors.New("no-op balancer invoked")),


The error returned here should be an error passed to NewNOPBalancer.

dfawley · 2023-06-09T19:55:41Z

internal/balancer/nop/nop.go

+
+// Balancer is a balancer with all of it's balancer operations as no-ops, other
+// than returning a Transient Failure Picker on a Client Conn update.
+type Balancer struct {


Unexport, then you don't need all these comments. Only NewNOPBalancer needs a comment.

I had it unexported, but it was throwing errors wrt lint. I had to change it to pass lint :/

dfawley · 2023-06-09T19:56:42Z

internal/balancer/nop/nop.go

+}
+
+// NewNOPBalancer returns a no-op balancer.
+func NewNOPBalancer(cc balancer.ClientConn) *Balancer {


NewBalancer since it's in the nop package? I.e. nop.NewBalancer() vs nop.NewNOPBalancer() ... the latter slightly stutters.

dfawley · 2023-06-09T20:00:16Z

xds/internal/balancer/clusterresolver/configbuilder_test.go

@@ -72,7 +73,10 @@ var (
 	}

 	noopODCfg = outlierdetection.LBConfig{


Wait how is this config used in these tests, then? It looked like they were inputs, which means they could be completely empty, right? Or are you trying to make the input test data match real-world input data?

dfawley · 2023-06-09T23:10:45Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

@@ -160,7 +160,7 @@ type exitIdle struct{}

 // cdsBalancer implements a CDS based LB policy. It instantiates a
 // cluster_resolver balancer to further resolve the serviceName received from
-// CDS, into localities and endpoints. Implements the balancer.Balancer
+// CDS, into localities and endpoints. Implements the balancer.bal


Whoops. Change back.

zasweq added the Type: Bug label Jun 8, 2023

zasweq requested a review from dfawley June 8, 2023 03:35

zasweq added this to the 1.56 Release milestone Jun 8, 2023

zasweq assigned dfawley Jun 8, 2023

Fix Outlier Detection Config handling

e9c6d27

zasweq force-pushed the fix-od-config-handling branch from d7701d2 to e9c6d27 Compare June 8, 2023 03:59

dfawley reviewed Jun 9, 2023

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jun 9, 2023

Responded to Doug's comments

a2552fe

zasweq commented Jun 9, 2023

View reviewed changes

dfawley reviewed Jun 9, 2023

View reviewed changes

zasweq added 2 commits June 9, 2023 16:21

Responded to Doug's comments

183d67f

unexport

65f380f

dfawley reviewed Jun 9, 2023

View reviewed changes

dfawley approved these changes Jun 9, 2023

View reviewed changes

zasweq added 2 commits June 9, 2023 19:12

Whoops

e5aaa8e

whoops part 2, electric bugaloo

f37646d

dfawley changed the title ~~Fix Outlier Detection Config handling~~ xds/outlierdetection: fix config handling Jun 9, 2023

dfawley approved these changes Jun 9, 2023

View reviewed changes

zasweq merged commit 3c6084b into grpc:master Jun 9, 2023
11 checks passed

zasweq added a commit to zasweq/grpc-go that referenced this pull request Jun 9, 2023

xds/outlierdetection: fix config handling (grpc#6361)

118e316

zasweq added a commit that referenced this pull request Jun 10, 2023

xds/outlierdetection: fix config handling (#6361) (#6367)

7996425

zasweq mentioned this pull request Jun 12, 2023

xDS: Fix Outlier Detection Config Handling #6274

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2023

		var cfg *LBConfig
		if err := json.Unmarshal(j, &cfg); err != nil {

		@@ -72,7 +73,10 @@ var (
		}

		noopODCfg = outlierdetection.LBConfig{

xds/outlierdetection: fix config handling #6361

xds/outlierdetection: fix config handling #6361

Conversation

zasweq commented Jun 8, 2023 • edited

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 Jun 9, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Jun 8, 2023 •

edited

zasweq Jun 9, 2023 •

edited