grpclb: recover after receiving an empty server list #4879
Conversation
|
The root cause is SubConn not handling empty addr list correctly, filed #4881 |
balancer/grpclb/grpclb_test.go
Outdated
| }} | ||
|
|
||
| creds := serverNameCheckCreds{} | ||
| ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) |
There was a problem hiding this comment.
We now have a defaultTestTimeout const in this file. You can use that instead.
balancer/grpclb/grpclb_test.go
Outdated
| for i := 0; i < 100 && !gotUnavailable; i++ { | ||
| func() { | ||
| ctx, cancel := context.WithTimeout(ctx, time.Second*1) | ||
| defer cancel() | ||
| _, err := testC.EmptyCall(ctx, &testpb.Empty{}) | ||
| if status.Code(err) == codes.Unavailable { | ||
| gotUnavailable = true | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Could we check clientConn state instead of this? Are we sure this will not be flaky?
There was a problem hiding this comment.
This is copied from Alex's reproduction code.
I don't think this will be flaky. But I simplified the test a bit.
| break | ||
| } | ||
| if sc != nil { | ||
| if len(backendAddrs) == 0 { | ||
| lb.cc.cc.RemoveSubConn(sc) |
There was a problem hiding this comment.
Is it better to handle this UpdateAddresses() so that every balancer need not handle this?
There was a problem hiding this comment.
Yes. That's what #4881 is tracking.
menghanl
force-pushed
the
grpclb_empty_address
branch
from
6a60fa9 to
db2b3bc
Compare
balancer/grpclb/grpclb_test.go
Outdated
| creds := serverNameCheckCreds{} | ||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
| cc, err := grpc.DialContext(ctx, r.Scheme()+":///"+beServerName, grpc.WithResolvers(r), |
There was a problem hiding this comment.
Minor nit: Could we not split these lines in a random way. In a recent PR, I cleaned up this test to instead do
cc, err := grpc.Dial(r.Scheme()+":///"+beServerName,
grpc.WithResolvers(r),
grpc.WithTransportCredentials(&serverNameCheckCreds{}),
grpc.WithContextDialer(fakeNameDialer))
if err != nil {
t.Fatalf("Failed to dial to the backend %v", err)
}which is a tad more readable I think.
balancer/grpclb/grpclb_test.go
Outdated
| r.UpdateState(resolver.State{ | ||
| Addresses: []resolver.Address{{ | ||
| Addr: tss.lbAddr, | ||
| Type: resolver.GRPCLB, | ||
| ServerName: lbServerName, | ||
| }}, | ||
| ServiceConfig: scpr, | ||
| }) |
There was a problem hiding this comment.
Nit: similarly I cleaned up calls to UpdateState to include balancer addresses through an attribute instead of the deprecated Type field.
rs = grpclbstate.Set(resolver.State{ServiceConfig: r.CC.ParseServiceConfig(grpclbConfig)},
&grpclbstate.State{BalancerAddresses: []resolver.Address{{
Addr: tss.lbAddr,
ServerName: lbServerName,
}}})
r.UpdateState(rs)
When grpclb receives an empty address list, it calls
sc.UpdateAddress([]), which closes the underlyingaddrConn. It won't recover if a new addr list is received later.RELEASE NOTES: