From f577cc4039b79effa922158127a935b4789bfefc Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 26 Jun 2019 13:09:56 -0700 Subject: [PATCH 01/14] resolver: add State fields to support error handling --- balancer/balancer.go | 13 ++- balancer/base/balancer.go | 9 ++- balancer/grpclb/grpclb.go | 18 +++-- balancer/grpclb/grpclb_test.go | 22 +++--- balancer_conn_wrappers.go | 63 ++++++++------- balancer_switching_test.go | 30 +++---- clientconn.go | 126 ++++++++++++++++++++---------- clientconn_test.go | 25 +++--- internal/internal.go | 3 - resolver/dns/dns_resolver_test.go | 9 +++ resolver/manual/manual.go | 8 +- resolver/resolver.go | 18 +++-- resolver_conn_wrapper.go | 90 ++++++++++++++++++--- service_config.go | 29 +++---- service_config_test.go | 5 +- serviceconfig/serviceconfig.go | 40 +++++++--- test/channelz_test.go | 14 ++-- test/end2end_test.go | 55 +++++++------ test/healthcheck_test.go | 36 ++++----- xds/internal/balancer/xds.go | 9 ++- 20 files changed, 396 insertions(+), 226 deletions(-) diff --git a/balancer/balancer.go b/balancer/balancer.go index c266f4ec102..917c242f8a9 100644 --- a/balancer/balancer.go +++ b/balancer/balancer.go @@ -305,14 +305,23 @@ type ClientConnState struct { BalancerConfig serviceconfig.LoadBalancingConfig } +// ErrBadResolverState may be returned by UpdateClientConnState to indicate a +// problem with the provided name resolver data. +var ErrBadResolverState = errors.New("bad resolver state") + // V2Balancer is defined for documentation purposes. If a Balancer also // implements V2Balancer, its UpdateClientConnState method will be called // instead of HandleResolvedAddrs and its UpdateSubConnState will be called // instead of HandleSubConnStateChange. type V2Balancer interface { // UpdateClientConnState is called by gRPC when the state of the ClientConn - // changes. - UpdateClientConnState(ClientConnState) + // changes. If the error returned is ErrBadResolverState, the ClientConn + // will begin calling ResolveNow on the active name resolver with + // exponential backoff until a subsequent call to UpdateClientConnState + // returns a nil error. Any other errors are currently ignored. + UpdateClientConnState(ClientConnState) error + // ResolverError is called by gRPC when the name resolver reports an error. + ResolverError(error) // UpdateSubConnState is called by gRPC when the state of a SubConn // changes. UpdateSubConnState(SubConn, SubConnState) diff --git a/balancer/base/balancer.go b/balancer/base/balancer.go index 1af88f0a3f1..1a5c1aa7e90 100644 --- a/balancer/base/balancer.go +++ b/balancer/base/balancer.go @@ -53,6 +53,8 @@ func (bb *baseBuilder) Name() string { return bb.name } +var _ balancer.V2Balancer = (*baseBalancer)(nil) // Assert that we implement V2Balancer + type baseBalancer struct { cc balancer.ClientConn pickerBuilder PickerBuilder @@ -70,7 +72,11 @@ func (b *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) panic("not implemented") } -func (b *baseBalancer) UpdateClientConnState(s balancer.ClientConnState) { +func (b *baseBalancer) ResolverError(error) { + // Ignore +} + +func (b *baseBalancer) UpdateClientConnState(s balancer.ClientConnState) error { // TODO: handle s.ResolverState.Err (log if not nil) once implemented. // TODO: handle s.ResolverState.ServiceConfig? if grpclog.V(2) { @@ -101,6 +107,7 @@ func (b *baseBalancer) UpdateClientConnState(s balancer.ClientConnState) { // The entry will be deleted in HandleSubConnStateChange. } } + return nil } // regeneratePicker takes a snapshot of the balancer, and generates a picker diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index 058f206cf7b..708b89c0fcb 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -161,6 +161,8 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal return lb } +var _ balancer.V2Balancer = (*lbBalancer)(nil) // Assert that we implement V2Balancer + type lbBalancer struct { cc *lbCacheClientConn target string @@ -410,7 +412,12 @@ func (lb *lbBalancer) handleServiceConfig(gc *grpclbServiceConfig) { lb.refreshSubConns(lb.backendAddrs, lb.inFallback, newUsePickFirst) } -func (lb *lbBalancer) UpdateClientConnState(ccs balancer.ClientConnState) { +func (lb *lbBalancer) ResolverError(error) { + // Ignore resolver errors. GRPCLB is not selected unless the resolver + // works at least once. +} + +func (lb *lbBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error { if grpclog.V(2) { grpclog.Infof("lbBalancer: UpdateClientConnState: %+v", ccs) } @@ -418,8 +425,8 @@ func (lb *lbBalancer) UpdateClientConnState(ccs balancer.ClientConnState) { lb.handleServiceConfig(gc) addrs := ccs.ResolverState.Addresses - if len(addrs) <= 0 { - return + if len(addrs) == 0 { + return balancer.ErrBadResolverState } var remoteBalancerAddrs, backendAddrs []resolver.Address @@ -433,9 +440,9 @@ func (lb *lbBalancer) UpdateClientConnState(ccs balancer.ClientConnState) { } if lb.ccRemoteLB == nil { - if len(remoteBalancerAddrs) <= 0 { + if len(remoteBalancerAddrs) == 0 { grpclog.Errorf("grpclb: no remote balancer address is available, should never happen") - return + return balancer.ErrBadResolverState } // First time receiving resolved addresses, create a cc to remote // balancers. @@ -457,6 +464,7 @@ func (lb *lbBalancer) UpdateClientConnState(ccs balancer.ClientConnState) { lb.refreshSubConns(lb.resolvedBackendAddrs, true, lb.usePickFirst) } lb.mu.Unlock() + return nil } func (lb *lbBalancer) Close() { diff --git a/balancer/grpclb/grpclb_test.go b/balancer/grpclb/grpclb_test.go index 278ca91a09c..9ced7240bcc 100644 --- a/balancer/grpclb/grpclb_test.go +++ b/balancer/grpclb/grpclb_test.go @@ -44,7 +44,6 @@ import ( "google.golang.org/grpc/peer" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" - "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/status" testpb "google.golang.org/grpc/test/grpc_testing" ) @@ -853,12 +852,6 @@ func TestFallback(t *testing.T) { } func TestGRPCLBPickFirst(t *testing.T) { - const pfc = `{"loadBalancingConfig":[{"grpclb":{"childPolicy":[{"pick_first":{}}]}}]}` - svcCfg, err := serviceconfig.Parse(pfc) - if err != nil { - t.Fatalf("Error parsing config %q: %v", pfc, err) - } - defer leakcheck.Check(t) r, cleanup := manual.GenerateAndRegisterManualResolver() @@ -908,6 +901,11 @@ func TestGRPCLBPickFirst(t *testing.T) { tss.ls.sls <- &lbpb.ServerList{Servers: beServers[0:3]} // Start with sub policy pick_first. + const pfc = `{"loadBalancingConfig":[{"grpclb":{"childPolicy":[{"pick_first":{}}]}}]}` + svcCfg := r.CC.ParseServiceConfig(pfc) + if _, err := svcCfg.Get(); err != nil { + t.Fatalf("Error parsing config %q: %v", pfc, err) + } r.UpdateState(resolver.State{ Addresses: []resolver.Address{{ @@ -915,7 +913,7 @@ func TestGRPCLBPickFirst(t *testing.T) { Type: resolver.GRPCLB, ServerName: lbServerName, }}, - ServiceConfig: svcCfg, + ServiceConfigGetter: svcCfg, }) result = "" @@ -954,9 +952,9 @@ func TestGRPCLBPickFirst(t *testing.T) { } // Switch sub policy to roundrobin. - grpclbServiceConfigEmpty, err := serviceconfig.Parse(`{}`) - if err != nil { - t.Fatalf("Error parsing config %q: %v", grpclbServiceConfigEmpty, err) + grpclbServiceConfigEmpty := r.CC.ParseServiceConfig(`{}`) + if _, err := grpclbServiceConfigEmpty.Get(); err != nil { + t.Fatalf("Error parsing config %q: %v", `{}`, err) } r.UpdateState(resolver.State{ @@ -965,7 +963,7 @@ func TestGRPCLBPickFirst(t *testing.T) { Type: resolver.GRPCLB, ServerName: lbServerName, }}, - ServiceConfig: grpclbServiceConfigEmpty, + ServiceConfigGetter: grpclbServiceConfigEmpty, }) result = "" diff --git a/balancer_conn_wrappers.go b/balancer_conn_wrappers.go index 8df4095ca95..42f0ab68291 100644 --- a/balancer_conn_wrappers.go +++ b/balancer_conn_wrappers.go @@ -25,6 +25,7 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/resolver" ) @@ -86,10 +87,10 @@ func (b *scStateUpdateBuffer) get() <-chan *scStateUpdate { // It implements balancer.ClientConn interface. type ccBalancerWrapper struct { cc *ClientConn + balancerMu sync.Mutex balancer balancer.Balancer stateChangeQueue *scStateUpdateBuffer - ccUpdateCh chan *balancer.ClientConnState - done chan struct{} + done *grpcsync.Event mu sync.Mutex subConns map[*acBalancerWrapper]struct{} @@ -99,8 +100,7 @@ func newCCBalancerWrapper(cc *ClientConn, b balancer.Builder, bopts balancer.Bui ccb := &ccBalancerWrapper{ cc: cc, stateChangeQueue: newSCStateUpdateBuffer(), - ccUpdateCh: make(chan *balancer.ClientConnState, 1), - done: make(chan struct{}), + done: grpcsync.NewEvent(), subConns: make(map[*acBalancerWrapper]struct{}), } go ccb.watcher() @@ -115,34 +115,20 @@ func (ccb *ccBalancerWrapper) watcher() { select { case t := <-ccb.stateChangeQueue.get(): ccb.stateChangeQueue.load() - select { - case <-ccb.done: - ccb.balancer.Close() - return - default: + if ccb.done.HasFired() { + break } + ccb.balancerMu.Lock() if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { ub.UpdateSubConnState(t.sc, balancer.SubConnState{ConnectivityState: t.state}) } else { ccb.balancer.HandleSubConnStateChange(t.sc, t.state) } - case s := <-ccb.ccUpdateCh: - select { - case <-ccb.done: - ccb.balancer.Close() - return - default: - } - if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { - ub.UpdateClientConnState(*s) - } else { - ccb.balancer.HandleResolvedAddrs(s.ResolverState.Addresses, nil) - } - case <-ccb.done: + ccb.balancerMu.Unlock() + case <-ccb.done.Done(): } - select { - case <-ccb.done: + if ccb.done.HasFired() { ccb.balancer.Close() ccb.mu.Lock() scs := ccb.subConns @@ -153,14 +139,13 @@ func (ccb *ccBalancerWrapper) watcher() { } ccb.UpdateBalancerState(connectivity.Connecting, nil) return - default: } ccb.cc.firstResolveEvent.Fire() } } func (ccb *ccBalancerWrapper) close() { - close(ccb.done) + ccb.done.Fire() } func (ccb *ccBalancerWrapper) handleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { @@ -180,8 +165,11 @@ func (ccb *ccBalancerWrapper) handleSubConnStateChange(sc balancer.SubConn, s co }) } -func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) { - if ccb.cc.curBalancerName != grpclbName { +func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) error { + cbn := ccb.cc.curBalancerName + ccb.cc.mu.Unlock() + defer ccb.cc.mu.Lock() + if cbn != grpclbName { // Filter any grpclb addresses since we don't have the grpclb balancer. s := &ccs.ResolverState for i := 0; i < len(s.Addresses); { @@ -193,11 +181,22 @@ func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnStat i++ } } - select { - case <-ccb.ccUpdateCh: - default: + + ccb.balancerMu.Lock() + defer ccb.balancerMu.Unlock() + if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { + return ub.UpdateClientConnState(*ccs) + } + ccb.balancer.HandleResolvedAddrs(ccs.ResolverState.Addresses, nil) + return nil +} + +func (ccb *ccBalancerWrapper) resolverError(err error) { + if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { + ccb.balancerMu.Lock() + ub.ResolverError(err) + ccb.balancerMu.Unlock() } - ccb.ccUpdateCh <- ccs } func (ccb *ccBalancerWrapper) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) { diff --git a/balancer_switching_test.go b/balancer_switching_test.go index 638139ab4c2..903e4732fe7 100644 --- a/balancer_switching_test.go +++ b/balancer_switching_test.go @@ -149,12 +149,12 @@ func (s) TestSwitchBalancer(t *testing.T) { t.Fatalf("check pickfirst returned non-nil error: %v", err) } // Switch to roundrobin. - cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}) + cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) if err := checkRoundRobin(cc, servers); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) } // Switch to pickfirst. - cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}) + cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) if err := checkPickFirst(cc, servers); err != nil { t.Fatalf("check pickfirst returned non-nil error: %v", err) } @@ -181,7 +181,7 @@ func (s) TestBalancerDialOption(t *testing.T) { t.Fatalf("check roundrobin returned non-nil error: %v", err) } // Switch to pickfirst. - cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}) + cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) // Balancer is still roundrobin. if err := checkRoundRobin(cc, servers); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) @@ -337,9 +337,9 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { } defer cc.Close() - sc := parseCfg(`{"loadBalancingPolicy": "round_robin"}`) + sc := parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`) - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) var isRoundRobin bool for i := 0; i < 5000; i++ { cc.mu.Lock() @@ -356,7 +356,7 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { // ClientConn will switch balancer to grpclb when receives an address of // type GRPCLB. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "grpclb", Type: resolver.GRPCLB}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "grpclb", Type: resolver.GRPCLB}}, ServiceConfigGetter: sc}) var isGRPCLB bool for i := 0; i < 5000; i++ { cc.mu.Lock() @@ -372,7 +372,7 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { } // Switch balancer back. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) for i := 0; i < 5000; i++ { cc.mu.Lock() isRoundRobin = cc.curBalancerName == "round_robin" @@ -433,8 +433,8 @@ func (s) TestSwitchBalancerGRPCLBServiceConfig(t *testing.T) { t.Fatalf("after 5 second, cc.balancer is of type %v, not grpclb", cc.curBalancerName) } - sc := parseCfg(`{"loadBalancingPolicy": "round_robin"}`) - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: sc}) + sc := parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: sc}) var isRoundRobin bool for i := 0; i < 200; i++ { cc.mu.Lock() @@ -452,7 +452,7 @@ func (s) TestSwitchBalancerGRPCLBServiceConfig(t *testing.T) { } // Switch balancer back. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) for i := 0; i < 5000; i++ { cc.mu.Lock() isRoundRobin = cc.curBalancerName == "round_robin" @@ -510,16 +510,16 @@ func (s) TestSwitchBalancerGRPCLBWithGRPCLBNotRegistered(t *testing.T) { t.Fatalf("check pickfirst returned non-nil error: %v", err) } // Switch to roundrobin, and check against server[1] and server[2]. - cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}) + cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) if err := checkRoundRobin(cc, servers[1:]); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) } } -func parseCfg(s string) serviceconfig.Config { - c, err := serviceconfig.Parse(s) - if err != nil { +func parseCfg(r *manual.Resolver, s string) *serviceconfig.Getter { + g := r.CC.ParseServiceConfig(s) + if _, err := g.Get(); err != nil { panic(fmt.Sprintf("Error parsing config %q: %v", s, err)) } - return c + return g } diff --git a/clientconn.go b/clientconn.go index d4fadfd68bb..7041dce2564 100644 --- a/clientconn.go +++ b/clientconn.go @@ -186,11 +186,11 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } if cc.dopts.defaultServiceConfigRawJSON != nil { - sc, err := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON) + cfg, err := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON).Get() if err != nil { return nil, fmt.Errorf("%s: %v", invalidDefaultServiceConfigErrPrefix, err) } - cc.dopts.defaultServiceConfig = sc + cc.dopts.defaultServiceConfig, _ = cfg.(*ServiceConfig) } cc.mkp = cc.dopts.copts.KeepaliveParams @@ -530,7 +530,29 @@ func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error { } } -func (cc *ClientConn) updateResolverState(s resolver.State) error { +var emptyServiceConfig *ServiceConfig + +func init() { + cfg, err := parseServiceConfig("{}").Get() + if err != nil { + panic(fmt.Sprintf("impossible error parsing empty service config: %v", err)) + } + emptyServiceConfig = cfg.(*ServiceConfig) +} + +func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { + if cc.sc != nil { + cc.applyServiceConfig(cc.sc, addrs) + return + } + if cc.dopts.defaultServiceConfig != nil { + cc.applyServiceConfig(cc.dopts.defaultServiceConfig, addrs) + } else { + cc.applyServiceConfig(emptyServiceConfig, addrs) + } +} + +func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { cc.mu.Lock() defer cc.mu.Unlock() // Check if the ClientConn is already closed. Some fields (e.g. @@ -540,48 +562,42 @@ func (cc *ClientConn) updateResolverState(s resolver.State) error { return nil } - if cc.dopts.disableServiceConfig || s.ServiceConfig == nil { - if cc.dopts.defaultServiceConfig != nil && cc.sc == nil { - cc.applyServiceConfig(cc.dopts.defaultServiceConfig) + if err != nil { + // May need to apply the initial service config + cc.maybeApplyDefaultServiceConfig(nil) + + if cc.balancerWrapper != nil { + cc.balancerWrapper.resolverError(err) } - } else if sc, ok := s.ServiceConfig.(*ServiceConfig); ok { - cc.applyServiceConfig(sc) + + // No addresses are valid with err set; return early. + return balancer.ErrBadResolverState } - var balCfg serviceconfig.LoadBalancingConfig - if cc.dopts.balancerBuilder == nil { - // Only look at balancer types and switch balancer if balancer dial - // option is not set. - var newBalancerName string - if cc.sc != nil && cc.sc.lbConfig != nil { - newBalancerName = cc.sc.lbConfig.name - balCfg = cc.sc.lbConfig.cfg + var ret error + if cc.dopts.disableServiceConfig || s.ServiceConfigGetter == nil { + cc.maybeApplyDefaultServiceConfig(s.Addresses) + // TODO: do we need to apply a failing LB policy if there is no + // default, per the error handling design? + } else { + scg, err := s.ServiceConfigGetter.Get() + if sc, ok := scg.(*ServiceConfig); err == nil && ok { + cc.applyServiceConfig(sc, s.Addresses) } else { - var isGRPCLB bool - for _, a := range s.Addresses { - if a.Type == resolver.GRPCLB { - isGRPCLB = true - break - } - } - if isGRPCLB { - newBalancerName = grpclbName - } else if cc.sc != nil && cc.sc.LB != nil { - newBalancerName = *cc.sc.LB - } else { - newBalancerName = PickFirstBalancerName - } + ret = balancer.ErrBadResolverState } - cc.switchBalancer(newBalancerName) - } else if cc.balancerWrapper == nil { - // Balancer dial option was set, and this is the first time handling - // resolved addresses. Build a balancer with dopts.balancerBuilder. - cc.curBalancerName = cc.dopts.balancerBuilder.Name() - cc.balancerWrapper = newCCBalancerWrapper(cc, cc.dopts.balancerBuilder, cc.balancerBuildOpts) } - cc.balancerWrapper.updateClientConnState(&balancer.ClientConnState{ResolverState: s, BalancerConfig: balCfg}) - return nil + var balCfg serviceconfig.LoadBalancingConfig + if cc.dopts.balancerBuilder == nil && cc.sc != nil && cc.sc.lbConfig != nil { + balCfg = cc.sc.lbConfig.cfg + } + uccsErr := cc.balancerWrapper.updateClientConnState(&balancer.ClientConnState{ResolverState: s, BalancerConfig: balCfg}) + if ret == nil { + ret = uccsErr // prefer ErrBadResolver state since any other error is + // currently meaningless to the caller. + } + return ret } // switchBalancer starts the switching from current balancer to the balancer @@ -829,10 +845,10 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st return t, done, nil } -func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig) error { +func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, addrs []resolver.Address) { if sc == nil { // should never reach here. - return fmt.Errorf("got nil pointer for service config") + return } cc.sc = sc @@ -848,7 +864,35 @@ func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig) error { cc.retryThrottler.Store((*retryThrottler)(nil)) } - return nil + if cc.dopts.balancerBuilder == nil { + // Only look at balancer types and switch balancer if balancer dial + // option is not set. + var newBalancerName string + if cc.sc != nil && cc.sc.lbConfig != nil { + newBalancerName = cc.sc.lbConfig.name + } else { + var isGRPCLB bool + for _, a := range addrs { + if a.Type == resolver.GRPCLB { + isGRPCLB = true + break + } + } + if isGRPCLB { + newBalancerName = grpclbName + } else if cc.sc != nil && cc.sc.LB != nil { + newBalancerName = *cc.sc.LB + } else { + newBalancerName = PickFirstBalancerName + } + } + cc.switchBalancer(newBalancerName) + } else if cc.balancerWrapper == nil { + // Balancer dial option was set, and this is the first time handling + // resolved addresses. Build a balancer with dopts.balancerBuilder. + cc.curBalancerName = cc.dopts.balancerBuilder.Name() + cc.balancerWrapper = newCCBalancerWrapper(cc, cc.dopts.balancerBuilder, cc.balancerBuildOpts) + } } func (cc *ClientConn) resolveNow(o resolver.ResolveNowOption) { diff --git a/clientconn_test.go b/clientconn_test.go index 421aa499d91..bae6d75c04c 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -780,7 +780,7 @@ func (s) TestResolverServiceConfigBeforeAddressNotPanic(t *testing.T) { // SwitchBalancer before NewAddress. There was no balancer created, this // makes sure we don't call close on nil balancerWrapper. - r.UpdateState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. + r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. time.Sleep(time.Second) // Sleep to make sure the service config is handled by ClientConn. } @@ -796,7 +796,7 @@ func (s) TestResolverServiceConfigWhileClosingNotPanic(t *testing.T) { } // Send a new service config while closing the ClientConn. go cc.Close() - go r.UpdateState(resolver.State{ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. + go r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. } } @@ -889,7 +889,7 @@ func (s) TestDisableServiceConfigOption(t *testing.T) { t.Fatalf("Dial(%s, _) = _, %v, want _, ", addr, err) } defer cc.Close() - r.UpdateState(resolver.State{ServiceConfig: parseCfg(`{ + r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1181,29 +1181,29 @@ func testInvalidDefaultServiceConfig(t *testing.T) { } } -func testDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T, r resolver.Resolver, addr string, js string) { +func testDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T, r *manual.Resolver, addr string, js string) { cc, err := Dial(addr, WithInsecure(), WithDisableServiceConfig(), WithDefaultServiceConfig(js)) if err != nil { t.Fatalf("Dial(%s, _) = _, %v, want _, ", addr, err) } defer cc.Close() // Resolver service config gets ignored since resolver service config is disabled. - r.(*manual.Resolver).UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfig: parseCfg("{}"), + r.UpdateState(resolver.State{ + Addresses: []resolver.Address{{Addr: addr}}, + ServiceConfigGetter: parseCfg(r, "{}"), }) if !verifyWaitForReadyEqualsTrue(cc) { t.Fatal("default service config failed to be applied after 1s") } } -func testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t *testing.T, r resolver.Resolver, addr string, js string) { +func testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t *testing.T, r *manual.Resolver, addr string, js string) { cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(js)) if err != nil { t.Fatalf("Dial(%s, _) = _, %v, want _, ", addr, err) } defer cc.Close() - r.(*manual.Resolver).UpdateState(resolver.State{ + r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, }) if !verifyWaitForReadyEqualsTrue(cc) { @@ -1211,15 +1211,14 @@ func testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t *testing.T } } -func testDefaultServiceConfigWhenResolverReturnInvalidServiceConfig(t *testing.T, r resolver.Resolver, addr string, js string) { +func testDefaultServiceConfigWhenResolverReturnInvalidServiceConfig(t *testing.T, r *manual.Resolver, addr string, js string) { cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(js)) if err != nil { t.Fatalf("Dial(%s, _) = _, %v, want _, ", addr, err) } defer cc.Close() - r.(*manual.Resolver).UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfig: nil, + r.UpdateState(resolver.State{ + Addresses: []resolver.Address{{Addr: addr}}, }) if !verifyWaitForReadyEqualsTrue(cc) { t.Fatal("default service config failed to be applied after 1s") diff --git a/internal/internal.go b/internal/internal.go index e535a5ff91e..82bae7c787d 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -39,9 +39,6 @@ var ( // KeepaliveMinPingTime is the minimum ping interval. This must be 10s by // default, but tests may wish to set it lower for convenience. KeepaliveMinPingTime = 10 * time.Second - // ParseServiceConfig is a function to parse JSON service configs into - // opaque data structures. - ParseServiceConfig func(sc string) (interface{}, error) // StatusRawProto is exported by status/status.go. This func returns a // pointer to the wrapped Status proto for a given status.Status without a // call to proto.Clone(). The returned Status proto should not be mutated by diff --git a/resolver/dns/dns_resolver_test.go b/resolver/dns/dns_resolver_test.go index b03486b11d7..ac0ab27968a 100644 --- a/resolver/dns/dns_resolver_test.go +++ b/resolver/dns/dns_resolver_test.go @@ -31,6 +31,7 @@ import ( "google.golang.org/grpc/internal/leakcheck" "google.golang.org/grpc/resolver" + "google.golang.org/grpc/serviceconfig" ) func TestMain(m *testing.M) { @@ -89,6 +90,14 @@ func (t *testClientConn) getSc() (string, int) { return t.sc, t.s } +func (t *testClientConn) ParseServiceConfig(string) *serviceconfig.Getter { + panic("not implemented") +} + +func (t *testClientConn) ReportError(error) { + panic("not implemented") +} + type testResolver struct { // A write to this channel is made when this resolver receives a resolution // request. Tests can rely on reading from this channel to be notified about diff --git a/resolver/manual/manual.go b/resolver/manual/manual.go index 85a4abe61b2..6b8d0e8a19c 100644 --- a/resolver/manual/manual.go +++ b/resolver/manual/manual.go @@ -40,7 +40,7 @@ type Resolver struct { scheme string // Fields actually belong to the resolver. - cc resolver.ClientConn + CC resolver.ClientConn bootstrapState *resolver.State } @@ -52,7 +52,7 @@ func (r *Resolver) InitialState(s resolver.State) { // Build returns itself for Resolver, because it's both a builder and a resolver. func (r *Resolver) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOption) (resolver.Resolver, error) { - r.cc = cc + r.CC = cc if r.bootstrapState != nil { r.UpdateState(*r.bootstrapState) } @@ -70,9 +70,9 @@ func (*Resolver) ResolveNow(o resolver.ResolveNowOption) {} // Close is a noop for Resolver. func (*Resolver) Close() {} -// UpdateState calls cc.UpdateState. +// UpdateState calls CC.UpdateState. func (r *Resolver) UpdateState(s resolver.State) { - r.cc.UpdateState(s) + r.CC.UpdateState(s) } // GenerateAndRegisterManualResolver generates a random scheme and a Resolver diff --git a/resolver/resolver.go b/resolver/resolver.go index e83da346a5c..5489738e20f 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -104,12 +104,13 @@ type BuildOption struct { // State contains the current Resolver state relevant to the ClientConn. type State struct { - Addresses []Address // Resolved addresses for the target - // ServiceConfig is the parsed service config; obtained from - // serviceconfig.Parse. - ServiceConfig serviceconfig.Config + // Addresses is the latest set of resolved addresses for the target. + Addresses []Address - // TODO: add Err error + // ServiceConfigGetter contains the result from parsing the latest service + // config. If it is nil, it indicates no service config is present or the + // resolver does not provide service configs. + ServiceConfigGetter *serviceconfig.Getter } // ClientConn contains the callbacks for resolver to notify any updates @@ -122,6 +123,10 @@ type State struct { type ClientConn interface { // UpdateState updates the state of the ClientConn appropriately. UpdateState(State) + // ReportError notifies the ClientConn that the Resolver encountered an + // error. The ClientConn will notify the load balancer and begin calling + // ResolveNow on the Resolver with exponential backoff. + ReportError(error) // NewAddress is called by resolver to notify ClientConn a new list // of resolved addresses. // The address list should be the complete list of resolved addresses. @@ -133,6 +138,9 @@ type ClientConn interface { // // Deprecated: Use UpdateState instead. NewServiceConfig(serviceConfig string) + // ParseServiceConfig parses the provided service config and returns an + // object that provides the parsed config. + ParseServiceConfig(serviceConfigJSON string) *serviceconfig.Getter } // Target represents a target for gRPC, as specified in: diff --git a/resolver_conn_wrapper.go b/resolver_conn_wrapper.go index 90b2d26157d..8982e4e3f3f 100644 --- a/resolver_conn_wrapper.go +++ b/resolver_conn_wrapper.go @@ -21,11 +21,16 @@ package grpc import ( "fmt" "strings" + "sync" "sync/atomic" + "time" + "google.golang.org/grpc/balancer" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/resolver" + "google.golang.org/grpc/serviceconfig" ) // ccResolverWrapper is a wrapper on top of cc for resolvers. @@ -35,6 +40,9 @@ type ccResolverWrapper struct { resolver resolver.Resolver done uint32 // accessed atomically; set to 1 when closed. curState resolver.State + + mu sync.Mutex // protects polling + polling chan struct{} } // split2 returns the values from strings.SplitN(s, sep, 2). @@ -96,6 +104,40 @@ func (ccr *ccResolverWrapper) close() { atomic.StoreUint32(&ccr.done, 1) } +var resolverBackoff = backoff.Exponential{MaxDelay: 2 * time.Minute} + +// poll begins or ends asynchronous polling of the resolver. +func (ccr *ccResolverWrapper) poll(run bool) { + ccr.mu.Lock() + defer ccr.mu.Unlock() + if !run { + // stop polling + if ccr.polling != nil { + close(ccr.polling) + ccr.polling = nil + } + return + } + if ccr.polling != nil { + // already polling + return + } + p := make(chan struct{}) + ccr.polling = p + go func() { + for i := 0; true; i++ { + ccr.resolveNow(resolver.ResolveNowOption{}) + t := time.NewTimer(resolverBackoff.Backoff(i)) + select { + case <-p: + t.Stop() + return + case <-t.C: + } + } + }() +} + func (ccr *ccResolverWrapper) isDone() bool { return atomic.LoadUint32(&ccr.done) == 1 } @@ -108,8 +150,22 @@ func (ccr *ccResolverWrapper) UpdateState(s resolver.State) { if channelz.IsOn() { ccr.addChannelzTraceEvent(s) } - ccr.cc.updateResolverState(s) ccr.curState = s + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) +} + +func (ccr *ccResolverWrapper) ReportError(err error) { + if ccr.isDone() { + return + } + grpclog.Warningf("ccResolverWrapper: reporting error to cc: %v", err) + if channelz.IsOn() { + channelz.AddTraceEvent(ccr.cc.channelzID, &channelz.TraceEventDesc{ + Desc: fmt.Sprintf("Resolver reported error: %v", err), + Severity: channelz.CtWarning, + }) + } + ccr.poll(ccr.cc.updateResolverState(resolver.State{}, err) == balancer.ErrBadResolverState) } // NewAddress is called by the resolver implementation to send addresses to gRPC. @@ -119,10 +175,10 @@ func (ccr *ccResolverWrapper) NewAddress(addrs []resolver.Address) { } grpclog.Infof("ccResolverWrapper: sending new addresses to cc: %v", addrs) if channelz.IsOn() { - ccr.addChannelzTraceEvent(resolver.State{Addresses: addrs, ServiceConfig: ccr.curState.ServiceConfig}) + ccr.addChannelzTraceEvent(resolver.State{Addresses: addrs, ServiceConfigGetter: ccr.curState.ServiceConfigGetter}) } ccr.curState.Addresses = addrs - ccr.cc.updateResolverState(ccr.curState) + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) } // NewServiceConfig is called by the resolver implementation to send service @@ -132,21 +188,35 @@ func (ccr *ccResolverWrapper) NewServiceConfig(sc string) { return } grpclog.Infof("ccResolverWrapper: got new service config: %v", sc) - c, err := parseServiceConfig(sc) - if err != nil { + scg := parseServiceConfig(sc) + if _, err := scg.Get(); err != nil { + grpclog.Warningf("ccResolverWrapper: error parsing service config: %v", err) + if channelz.IsOn() { + channelz.AddTraceEvent(ccr.cc.channelzID, &channelz.TraceEventDesc{ + Desc: fmt.Sprintf("Error parsing service config: %v", err), + Severity: channelz.CtWarning, + }) + } + ccr.poll(true) return } if channelz.IsOn() { - ccr.addChannelzTraceEvent(resolver.State{Addresses: ccr.curState.Addresses, ServiceConfig: c}) + ccr.addChannelzTraceEvent(resolver.State{Addresses: ccr.curState.Addresses, ServiceConfigGetter: scg}) } - ccr.curState.ServiceConfig = c - ccr.cc.updateResolverState(ccr.curState) + ccr.curState.ServiceConfigGetter = scg + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) +} + +func (ccr *ccResolverWrapper) ParseServiceConfig(scJSON string) *serviceconfig.Getter { + return parseServiceConfig(scJSON) } func (ccr *ccResolverWrapper) addChannelzTraceEvent(s resolver.State) { var updates []string - oldSC, oldOK := ccr.curState.ServiceConfig.(*ServiceConfig) - newSC, newOK := s.ServiceConfig.(*ServiceConfig) + oldSCI, _ := ccr.curState.ServiceConfigGetter.Get() + oldSC, oldOK := oldSCI.(*ServiceConfig) + newSCI, _ := s.ServiceConfigGetter.Get() + newSC, newOK := newSCI.(*ServiceConfig) if oldOK != newOK || (oldOK && newOK && oldSC.rawJSONString != newSC.rawJSONString) { updates = append(updates, "service config updated") } diff --git a/service_config.go b/service_config.go index 686ad7ba616..210a4cd90b0 100644 --- a/service_config.go +++ b/service_config.go @@ -28,7 +28,6 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/codes" "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/internal" "google.golang.org/grpc/serviceconfig" ) @@ -260,21 +259,15 @@ type jsonSC struct { HealthCheckConfig *healthCheckConfig } -func init() { - internal.ParseServiceConfig = func(sc string) (interface{}, error) { - return parseServiceConfig(sc) - } -} - -func parseServiceConfig(js string) (*ServiceConfig, error) { +func parseServiceConfig(js string) *serviceconfig.Getter { if len(js) == 0 { - return nil, fmt.Errorf("no JSON service config provided") + return serviceconfig.NewGetter(fmt.Errorf("no JSON service config provided")) } var rsc jsonSC err := json.Unmarshal([]byte(js), &rsc) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return nil, err + return serviceconfig.NewGetter(err) } sc := ServiceConfig{ LB: rsc.LoadBalancingPolicy, @@ -288,7 +281,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { if len(lbcfg) != 1 { err := fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg) grpclog.Warningf(err.Error()) - return nil, err + return serviceconfig.NewGetter(err) } var name string var jsonCfg json.RawMessage @@ -303,7 +296,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { var err error sc.lbConfig.cfg, err = parser.ParseConfig(jsonCfg) if err != nil { - return nil, fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err) + return serviceconfig.NewGetter(fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)) } } else if string(jsonCfg) != "{}" { grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg)) @@ -321,7 +314,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { } if rsc.MethodConfig == nil { - return &sc, nil + return serviceconfig.NewGetter(&sc) } for _, m := range *rsc.MethodConfig { if m.Name == nil { @@ -330,7 +323,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { d, err := parseDuration(m.Timeout) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return nil, err + return serviceconfig.NewGetter(err) } mc := MethodConfig{ @@ -339,7 +332,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { } if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return nil, err + return serviceconfig.NewGetter(err) } if m.MaxRequestMessageBytes != nil { if *m.MaxRequestMessageBytes > int64(maxInt) { @@ -364,13 +357,13 @@ func parseServiceConfig(js string) (*ServiceConfig, error) { if sc.retryThrottling != nil { if mt := sc.retryThrottling.MaxTokens; mt <= 0 || mt > 1000 { - return nil, fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt) + return serviceconfig.NewGetter(fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)) } if tr := sc.retryThrottling.TokenRatio; tr <= 0 { - return nil, fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr) + return serviceconfig.NewGetter(fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)) } } - return &sc, nil + return serviceconfig.NewGetter(&sc) } func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) { diff --git a/service_config_test.go b/service_config_test.go index e01e46169f4..1dde66dd08a 100644 --- a/service_config_test.go +++ b/service_config_test.go @@ -37,8 +37,11 @@ type parseTestCase struct { } func runParseTests(t *testing.T, testCases []parseTestCase) { + t.Helper() for _, c := range testCases { - sc, err := parseServiceConfig(c.scjs) + sci, err := parseServiceConfig(c.scjs).Get() + var sc *ServiceConfig + sc, _ = sci.(*ServiceConfig) if !c.wantErr { c.wantSC.rawJSONString = c.scjs } diff --git a/serviceconfig/serviceconfig.go b/serviceconfig/serviceconfig.go index 53b27875a1a..341212d5c08 100644 --- a/serviceconfig/serviceconfig.go +++ b/serviceconfig/serviceconfig.go @@ -23,26 +23,46 @@ package serviceconfig import ( - "google.golang.org/grpc/internal" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/status" ) // Config represents an opaque data structure holding a service config. type Config interface { - isConfig() + isServiceConfig() } // LoadBalancingConfig represents an opaque data structure holding a load -// balancer config. +// balancing config. type LoadBalancingConfig interface { isLoadBalancingConfig() } -// Parse parses the JSON service config provided into an internal form or -// returns an error if the config is invalid. -func Parse(ServiceConfigJSON string) (Config, error) { - c, err := internal.ParseServiceConfig(ServiceConfigJSON) - if err != nil { - return nil, err +// Getter provides a service config or an error. +type Getter struct { + err error + config Config +} + +// Get returns either a service config or an error. +func (g *Getter) Get() (Config, error) { + if g == nil { + return nil, nil + } + // Only one field in g may be non-nil. + return g.config, g.err +} + +// NewGetter returns a Getter returning the provided parameter as either the +// serviceconfig.Config or error return value, depending upon the type. +func NewGetter(configOrError interface{}) *Getter { + if e, ok := configOrError.(error); ok { + return &Getter{err: e} + } + if c, ok := configOrError.(Config); ok { + return &Getter{config: c} } - return c.(Config), err + grpclog.Errorf("Unexpected configOrError type: %T", configOrError) + return &Getter{err: status.Errorf(codes.Internal, "unexpected configOrError type: %T", configOrError)} } diff --git a/test/channelz_test.go b/test/channelz_test.go index 3324e91ed27..f488eb20e71 100644 --- a/test/channelz_test.go +++ b/test/channelz_test.go @@ -230,7 +230,7 @@ func (s) TestCZNestedChannelRegistrationAndDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { @@ -1423,7 +1423,7 @@ func (s) TestCZChannelTraceCreationDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { @@ -1567,7 +1567,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { }); err != nil { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1584,7 +1584,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { t.Fatal(err) } - newSC := parseCfg(`{ + newSC := parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1598,7 +1598,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { } ] }`) - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: newSC}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: newSC}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1618,7 +1618,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: newSC}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: newSC}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1882,7 +1882,7 @@ func (s) TestCZTraceOverwriteChannelDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(`{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { diff --git a/test/end2end_test.go b/test/end2end_test.go index 31e10741768..242f9374094 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -1461,7 +1461,7 @@ func (s) TestGetMethodConfig(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1500,7 +1500,7 @@ func (s) TestGetMethodConfig(t *testing.T) { t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want _, %s", err, codes.DeadlineExceeded) } - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: parseCfg(`{ + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1548,7 +1548,7 @@ func (s) TestServiceConfigWaitForReady(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1590,7 +1590,7 @@ func (s) TestServiceConfigWaitForReady(t *testing.T) { // Case2:Client API set failfast to be false, and service config set wait_for_ready to be true, and the rpc will wait until deadline exceeds. r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1637,7 +1637,7 @@ func (s) TestServiceConfigTimeout(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1684,7 +1684,7 @@ func (s) TestServiceConfigTimeout(t *testing.T) { // Case2: Client API sets timeout to be 1hr and ServiceConfig sets timeout to be 1ns. Timeout should be 1ns (min of 1ns and 1hr) and the rpc will wait until deadline exceeds. r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1747,7 +1747,17 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { t.Fatal(err) } - sc := parseCfg(`{ + // Case1: sc set maxReqSize to 2048 (send), maxRespSize to 2048 (recv). + te1 := testServiceConfigSetup(t, e) + defer te1.tearDown() + + te1.resolverScheme = r.Scheme() + te1.nonBlockingDial = true + te1.startServer(&testServer{security: e.security}) + cc1 := te1.clientConn() + + addrs := []resolver.Address{{Addr: te1.srvAddr}} + sc := parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1765,18 +1775,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { } ] }`) - - // Case1: sc set maxReqSize to 2048 (send), maxRespSize to 2048 (recv). - te1 := testServiceConfigSetup(t, e) - defer te1.tearDown() - - te1.resolverScheme = r.Scheme() - te1.nonBlockingDial = true - te1.startServer(&testServer{security: e.security}) - cc1 := te1.clientConn() - - addrs := []resolver.Address{{Addr: te1.srvAddr}} - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: sc}) tc := testpb.NewTestServiceClient(cc1) req := &testpb.SimpleRequest{ @@ -1847,7 +1846,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { te2.startServer(&testServer{security: e.security}) defer te2.tearDown() cc2 := te2.clientConn() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te2.srvAddr}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te2.srvAddr}}, ServiceConfigGetter: sc}) tc = testpb.NewTestServiceClient(cc2) for { @@ -1908,7 +1907,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { defer te3.tearDown() cc3 := te3.clientConn() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te3.srvAddr}}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te3.srvAddr}}, ServiceConfigGetter: sc}) tc = testpb.NewTestServiceClient(cc3) for { @@ -2000,7 +1999,7 @@ func (s) TestStreamingRPCWithTimeoutInServiceConfigRecv(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: te.srvAddr}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -5312,7 +5311,7 @@ func (ss *stubServer) Start(sopts []grpc.ServerOption, dopts ...grpc.DialOption) } func (ss *stubServer) newServiceConfig(sc string) { - ss.r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.addr}}, ServiceConfig: parseCfg(sc)}) + ss.r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.addr}}, ServiceConfigGetter: parseCfg(ss.r, sc)}) } func (ss *stubServer) waitForReady(cc *grpc.ClientConn) error { @@ -7230,7 +7229,7 @@ func (s) TestRPCWaitsForResolver(t *testing.T) { time.Sleep(time.Second) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: te.srvAddr}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -7477,12 +7476,12 @@ func doHTTPHeaderTest(t *testing.T, errCode codes.Code, headerFields ...[]string } } -func parseCfg(s string) serviceconfig.Config { - c, err := serviceconfig.Parse(s) - if err != nil { +func parseCfg(r *manual.Resolver, s string) *serviceconfig.Getter { + g := r.CC.ParseServiceConfig(s) + if _, err := g.Get(); err != nil { panic(fmt.Sprintf("Error parsing config %q: %v", s, err)) } - return c + return g } type methodTestCreds struct{} diff --git a/test/healthcheck_test.go b/test/healthcheck_test.go index cabf1b377f6..0fd3cd6009b 100644 --- a/test/healthcheck_test.go +++ b/test/healthcheck_test.go @@ -194,7 +194,7 @@ func (s) TestHealthCheckWatchStateChange(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -262,7 +262,7 @@ func (s) TestHealthCheckHealthServerNotRegistered(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -306,7 +306,7 @@ func (s) TestHealthCheckWithGoAway(t *testing.T) { tc := testpb.NewTestServiceClient(cc) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -398,7 +398,7 @@ func (s) TestHealthCheckWithConnClose(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -457,14 +457,14 @@ func (s) TestHealthCheckWithAddrConnDrain(t *testing.T) { defer deferFunc() tc := testpb.NewTestServiceClient(cc) - sc := parseCfg(`{ + sc := parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } }`) r.UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: sc, + Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, + ServiceConfigGetter: sc, }) // make some rpcs to make sure connection is working. @@ -506,7 +506,7 @@ func (s) TestHealthCheckWithAddrConnDrain(t *testing.T) { default: } // trigger teardown of the ac - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: sc}) select { case <-hcExitChan: @@ -552,7 +552,7 @@ func (s) TestHealthCheckWithClientConnClose(t *testing.T) { tc := testpb.NewTestServiceClient(cc) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -630,14 +630,14 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalledAddrConnShutDown(t *tes // The serviceName "delay" is specially handled at server side, where response will not be sent // back to client immediately upon receiving the request (client should receive no response until // test ends). - sc := parseCfg(`{ + sc := parseCfg(r, `{ "healthCheckConfig": { "serviceName": "delay" } }`) r.UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: sc, + Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, + ServiceConfigGetter: sc, }) select { @@ -652,7 +652,7 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalledAddrConnShutDown(t *tes t.Fatal("Health check function has not been invoked after 5s.") } // trigger teardown of the ac, ac in SHUTDOWN state - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: sc}) // The health check func should exit without calling the setConnectivityState func, as server hasn't sent // any response. @@ -708,7 +708,7 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalled(t *testing.T) { // test ends). r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "delay" } @@ -756,7 +756,7 @@ func testHealthCheckDisableWithDialOption(t *testing.T, addr string) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -795,7 +795,7 @@ func testHealthCheckDisableWithBalancer(t *testing.T, addr string) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -888,7 +888,7 @@ func (s) TestHealthCheckChannelzCountingCallSuccess(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "channelzSuccess" } @@ -944,7 +944,7 @@ func (s) TestHealthCheckChannelzCountingCallFailure(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfig: parseCfg(`{ + ServiceConfigGetter: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "channelzFailure" } diff --git a/xds/internal/balancer/xds.go b/xds/internal/balancer/xds.go index 3f80d411160..c925a52ae74 100644 --- a/xds/internal/balancer/xds.go +++ b/xds/internal/balancer/xds.go @@ -112,6 +112,8 @@ type edsBalancerInterface interface { Close() } +var _ balancer.V2Balancer = (*xdsBalancer)(nil) // Assert that we implement V2Balancer + // xdsBalancer manages xdsClient and the actual balancer that does load balancing (either edsBalancer, // or fallback LB). type xdsBalancer struct { @@ -400,11 +402,16 @@ func (x *xdsBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.Sub } } -func (x *xdsBalancer) UpdateClientConnState(s balancer.ClientConnState) { +func (x *xdsBalancer) ResolverError(error) { + // Ignore for now +} + +func (x *xdsBalancer) UpdateClientConnState(s balancer.ClientConnState) error { select { case x.grpcUpdate <- &s: case <-x.ctx.Done(): } + return nil } type cdsResp struct { From 901731a23a93f0a7346e2429de1a08445352b0b5 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 16 Aug 2019 10:57:27 -0700 Subject: [PATCH 02/14] address review comments --- balancer_conn_wrappers.go | 18 +------------ clientconn.go | 24 ++++++++++++++--- resolver_conn_wrapper.go | 55 +++++++++++++++++++++++---------------- 3 files changed, 55 insertions(+), 42 deletions(-) diff --git a/balancer_conn_wrappers.go b/balancer_conn_wrappers.go index 42f0ab68291..7dd437e7f5c 100644 --- a/balancer_conn_wrappers.go +++ b/balancer_conn_wrappers.go @@ -87,7 +87,7 @@ func (b *scStateUpdateBuffer) get() <-chan *scStateUpdate { // It implements balancer.ClientConn interface. type ccBalancerWrapper struct { cc *ClientConn - balancerMu sync.Mutex + balancerMu sync.Mutex // synchronizes calls to the balancer balancer balancer.Balancer stateChangeQueue *scStateUpdateBuffer done *grpcsync.Event @@ -166,22 +166,6 @@ func (ccb *ccBalancerWrapper) handleSubConnStateChange(sc balancer.SubConn, s co } func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) error { - cbn := ccb.cc.curBalancerName - ccb.cc.mu.Unlock() - defer ccb.cc.mu.Lock() - if cbn != grpclbName { - // Filter any grpclb addresses since we don't have the grpclb balancer. - s := &ccs.ResolverState - for i := 0; i < len(s.Addresses); { - if s.Addresses[i].Type == resolver.GRPCLB { - copy(s.Addresses[i:], s.Addresses[i+1:]) - s.Addresses = s.Addresses[:len(s.Addresses)-1] - continue - } - i++ - } - } - ccb.balancerMu.Lock() defer ccb.balancerMu.Unlock() if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { diff --git a/clientconn.go b/clientconn.go index 7041dce2564..a6e333c8eaf 100644 --- a/clientconn.go +++ b/clientconn.go @@ -554,16 +554,18 @@ func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { cc.mu.Lock() - defer cc.mu.Unlock() // Check if the ClientConn is already closed. Some fields (e.g. // balancerWrapper) are set to nil when closing the ClientConn, and could // cause nil pointer panic if we don't have this check. if cc.conns == nil { + cc.mu.Unlock() return nil } if err != nil { - // May need to apply the initial service config + // May need to apply the initial service config in case the resolver + // doesn't support service configs, or doesn't provide a service config + // with the new addresses. cc.maybeApplyDefaultServiceConfig(nil) if cc.balancerWrapper != nil { @@ -571,6 +573,7 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { } // No addresses are valid with err set; return early. + cc.mu.Unlock() return balancer.ErrBadResolverState } @@ -592,7 +595,22 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { if cc.dopts.balancerBuilder == nil && cc.sc != nil && cc.sc.lbConfig != nil { balCfg = cc.sc.lbConfig.cfg } - uccsErr := cc.balancerWrapper.updateClientConnState(&balancer.ClientConnState{ResolverState: s, BalancerConfig: balCfg}) + + cbn := cc.curBalancerName + bw := cc.balancerWrapper + cc.mu.Unlock() + if cbn != grpclbName { + // Filter any grpclb addresses since we don't have the grpclb balancer. + for i := 0; i < len(s.Addresses); { + if s.Addresses[i].Type == resolver.GRPCLB { + copy(s.Addresses[i:], s.Addresses[i+1:]) + s.Addresses = s.Addresses[:len(s.Addresses)-1] + continue + } + i++ + } + } + uccsErr := bw.updateClientConnState(&balancer.ClientConnState{ResolverState: s, BalancerConfig: balCfg}) if ret == nil { ret = uccsErr // prefer ErrBadResolver state since any other error is // currently meaningless to the caller. diff --git a/resolver_conn_wrapper.go b/resolver_conn_wrapper.go index 8982e4e3f3f..ed1417f2ee4 100644 --- a/resolver_conn_wrapper.go +++ b/resolver_conn_wrapper.go @@ -22,13 +22,13 @@ import ( "fmt" "strings" "sync" - "sync/atomic" "time" "google.golang.org/grpc/balancer" "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/channelz" + "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" ) @@ -38,7 +38,7 @@ import ( type ccResolverWrapper struct { cc *ClientConn resolver resolver.Resolver - done uint32 // accessed atomically; set to 1 when closed. + done *grpcsync.Event curState resolver.State mu sync.Mutex // protects polling @@ -85,7 +85,10 @@ func newCCResolverWrapper(cc *ClientConn) (*ccResolverWrapper, error) { return nil, fmt.Errorf("could not get resolver for scheme: %q", cc.parsedTarget.Scheme) } - ccr := &ccResolverWrapper{cc: cc} + ccr := &ccResolverWrapper{ + cc: cc, + done: grpcsync.NewEvent(), + } var err error ccr.resolver, err = rb.Build(cc.parsedTarget, ccr, resolver.BuildOption{DisableServiceConfig: cc.dopts.disableServiceConfig}) @@ -96,21 +99,28 @@ func newCCResolverWrapper(cc *ClientConn) (*ccResolverWrapper, error) { } func (ccr *ccResolverWrapper) resolveNow(o resolver.ResolveNowOption) { - ccr.resolver.ResolveNow(o) + ccr.mu.Lock() + if !ccr.done.HasFired() { + ccr.resolver.ResolveNow(o) + } + ccr.mu.Unlock() } func (ccr *ccResolverWrapper) close() { + ccr.mu.Lock() ccr.resolver.Close() - atomic.StoreUint32(&ccr.done, 1) + ccr.done.Fire() + ccr.mu.Unlock() } var resolverBackoff = backoff.Exponential{MaxDelay: 2 * time.Minute} -// poll begins or ends asynchronous polling of the resolver. -func (ccr *ccResolverWrapper) poll(run bool) { +// poll begins or ends asynchronous polling of the resolver based on whether +// err is ErrBadResolverState. +func (ccr *ccResolverWrapper) poll(err error) { ccr.mu.Lock() defer ccr.mu.Unlock() - if !run { + if err != balancer.ErrBadResolverState { // stop polling if ccr.polling != nil { close(ccr.polling) @@ -125,25 +135,26 @@ func (ccr *ccResolverWrapper) poll(run bool) { p := make(chan struct{}) ccr.polling = p go func() { - for i := 0; true; i++ { + for i := 0; ; i++ { ccr.resolveNow(resolver.ResolveNowOption{}) t := time.NewTimer(resolverBackoff.Backoff(i)) select { case <-p: t.Stop() return + case <-ccr.done.Done(): + // Resolver has been closed. + t.Stop() + return case <-t.C: + // Timer expired; re-resolve. } } }() } -func (ccr *ccResolverWrapper) isDone() bool { - return atomic.LoadUint32(&ccr.done) == 1 -} - func (ccr *ccResolverWrapper) UpdateState(s resolver.State) { - if ccr.isDone() { + if ccr.done.HasFired() { return } grpclog.Infof("ccResolverWrapper: sending update to cc: %v", s) @@ -151,11 +162,11 @@ func (ccr *ccResolverWrapper) UpdateState(s resolver.State) { ccr.addChannelzTraceEvent(s) } ccr.curState = s - ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil)) } func (ccr *ccResolverWrapper) ReportError(err error) { - if ccr.isDone() { + if ccr.done.HasFired() { return } grpclog.Warningf("ccResolverWrapper: reporting error to cc: %v", err) @@ -165,12 +176,12 @@ func (ccr *ccResolverWrapper) ReportError(err error) { Severity: channelz.CtWarning, }) } - ccr.poll(ccr.cc.updateResolverState(resolver.State{}, err) == balancer.ErrBadResolverState) + ccr.poll(ccr.cc.updateResolverState(resolver.State{}, err)) } // NewAddress is called by the resolver implementation to send addresses to gRPC. func (ccr *ccResolverWrapper) NewAddress(addrs []resolver.Address) { - if ccr.isDone() { + if ccr.done.HasFired() { return } grpclog.Infof("ccResolverWrapper: sending new addresses to cc: %v", addrs) @@ -178,13 +189,13 @@ func (ccr *ccResolverWrapper) NewAddress(addrs []resolver.Address) { ccr.addChannelzTraceEvent(resolver.State{Addresses: addrs, ServiceConfigGetter: ccr.curState.ServiceConfigGetter}) } ccr.curState.Addresses = addrs - ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil)) } // NewServiceConfig is called by the resolver implementation to send service // configs to gRPC. func (ccr *ccResolverWrapper) NewServiceConfig(sc string) { - if ccr.isDone() { + if ccr.done.HasFired() { return } grpclog.Infof("ccResolverWrapper: got new service config: %v", sc) @@ -197,14 +208,14 @@ func (ccr *ccResolverWrapper) NewServiceConfig(sc string) { Severity: channelz.CtWarning, }) } - ccr.poll(true) + ccr.poll(balancer.ErrBadResolverState) return } if channelz.IsOn() { ccr.addChannelzTraceEvent(resolver.State{Addresses: ccr.curState.Addresses, ServiceConfigGetter: scg}) } ccr.curState.ServiceConfigGetter = scg - ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil) == balancer.ErrBadResolverState) + ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil)) } func (ccr *ccResolverWrapper) ParseServiceConfig(scJSON string) *serviceconfig.Getter { From 736d0f1310be28cbbaefa6fb0e7f70e8fc58bdf7 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 21 Aug 2019 15:28:46 -0700 Subject: [PATCH 03/14] shed bikes --- clientconn.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/clientconn.go b/clientconn.go index a6e333c8eaf..840d5583dd1 100644 --- a/clientconn.go +++ b/clientconn.go @@ -542,13 +542,13 @@ func init() { func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { if cc.sc != nil { - cc.applyServiceConfig(cc.sc, addrs) + cc.applyServiceConfigAndBalancer(cc.sc, addrs) return } if cc.dopts.defaultServiceConfig != nil { - cc.applyServiceConfig(cc.dopts.defaultServiceConfig, addrs) + cc.applyServiceConfigAndBalancer(cc.dopts.defaultServiceConfig, addrs) } else { - cc.applyServiceConfig(emptyServiceConfig, addrs) + cc.applyServiceConfigAndBalancer(emptyServiceConfig, addrs) } } @@ -585,7 +585,7 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { } else { scg, err := s.ServiceConfigGetter.Get() if sc, ok := scg.(*ServiceConfig); err == nil && ok { - cc.applyServiceConfig(sc, s.Addresses) + cc.applyServiceConfigAndBalancer(sc, s.Addresses) } else { ret = balancer.ErrBadResolverState } @@ -863,7 +863,7 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st return t, done, nil } -func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig, addrs []resolver.Address) { +func (cc *ClientConn) applyServiceConfigAndBalancer(sc *ServiceConfig, addrs []resolver.Address) { if sc == nil { // should never reach here. return From f17850681ca1a0982100dea281b57d95e0888afe Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Tue, 17 Sep 2019 09:20:00 -0700 Subject: [PATCH 04/14] updates after sync --- internal/internal.go | 3 +++ service_config.go | 4 +++ xds/internal/resolver/xds_resolver.go | 29 ++++++---------------- xds/internal/resolver/xds_resolver_test.go | 28 +++++++++++++++------ 4 files changed, 34 insertions(+), 30 deletions(-) diff --git a/internal/internal.go b/internal/internal.go index 82bae7c787d..39d21c1bc06 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -47,6 +47,9 @@ var ( // NewRequestInfoContext creates a new context based on the argument context attaching // the passed in RequestInfo to the new context. NewRequestInfoContext interface{} // func(context.Context, credentials.RequestInfo) context.Context + // ParseServiceConfigForTesting is for creating a fake + // ClientConn for resolver testing only + ParseServiceConfigForTesting interface{} // func(string) *serviceconfig.Getter ) // HealthChecker defines the signature of the client-side LB channel health checking function. diff --git a/service_config.go b/service_config.go index 210a4cd90b0..6a4ee480f29 100644 --- a/service_config.go +++ b/service_config.go @@ -28,6 +28,7 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/codes" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal" "google.golang.org/grpc/serviceconfig" ) @@ -259,6 +260,9 @@ type jsonSC struct { HealthCheckConfig *healthCheckConfig } +func init() { + internal.ParseServiceConfigForTesting = parseServiceConfig +} func parseServiceConfig(js string) *serviceconfig.Getter { if len(js) == 0 { return serviceconfig.NewGetter(fmt.Errorf("no JSON service config provided")) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index a2e87738e88..02f8aa2b32e 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -25,12 +25,8 @@ package resolver import ( "fmt" - "sync" - "google.golang.org/grpc" - "google.golang.org/grpc/internal" "google.golang.org/grpc/resolver" - "google.golang.org/grpc/serviceconfig" ) const ( @@ -54,11 +50,6 @@ const ( xdsScheme = "xds-experimental" ) -var ( - parseOnce sync.Once - parsedSC serviceconfig.Config -) - // NewBuilder creates a new implementation of the resolver.Builder interface // for the xDS resolver. func NewBuilder() resolver.Builder { @@ -69,23 +60,17 @@ type xdsBuilder struct{} // Build helps implement the resolver.Builder interface. func (b *xdsBuilder) Build(t resolver.Target, cc resolver.ClientConn, o resolver.BuildOption) (resolver.Resolver, error) { - parseOnce.Do(func() { - // The xds balancer must have been registered at this point for the service - // config to be parsed properly. - psc, err := internal.ParseServiceConfig(jsonSC) - if err != nil { - panic(fmt.Sprintf("service config %s parsing failed: %v", jsonSC, err)) - } + // The xds balancer must have been registered at this point for the service + // config to be parsed properly. + sc := cc.ParseServiceConfig(jsonSC) - var ok bool - if parsedSC, ok = psc.(*grpc.ServiceConfig); !ok { - panic(fmt.Sprintf("service config type is [%T], want [grpc.ServiceConfig]", psc)) - } - }) + if _, err := sc.Get(); err != nil { + panic(fmt.Sprintf("error parsing service config %q: %v", jsonSC, err)) + } // We return a resolver which bacically does nothing. The hard-coded service // config returned here picks the xds balancer. - cc.UpdateState(resolver.State{ServiceConfig: parsedSC}) + cc.UpdateState(resolver.State{ServiceConfigGetter: sc}) return &xdsResolver{}, nil } diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index bfcbfa1404e..408a90f054d 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -28,6 +28,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/internal" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" xdsinternal "google.golang.org/grpc/xds/internal" @@ -49,7 +50,7 @@ func init() { } ] }`)}, - errCh: make(chan error), + errCh: make(chan error, 1), } balancer.Register(fbb) } @@ -67,8 +68,13 @@ func (t *testClientConn) UpdateState(s resolver.State) { close(t.done) } -func (*testClientConn) NewAddress(addresses []resolver.Address) { panic("unimplemented") } -func (*testClientConn) NewServiceConfig(serviceConfig string) { panic("unimplemented") } +func (*testClientConn) ParseServiceConfig(jsonSC string) *serviceconfig.Getter { + return internal.ParseServiceConfigForTesting.(func(string) *serviceconfig.Getter)(jsonSC) +} + +func (*testClientConn) NewAddress([]resolver.Address) { panic("unimplemented") } +func (*testClientConn) NewServiceConfig(string) { panic("unimplemented") } +func (*testClientConn) ReportError(error) { panic("unimplemented") } // TestXDSRsolverSchemeAndAddresses creates a new xds resolver, verifies that // it returns an empty address list and the appropriate xds-experimental @@ -93,6 +99,8 @@ func TestXDSRsolverSchemeAndAddresses(t *testing.T) { } } +var _ balancer.V2Balancer = (*fakeBalancer)(nil) + // fakeBalancer is used to verify that the xds_resolver returns the expected // serice config. type fakeBalancer struct { @@ -106,31 +114,35 @@ func (*fakeBalancer) HandleSubConnStateChange(_ balancer.SubConn, _ connectivity func (*fakeBalancer) HandleResolvedAddrs(_ []resolver.Address, _ error) { panic("unimplemented") } +func (*fakeBalancer) ResolverError(error) { + panic("unimplemented") +} // UpdateClientConnState verifies that the received LBConfig matches the // provided one, and if not, sends an error on the provided channel. -func (f *fakeBalancer) UpdateClientConnState(ccs balancer.ClientConnState) { +func (f *fakeBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error { gotLBConfig, ok := ccs.BalancerConfig.(*wrappedLBConfig) if !ok { f.errCh <- fmt.Errorf("in fakeBalancer got lbConfig of type %T, want %T", ccs.BalancerConfig, &wrappedLBConfig{}) - return + return balancer.ErrBadResolverState } var gotCfg, wantCfg xdsinternal.LBConfig if err := wantCfg.UnmarshalJSON(f.wantLBConfig.lbCfg); err != nil { f.errCh <- fmt.Errorf("unable to unmarshal balancer config %s into xds config", string(f.wantLBConfig.lbCfg)) - return + return balancer.ErrBadResolverState } if err := gotCfg.UnmarshalJSON(gotLBConfig.lbCfg); err != nil { f.errCh <- fmt.Errorf("unable to unmarshal balancer config %s into xds config", string(gotLBConfig.lbCfg)) - return + return balancer.ErrBadResolverState } if !reflect.DeepEqual(gotCfg, wantCfg) { f.errCh <- fmt.Errorf("in fakeBalancer got lbConfig %v, want %v", gotCfg, wantCfg) - return + return balancer.ErrBadResolverState } f.errCh <- nil + return nil } func (*fakeBalancer) UpdateSubConnState(_ balancer.SubConn, _ balancer.SubConnState) { From a37cf5ab1b0a0212b069de15eb5d72e4d73a2ec8 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 18 Sep 2019 13:16:20 -0700 Subject: [PATCH 05/14] add tests --- balancer_conn_wrappers_test.go | 124 +++++++++++++++++++++++++++++++++ resolver/manual/manual.go | 13 +++- resolver_conn_wrapper.go | 9 ++- resolver_conn_wrapper_test.go | 54 ++++++++++++++ 4 files changed, 195 insertions(+), 5 deletions(-) create mode 100644 balancer_conn_wrappers_test.go diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go new file mode 100644 index 00000000000..3739eab881d --- /dev/null +++ b/balancer_conn_wrappers_test.go @@ -0,0 +1,124 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package grpc + +import ( + "testing" + "time" + + "google.golang.org/grpc/balancer" + "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/resolver" + "google.golang.org/grpc/resolver/manual" +) + +var _ balancer.V2Balancer = &funcBalancer{} + +type funcBalancer struct { + updateClientConnState func(s balancer.ClientConnState) error +} + +func (*funcBalancer) HandleSubConnStateChange(balancer.SubConn, connectivity.State) { + panic("unimplemented") // v1 API +} +func (*funcBalancer) HandleResolvedAddrs([]resolver.Address, error) { + panic("unimplemented") // v1 API +} +func (b *funcBalancer) UpdateClientConnState(s balancer.ClientConnState) error { + return b.updateClientConnState(s) +} +func (*funcBalancer) ResolverError(error) { + panic("unimplemented") // resolver never reports error +} +func (*funcBalancer) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) { + panic("unimplemented") // we never have sub-conns +} +func (*funcBalancer) Close() {} + +type funcBalancerBuilder struct { + name string + instance *funcBalancer +} + +func (b *funcBalancerBuilder) Build(balancer.ClientConn, balancer.BuildOptions) balancer.Balancer { + return b.instance +} +func (b *funcBalancerBuilder) Name() string { return b.name } + +// TestResolverErrorPolling injects balancer errors and verifies ResolveNow is +// called on the resolver with the appropriate backoff strategy being consulted +// between ResolveNow calls. +func (s) TestBalancerErrorResolverPolling(t *testing.T) { + defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) + + boTime := time.Duration(0) + boIter := make(chan int) + resolverBackoff = func(v int) time.Duration { + t := boTime + boIter <- v + return t + } + + r, rcleanup := manual.GenerateAndRegisterManualResolver() + defer rcleanup() + rn := make(chan struct{}) + defer func() { close(rn) }() + r.ResolveNowCallback = func(resolver.ResolveNowOption) { rn <- struct{}{} } + + uccsErr := make(chan error) + fb := &funcBalancer{ + updateClientConnState: func(s balancer.ClientConnState) error { + return <-uccsErr + }, + } + balancer.Register(&funcBalancerBuilder{name: "BalancerErrorResolverPolling", instance: fb}) + + cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithBalancerName("BalancerErrorResolverPolling")) + if err != nil { + t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) + } + defer cc.Close() + go func() { uccsErr <- balancer.ErrBadResolverState }() + r.CC.UpdateState(resolver.State{}) + timer := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + // Ensure ResolveNow is called, then Backoff with the right parameter, several times + for i := 0; i < 7; i++ { + <-rn + if v := <-boIter; v != i { + t.Errorf("Backoff call %v uses value %v", i, v) + } + } + boTime = 50 * time.Millisecond + <-rn + <-boIter + go func() { uccsErr <- nil }() + r.CC.UpdateState(resolver.State{}) + boTime = 0 + timer.Stop() + // Wait awhile to ensure ResolveNow and Backoff are not called when the + // state is OK (i.e. polling was cancelled). + timer = time.NewTimer(60 * time.Millisecond) + select { + case <-boIter: + t.Errorf("Received Backoff call after successful resolver state") + case <-rn: + t.Errorf("Received ResolveNow after successful resolver state") + case <-timer.C: + } +} diff --git a/resolver/manual/manual.go b/resolver/manual/manual.go index 6b8d0e8a19c..766df467ca5 100644 --- a/resolver/manual/manual.go +++ b/resolver/manual/manual.go @@ -30,14 +30,19 @@ import ( // NewBuilderWithScheme creates a new test resolver builder with the given scheme. func NewBuilderWithScheme(scheme string) *Resolver { return &Resolver{ - scheme: scheme, + ResolveNowCallback: func(resolver.ResolveNowOption) {}, + scheme: scheme, } } // Resolver is also a resolver builder. // It's build() function always returns itself. type Resolver struct { - scheme string + // ResolveNowCallback is called when the ResolveNow method is called on the + // resolver. Must not be nil. Must not be changed after the resolver may + // be built. + ResolveNowCallback func(resolver.ResolveNowOption) + scheme string // Fields actually belong to the resolver. CC resolver.ClientConn @@ -65,7 +70,9 @@ func (r *Resolver) Scheme() string { } // ResolveNow is a noop for Resolver. -func (*Resolver) ResolveNow(o resolver.ResolveNowOption) {} +func (r *Resolver) ResolveNow(o resolver.ResolveNowOption) { + r.ResolveNowCallback(o) +} // Close is a noop for Resolver. func (*Resolver) Close() {} diff --git a/resolver_conn_wrapper.go b/resolver_conn_wrapper.go index ed1417f2ee4..d4b523abc2b 100644 --- a/resolver_conn_wrapper.go +++ b/resolver_conn_wrapper.go @@ -113,7 +113,7 @@ func (ccr *ccResolverWrapper) close() { ccr.mu.Unlock() } -var resolverBackoff = backoff.Exponential{MaxDelay: 2 * time.Minute} +var resolverBackoff = backoff.Exponential{MaxDelay: 2 * time.Minute}.Backoff // poll begins or ends asynchronous polling of the resolver based on whether // err is ErrBadResolverState. @@ -137,7 +137,7 @@ func (ccr *ccResolverWrapper) poll(err error) { go func() { for i := 0; ; i++ { ccr.resolveNow(resolver.ResolveNowOption{}) - t := time.NewTimer(resolverBackoff.Backoff(i)) + t := time.NewTimer(resolverBackoff(i)) select { case <-p: t.Stop() @@ -147,6 +147,11 @@ func (ccr *ccResolverWrapper) poll(err error) { t.Stop() return case <-t.C: + select { + case <-p: + return + default: + } // Timer expired; re-resolve. } } diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 4a16d7a88c9..1760f517f67 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -19,12 +19,14 @@ package grpc import ( + "errors" "fmt" "net" "testing" "time" "google.golang.org/grpc/resolver" + "google.golang.org/grpc/resolver/manual" ) func (s) TestParseTarget(t *testing.T) { @@ -114,3 +116,55 @@ func (s) TestDialParseTargetUnknownScheme(t *testing.T) { } } } + +// TestResolverErrorPolling injects resolver errors and verifies ResolveNow is +// called with the appropriate backoff strategy being consulted between +// ResolveNow calls. +func (s) TestResolverErrorPolling(t *testing.T) { + defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) + + boTime := time.Duration(0) + boIter := make(chan int) + resolverBackoff = func(v int) time.Duration { + t := boTime + boIter <- v + return t + } + + r, rcleanup := manual.GenerateAndRegisterManualResolver() + defer rcleanup() + rn := make(chan struct{}) + defer func() { close(rn) }() + r.ResolveNowCallback = func(resolver.ResolveNowOption) { rn <- struct{}{} } + + cc, err := Dial(r.Scheme()+":///test.server", WithInsecure()) + if err != nil { + t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) + } + defer cc.Close() + r.CC.ReportError(errors.New("res err")) + timer := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + // Ensure ResolveNow is called, then Backoff with the right parameter, several times + for i := 0; i < 7; i++ { + <-rn + if v := <-boIter; v != i { + t.Errorf("Backoff call %v uses value %v", i, v) + } + } + boTime = 50 * time.Millisecond + <-rn + <-boIter + r.CC.UpdateState(resolver.State{}) + boTime = 0 + timer.Stop() + // Wait awhile to ensure ResolveNow and Backoff are not called when the + // state is OK (i.e. polling was cancelled). + timer = time.NewTimer(60 * time.Millisecond) + select { + case <-boIter: + t.Errorf("Received Backoff call after successful resolver state") + case <-rn: + t.Errorf("Received ResolveNow after successful resolver state") + case <-timer.C: + } +} From a82da16e2d6eefec9a557a6c9bea777088f47315 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 26 Sep 2019 10:42:19 -0700 Subject: [PATCH 06/14] merge fixes --- balancer_conn_wrappers_test.go | 7 +++++-- service_config.go | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go index 3739eab881d..57843ed48d8 100644 --- a/balancer_conn_wrappers_test.go +++ b/balancer_conn_wrappers_test.go @@ -61,7 +61,7 @@ func (b *funcBalancerBuilder) Build(balancer.ClientConn, balancer.BuildOptions) } func (b *funcBalancerBuilder) Name() string { return b.name } -// TestResolverErrorPolling injects balancer errors and verifies ResolveNow is +// TestBalancerErrorResolverPolling injects balancer errors and verifies ResolveNow is // called on the resolver with the appropriate backoff strategy being consulted // between ResolveNow calls. func (s) TestBalancerErrorResolverPolling(t *testing.T) { @@ -89,7 +89,10 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { } balancer.Register(&funcBalancerBuilder{name: "BalancerErrorResolverPolling", instance: fb}) - cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithBalancerName("BalancerErrorResolverPolling")) + cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithDefaultServiceConfig(` +{ + "loadBalancingConfig": [{"BalancerErrorResolverPolling": {}}] +}`)) if err != nil { t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) } diff --git a/service_config.go b/service_config.go index 6a4ee480f29..e6bfc82211e 100644 --- a/service_config.go +++ b/service_config.go @@ -313,7 +313,7 @@ func parseServiceConfig(js string) *serviceconfig.Getter { // case. err := fmt.Errorf("invalid loadBalancingConfig: no supported policies found") grpclog.Warningf(err.Error()) - return nil, err + return serviceconfig.NewGetter(err) } } From 8115d1117044c8fcffd034c7f731f0047d68715b Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 26 Sep 2019 14:28:59 -0700 Subject: [PATCH 07/14] update tests --- balancer_conn_wrappers_test.go | 62 ++++++++++++++++++++-------------- resolver_conn_wrapper_test.go | 41 ++++++++++++---------- 2 files changed, 59 insertions(+), 44 deletions(-) diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go index 57843ed48d8..b4a50b0238a 100644 --- a/balancer_conn_wrappers_test.go +++ b/balancer_conn_wrappers_test.go @@ -19,6 +19,7 @@ package grpc import ( + "fmt" "testing" "time" @@ -67,12 +68,10 @@ func (b *funcBalancerBuilder) Name() string { return b.name } func (s) TestBalancerErrorResolverPolling(t *testing.T) { defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) - boTime := time.Duration(0) boIter := make(chan int) resolverBackoff = func(v int) time.Duration { - t := boTime boIter <- v - return t + return 0 } r, rcleanup := manual.GenerateAndRegisterManualResolver() @@ -81,25 +80,32 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { defer func() { close(rn) }() r.ResolveNowCallback = func(resolver.ResolveNowOption) { rn <- struct{}{} } - uccsErr := make(chan error) + // The test balancer will return ErrBadResolverState iff the + // ClientConnState contains no addresses. fb := &funcBalancer{ updateClientConnState: func(s balancer.ClientConnState) error { - return <-uccsErr + if len(s.ResolverState.Addresses) == 0 { + return balancer.ErrBadResolverState + } + return nil }, } - balancer.Register(&funcBalancerBuilder{name: "BalancerErrorResolverPolling", instance: fb}) + const balName = "BalancerErrorResolverPolling" + balancer.Register(&funcBalancerBuilder{name: balName, instance: fb}) - cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithDefaultServiceConfig(` + cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithDefaultServiceConfig(fmt.Sprintf(` { - "loadBalancingConfig": [{"BalancerErrorResolverPolling": {}}] -}`)) + "loadBalancingConfig": [{"%v": {}}] +}`, balName))) if err != nil { t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) } defer cc.Close() - go func() { uccsErr <- balancer.ErrBadResolverState }() r.CC.UpdateState(resolver.State{}) - timer := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + + panicAfter := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + defer panicAfter.Stop() + // Ensure ResolveNow is called, then Backoff with the right parameter, several times for i := 0; i < 7; i++ { <-rn @@ -107,21 +113,25 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { t.Errorf("Backoff call %v uses value %v", i, v) } } - boTime = 50 * time.Millisecond - <-rn - <-boIter - go func() { uccsErr <- nil }() - r.CC.UpdateState(resolver.State{}) - boTime = 0 - timer.Stop() - // Wait awhile to ensure ResolveNow and Backoff are not called when the + + // UpdateState will block if ResolveNow is being called (which blocks on + // rn), so call it in a goroutine. Include some address so the balancer + // will be happy. + go r.CC.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "x"}}}) + + // Wait awhile to ensure ResolveNow and Backoff stop being called when the // state is OK (i.e. polling was cancelled). - timer = time.NewTimer(60 * time.Millisecond) - select { - case <-boIter: - t.Errorf("Received Backoff call after successful resolver state") - case <-rn: - t.Errorf("Received ResolveNow after successful resolver state") - case <-timer.C: + for { + t := time.NewTimer(50 * time.Millisecond) + select { + case <-rn: + // ClientConn is still calling ResolveNow + <-boIter + time.Sleep(5 * time.Millisecond) + continue + case <-t.C: + // ClientConn stopped calling ResolveNow; success + } + break } } diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 1760f517f67..06d87b8f725 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -123,12 +123,10 @@ func (s) TestDialParseTargetUnknownScheme(t *testing.T) { func (s) TestResolverErrorPolling(t *testing.T) { defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) - boTime := time.Duration(0) boIter := make(chan int) resolverBackoff = func(v int) time.Duration { - t := boTime boIter <- v - return t + return 0 } r, rcleanup := manual.GenerateAndRegisterManualResolver() @@ -143,7 +141,10 @@ func (s) TestResolverErrorPolling(t *testing.T) { } defer cc.Close() r.CC.ReportError(errors.New("res err")) - timer := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + + panicAfter := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) + defer panicAfter.Stop() + // Ensure ResolveNow is called, then Backoff with the right parameter, several times for i := 0; i < 7; i++ { <-rn @@ -151,20 +152,24 @@ func (s) TestResolverErrorPolling(t *testing.T) { t.Errorf("Backoff call %v uses value %v", i, v) } } - boTime = 50 * time.Millisecond - <-rn - <-boIter - r.CC.UpdateState(resolver.State{}) - boTime = 0 - timer.Stop() - // Wait awhile to ensure ResolveNow and Backoff are not called when the + + // UpdateState will block if ResolveNow is being called (which blocks on + // rn), so call it in a goroutine. + go r.CC.UpdateState(resolver.State{}) + + // Wait awhile to ensure ResolveNow and Backoff stop being called when the // state is OK (i.e. polling was cancelled). - timer = time.NewTimer(60 * time.Millisecond) - select { - case <-boIter: - t.Errorf("Received Backoff call after successful resolver state") - case <-rn: - t.Errorf("Received ResolveNow after successful resolver state") - case <-timer.C: + for { + t := time.NewTimer(50 * time.Millisecond) + select { + case <-rn: + // ClientConn is still calling ResolveNow + <-boIter + time.Sleep(5 * time.Millisecond) + continue + case <-t.C: + // ClientConn stopped calling ResolveNow; success + } + break } } From e185996eff530dcfc502878ad5a2adba18be9a19 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 27 Sep 2019 16:46:32 -0700 Subject: [PATCH 08/14] add service config error test; fix bug where this happens when there is no current balancer --- balancer_conn_wrappers_test.go | 75 +++++++--------------------------- clientconn.go | 3 ++ resolver_conn_wrapper_test.go | 38 +++++++++++++---- 3 files changed, 49 insertions(+), 67 deletions(-) diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go index b4a50b0238a..6d8bf74e159 100644 --- a/balancer_conn_wrappers_test.go +++ b/balancer_conn_wrappers_test.go @@ -21,7 +21,6 @@ package grpc import ( "fmt" "testing" - "time" "google.golang.org/grpc/balancer" "google.golang.org/grpc/connectivity" @@ -62,24 +61,10 @@ func (b *funcBalancerBuilder) Build(balancer.ClientConn, balancer.BuildOptions) } func (b *funcBalancerBuilder) Name() string { return b.name } -// TestBalancerErrorResolverPolling injects balancer errors and verifies ResolveNow is -// called on the resolver with the appropriate backoff strategy being consulted -// between ResolveNow calls. +// TestBalancerErrorResolverPolling injects balancer errors and verifies +// ResolveNow is called on the resolver with the appropriate backoff strategy +// being consulted between ResolveNow calls. func (s) TestBalancerErrorResolverPolling(t *testing.T) { - defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) - - boIter := make(chan int) - resolverBackoff = func(v int) time.Duration { - boIter <- v - return 0 - } - - r, rcleanup := manual.GenerateAndRegisterManualResolver() - defer rcleanup() - rn := make(chan struct{}) - defer func() { close(rn) }() - r.ResolveNowCallback = func(resolver.ResolveNowOption) { rn <- struct{}{} } - // The test balancer will return ErrBadResolverState iff the // ClientConnState contains no addresses. fb := &funcBalancer{ @@ -90,48 +75,18 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { return nil }, } - const balName = "BalancerErrorResolverPolling" + const balName = "BalancerErrorResolverPolling2" balancer.Register(&funcBalancerBuilder{name: balName, instance: fb}) - cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithDefaultServiceConfig(fmt.Sprintf(` -{ - "loadBalancingConfig": [{"%v": {}}] -}`, balName))) - if err != nil { - t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) - } - defer cc.Close() - r.CC.UpdateState(resolver.State{}) - - panicAfter := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) - defer panicAfter.Stop() - - // Ensure ResolveNow is called, then Backoff with the right parameter, several times - for i := 0; i < 7; i++ { - <-rn - if v := <-boIter; v != i { - t.Errorf("Backoff call %v uses value %v", i, v) - } - } - - // UpdateState will block if ResolveNow is being called (which blocks on - // rn), so call it in a goroutine. Include some address so the balancer - // will be happy. - go r.CC.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "x"}}}) - - // Wait awhile to ensure ResolveNow and Backoff stop being called when the - // state is OK (i.e. polling was cancelled). - for { - t := time.NewTimer(50 * time.Millisecond) - select { - case <-rn: - // ClientConn is still calling ResolveNow - <-boIter - time.Sleep(5 * time.Millisecond) - continue - case <-t.C: - // ClientConn stopped calling ResolveNow; success - } - break - } + testResolverErrorPolling(t, + func(r *manual.Resolver) { + // No addresses so the balancer will fail. + r.CC.UpdateState(resolver.State{}) + }, func(r *manual.Resolver) { + // UpdateState will block if ResolveNow is being called (which blocks on + // rn), so call it in a goroutine. Include some address so the balancer + // will be happy. + go r.CC.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "x"}}}) + }, + WithDefaultServiceConfig(fmt.Sprintf(`{ "loadBalancingConfig": [{"%v": {}}] }`, balName))) } diff --git a/clientconn.go b/clientconn.go index 840d5583dd1..1e4de76bb42 100644 --- a/clientconn.go +++ b/clientconn.go @@ -587,6 +587,9 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { if sc, ok := scg.(*ServiceConfig); err == nil && ok { cc.applyServiceConfigAndBalancer(sc, s.Addresses) } else { + if cc.balancerWrapper == nil { + cc.applyServiceConfigAndBalancer(emptyServiceConfig, s.Addresses) + } ret = balancer.ErrBadResolverState } } diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 06d87b8f725..12194c8abfa 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -117,10 +117,7 @@ func (s) TestDialParseTargetUnknownScheme(t *testing.T) { } } -// TestResolverErrorPolling injects resolver errors and verifies ResolveNow is -// called with the appropriate backoff strategy being consulted between -// ResolveNow calls. -func (s) TestResolverErrorPolling(t *testing.T) { +func testResolverErrorPolling(t *testing.T, badUpdate func(*manual.Resolver), goodUpdate func(*manual.Resolver), dopts ...DialOption) { defer func(o func(int) time.Duration) { resolverBackoff = o }(resolverBackoff) boIter := make(chan int) @@ -135,12 +132,12 @@ func (s) TestResolverErrorPolling(t *testing.T) { defer func() { close(rn) }() r.ResolveNowCallback = func(resolver.ResolveNowOption) { rn <- struct{}{} } - cc, err := Dial(r.Scheme()+":///test.server", WithInsecure()) + cc, err := Dial(r.Scheme()+":///test.server", append([]DialOption{WithInsecure()}, dopts...)...) if err != nil { t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) } defer cc.Close() - r.CC.ReportError(errors.New("res err")) + badUpdate(r) panicAfter := time.AfterFunc(5*time.Second, func() { panic("timed out polling resolver") }) defer panicAfter.Stop() @@ -155,7 +152,7 @@ func (s) TestResolverErrorPolling(t *testing.T) { // UpdateState will block if ResolveNow is being called (which blocks on // rn), so call it in a goroutine. - go r.CC.UpdateState(resolver.State{}) + goodUpdate(r) // Wait awhile to ensure ResolveNow and Backoff stop being called when the // state is OK (i.e. polling was cancelled). @@ -173,3 +170,30 @@ func (s) TestResolverErrorPolling(t *testing.T) { break } } + +// TestResolverErrorPolling injects resolver errors and verifies ResolveNow is +// called with the appropriate backoff strategy being consulted between +// ResolveNow calls. +func (s) TestResolverErrorPolling(t *testing.T) { + testResolverErrorPolling(t, func(r *manual.Resolver) { + r.CC.ReportError(errors.New("res err")) + }, func(r *manual.Resolver) { + // UpdateState will block if ResolveNow is being called (which blocks on + // rn), so call it in a goroutine. + go r.CC.UpdateState(resolver.State{}) + }) +} + +// TestServiceConfigErrorPolling injects a service config error and verifies +// ResolveNow is called with the appropriate backoff strategy being consulted +// between ResolveNow calls. +func (s) TestServiceConfigErrorPolling(t *testing.T) { + testResolverErrorPolling(t, func(r *manual.Resolver) { + badsc := r.CC.ParseServiceConfig("bad config") + r.UpdateState(resolver.State{ServiceConfigGetter: badsc}) + }, func(r *manual.Resolver) { + // UpdateState will block if ResolveNow is being called (which blocks on + // rn), so call it in a goroutine. + go r.CC.UpdateState(resolver.State{}) + }) +} From 9712cf58d0973b6b0c1ba5624f2ddcdfe373fdcb Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 2 Oct 2019 09:45:32 -0700 Subject: [PATCH 09/14] convert serviceconfig.Getter to serviceconfig.ParseResult --- balancer/grpclb/grpclb_test.go | 14 +++++----- balancer_switching_test.go | 28 +++++++++---------- clientconn.go | 21 +++++++------- clientconn_test.go | 10 +++---- internal/internal.go | 2 +- resolver/dns/dns_resolver_test.go | 2 +- resolver/resolver.go | 6 ++-- resolver_conn_wrapper.go | 28 +++++++++++-------- resolver_conn_wrapper_test.go | 2 +- service_config.go | 24 ++++++++-------- service_config_test.go | 8 +++--- serviceconfig/serviceconfig.go | 29 +++++++------------- test/channelz_test.go | 12 ++++---- test/end2end_test.go | 30 ++++++++++---------- test/healthcheck_test.go | 32 +++++++++++----------- xds/internal/resolver/xds_resolver.go | 8 +++--- xds/internal/resolver/xds_resolver_test.go | 4 +-- 17 files changed, 127 insertions(+), 133 deletions(-) diff --git a/balancer/grpclb/grpclb_test.go b/balancer/grpclb/grpclb_test.go index 9ced7240bcc..972fc617e6a 100644 --- a/balancer/grpclb/grpclb_test.go +++ b/balancer/grpclb/grpclb_test.go @@ -902,9 +902,9 @@ func TestGRPCLBPickFirst(t *testing.T) { // Start with sub policy pick_first. const pfc = `{"loadBalancingConfig":[{"grpclb":{"childPolicy":[{"pick_first":{}}]}}]}` - svcCfg := r.CC.ParseServiceConfig(pfc) - if _, err := svcCfg.Get(); err != nil { - t.Fatalf("Error parsing config %q: %v", pfc, err) + scpr := r.CC.ParseServiceConfig(pfc) + if scpr.Err != nil { + t.Fatalf("Error parsing config %q: %v", pfc, scpr.Err) } r.UpdateState(resolver.State{ @@ -913,7 +913,7 @@ func TestGRPCLBPickFirst(t *testing.T) { Type: resolver.GRPCLB, ServerName: lbServerName, }}, - ServiceConfigGetter: svcCfg, + ServiceConfig: scpr, }) result = "" @@ -953,8 +953,8 @@ func TestGRPCLBPickFirst(t *testing.T) { // Switch sub policy to roundrobin. grpclbServiceConfigEmpty := r.CC.ParseServiceConfig(`{}`) - if _, err := grpclbServiceConfigEmpty.Get(); err != nil { - t.Fatalf("Error parsing config %q: %v", `{}`, err) + if grpclbServiceConfigEmpty.Err != nil { + t.Fatalf("Error parsing config %q: %v", `{}`, grpclbServiceConfigEmpty.Err) } r.UpdateState(resolver.State{ @@ -963,7 +963,7 @@ func TestGRPCLBPickFirst(t *testing.T) { Type: resolver.GRPCLB, ServerName: lbServerName, }}, - ServiceConfigGetter: grpclbServiceConfigEmpty, + ServiceConfig: grpclbServiceConfigEmpty, }) result = "" diff --git a/balancer_switching_test.go b/balancer_switching_test.go index 903e4732fe7..7a6449fc0f3 100644 --- a/balancer_switching_test.go +++ b/balancer_switching_test.go @@ -149,12 +149,12 @@ func (s) TestSwitchBalancer(t *testing.T) { t.Fatalf("check pickfirst returned non-nil error: %v", err) } // Switch to roundrobin. - cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) + cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) if err := checkRoundRobin(cc, servers); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) } // Switch to pickfirst. - cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) + cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) if err := checkPickFirst(cc, servers); err != nil { t.Fatalf("check pickfirst returned non-nil error: %v", err) } @@ -181,7 +181,7 @@ func (s) TestBalancerDialOption(t *testing.T) { t.Fatalf("check roundrobin returned non-nil error: %v", err) } // Switch to pickfirst. - cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) + cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "pick_first"}`), Addresses: addrs}, nil) // Balancer is still roundrobin. if err := checkRoundRobin(cc, servers); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) @@ -339,7 +339,7 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { sc := parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`) - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) var isRoundRobin bool for i := 0; i < 5000; i++ { cc.mu.Lock() @@ -356,7 +356,7 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { // ClientConn will switch balancer to grpclb when receives an address of // type GRPCLB. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "grpclb", Type: resolver.GRPCLB}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "grpclb", Type: resolver.GRPCLB}}, ServiceConfig: sc}) var isGRPCLB bool for i := 0; i < 5000; i++ { cc.mu.Lock() @@ -372,7 +372,7 @@ func (s) TestSwitchBalancerGRPCLBRoundRobin(t *testing.T) { } // Switch balancer back. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) for i := 0; i < 5000; i++ { cc.mu.Lock() isRoundRobin = cc.curBalancerName == "round_robin" @@ -434,7 +434,7 @@ func (s) TestSwitchBalancerGRPCLBServiceConfig(t *testing.T) { } sc := parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`) - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: sc}) var isRoundRobin bool for i := 0; i < 200; i++ { cc.mu.Lock() @@ -452,7 +452,7 @@ func (s) TestSwitchBalancerGRPCLBServiceConfig(t *testing.T) { } // Switch balancer back. - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "backend"}}, ServiceConfig: sc}) for i := 0; i < 5000; i++ { cc.mu.Lock() isRoundRobin = cc.curBalancerName == "round_robin" @@ -510,16 +510,16 @@ func (s) TestSwitchBalancerGRPCLBWithGRPCLBNotRegistered(t *testing.T) { t.Fatalf("check pickfirst returned non-nil error: %v", err) } // Switch to roundrobin, and check against server[1] and server[2]. - cc.updateResolverState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) + cc.updateResolverState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`), Addresses: addrs}, nil) if err := checkRoundRobin(cc, servers[1:]); err != nil { t.Fatalf("check roundrobin returned non-nil error: %v", err) } } -func parseCfg(r *manual.Resolver, s string) *serviceconfig.Getter { - g := r.CC.ParseServiceConfig(s) - if _, err := g.Get(); err != nil { - panic(fmt.Sprintf("Error parsing config %q: %v", s, err)) +func parseCfg(r *manual.Resolver, s string) *serviceconfig.ParseResult { + scpr := r.CC.ParseServiceConfig(s) + if scpr.Err != nil { + panic(fmt.Sprintf("Error parsing config %q: %v", s, scpr.Err)) } - return g + return scpr } diff --git a/clientconn.go b/clientconn.go index 1e4de76bb42..f869027b247 100644 --- a/clientconn.go +++ b/clientconn.go @@ -186,11 +186,11 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } if cc.dopts.defaultServiceConfigRawJSON != nil { - cfg, err := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON).Get() - if err != nil { - return nil, fmt.Errorf("%s: %v", invalidDefaultServiceConfigErrPrefix, err) + scpr := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON) + if scpr.Err != nil { + return nil, fmt.Errorf("%s: %v", invalidDefaultServiceConfigErrPrefix, scpr.Err) } - cc.dopts.defaultServiceConfig, _ = cfg.(*ServiceConfig) + cc.dopts.defaultServiceConfig, _ = scpr.Config.(*ServiceConfig) } cc.mkp = cc.dopts.copts.KeepaliveParams @@ -533,11 +533,11 @@ func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error { var emptyServiceConfig *ServiceConfig func init() { - cfg, err := parseServiceConfig("{}").Get() - if err != nil { - panic(fmt.Sprintf("impossible error parsing empty service config: %v", err)) + cfg := parseServiceConfig("{}") + if cfg.Err != nil { + panic(fmt.Sprintf("impossible error parsing empty service config: %v", cfg.Err)) } - emptyServiceConfig = cfg.(*ServiceConfig) + emptyServiceConfig = cfg.Config.(*ServiceConfig) } func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { @@ -578,13 +578,12 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { } var ret error - if cc.dopts.disableServiceConfig || s.ServiceConfigGetter == nil { + if cc.dopts.disableServiceConfig || s.ServiceConfig == nil { cc.maybeApplyDefaultServiceConfig(s.Addresses) // TODO: do we need to apply a failing LB policy if there is no // default, per the error handling design? } else { - scg, err := s.ServiceConfigGetter.Get() - if sc, ok := scg.(*ServiceConfig); err == nil && ok { + if sc, ok := s.ServiceConfig.Config.(*ServiceConfig); s.ServiceConfig.Err == nil && ok { cc.applyServiceConfigAndBalancer(sc, s.Addresses) } else { if cc.balancerWrapper == nil { diff --git a/clientconn_test.go b/clientconn_test.go index bae6d75c04c..2b8b8176d80 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -780,7 +780,7 @@ func (s) TestResolverServiceConfigBeforeAddressNotPanic(t *testing.T) { // SwitchBalancer before NewAddress. There was no balancer created, this // makes sure we don't call close on nil balancerWrapper. - r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. + r.UpdateState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. time.Sleep(time.Second) // Sleep to make sure the service config is handled by ClientConn. } @@ -796,7 +796,7 @@ func (s) TestResolverServiceConfigWhileClosingNotPanic(t *testing.T) { } // Send a new service config while closing the ClientConn. go cc.Close() - go r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. + go r.UpdateState(resolver.State{ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // This should not panic. } } @@ -889,7 +889,7 @@ func (s) TestDisableServiceConfigOption(t *testing.T) { t.Fatalf("Dial(%s, _) = _, %v, want _, ", addr, err) } defer cc.Close() - r.UpdateState(resolver.State{ServiceConfigGetter: parseCfg(r, `{ + r.UpdateState(resolver.State{ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1189,8 +1189,8 @@ func testDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T, r * defer cc.Close() // Resolver service config gets ignored since resolver service config is disabled. r.UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfigGetter: parseCfg(r, "{}"), + Addresses: []resolver.Address{{Addr: addr}}, + ServiceConfig: parseCfg(r, "{}"), }) if !verifyWaitForReadyEqualsTrue(cc) { t.Fatal("default service config failed to be applied after 1s") diff --git a/internal/internal.go b/internal/internal.go index 39d21c1bc06..cbfe345b92c 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -49,7 +49,7 @@ var ( NewRequestInfoContext interface{} // func(context.Context, credentials.RequestInfo) context.Context // ParseServiceConfigForTesting is for creating a fake // ClientConn for resolver testing only - ParseServiceConfigForTesting interface{} // func(string) *serviceconfig.Getter + ParseServiceConfigForTesting interface{} // func(string) *serviceconfig.ParseResult ) // HealthChecker defines the signature of the client-side LB channel health checking function. diff --git a/resolver/dns/dns_resolver_test.go b/resolver/dns/dns_resolver_test.go index ac0ab27968a..7a845485255 100644 --- a/resolver/dns/dns_resolver_test.go +++ b/resolver/dns/dns_resolver_test.go @@ -90,7 +90,7 @@ func (t *testClientConn) getSc() (string, int) { return t.sc, t.s } -func (t *testClientConn) ParseServiceConfig(string) *serviceconfig.Getter { +func (t *testClientConn) ParseServiceConfig(string) *serviceconfig.ParseResult { panic("not implemented") } diff --git a/resolver/resolver.go b/resolver/resolver.go index 5489738e20f..afc8e6a55c0 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -107,10 +107,10 @@ type State struct { // Addresses is the latest set of resolved addresses for the target. Addresses []Address - // ServiceConfigGetter contains the result from parsing the latest service + // ServiceConfig contains the result from parsing the latest service // config. If it is nil, it indicates no service config is present or the // resolver does not provide service configs. - ServiceConfigGetter *serviceconfig.Getter + ServiceConfig *serviceconfig.ParseResult } // ClientConn contains the callbacks for resolver to notify any updates @@ -140,7 +140,7 @@ type ClientConn interface { NewServiceConfig(serviceConfig string) // ParseServiceConfig parses the provided service config and returns an // object that provides the parsed config. - ParseServiceConfig(serviceConfigJSON string) *serviceconfig.Getter + ParseServiceConfig(serviceConfigJSON string) *serviceconfig.ParseResult } // Target represents a target for gRPC, as specified in: diff --git a/resolver_conn_wrapper.go b/resolver_conn_wrapper.go index d4b523abc2b..ead72259dad 100644 --- a/resolver_conn_wrapper.go +++ b/resolver_conn_wrapper.go @@ -191,7 +191,7 @@ func (ccr *ccResolverWrapper) NewAddress(addrs []resolver.Address) { } grpclog.Infof("ccResolverWrapper: sending new addresses to cc: %v", addrs) if channelz.IsOn() { - ccr.addChannelzTraceEvent(resolver.State{Addresses: addrs, ServiceConfigGetter: ccr.curState.ServiceConfigGetter}) + ccr.addChannelzTraceEvent(resolver.State{Addresses: addrs, ServiceConfig: ccr.curState.ServiceConfig}) } ccr.curState.Addresses = addrs ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil)) @@ -204,12 +204,12 @@ func (ccr *ccResolverWrapper) NewServiceConfig(sc string) { return } grpclog.Infof("ccResolverWrapper: got new service config: %v", sc) - scg := parseServiceConfig(sc) - if _, err := scg.Get(); err != nil { - grpclog.Warningf("ccResolverWrapper: error parsing service config: %v", err) + scpr := parseServiceConfig(sc) + if scpr.Err != nil { + grpclog.Warningf("ccResolverWrapper: error parsing service config: %v", scpr.Err) if channelz.IsOn() { channelz.AddTraceEvent(ccr.cc.channelzID, &channelz.TraceEventDesc{ - Desc: fmt.Sprintf("Error parsing service config: %v", err), + Desc: fmt.Sprintf("Error parsing service config: %v", scpr.Err), Severity: channelz.CtWarning, }) } @@ -217,22 +217,26 @@ func (ccr *ccResolverWrapper) NewServiceConfig(sc string) { return } if channelz.IsOn() { - ccr.addChannelzTraceEvent(resolver.State{Addresses: ccr.curState.Addresses, ServiceConfigGetter: scg}) + ccr.addChannelzTraceEvent(resolver.State{Addresses: ccr.curState.Addresses, ServiceConfig: scpr}) } - ccr.curState.ServiceConfigGetter = scg + ccr.curState.ServiceConfig = scpr ccr.poll(ccr.cc.updateResolverState(ccr.curState, nil)) } -func (ccr *ccResolverWrapper) ParseServiceConfig(scJSON string) *serviceconfig.Getter { +func (ccr *ccResolverWrapper) ParseServiceConfig(scJSON string) *serviceconfig.ParseResult { return parseServiceConfig(scJSON) } func (ccr *ccResolverWrapper) addChannelzTraceEvent(s resolver.State) { var updates []string - oldSCI, _ := ccr.curState.ServiceConfigGetter.Get() - oldSC, oldOK := oldSCI.(*ServiceConfig) - newSCI, _ := s.ServiceConfigGetter.Get() - newSC, newOK := newSCI.(*ServiceConfig) + var oldSC, newSC *ServiceConfig + var oldOK, newOK bool + if ccr.curState.ServiceConfig != nil { + oldSC, oldOK = ccr.curState.ServiceConfig.Config.(*ServiceConfig) + } + if s.ServiceConfig != nil { + newSC, newOK = s.ServiceConfig.Config.(*ServiceConfig) + } if oldOK != newOK || (oldOK && newOK && oldSC.rawJSONString != newSC.rawJSONString) { updates = append(updates, "service config updated") } diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 12194c8abfa..4127d644784 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -190,7 +190,7 @@ func (s) TestResolverErrorPolling(t *testing.T) { func (s) TestServiceConfigErrorPolling(t *testing.T) { testResolverErrorPolling(t, func(r *manual.Resolver) { badsc := r.CC.ParseServiceConfig("bad config") - r.UpdateState(resolver.State{ServiceConfigGetter: badsc}) + r.UpdateState(resolver.State{ServiceConfig: badsc}) }, func(r *manual.Resolver) { // UpdateState will block if ResolveNow is being called (which blocks on // rn), so call it in a goroutine. diff --git a/service_config.go b/service_config.go index e6bfc82211e..926adb0a782 100644 --- a/service_config.go +++ b/service_config.go @@ -263,15 +263,15 @@ type jsonSC struct { func init() { internal.ParseServiceConfigForTesting = parseServiceConfig } -func parseServiceConfig(js string) *serviceconfig.Getter { +func parseServiceConfig(js string) *serviceconfig.ParseResult { if len(js) == 0 { - return serviceconfig.NewGetter(fmt.Errorf("no JSON service config provided")) + return serviceconfig.NewParseResult(fmt.Errorf("no JSON service config provided")) } var rsc jsonSC err := json.Unmarshal([]byte(js), &rsc) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewGetter(err) + return serviceconfig.NewParseResult(err) } sc := ServiceConfig{ LB: rsc.LoadBalancingPolicy, @@ -285,7 +285,7 @@ func parseServiceConfig(js string) *serviceconfig.Getter { if len(lbcfg) != 1 { err := fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg) grpclog.Warningf(err.Error()) - return serviceconfig.NewGetter(err) + return serviceconfig.NewParseResult(err) } var name string var jsonCfg json.RawMessage @@ -300,7 +300,7 @@ func parseServiceConfig(js string) *serviceconfig.Getter { var err error sc.lbConfig.cfg, err = parser.ParseConfig(jsonCfg) if err != nil { - return serviceconfig.NewGetter(fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)) + return serviceconfig.NewParseResult(fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)) } } else if string(jsonCfg) != "{}" { grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg)) @@ -313,12 +313,12 @@ func parseServiceConfig(js string) *serviceconfig.Getter { // case. err := fmt.Errorf("invalid loadBalancingConfig: no supported policies found") grpclog.Warningf(err.Error()) - return serviceconfig.NewGetter(err) + return serviceconfig.NewParseResult(err) } } if rsc.MethodConfig == nil { - return serviceconfig.NewGetter(&sc) + return serviceconfig.NewParseResult(&sc) } for _, m := range *rsc.MethodConfig { if m.Name == nil { @@ -327,7 +327,7 @@ func parseServiceConfig(js string) *serviceconfig.Getter { d, err := parseDuration(m.Timeout) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewGetter(err) + return serviceconfig.NewParseResult(err) } mc := MethodConfig{ @@ -336,7 +336,7 @@ func parseServiceConfig(js string) *serviceconfig.Getter { } if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewGetter(err) + return serviceconfig.NewParseResult(err) } if m.MaxRequestMessageBytes != nil { if *m.MaxRequestMessageBytes > int64(maxInt) { @@ -361,13 +361,13 @@ func parseServiceConfig(js string) *serviceconfig.Getter { if sc.retryThrottling != nil { if mt := sc.retryThrottling.MaxTokens; mt <= 0 || mt > 1000 { - return serviceconfig.NewGetter(fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)) + return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)) } if tr := sc.retryThrottling.TokenRatio; tr <= 0 { - return serviceconfig.NewGetter(fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)) + return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)) } } - return serviceconfig.NewGetter(&sc) + return serviceconfig.NewParseResult(&sc) } func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) { diff --git a/service_config_test.go b/service_config_test.go index 1dde66dd08a..ec9f56db2c7 100644 --- a/service_config_test.go +++ b/service_config_test.go @@ -39,14 +39,14 @@ type parseTestCase struct { func runParseTests(t *testing.T, testCases []parseTestCase) { t.Helper() for _, c := range testCases { - sci, err := parseServiceConfig(c.scjs).Get() + scpr := parseServiceConfig(c.scjs) var sc *ServiceConfig - sc, _ = sci.(*ServiceConfig) + sc, _ = scpr.Config.(*ServiceConfig) if !c.wantErr { c.wantSC.rawJSONString = c.scjs } - if c.wantErr != (err != nil) || !reflect.DeepEqual(sc, c.wantSC) { - t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr) + if c.wantErr != (scpr.Err != nil) || !reflect.DeepEqual(sc, c.wantSC) { + t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, scpr.Err, c.wantSC, c.wantErr) } } } diff --git a/serviceconfig/serviceconfig.go b/serviceconfig/serviceconfig.go index 341212d5c08..7779e8c97c7 100644 --- a/serviceconfig/serviceconfig.go +++ b/serviceconfig/serviceconfig.go @@ -39,30 +39,21 @@ type LoadBalancingConfig interface { isLoadBalancingConfig() } -// Getter provides a service config or an error. -type Getter struct { - err error - config Config +// ParseResult contains a service config or an error. +type ParseResult struct { + Config Config + Err error } -// Get returns either a service config or an error. -func (g *Getter) Get() (Config, error) { - if g == nil { - return nil, nil - } - // Only one field in g may be non-nil. - return g.config, g.err -} - -// NewGetter returns a Getter returning the provided parameter as either the -// serviceconfig.Config or error return value, depending upon the type. -func NewGetter(configOrError interface{}) *Getter { +// NewParseResult returns a ParseResult returning the provided parameter as +// either the Config or Err field, depending upon its type. +func NewParseResult(configOrError interface{}) *ParseResult { if e, ok := configOrError.(error); ok { - return &Getter{err: e} + return &ParseResult{Err: e} } if c, ok := configOrError.(Config); ok { - return &Getter{config: c} + return &ParseResult{Config: c} } grpclog.Errorf("Unexpected configOrError type: %T", configOrError) - return &Getter{err: status.Errorf(codes.Internal, "unexpected configOrError type: %T", configOrError)} + return &ParseResult{Err: status.Errorf(codes.Internal, "unexpected configOrError type: %T", configOrError)} } diff --git a/test/channelz_test.go b/test/channelz_test.go index f488eb20e71..8ce0a677f47 100644 --- a/test/channelz_test.go +++ b/test/channelz_test.go @@ -230,7 +230,7 @@ func (s) TestCZNestedChannelRegistrationAndDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { @@ -1423,7 +1423,7 @@ func (s) TestCZChannelTraceCreationDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { @@ -1567,7 +1567,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { }); err != nil { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1598,7 +1598,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { } ] }`) - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: newSC}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: newSC}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1618,7 +1618,7 @@ func (s) TestCZChannelAddressResolutionChange(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: newSC}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: newSC}) if err := verifyResultWithDelay(func() (bool, error) { cm := channelz.GetChannel(cid) @@ -1882,7 +1882,7 @@ func (s) TestCZTraceOverwriteChannelDeletion(t *testing.T) { t.Fatal(err) } - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfigGetter: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: "127.0.0.1:0"}}, ServiceConfig: parseCfg(r, `{"loadBalancingPolicy": "round_robin"}`)}) // wait for the shutdown of grpclb balancer if err := verifyResultWithDelay(func() (bool, error) { diff --git a/test/end2end_test.go b/test/end2end_test.go index 242f9374094..83ae7aab1cc 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -1461,7 +1461,7 @@ func (s) TestGetMethodConfig(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1500,7 +1500,7 @@ func (s) TestGetMethodConfig(t *testing.T) { t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want _, %s", err, codes.DeadlineExceeded) } - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: parseCfg(r, `{ + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1548,7 +1548,7 @@ func (s) TestServiceConfigWaitForReady(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1590,7 +1590,7 @@ func (s) TestServiceConfigWaitForReady(t *testing.T) { // Case2:Client API set failfast to be false, and service config set wait_for_ready to be true, and the rpc will wait until deadline exceeds. r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1637,7 +1637,7 @@ func (s) TestServiceConfigTimeout(t *testing.T) { addrs := []resolver.Address{{Addr: te.srvAddr}} r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1684,7 +1684,7 @@ func (s) TestServiceConfigTimeout(t *testing.T) { // Case2: Client API sets timeout to be 1hr and ServiceConfig sets timeout to be 1ns. Timeout should be 1ns (min of 1ns and 1hr) and the rpc will wait until deadline exceeds. r.UpdateState(resolver.State{ Addresses: addrs, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -1775,7 +1775,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { } ] }`) - r.UpdateState(resolver.State{Addresses: addrs, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: addrs, ServiceConfig: sc}) tc := testpb.NewTestServiceClient(cc1) req := &testpb.SimpleRequest{ @@ -1846,7 +1846,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { te2.startServer(&testServer{security: e.security}) defer te2.tearDown() cc2 := te2.clientConn() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te2.srvAddr}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te2.srvAddr}}, ServiceConfig: sc}) tc = testpb.NewTestServiceClient(cc2) for { @@ -1907,7 +1907,7 @@ func (s) TestServiceConfigMaxMsgSize(t *testing.T) { defer te3.tearDown() cc3 := te3.clientConn() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te3.srvAddr}}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: te3.srvAddr}}, ServiceConfig: sc}) tc = testpb.NewTestServiceClient(cc3) for { @@ -1999,7 +1999,7 @@ func (s) TestStreamingRPCWithTimeoutInServiceConfigRecv(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: te.srvAddr}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -5311,7 +5311,7 @@ func (ss *stubServer) Start(sopts []grpc.ServerOption, dopts ...grpc.DialOption) } func (ss *stubServer) newServiceConfig(sc string) { - ss.r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.addr}}, ServiceConfigGetter: parseCfg(ss.r, sc)}) + ss.r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.addr}}, ServiceConfig: parseCfg(ss.r, sc)}) } func (ss *stubServer) waitForReady(cc *grpc.ClientConn) error { @@ -7229,7 +7229,7 @@ func (s) TestRPCWaitsForResolver(t *testing.T) { time.Sleep(time.Second) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: te.srvAddr}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "methodConfig": [ { "name": [ @@ -7476,10 +7476,10 @@ func doHTTPHeaderTest(t *testing.T, errCode codes.Code, headerFields ...[]string } } -func parseCfg(r *manual.Resolver, s string) *serviceconfig.Getter { +func parseCfg(r *manual.Resolver, s string) *serviceconfig.ParseResult { g := r.CC.ParseServiceConfig(s) - if _, err := g.Get(); err != nil { - panic(fmt.Sprintf("Error parsing config %q: %v", s, err)) + if g.Err != nil { + panic(fmt.Sprintf("Error parsing config %q: %v", s, g.Err)) } return g } diff --git a/test/healthcheck_test.go b/test/healthcheck_test.go index 0fd3cd6009b..1cf93a1f80b 100644 --- a/test/healthcheck_test.go +++ b/test/healthcheck_test.go @@ -194,7 +194,7 @@ func (s) TestHealthCheckWatchStateChange(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -262,7 +262,7 @@ func (s) TestHealthCheckHealthServerNotRegistered(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -306,7 +306,7 @@ func (s) TestHealthCheckWithGoAway(t *testing.T) { tc := testpb.NewTestServiceClient(cc) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -398,7 +398,7 @@ func (s) TestHealthCheckWithConnClose(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -463,8 +463,8 @@ func (s) TestHealthCheckWithAddrConnDrain(t *testing.T) { } }`) r.UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: sc, + Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, + ServiceConfig: sc, }) // make some rpcs to make sure connection is working. @@ -506,7 +506,7 @@ func (s) TestHealthCheckWithAddrConnDrain(t *testing.T) { default: } // trigger teardown of the ac - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: sc}) select { case <-hcExitChan: @@ -552,7 +552,7 @@ func (s) TestHealthCheckWithClientConnClose(t *testing.T) { tc := testpb.NewTestServiceClient(cc) r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -636,8 +636,8 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalledAddrConnShutDown(t *tes } }`) r.UpdateState(resolver.State{ - Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: sc, + Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, + ServiceConfig: sc, }) select { @@ -652,7 +652,7 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalledAddrConnShutDown(t *tes t.Fatal("Health check function has not been invoked after 5s.") } // trigger teardown of the ac, ac in SHUTDOWN state - r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfigGetter: sc}) + r.UpdateState(resolver.State{Addresses: []resolver.Address{}, ServiceConfig: sc}) // The health check func should exit without calling the setConnectivityState func, as server hasn't sent // any response. @@ -708,7 +708,7 @@ func (s) TestHealthCheckWithoutSetConnectivityStateCalled(t *testing.T) { // test ends). r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "delay" } @@ -756,7 +756,7 @@ func testHealthCheckDisableWithDialOption(t *testing.T, addr string) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -795,7 +795,7 @@ func testHealthCheckDisableWithBalancer(t *testing.T, addr string) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "foo" } @@ -888,7 +888,7 @@ func (s) TestHealthCheckChannelzCountingCallSuccess(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "channelzSuccess" } @@ -944,7 +944,7 @@ func (s) TestHealthCheckChannelzCountingCallFailure(t *testing.T) { r.UpdateState(resolver.State{ Addresses: []resolver.Address{{Addr: lis.Addr().String()}}, - ServiceConfigGetter: parseCfg(r, `{ + ServiceConfig: parseCfg(r, `{ "healthCheckConfig": { "serviceName": "channelzFailure" } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 02f8aa2b32e..352caeaca31 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -62,15 +62,15 @@ type xdsBuilder struct{} func (b *xdsBuilder) Build(t resolver.Target, cc resolver.ClientConn, o resolver.BuildOption) (resolver.Resolver, error) { // The xds balancer must have been registered at this point for the service // config to be parsed properly. - sc := cc.ParseServiceConfig(jsonSC) + scpr := cc.ParseServiceConfig(jsonSC) - if _, err := sc.Get(); err != nil { - panic(fmt.Sprintf("error parsing service config %q: %v", jsonSC, err)) + if scpr.Err != nil { + panic(fmt.Sprintf("error parsing service config %q: %v", jsonSC, scpr.Err)) } // We return a resolver which bacically does nothing. The hard-coded service // config returned here picks the xds balancer. - cc.UpdateState(resolver.State{ServiceConfigGetter: sc}) + cc.UpdateState(resolver.State{ServiceConfig: scpr}) return &xdsResolver{}, nil } diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index 408a90f054d..372dad8c480 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -68,8 +68,8 @@ func (t *testClientConn) UpdateState(s resolver.State) { close(t.done) } -func (*testClientConn) ParseServiceConfig(jsonSC string) *serviceconfig.Getter { - return internal.ParseServiceConfigForTesting.(func(string) *serviceconfig.Getter)(jsonSC) +func (*testClientConn) ParseServiceConfig(jsonSC string) *serviceconfig.ParseResult { + return internal.ParseServiceConfigForTesting.(func(string) *serviceconfig.ParseResult)(jsonSC) } func (*testClientConn) NewAddress([]resolver.Address) { panic("unimplemented") } From ef23040e078482b38fba32084145a099d40ff8dd Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 2 Oct 2019 10:21:54 -0700 Subject: [PATCH 10/14] no2 --- balancer_conn_wrappers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go index 6d8bf74e159..1b4ad1c1916 100644 --- a/balancer_conn_wrappers_test.go +++ b/balancer_conn_wrappers_test.go @@ -75,7 +75,7 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { return nil }, } - const balName = "BalancerErrorResolverPolling2" + const balName = "BalancerErrorResolverPolling" balancer.Register(&funcBalancerBuilder{name: balName, instance: fb}) testResolverErrorPolling(t, From d00f13b23f2014ba8bacb876bbcb442ab6d5e96d Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 2 Oct 2019 12:52:34 -0700 Subject: [PATCH 11/14] review comments --- clientconn.go | 8 ++++++-- service_config.go | 22 +++++++++++----------- serviceconfig/serviceconfig.go | 22 ++-------------------- 3 files changed, 19 insertions(+), 33 deletions(-) diff --git a/clientconn.go b/clientconn.go index f869027b247..79f404a7810 100644 --- a/clientconn.go +++ b/clientconn.go @@ -31,6 +31,7 @@ import ( "time" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/base" _ "google.golang.org/grpc/balancer/roundrobin" // To register roundrobin. "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" @@ -586,10 +587,13 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { if sc, ok := s.ServiceConfig.Config.(*ServiceConfig); s.ServiceConfig.Err == nil && ok { cc.applyServiceConfigAndBalancer(sc, s.Addresses) } else { + ret = balancer.ErrBadResolverState if cc.balancerWrapper == nil { - cc.applyServiceConfigAndBalancer(emptyServiceConfig, s.Addresses) + cc.blockingpicker.updatePicker(base.NewErrPicker(status.Errorf(codes.Unavailable, "error parsing service config: %v", s.ServiceConfig.Err))) + cc.csMgr.updateState(connectivity.TransientFailure) + cc.mu.Unlock() + return ret } - ret = balancer.ErrBadResolverState } } diff --git a/service_config.go b/service_config.go index 926adb0a782..4f8836d48f6 100644 --- a/service_config.go +++ b/service_config.go @@ -265,13 +265,13 @@ func init() { } func parseServiceConfig(js string) *serviceconfig.ParseResult { if len(js) == 0 { - return serviceconfig.NewParseResult(fmt.Errorf("no JSON service config provided")) + return &serviceconfig.ParseResult{Err: fmt.Errorf("no JSON service config provided")} } var rsc jsonSC err := json.Unmarshal([]byte(js), &rsc) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewParseResult(err) + return &serviceconfig.ParseResult{Err: err} } sc := ServiceConfig{ LB: rsc.LoadBalancingPolicy, @@ -285,7 +285,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { if len(lbcfg) != 1 { err := fmt.Errorf("invalid loadBalancingConfig: entry %v does not contain exactly 1 policy/config pair: %q", i, lbcfg) grpclog.Warningf(err.Error()) - return serviceconfig.NewParseResult(err) + return &serviceconfig.ParseResult{Err: err} } var name string var jsonCfg json.RawMessage @@ -300,7 +300,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { var err error sc.lbConfig.cfg, err = parser.ParseConfig(jsonCfg) if err != nil { - return serviceconfig.NewParseResult(fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)) + return &serviceconfig.ParseResult{Err: fmt.Errorf("error parsing loadBalancingConfig for policy %q: %v", name, err)} } } else if string(jsonCfg) != "{}" { grpclog.Warningf("non-empty balancer configuration %q, but balancer does not implement ParseConfig", string(jsonCfg)) @@ -313,12 +313,12 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { // case. err := fmt.Errorf("invalid loadBalancingConfig: no supported policies found") grpclog.Warningf(err.Error()) - return serviceconfig.NewParseResult(err) + return &serviceconfig.ParseResult{Err: err} } } if rsc.MethodConfig == nil { - return serviceconfig.NewParseResult(&sc) + return &serviceconfig.ParseResult{Config: &sc} } for _, m := range *rsc.MethodConfig { if m.Name == nil { @@ -327,7 +327,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { d, err := parseDuration(m.Timeout) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewParseResult(err) + return &serviceconfig.ParseResult{Err: err} } mc := MethodConfig{ @@ -336,7 +336,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { } if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) - return serviceconfig.NewParseResult(err) + return &serviceconfig.ParseResult{Err: err} } if m.MaxRequestMessageBytes != nil { if *m.MaxRequestMessageBytes > int64(maxInt) { @@ -361,13 +361,13 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult { if sc.retryThrottling != nil { if mt := sc.retryThrottling.MaxTokens; mt <= 0 || mt > 1000 { - return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)) + return &serviceconfig.ParseResult{Err: fmt.Errorf("invalid retry throttling config: maxTokens (%v) out of range (0, 1000]", mt)} } if tr := sc.retryThrottling.TokenRatio; tr <= 0 { - return serviceconfig.NewParseResult(fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)) + return &serviceconfig.ParseResult{Err: fmt.Errorf("invalid retry throttling config: tokenRatio (%v) may not be negative", tr)} } } - return serviceconfig.NewParseResult(&sc) + return &serviceconfig.ParseResult{Config: &sc} } func convertRetryPolicy(jrp *jsonRetryPolicy) (p *retryPolicy, err error) { diff --git a/serviceconfig/serviceconfig.go b/serviceconfig/serviceconfig.go index 7779e8c97c7..187c304421c 100644 --- a/serviceconfig/serviceconfig.go +++ b/serviceconfig/serviceconfig.go @@ -22,12 +22,6 @@ // This package is EXPERIMENTAL. package serviceconfig -import ( - "google.golang.org/grpc/codes" - "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/status" -) - // Config represents an opaque data structure holding a service config. type Config interface { isServiceConfig() @@ -39,21 +33,9 @@ type LoadBalancingConfig interface { isLoadBalancingConfig() } -// ParseResult contains a service config or an error. +// ParseResult contains a service config or an error. Exactly one must be +// non-nil. type ParseResult struct { Config Config Err error } - -// NewParseResult returns a ParseResult returning the provided parameter as -// either the Config or Err field, depending upon its type. -func NewParseResult(configOrError interface{}) *ParseResult { - if e, ok := configOrError.(error); ok { - return &ParseResult{Err: e} - } - if c, ok := configOrError.(Config); ok { - return &ParseResult{Config: c} - } - grpclog.Errorf("Unexpected configOrError type: %T", configOrError) - return &ParseResult{Err: status.Errorf(codes.Internal, "unexpected configOrError type: %T", configOrError)} -} From 0a3f7b9430eedbff61a2a35e057d767dbdad622c Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 3 Oct 2019 09:09:28 -0700 Subject: [PATCH 12/14] fix err; add test; fix bug --- clientconn.go | 9 ++++++++- resolver_conn_wrapper_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/clientconn.go b/clientconn.go index 79f404a7810..e7e1584f94a 100644 --- a/clientconn.go +++ b/clientconn.go @@ -589,9 +589,16 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { } else { ret = balancer.ErrBadResolverState if cc.balancerWrapper == nil { - cc.blockingpicker.updatePicker(base.NewErrPicker(status.Errorf(codes.Unavailable, "error parsing service config: %v", s.ServiceConfig.Err))) + var err error + if s.ServiceConfig.Err != nil { + err = status.Errorf(codes.Unavailable, "error parsing service config: %v", s.ServiceConfig.Err) + } else { + err = status.Errorf(codes.Unavailable, "illegal service config type: %T", s.ServiceConfig.Config) + } + cc.blockingpicker.updatePicker(base.NewErrPicker(err)) cc.csMgr.updateState(connectivity.TransientFailure) cc.mu.Unlock() + cc.firstResolveEvent.Fire() return ret } } diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 4127d644784..60e7d0d7ad8 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -19,14 +19,18 @@ package grpc import ( + "context" "errors" "fmt" "net" + "strings" "testing" "time" + "google.golang.org/grpc/codes" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" + "google.golang.org/grpc/status" ) func (s) TestParseTarget(t *testing.T) { @@ -197,3 +201,25 @@ func (s) TestServiceConfigErrorPolling(t *testing.T) { go r.CC.UpdateState(resolver.State{}) }) } + +func (s) TestServiceConfigErrorRPC(t *testing.T) { + r, rcleanup := manual.GenerateAndRegisterManualResolver() + defer rcleanup() + + cc, err := Dial(r.Scheme()+":///test.server", WithInsecure()) + if err != nil { + t.Fatalf("Dial(_, _) = _, %v; want _, nil", err) + } + defer cc.Close() + badsc := r.CC.ParseServiceConfig("bad config") + r.UpdateState(resolver.State{ServiceConfig: badsc}) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + var dummy int + const wantMsg = "error parsing service config" + const wantCode = codes.Unavailable + if err := cc.Invoke(ctx, "/foo/bar", &dummy, &dummy); status.Code(err) != wantCode || !strings.Contains(status.Convert(err).Message(), wantMsg) { + t.Fatalf("cc.Invoke(_, _, _, _) = %v; want status.Code()==%v, status.Message() contains %q", err, wantCode, wantMsg) + } +} From cf12ff3ac050f165dcb5058511aa6e3b95dcdc27 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 3 Oct 2019 14:28:26 -0700 Subject: [PATCH 13/14] fire firstResolveEvent when done updating resolver state --- balancer_conn_wrappers.go | 1 - clientconn.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/balancer_conn_wrappers.go b/balancer_conn_wrappers.go index 7dd437e7f5c..8a5b89bc39b 100644 --- a/balancer_conn_wrappers.go +++ b/balancer_conn_wrappers.go @@ -140,7 +140,6 @@ func (ccb *ccBalancerWrapper) watcher() { ccb.UpdateBalancerState(connectivity.Connecting, nil) return } - ccb.cc.firstResolveEvent.Fire() } } diff --git a/clientconn.go b/clientconn.go index e7e1584f94a..9209eda8884 100644 --- a/clientconn.go +++ b/clientconn.go @@ -554,6 +554,7 @@ func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) { } func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { + defer cc.firstResolveEvent.Fire() cc.mu.Lock() // Check if the ClientConn is already closed. Some fields (e.g. // balancerWrapper) are set to nil when closing the ClientConn, and could @@ -598,7 +599,6 @@ func (cc *ClientConn) updateResolverState(s resolver.State, err error) error { cc.blockingpicker.updatePicker(base.NewErrPicker(err)) cc.csMgr.updateState(connectivity.TransientFailure) cc.mu.Unlock() - cc.firstResolveEvent.Fire() return ret } } From 433f3e0b1a65f2d8140f9eb157ba68c698bc1710 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Fri, 4 Oct 2019 12:26:10 -0700 Subject: [PATCH 14/14] merge resolution --- resolver_conn_wrapper.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/resolver_conn_wrapper.go b/resolver_conn_wrapper.go index ead72259dad..b4f2a5714c8 100644 --- a/resolver_conn_wrapper.go +++ b/resolver_conn_wrapper.go @@ -24,13 +24,15 @@ import ( "sync" "time" + "google.golang.org/grpc/backoff" "google.golang.org/grpc/balancer" "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" + + internalbackoff "google.golang.org/grpc/internal/backoff" ) // ccResolverWrapper is a wrapper on top of cc for resolvers. @@ -113,7 +115,7 @@ func (ccr *ccResolverWrapper) close() { ccr.mu.Unlock() } -var resolverBackoff = backoff.Exponential{MaxDelay: 2 * time.Minute}.Backoff +var resolverBackoff = internalbackoff.Exponential{Config: backoff.Config{MaxDelay: 2 * time.Minute}}.Backoff // poll begins or ends asynchronous polling of the resolver based on whether // err is ErrBadResolverState.