xds: add Outlier Detection Balancer #5435

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

This PR adds the Outlier Detection Balancer as part of the xDS configured tree of balancers (see https://github.com/grpc/proposal/blob/master/A50-xds-outlier-detection.md).

Contains #5371.

RELEASE NOTES:

xDS: add support for Outlier Detection

dfawley · 2022-06-17T23:02:35Z

It seems this still needs to be rebased due to outlierdetection files.

easwars

Just skimmed through. Expect more comments as I spend more time on this.

easwars · 2022-07-06T17:46:07Z

test/xds/xds_client_outlier_detection_test.go

+	v3clusterpb "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
+	v3endpointpb "github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3"
+	v3listenerpb "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
+	v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/internal"
+	"google.golang.org/grpc/internal/envconfig"
+	"google.golang.org/grpc/internal/stubserver"
+	"google.golang.org/grpc/internal/testutils/xds/e2e"
+	testgrpc "google.golang.org/grpc/test/grpc_testing"
+	testpb "google.golang.org/grpc/test/grpc_testing"
+	"google.golang.org/protobuf/types/known/durationpb"
+	"google.golang.org/protobuf/types/known/wrapperspb"


Nit: please group renamed proto imports separately.

easwars · 2022-07-06T17:52:21Z

test/xds/xds_client_outlier_detection_test.go

+// TestOutlierDetection tests an xDS configured ClientConn with an Outlier
+// Detection present in the system which is a logical no-op. An RPC should
+// proceed as normal.
+func (s) TestOutlierDetection(t *testing.T) {


Maybe TestOutlierDetection_EmptyConfig or TestOutlierDetection_NoopConfig to be explicit about what is being tested here?

Chose no-op config, empty management server update but od config is better termed "no-op". Thanks for suggestion.

easwars · 2022-07-06T17:56:18Z

test/xds/xds_client_outlier_detection_test.go

+	"google.golang.org/protobuf/types/known/wrapperspb"
+)
+
+// TestOutlierDetection tests an xDS configured ClientConn with an Outlier


How about?

// TestOutlierDetectionXxx tests the scenario where the outlier detection feature
// is enabled on the gRPC client, but it receives no outlier detection configuration
// from the management server. This should result in a no-op outlier configuration
// being used. This test verifies that an RPC is able to proceed normally with this
// configuration.

Done (with a little rewording).

easwars · 2022-07-06T17:57:35Z

test/xds/xds_client_outlier_detection_test.go

+	}
+}
+
+// defaultClientResourcesSpecifyingMultipleBackendsAndOutlierDetection returns


And the award for the author with the longest identifier name in our codebase goes to ...... lol

Lol. This Java book I read called clean code told me to be verbose and explanatory in my variable names. Switched to "defaultClientResourcesMultipleBackendsAndOD". Is that better or too verbose/if so do you have a better suggestion?

easwars · 2022-07-06T17:58:57Z

test/xds/xds_client_outlier_detection_test.go

+		Interval: &durationpb.Duration{
+			Nanos: 500000000,
+		},


Nit: single line
And probably a comment with a human readable value for this time duration.

Done. Added comment // .5 seconds.

easwars · 2022-07-06T18:34:47Z

xds/internal/balancer/outlierdetection/balancer.go

+	}
+}
+
+type object struct {


object is not a very descriptive name for a type which is quite central to the implementation. Maybe something like subConnState or subConnInfo or subConnRunTimeInfo or subConnRunTimeState or something better. And a comment for the type describing its purpose would be useful too.

Also, this type does not contain any mutexes. It would be helpful for the above comment to talk about how synchronization is handled for this type.

This struct represents information about a certain address, not a SubConn. Each SubConn does hold a ref to this though. As such, I chose the name "addressInfo".

Added comment for addressInfo explaining what it is and also explaining how it is synchronized. Let me know if it's too verbose or if you like it.

easwars · 2022-07-06T18:37:25Z

xds/internal/balancer/outlierdetection/balancer.go

+	// The call result counter object
+	callCounter *callCounter
+
+	// The latest ejection timestamp, or null if the address is currently not


Replace with
// The latest ejection timestamp, or zero if the address is currently not ejected.
And get rid of the // We represent the branching logic on the null with a time.Zero() value.

Please terminate comment sentences with periods. go/go-style/decisions#comment-sentences

Ah, good merging. Switched. Added periods to every comment about each field in this struct.

easwars · 2022-07-06T18:54:55Z

xds/internal/balancer/outlierdetection/subconn_wrapper.go

+type subConnWrapper struct {
+	balancer.SubConn
+
+	// "The subchannel wrappers created by the outlier_detection LB policy will


Please consider not quoting the gRFC verbatim. The gRFC text makes sense in its context. But here (and everywhere else in this PR), please make comments self-explanatory and self-contained. Thanks.

Switched as many as I could over. I left the one for swap() though, since that defines an implementation detail that I didn't follow with a subsequent explanation. I also left the ones in NewSubConn() as I thought they made sense, and the actual Outlier Detection Interval algorithm itself as it's explaining the algorithm step by step.

xds/internal/balancer/outlierdetection/subconn_wrapper.go

easwars · 2022-07-06T19:04:14Z

xds/internal/balancer/outlierdetection/subconn_wrapper.go

+// eject(): "The wrapper will report a state update with the TRANSIENT_FAILURE
+// state, and will stop passing along updates from the underlying subchannel."
+func (scw *subConnWrapper) eject() {
+	scw.scUpdateCh.Put(&ejectedUpdate{
+		scw:     scw,
+		ejected: true,
+	})
+}
+
+// uneject(): "The wrapper will report a state update with the latest update
+// from the underlying subchannel, and resume passing along updates from the
+// underlying subchannel."
+func (scw *subConnWrapper) uneject() {
+	scw.scUpdateCh.Put(&ejectedUpdate{
+		scw:     scw,
+		ejected: false,
+	})
+}


Do these have to be methods on the subConnWrapper type?

Can callsites of scw.eject() be replaced by:

b.scUpdateCh.Put(&ejectedUpdate{scw: scw, ejected: true})

and similarly for uneject().

They don't have to be, technically, but A50 clearly states to have methods on the subChannelWrapper type: https://github.com/grpc/proposal/blob/master/A50-xds-outlier-detection.md#subchannel-wrapper. To keep implementations similar, I think it's best to do it as such. Plus, there's 8 callsites for both of these methods and I think it's cleaner for scw.eject()/scw.uneject() instead of putting stuff on update channel for each call site. However, main reason is that is explicitly defined in gRFC.

zasweq

Thanks for the comments :D!

zasweq · 2022-07-06T22:31:04Z

test/xds/xds_client_outlier_detection_test.go

+// TestOutlierDetection tests an xDS configured ClientConn with an Outlier
+// Detection present in the system which is a logical no-op. An RPC should
+// proceed as normal.
+func (s) TestOutlierDetection(t *testing.T) {


Chose no-op config, empty management server update but od config is better termed "no-op". Thanks for suggestion.

zasweq · 2022-07-06T22:34:45Z

test/xds/xds_client_outlier_detection_test.go

+	"google.golang.org/protobuf/types/known/wrapperspb"
+)
+
+// TestOutlierDetection tests an xDS configured ClientConn with an Outlier


Done (with a little rewording).

zasweq · 2022-07-06T22:38:55Z

test/xds/xds_client_outlier_detection_test.go

+	}
+}
+
+// defaultClientResourcesSpecifyingMultipleBackendsAndOutlierDetection returns


Lol. This Java book I read called clean code told me to be verbose and explanatory in my variable names. Switched to "defaultClientResourcesMultipleBackendsAndOD". Is that better or too verbose/if so do you have a better suggestion?

zasweq · 2022-07-06T22:41:01Z

test/xds/xds_client_outlier_detection_test.go

+		Interval: &durationpb.Duration{
+			Nanos: 500000000,
+		},


Done. Added comment // .5 seconds.

zasweq · 2022-07-06T22:48:29Z

xds/internal/balancer/outlierdetection/subconn_wrapper.go

+// eject(): "The wrapper will report a state update with the TRANSIENT_FAILURE
+// state, and will stop passing along updates from the underlying subchannel."
+func (scw *subConnWrapper) eject() {
+	scw.scUpdateCh.Put(&ejectedUpdate{
+		scw:     scw,
+		ejected: true,
+	})
+}
+
+// uneject(): "The wrapper will report a state update with the latest update
+// from the underlying subchannel, and resume passing along updates from the
+// underlying subchannel."
+func (scw *subConnWrapper) uneject() {
+	scw.scUpdateCh.Put(&ejectedUpdate{
+		scw:     scw,
+		ejected: false,
+	})
+}


They don't have to be, technically, but A50 clearly states to have methods on the subChannelWrapper type: https://github.com/grpc/proposal/blob/master/A50-xds-outlier-detection.md#subchannel-wrapper. To keep implementations similar, I think it's best to do it as such. Plus, there's 8 callsites for both of these methods and I think it's cleaner for scw.eject()/scw.uneject() instead of putting stuff on update channel for each call site. However, main reason is that is explicitly defined in gRFC.

zasweq · 2022-07-06T23:02:52Z

xds/internal/balancer/outlierdetection/balancer.go

+	// The call result counter object
+	callCounter *callCounter
+
+	// The latest ejection timestamp, or null if the address is currently not


Ah, good merging. Switched. Added periods to every comment about each field in this struct.

zasweq · 2022-07-06T23:05:54Z

xds/internal/balancer/outlierdetection/balancer.go

+	}
+}
+
+type object struct {


This struct represents information about a certain address, not a SubConn. Each SubConn does hold a ref to this though. As such, I chose the name "addressInfo".

zasweq · 2022-07-06T23:17:08Z

xds/internal/balancer/outlierdetection/balancer.go

+	}
+}
+
+type object struct {


Added comment for addressInfo explaining what it is and also explaining how it is synchronized. Let me know if it's too verbose or if you like it.

zasweq · 2022-07-06T23:19:44Z

xds/internal/balancer/outlierdetection/balancer.go

+	odAddrs        map[string]*object
+	odCfg          *LBConfig


Deleted od prefix. Kept outlierDetectionBalancer though, as clusterImplBalancer had prefix on Balancer type. GracefulSwitch doesn't though so let me know if you want me to switch outlierDetectionBalancer too.

zasweq · 2022-07-06T23:37:29Z

xds/internal/balancer/outlierdetection/subconn_wrapper.go

+type subConnWrapper struct {
+	balancer.SubConn
+
+	// "The subchannel wrappers created by the outlier_detection LB policy will


Switched as many as I could over. I left the one for swap() though, since that defines an implementation detail that I didn't follow with a subsequent explanation. I also left the ones in NewSubConn() as I thought they made sense, and the actual Outlier Detection Interval algorithm itself as it's explaining the algorithm step by step.

dfawley · 2022-07-07T18:03:41Z

xds/internal/balancer/outlierdetection/balancer.go

+
+}
+
+func (b *outlierDetectionBalancer) successRateAlgorithm() {


Nit: ejectLowSuccessRateBackends? or something like that to indicate more precisely what the function is doing. Same with the other Algorithm functions.

Ummmmmmm this is the configuration defines the algorithm as and A50, I agree that would be nicer, I'll write a comment for each xAlgorithm function.

dfawley · 2022-07-07T18:05:04Z

xds/internal/balancer/outlierdetection/balancer.go

+	return numAddrs
+}
+
+// meanAndStdDevOfSucceseesAtLeastRequestVolume returns the mean and std dev of


Succesees?

Whoops. Switched.

dfawley · 2022-07-07T18:19:58Z

xds/internal/balancer/outlierdetection/balancer.go

+	// Reject whole config if any errors, don't persist it for later
+	bb := balancer.Get(lbCfg.ChildPolicy.Name)
+	if bb == nil {
+		return fmt.Errorf("balancer %q not registered", lbCfg.ChildPolicy.Name)


Include the name of this LB policy, please.

Switched to "outlier detection: child balancer...".

xds/internal/balancer/outlierdetection/balancer.go

dfawley · 2022-07-07T18:21:34Z

xds/internal/balancer/outlierdetection/balancer.go

+		if b.child != nil {
+			b.child.Close()
+		}
+		// What if this is nil? Seems fine


dfawley · 2022-07-07T18:30:20Z

xds/internal/balancer/outlierdetection/balancer.go

+	// Note that 0 addresses is a valid update/state for a SubConn to be in.
+	// This is correctly handled by this algorithm (handled as part of a non singular
+	// old address/new address).
+	if len(scw.addresses) == 1 {


This would probably be a little clearer as:

if len(scw.addresses == 1) && len(addrs) == 1 { // single address to single address } else if len(scw.addresses == 1) { // single address to multiple/no addresses } else if len(addrs) == 1 { // multiple address to single address } // else multiple/no addresses to multiple/no addresses; ignore

or:

switch { case len(scw.addresses == 1) && len(addrs) == 1: case len(scw.addresses == 1): case len(addrs) == 1: }

Wow, great clean suggestion. I chose option two, Easwar has made me use switches for scenarios like this and he knows go style so I'll go with option 2 thanks.

dfawley · 2022-07-07T18:33:13Z

xds/internal/balancer/outlierdetection/balancer.go

+			obj := b.appendIfPresent(addrs[0].Addr, scw)
+			// 3. Relay state with eject() recalculated (using the corresponding
+			// map entry to see if it's currently ejected).
+			if obj == nil { // uneject unconditionally because could have come from an ejected address


This says "uneject unconditionally" but goes on to eject?

Whoops, good catch. Switched to uneject().

dfawley · 2022-07-07T18:34:58Z

xds/internal/balancer/outlierdetection/balancer.go

+		// ejection_timestamp + min(base_ejection_time (type: time.Time) *
+		// multiplier (type: int), max(base_ejection_time (type: time.Time),
+		// max_ejection_time (type: time.Time))), un-eject the address.
+		if !obj.latestEjectionTimestamp.IsZero() && now().After(obj.latestEjectionTimestamp.Add(time.Duration(min(b.cfg.BaseEjectionTime.Nanoseconds()*obj.ejectionTimeMultiplier, max(b.cfg.BaseEjectionTime.Nanoseconds(), b.cfg.MaxEjectionTime.Nanoseconds()))))) {


Even with the use of max and min, this is still too long to follow. Please simplify into multiple statements or temporary variables if possible.

I tried my best. Refactored. Let me know if it looks off still.

dfawley · 2022-07-07T18:35:37Z

xds/internal/balancer/outlierdetection/balancer.go

+	// This conditional only for testing (since the interval timer algorithm is
+	// called manually), will never hit in production.


Maybe the test can set this field manually first, then?

This fixed a race condition where the intervalTimer algorithm would go off at an arbitrary point in the future if not manually calling Stop() here. (Since we call the intervalTimerAlgorithm manually in test (much cleaner than setting field in test, since it'll get written to in UpdateClientConnState(), there would still be a configured timer from the UpdateClientConnState() that would leak if not closed here).

dfawley · 2022-07-07T18:37:42Z

xds/internal/balancer/outlierdetection/balancer.go

+	obj, ok := b.addrs[addr]
+	if !ok { // Shouldn't happen


Can we pass b.addrs[addr] here (and unejectAddress) instead to guarantee this won't happen?

Oh I just realized at the call site you have access to the addrInfo (for addr, addrInfo := range b.addrs. Passed that instead.

easwars · 2022-08-29T18:16:57Z

xds/internal/balancer/outlierdetection/balancer_test.go

+	od, tcc, _ := setup(t)
+	defer internal.UnregisterOutlierDetectionBalancerForTesting()
+
+	// This first config update should a child to be built and forwarded it's


Missing verb between should and a child?

Oh lol, sorry. Added "should".

easwars · 2022-08-29T18:20:01Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		BalancerConfig: &LBConfig{
+			Interval:           10 * time.Second,
+			BaseEjectionTime:   30 * time.Second,
+			MaxEjectionTime:    300 * time.Second,
+			MaxEjectionPercent: 10,
+			SuccessRateEjection: &SuccessRateEjection{
+				StdevFactor:           1900,
+				EnforcementPercentage: 100,
+				MinimumHosts:          5,
+				RequestVolume:         100,
+			},
+			FailurePercentageEjection: &FailurePercentageEjection{
+				Threshold:             85,
+				EnforcementPercentage: 5,
+				MinimumHosts:          5,
+				RequestVolume:         50,
+			},


You could probably reduce some lines in the test by making a variable for the configuration without the child policy config.

var baseCfgWithoutChildConfig = BalancerConfig: &LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, MaxEjectionTime: 300 * time.Second, MaxEjectionPercent: 10, SuccessRateEjection: &SuccessRateEjection{ StdevFactor: 1900, EnforcementPercentage: 100, MinimumHosts: 5, RequestVolume: 100, }, FailurePercentageEjection: &FailurePercentageEjection{ Threshold: 85, EnforcementPercentage: 5, MinimumHosts: 5, RequestVolume: 50, }, }

And change just the child policy config here and down below when calling UpdateClientConnState.

Ummmm sorry I don't like this suggestion, it feels hacky and takes away readability in my opinion, since it's appending to something declared earlier in the funciton (think your comment on the other file about moving var declarations nearer to where they're used). I'll switch it to noop config though, since it doesn't need all the other fields, similar to your comment down below.

easwars · 2022-08-29T18:23:28Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			}
+			scw2, err = bd.ClientConn.NewSubConn([]resolver.Address{{Addr: "address2"}}, balancer.NewSubConnOptions{})
+			if err != nil {
+				t.Fatalf("error in od.NewSubConn call: %v", err)


Just FYI: Calling t.Fatal from goroutines other than the main test goroutine only causes that particular goroutine to exit. The main test goroutine will keep running. So, it is better to call t.Error in these cases, to make it clear to the reader of the test.

Also, the err != nil check is repeated here.

Oh, interesting. I actually did not know that. Switched to error and deleted duplicate err != nil check.

This actually breaks the package documentation: "FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test."

Wait. This isn't created in a new goroutine, this is called in the main test goroutine. The calls down to the child all happen inline from the test goroutine, which then calls this. Switched regardless.

easwars · 2022-08-29T18:28:07Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			Interval:           8 * time.Second,
+			BaseEjectionTime:   30 * time.Second,
+			MaxEjectionTime:    300 * time.Second,
+			MaxEjectionPercent: 10,
+			SuccessRateEjection: &SuccessRateEjection{
+				StdevFactor:           1900,
+				EnforcementPercentage: 100,
+				MinimumHosts:          5,
+				RequestVolume:         100,
+			},
+			FailurePercentageEjection: &FailurePercentageEjection{
+				Threshold:             85,
+				EnforcementPercentage: 5,
+				MinimumHosts:          5,
+				RequestVolume:         50,
+			},
+			ChildPolicy: &internalserviceconfig.BalancerConfig{
+				Name:   t.Name(),
+				Config: emptyChildConfig{},
+			},


Is the complete config required for this test case? Can some fields be omitted?

Good point. Deleted everything but interval and SuccessRateEjection, since that causes it to be a non noop config. For this and the next UpdateClientConnState() below.

easwars · 2022-08-29T18:31:16Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			},
+		},
+		BalancerConfig: &LBConfig{
+			Interval:           1<<63 - 1, // so the interval will never run unless called manually in test.


Use maxInt here as well. Thanks.

Done here and everywhere else.

easwars · 2022-08-29T18:56:12Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+			okAddresses := []resolver.Address{
+				{Addr: addresses[0]},
+				{Addr: addresses[1]},
+			}


Move this definition closer to where it is used.

easwars · 2022-08-29T18:56:42Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+			}
+
+			// The full list of addresses.
+			fullAddresses := []resolver.Address{


easwars · 2022-08-29T18:56:50Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+			defer mr.Close()
+
+			sc := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(test.odscJSON)
+


Nix newline here.

easwars · 2022-08-29T18:58:06Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+	// The addresses which don't return errors.
+	okAddresses := []resolver.Address{
+		{Addr: addresses[0]},
+		{Addr: addresses[1]},
+	}
+
+	// The full list of addresses.
+	fullAddresses := []resolver.Address{
+		{Addr: addresses[0]},
+		{Addr: addresses[1]},
+		{Addr: addresses[2]},
+	}


easwars · 2022-08-29T18:58:18Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+        "childPolicy": [
+		{
+			"round_robin": {}
+		}
+		]


And this one.

Done here and everywhere else.

zasweq

Thanks for the comments :D

zasweq · 2022-08-29T19:50:37Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+//
+// Returns a non-nil error if context deadline expires before RPCs start to get
+// roundrobined across the given backends.
+func checkRoundRobinRPCs(ctx context.Context, client testpb.TestServiceClient, addrs []resolver.Address) error {


Actually, unfortunately this can't. I had to switch the logic for this one, because I start bad backends which return errors that I know will get called. Yours expect the EmptyCall RPC to return a non nil error.

zasweq · 2022-08-29T19:52:32Z

xds/internal/balancer/outlierdetection/balancer_test.go

+	od, tcc, _ := setup(t)
+	defer internal.UnregisterOutlierDetectionBalancerForTesting()
+
+	// This first config update should a child to be built and forwarded it's


Oh lol, sorry. Added "should".

zasweq · 2022-08-29T20:04:32Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			},
+		},
+		BalancerConfig: &LBConfig{
+			Interval:           1<<63 - 1, // so the interval will never run unless called manually in test.


Done here and everywhere else.

zasweq · 2022-08-29T20:06:23Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			},
+		},
+		BalancerConfig: &LBConfig{
+			Interval:           1<<63 - 1, // so the interval will never run unless called manually in test.


Done here and everywhere else.

zasweq · 2022-08-29T20:07:05Z

xds/internal/balancer/outlierdetection/e2e_test/outlierdetection_test.go

+	if err := backend.StartServer(); err != nil {
+		t.Fatalf("Failed to start backend: %v", err)
+	}
+	t.Logf("Started good TestService backend at: %q", backend.Address)


Lol, good point. Switched.

zasweq · 2022-08-29T20:38:25Z

xds/internal/balancer/outlierdetection/balancer_test.go

+		BalancerConfig: &LBConfig{
+			Interval:           10 * time.Second,
+			BaseEjectionTime:   30 * time.Second,
+			MaxEjectionTime:    300 * time.Second,
+			MaxEjectionPercent: 10,
+			SuccessRateEjection: &SuccessRateEjection{
+				StdevFactor:           1900,
+				EnforcementPercentage: 100,
+				MinimumHosts:          5,
+				RequestVolume:         100,
+			},
+			FailurePercentageEjection: &FailurePercentageEjection{
+				Threshold:             85,
+				EnforcementPercentage: 5,
+				MinimumHosts:          5,
+				RequestVolume:         50,
+			},


Ummmm sorry I don't like this suggestion, it feels hacky and takes away readability in my opinion, since it's appending to something declared earlier in the funciton (think your comment on the other file about moving var declarations nearer to where they're used). I'll switch it to noop config though, since it doesn't need all the other fields, similar to your comment down below.

zasweq · 2022-08-29T20:44:43Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			Interval:           8 * time.Second,
+			BaseEjectionTime:   30 * time.Second,
+			MaxEjectionTime:    300 * time.Second,
+			MaxEjectionPercent: 10,
+			SuccessRateEjection: &SuccessRateEjection{
+				StdevFactor:           1900,
+				EnforcementPercentage: 100,
+				MinimumHosts:          5,
+				RequestVolume:         100,
+			},
+			FailurePercentageEjection: &FailurePercentageEjection{
+				Threshold:             85,
+				EnforcementPercentage: 5,
+				MinimumHosts:          5,
+				RequestVolume:         50,
+			},
+			ChildPolicy: &internalserviceconfig.BalancerConfig{
+				Name:   t.Name(),
+				Config: emptyChildConfig{},
+			},


Good point. Deleted everything but interval and SuccessRateEjection, since that causes it to be a non noop config. For this and the next UpdateClientConnState() below.

zasweq · 2022-08-29T20:48:13Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			}
+			scw2, err = bd.ClientConn.NewSubConn([]resolver.Address{{Addr: "address2"}}, balancer.NewSubConnOptions{})
+			if err != nil {
+				t.Fatalf("error in od.NewSubConn call: %v", err)


Oh, interesting. I actually did not know that. Switched to error and deleted duplicate err != nil check.

zasweq · 2022-08-29T20:52:17Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			}
+			scw2, err = bd.ClientConn.NewSubConn([]resolver.Address{{Addr: "address2"}}, balancer.NewSubConnOptions{})
+			if err != nil {
+				t.Fatalf("error in od.NewSubConn call: %v", err)


This actually breaks the package documentation: "FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test."

zasweq · 2022-08-29T20:55:20Z

xds/internal/balancer/outlierdetection/balancer_test.go

+			}
+			scw2, err = bd.ClientConn.NewSubConn([]resolver.Address{{Addr: "address2"}}, balancer.NewSubConnOptions{})
+			if err != nil {
+				t.Fatalf("error in od.NewSubConn call: %v", err)


Wait. This isn't created in a new goroutine, this is called in the main test goroutine. The calls down to the child all happen inline from the test goroutine, which then calls this. Switched regardless.

dfawley · 2022-07-29T21:17:53Z

xds/internal/balancer/outlierdetection/balancer.go

+			pr.Done(di)
+		}
+	}
+	// Shouldn't happen, defensive programming.


That is illegal (though it should be handled gracefully here, of course). SubConn is required to be valid or the channel will re-pick after a new picker is provided. Can you make the tests not set it to nil?

dfawley · 2022-08-29T23:36:06Z

xds/internal/balancer/outlierdetection/balancer.go

+		pickerUpdateCh: buffer.NewUnbounded(),
+	}
+	b.logger = prefixLogger(b)
+	b.logger.Infof("Created")


Should this show the config too?

Umm I wouldn't mind that, but I just triaged the rest of the xDS balancer tree (which I based this on in the first place), and every balancer logs this word exactly. Thus, for the sake of consistency, I would prefer to keep it as such. Of course, if you feel strongly about this I'll go ahead and switch it over.

dfawley · 2022-08-29T23:38:03Z

xds/internal/balancer/outlierdetection/balancer.go

-	return nil
+	b := &outlierDetectionBalancer{
+		cc:             cc,
+		bOpts:          bOpts,


This field seems to be unused?

Good point. Graceful Switch gets constructed with this so we are good here. Removed this field.

dfawley · 2022-08-29T23:44:16Z

xds/internal/balancer/outlierdetection/balancer.go

+		err := b.child.SwitchTo(bb)
+		if err != nil {
+			b.childMu.Unlock()
+			return err


You should wrap this error so there is more context whenever it's printed.

Switched to (pun unintended): return fmt.Errorf("outlier detection: error switching to child of type %q: %v", lbCfg.ChildPolicy.Name, err)

xds/internal/balancer/outlierdetection/balancer.go

xds/internal/balancer/outlierdetection/callcounter.go

zasweq

Thanks for the pass bossman!

xds/internal/balancer/outlierdetection/balancer.go

xds/internal/balancer/outlierdetection/callcounter.go

zasweq · 2022-08-31T23:58:40Z

xds/internal/balancer/outlierdetection/balancer.go

-	return nil
+	b := &outlierDetectionBalancer{
+		cc:             cc,
+		bOpts:          bOpts,


Good point. Graceful Switch gets constructed with this so we are good here. Removed this field.

zasweq · 2022-09-01T00:03:21Z

xds/internal/balancer/outlierdetection/balancer.go

+		pickerUpdateCh: buffer.NewUnbounded(),
+	}
+	b.logger = prefixLogger(b)
+	b.logger.Infof("Created")


Umm I wouldn't mind that, but I just triaged the rest of the xDS balancer tree (which I based this on in the first place), and every balancer logs this word exactly. Thus, for the sake of consistency, I would prefer to keep it as such. Of course, if you feel strongly about this I'll go ahead and switch it over.

zasweq · 2022-09-01T00:17:12Z

xds/internal/balancer/outlierdetection/balancer.go

+		err := b.child.SwitchTo(bb)
+		if err != nil {
+			b.childMu.Unlock()
+			return err


Switched to (pun unintended): return fmt.Errorf("outlier detection: error switching to child of type %q: %v", lbCfg.ChildPolicy.Name, err)

xds/internal/balancer/outlierdetection/balancer.go

dfawley · 2022-09-01T22:21:15Z

xds/internal/balancer/outlierdetection/balancer.go

+	// childMu protects the closing of the child and also guarantees updates to
+	// the child are sent synchronously (to uphold the balancer.Balancer API
+	// guarantee of synchronous calls).
+	//
+	// For example, run() could read that the child is not nil while processing
+	// SubConn updates, and then Close() could write to the the child, clearing
+	// the child, making it nil, then you try and update a cleared and already
+	// closed child, which breaks the balancer.Balancer API.


How about:

// childMu guards calls into child and also synchronizes reads of child in run() and the write in Close().

If you make sure b.run() exits before calling b.child.Close(), though, you don't need that last part of the sentence.

Yup, waited for b.run() to exit. Changed docstring to just the first part of that sentence.

Also removed the clearing of the child as an invariant of close, and the reading of the child (determining whether it's been cleared or not).

dfawley

🎉

zasweq mentioned this pull request Jun 17, 2022

xds: add Outlier Detection Balancer #5413

Closed

zasweq requested review from easwars and dfawley June 17, 2022 01:52

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

zasweq added this to the 1.48 Release milestone Jun 17, 2022

zasweq added the Type: Feature New features or improvements in behavior label Jun 17, 2022

zasweq force-pushed the od-bal-rebased-again branch from 6a1f44b to b364c84 Compare June 17, 2022 06:17

zasweq force-pushed the od-bal-rebased-again branch from b364c84 to 0c2ef7c Compare June 20, 2022 04:47

zasweq force-pushed the od-bal-rebased-again branch 2 times, most recently from aeb3c19 to d8e42c2 Compare July 1, 2022 22:32

dfawley modified the milestones: 1.48 Release, 1.49 Release Jul 1, 2022

easwars reviewed Jul 6, 2022

View reviewed changes

easwars assigned zasweq and unassigned easwars Jul 6, 2022

zasweq commented Jul 6, 2022

View reviewed changes

zasweq assigned easwars and dfawley and unassigned dfawley and zasweq Jul 6, 2022

zasweq and others added 3 commits July 7, 2022 12:21

Added Outlier Detection Balancer

1dd14ce

e2e test and bug fix

32ea80a

Responded to Easwar's comments

73528ce

zasweq force-pushed the od-bal-rebased-again branch from c688b0d to 73528ce Compare July 7, 2022 16:28

dfawley reviewed Jul 7, 2022

View reviewed changes

zasweq removed their assignment Aug 27, 2022

easwars reviewed Aug 29, 2022

View reviewed changes

easwars assigned zasweq and unassigned easwars and dfawley Aug 29, 2022

zasweq commented Aug 29, 2022

View reviewed changes

Responded to Easwar's comments

7fc669c

zasweq assigned easwars and dfawley and unassigned zasweq Aug 29, 2022

easwars approved these changes Aug 29, 2022

View reviewed changes

easwars removed their assignment Aug 29, 2022

dfawley reviewed Aug 31, 2022

View reviewed changes

zasweq commented Sep 1, 2022

View reviewed changes

Responded to Doug's comments

414dba1

dfawley assigned zasweq and unassigned dfawley Sep 1, 2022

dfawley reviewed Sep 1, 2022

View reviewed changes

zasweq added 2 commits September 1, 2022 19:25

Wait for run() to exit in Close() using Wait Group

8c143d1

Got rid of the clearing of child and checking of cleared child

bdba12a

zasweq assigned dfawley and unassigned zasweq Sep 1, 2022

Switched done from wait group to grpcsync.Event

5482af7

dfawley approved these changes Sep 6, 2022

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Sep 6, 2022

zasweq merged commit f7d2036 into grpc:master Sep 6, 2022

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

moul-bot mentioned this pull request Oct 17, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.1 moul/depviz#586

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2023


		}

		func (b *outlierDetectionBalancer) successRateAlgorithm() {

		// This conditional only for testing (since the interval timer algorithm is
		// called manually), will never hit in production.

		defer mr.Close()

		sc := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(test.odscJSON)

xds: add Outlier Detection Balancer #5435

xds: add Outlier Detection Balancer #5435

Conversation

zasweq commented Jun 17, 2022 • edited Loading

dfawley commented Jun 17, 2022

easwars 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

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

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

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 17, 2022 •

edited

Loading