grpclb: improve grpclb tests #5826
Conversation
|
Is there a tl;dr description you can provide about the race(s) that were making it flaky before these changes? |
balancer/grpclb/grpclb_test.go
Outdated
| // TestGRPCLB_Basic tests the basic case of a channel being configured with | ||
| // grpclb as the load balancing policy. | ||
| func (s) TestGRPCLB_Basic(t *testing.T) { | ||
| // Start a test backend and the remote balancer. |
There was a problem hiding this comment.
This comment is redundant with the function name being called. Consider instead a t.Log in startBackendsAndRemoveLoadBalancer (optional), or just delete.
balancer/grpclb/grpclb_test.go
Outdated
| }, | ||
| }, | ||
| } | ||
| rs := state.Set(resolver.State{ServiceConfig: r.CC.ParseServiceConfig(grpclbConfig)}, s) | ||
| r.UpdateState(rs) |
There was a problem hiding this comment.
Should we be using r.InitialState instead before dialing?
| @@ -36,11 +36,13 @@ import ( | |||
|
|
|||
| "google.golang.org/grpc" | |||
| "google.golang.org/grpc/balancer" | |||
| grpclbstate "google.golang.org/grpc/balancer/grpclb/state" | |||
There was a problem hiding this comment.
This was being renamed because state is such a common name to want to use as a variable, and it's also not descriptive enough at the call sites ("state.Set: what's that?"). Can we change this back? Or rename the package itself to grpclbstate?
There was a problem hiding this comment.
Done. Renamed the package.
The main reason for the flakiness was the logic used to determine whether RPCs were reaching expected backends. The logic used was mostly ad-hoc, and included just waiting for 1 or 2 seconds for the expected distribution. Over time, we have added some routines in |
- rename `state` package to `grpclbstate` - add test logs for setup - use InitialState() on the manual resolver wherever possible
easwars
force-pushed
the
grpclb_flaky_tests
branch
from
7242574 to
86b59e8
Compare
There is more cleanup that can be done on these tests, but given that we don't care that much about grpclb, I had to stop here. I was able to get it down to
1failure in100Kacross all tests in this package, and that too looked like some infra flake where a backend did not spin up properly.Fixes #4392.
RELEASE NOTES: n/a