From 6d3e99db5ea15d9df0d4b0fbdcbf6fee9f78adf4 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 23 Apr 2020 12:34:39 -0700 Subject: [PATCH 1/3] balancer: move Balancer and Picker to V2; delete legacy API --- balancer.go | 391 --------- balancer/balancer.go | 174 ++-- balancer/base/balancer.go | 109 +-- balancer/base/base.go | 37 +- balancer/grpclb/grpclb.go | 15 +- balancer/grpclb/grpclb_picker.go | 2 +- balancer/roundrobin/roundrobin.go | 6 +- balancer_conn_wrappers.go | 37 +- balancer_conn_wrappers_test.go | 39 +- balancer_switching_test.go | 30 +- balancer_test.go | 789 ----------------- balancer_v1_wrapper.go | 334 -------- clientconn.go | 2 - clientconn_state_transition_test.go | 6 +- clientconn_test.go | 127 --- dialoptions.go | 13 - picker_wrapper.go | 48 +- picker_wrapper_test.go | 8 +- pickfirst.go | 23 +- pickfirst_test.go | 14 +- resolver_conn_wrapper_test.go | 7 +- test/balancer_test.go | 256 +++++- test/creds_test.go | 6 +- test/end2end_test.go | 259 +++++- vet.sh | 1 - .../balancer/balancergroup/balancergroup.go | 30 +- .../balancer/cdsbalancer/cdsbalancer.go | 15 +- .../balancer/cdsbalancer/cdsbalancer_test.go | 4 +- .../edsbalancer/balancergroup_test.go | 796 ++++++++++++++++++ xds/internal/balancer/edsbalancer/eds.go | 11 - xds/internal/balancer/edsbalancer/eds_impl.go | 10 +- .../balancer/edsbalancer/eds_impl_priority.go | 4 +- .../balancer/edsbalancer/eds_impl_test.go | 63 +- xds/internal/balancer/edsbalancer/eds_test.go | 8 +- .../balancer/edsbalancer/test_util_test.go | 346 ++++++++ 35 files changed, 1889 insertions(+), 2131 deletions(-) delete mode 100644 balancer.go delete mode 100644 balancer_test.go delete mode 100644 balancer_v1_wrapper.go create mode 100644 xds/internal/balancer/edsbalancer/balancergroup_test.go create mode 100644 xds/internal/balancer/edsbalancer/test_util_test.go diff --git a/balancer.go b/balancer.go deleted file mode 100644 index a8eb0f47609..00000000000 --- a/balancer.go +++ /dev/null @@ -1,391 +0,0 @@ -/* - * - * Copyright 2016 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 ( - "context" - "net" - "sync" - - "google.golang.org/grpc/codes" - "google.golang.org/grpc/credentials" - "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/naming" - "google.golang.org/grpc/status" -) - -// Address represents a server the client connects to. -// -// Deprecated: please use package balancer. -type Address struct { - // Addr is the server address on which a connection will be established. - Addr string - // Metadata is the information associated with Addr, which may be used - // to make load balancing decision. - Metadata interface{} -} - -// BalancerConfig specifies the configurations for Balancer. -// -// Deprecated: please use package balancer. May be removed in a future 1.x release. -type BalancerConfig struct { - // DialCreds is the transport credential the Balancer implementation can - // use to dial to a remote load balancer server. The Balancer implementations - // can ignore this if it does not need to talk to another party securely. - DialCreds credentials.TransportCredentials - // Dialer is the custom dialer the Balancer implementation can use to dial - // to a remote load balancer server. The Balancer implementations - // can ignore this if it doesn't need to talk to remote balancer. - Dialer func(context.Context, string) (net.Conn, error) -} - -// BalancerGetOptions configures a Get call. -// -// Deprecated: please use package balancer. May be removed in a future 1.x release. -type BalancerGetOptions struct { - // BlockingWait specifies whether Get should block when there is no - // connected address. - BlockingWait bool -} - -// Balancer chooses network addresses for RPCs. -// -// Deprecated: please use package balancer. May be removed in a future 1.x release. -type Balancer interface { - // Start does the initialization work to bootstrap a Balancer. For example, - // this function may start the name resolution and watch the updates. It will - // be called when dialing. - Start(target string, config BalancerConfig) error - // Up informs the Balancer that gRPC has a connection to the server at - // addr. It returns down which is called once the connection to addr gets - // lost or closed. - // TODO: It is not clear how to construct and take advantage of the meaningful error - // parameter for down. Need realistic demands to guide. - Up(addr Address) (down func(error)) - // Get gets the address of a server for the RPC corresponding to ctx. - // i) If it returns a connected address, gRPC internals issues the RPC on the - // connection to this address; - // ii) If it returns an address on which the connection is under construction - // (initiated by Notify(...)) but not connected, gRPC internals - // * fails RPC if the RPC is fail-fast and connection is in the TransientFailure or - // Shutdown state; - // or - // * issues RPC on the connection otherwise. - // iii) If it returns an address on which the connection does not exist, gRPC - // internals treats it as an error and will fail the corresponding RPC. - // - // Therefore, the following is the recommended rule when writing a custom Balancer. - // If opts.BlockingWait is true, it should return a connected address or - // block if there is no connected address. It should respect the timeout or - // cancellation of ctx when blocking. If opts.BlockingWait is false (for fail-fast - // RPCs), it should return an address it has notified via Notify(...) immediately - // instead of blocking. - // - // The function returns put which is called once the rpc has completed or failed. - // put can collect and report RPC stats to a remote load balancer. - // - // This function should only return the errors Balancer cannot recover by itself. - // gRPC internals will fail the RPC if an error is returned. - Get(ctx context.Context, opts BalancerGetOptions) (addr Address, put func(), err error) - // Notify returns a channel that is used by gRPC internals to watch the addresses - // gRPC needs to connect. The addresses might be from a name resolver or remote - // load balancer. gRPC internals will compare it with the existing connected - // addresses. If the address Balancer notified is not in the existing connected - // addresses, gRPC starts to connect the address. If an address in the existing - // connected addresses is not in the notification list, the corresponding connection - // is shutdown gracefully. Otherwise, there are no operations to take. Note that - // the Address slice must be the full list of the Addresses which should be connected. - // It is NOT delta. - Notify() <-chan []Address - // Close shuts down the balancer. - Close() error -} - -// RoundRobin returns a Balancer that selects addresses round-robin. It uses r to watch -// the name resolution updates and updates the addresses available correspondingly. -// -// Deprecated: please use package balancer/roundrobin. May be removed in a future 1.x release. -func RoundRobin(r naming.Resolver) Balancer { - return &roundRobin{r: r} -} - -type addrInfo struct { - addr Address - connected bool -} - -type roundRobin struct { - r naming.Resolver - w naming.Watcher - addrs []*addrInfo // all the addresses the client should potentially connect - mu sync.Mutex - addrCh chan []Address // the channel to notify gRPC internals the list of addresses the client should connect to. - next int // index of the next address to return for Get() - waitCh chan struct{} // the channel to block when there is no connected address available - done bool // The Balancer is closed. -} - -func (rr *roundRobin) watchAddrUpdates() error { - updates, err := rr.w.Next() - if err != nil { - grpclog.Warningf("grpc: the naming watcher stops working due to %v.", err) - return err - } - rr.mu.Lock() - defer rr.mu.Unlock() - for _, update := range updates { - addr := Address{ - Addr: update.Addr, - Metadata: update.Metadata, - } - switch update.Op { - case naming.Add: - var exist bool - for _, v := range rr.addrs { - if addr == v.addr { - exist = true - grpclog.Infoln("grpc: The name resolver wanted to add an existing address: ", addr) - break - } - } - if exist { - continue - } - rr.addrs = append(rr.addrs, &addrInfo{addr: addr}) - case naming.Delete: - for i, v := range rr.addrs { - if addr == v.addr { - copy(rr.addrs[i:], rr.addrs[i+1:]) - rr.addrs = rr.addrs[:len(rr.addrs)-1] - break - } - } - default: - grpclog.Errorln("Unknown update.Op ", update.Op) - } - } - // Make a copy of rr.addrs and write it onto rr.addrCh so that gRPC internals gets notified. - open := make([]Address, len(rr.addrs)) - for i, v := range rr.addrs { - open[i] = v.addr - } - if rr.done { - return ErrClientConnClosing - } - select { - case <-rr.addrCh: - default: - } - rr.addrCh <- open - return nil -} - -func (rr *roundRobin) Start(target string, config BalancerConfig) error { - rr.mu.Lock() - defer rr.mu.Unlock() - if rr.done { - return ErrClientConnClosing - } - if rr.r == nil { - // If there is no name resolver installed, it is not needed to - // do name resolution. In this case, target is added into rr.addrs - // as the only address available and rr.addrCh stays nil. - rr.addrs = append(rr.addrs, &addrInfo{addr: Address{Addr: target}}) - return nil - } - w, err := rr.r.Resolve(target) - if err != nil { - return err - } - rr.w = w - rr.addrCh = make(chan []Address, 1) - go func() { - for { - if err := rr.watchAddrUpdates(); err != nil { - return - } - } - }() - return nil -} - -// Up sets the connected state of addr and sends notification if there are pending -// Get() calls. -func (rr *roundRobin) Up(addr Address) func(error) { - rr.mu.Lock() - defer rr.mu.Unlock() - var cnt int - for _, a := range rr.addrs { - if a.addr == addr { - if a.connected { - return nil - } - a.connected = true - } - if a.connected { - cnt++ - } - } - // addr is only one which is connected. Notify the Get() callers who are blocking. - if cnt == 1 && rr.waitCh != nil { - close(rr.waitCh) - rr.waitCh = nil - } - return func(err error) { - rr.down(addr, err) - } -} - -// down unsets the connected state of addr. -func (rr *roundRobin) down(addr Address, err error) { - rr.mu.Lock() - defer rr.mu.Unlock() - for _, a := range rr.addrs { - if addr == a.addr { - a.connected = false - break - } - } -} - -// Get returns the next addr in the rotation. -func (rr *roundRobin) Get(ctx context.Context, opts BalancerGetOptions) (addr Address, put func(), err error) { - var ch chan struct{} - rr.mu.Lock() - if rr.done { - rr.mu.Unlock() - err = ErrClientConnClosing - return - } - - if len(rr.addrs) > 0 { - if rr.next >= len(rr.addrs) { - rr.next = 0 - } - next := rr.next - for { - a := rr.addrs[next] - next = (next + 1) % len(rr.addrs) - if a.connected { - addr = a.addr - rr.next = next - rr.mu.Unlock() - return - } - if next == rr.next { - // Has iterated all the possible address but none is connected. - break - } - } - } - if !opts.BlockingWait { - if len(rr.addrs) == 0 { - rr.mu.Unlock() - err = status.Errorf(codes.Unavailable, "there is no address available") - return - } - // Returns the next addr on rr.addrs for failfast RPCs. - addr = rr.addrs[rr.next].addr - rr.next++ - rr.mu.Unlock() - return - } - // Wait on rr.waitCh for non-failfast RPCs. - if rr.waitCh == nil { - ch = make(chan struct{}) - rr.waitCh = ch - } else { - ch = rr.waitCh - } - rr.mu.Unlock() - for { - select { - case <-ctx.Done(): - err = ctx.Err() - return - case <-ch: - rr.mu.Lock() - if rr.done { - rr.mu.Unlock() - err = ErrClientConnClosing - return - } - - if len(rr.addrs) > 0 { - if rr.next >= len(rr.addrs) { - rr.next = 0 - } - next := rr.next - for { - a := rr.addrs[next] - next = (next + 1) % len(rr.addrs) - if a.connected { - addr = a.addr - rr.next = next - rr.mu.Unlock() - return - } - if next == rr.next { - // Has iterated all the possible address but none is connected. - break - } - } - } - // The newly added addr got removed by Down() again. - if rr.waitCh == nil { - ch = make(chan struct{}) - rr.waitCh = ch - } else { - ch = rr.waitCh - } - rr.mu.Unlock() - } - } -} - -func (rr *roundRobin) Notify() <-chan []Address { - return rr.addrCh -} - -func (rr *roundRobin) Close() error { - rr.mu.Lock() - defer rr.mu.Unlock() - if rr.done { - return errBalancerClosed - } - rr.done = true - if rr.w != nil { - rr.w.Close() - } - if rr.waitCh != nil { - close(rr.waitCh) - rr.waitCh = nil - } - if rr.addrCh != nil { - close(rr.addrCh) - } - return nil -} - -// pickFirst is used to test multi-addresses in one addrConn in which all addresses share the same addrConn. -// It is a wrapper around roundRobin balancer. The logic of all methods works fine because balancer.Get() -// returns the only address Up by resetTransport(). -type pickFirst struct { - *roundRobin -} diff --git a/balancer/balancer.go b/balancer/balancer.go index 080fe0d189e..11a6bab2864 100644 --- a/balancer/balancer.go +++ b/balancer/balancer.go @@ -33,6 +33,7 @@ import ( "google.golang.org/grpc/metadata" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" + "google.golang.org/grpc/status" ) var ( @@ -126,7 +127,7 @@ type State struct { // determine the state of the ClientConn. ConnectivityState connectivity.State // Picker is used to choose connections (SubConns) for RPCs. - Picker V2Picker + Picker Picker } // ClientConn represents a gRPC ClientConn. @@ -144,20 +145,11 @@ type ClientConn interface { // The SubConn will be shutdown. RemoveSubConn(SubConn) - // UpdateBalancerState is called by balancer to notify gRPC that some internal - // state in balancer has changed. - // - // gRPC will update the connectivity state of the ClientConn, and will call pick - // on the new picker to pick new SubConn. - // - // Deprecated: use UpdateState instead - UpdateBalancerState(s connectivity.State, p Picker) - // UpdateState notifies gRPC that the balancer's internal state has // changed. // - // gRPC will update the connectivity state of the ClientConn, and will call pick - // on the new picker to pick new SubConns. + // gRPC will update the connectivity state of the ClientConn, and will call + // pick on the new picker to pick new SubConns. UpdateState(State) // ResolveNow is called by balancer to notify gRPC to do a name resolving. @@ -235,56 +227,13 @@ type DoneInfo struct { var ( // ErrNoSubConnAvailable indicates no SubConn is available for pick(). - // gRPC will block the RPC until a new picker is available via UpdateBalancerState(). + // gRPC will block the RPC until a new picker is available via UpdateState(). ErrNoSubConnAvailable = errors.New("no SubConn is available") // ErrTransientFailure indicates all SubConns are in TransientFailure. // WaitForReady RPCs will block, non-WaitForReady RPCs will fail. - ErrTransientFailure = TransientFailureError(errors.New("all SubConns are in TransientFailure")) + ErrTransientFailure = errors.New("all SubConns are in TransientFailure") ) -// Picker is used by gRPC to pick a SubConn to send an RPC. -// Balancer is expected to generate a new picker from its snapshot every time its -// internal state has changed. -// -// The pickers used by gRPC can be updated by ClientConn.UpdateBalancerState(). -// -// Deprecated: use V2Picker instead -type Picker interface { - // Pick returns the SubConn to be used to send the RPC. - // The returned SubConn must be one returned by NewSubConn(). - // - // This functions is expected to return: - // - a SubConn that is known to be READY; - // - ErrNoSubConnAvailable if no SubConn is available, but progress is being - // made (for example, some SubConn is in CONNECTING mode); - // - other errors if no active connecting is happening (for example, all SubConn - // are in TRANSIENT_FAILURE mode). - // - // If a SubConn is returned: - // - If it is READY, gRPC will send the RPC on it; - // - If it is not ready, or becomes not ready after it's returned, gRPC will - // block until UpdateBalancerState() is called and will call pick on the - // new picker. The done function returned from Pick(), if not nil, will be - // called with nil error, no bytes sent and no bytes received. - // - // If the returned error is not nil: - // - If the error is ErrNoSubConnAvailable, gRPC will block until UpdateBalancerState() - // - If the error is ErrTransientFailure or implements IsTransientFailure() - // bool, returning true: - // - If the RPC is wait-for-ready, gRPC will block until UpdateBalancerState() - // is called to pick again; - // - Otherwise, RPC will fail with unavailable error. - // - Else (error is other non-nil error): - // - The RPC will fail with the error's status code, or Unknown if it is - // not a status error. - // - // The returned done() function will be called once the rpc has finished, - // with the final status of that RPC. If the SubConn returned is not a - // valid SubConn type, done may not be called. done may be nil if balancer - // doesn't care about the RPC status. - Pick(ctx context.Context, info PickInfo) (conn SubConn, done func(DoneInfo), err error) -} - // PickResult contains information related to a connection chosen for an RPC. type PickResult struct { // SubConn is the connection to use for this pick, if its state is Ready. @@ -300,24 +249,36 @@ type PickResult struct { Done func(DoneInfo) } -type transientFailureError struct { +type dropRPCError struct { error + status *status.Status } -func (e *transientFailureError) IsTransientFailure() bool { return true } +func (e *dropRPCError) DropRPC() bool { return true } + +func (e *dropRPCError) GRPCStatus() *status.Status { return e.status } -// TransientFailureError wraps err in an error implementing -// IsTransientFailure() bool, returning true. -func TransientFailureError(err error) error { - return &transientFailureError{error: err} +// DropRPCError wraps err in an error implementing DropRPC() bool, returning +// true. If err is not a status error, it will be converted to one with code +// Unknown and the message containing the err.Error() result. +func DropRPCError(err error) error { + st := status.Convert(err) + return &dropRPCError{error: st.Err(), status: st} } -// V2Picker is used by gRPC to pick a SubConn to send an RPC. +// TransientFailureError returns e. It exists for backward compatibility and +// will be deleted soon. +// +// Deprecated: no longer necessary, picker errors are treated this way by +// default. +func TransientFailureError(e error) error { return e } + +// Picker is used by gRPC to pick a SubConn to send an RPC. // Balancer is expected to generate a new picker from its snapshot every time its // internal state has changed. // -// The pickers used by gRPC can be updated by ClientConn.UpdateBalancerState(). -type V2Picker interface { +// The pickers used by gRPC can be updated by ClientConn.UpdateState(). +type Picker interface { // Pick returns the connection to use for this RPC and related information. // // Pick should not block. If the balancer needs to do I/O or any blocking @@ -330,14 +291,14 @@ type V2Picker interface { // - If the error is ErrNoSubConnAvailable, gRPC will block until a new // Picker is provided by the balancer (using ClientConn.UpdateState). // - // - If the error implements IsTransientFailure() bool, returning true, - // wait for ready RPCs will wait, but non-wait for ready RPCs will be - // terminated with this error's Error() string and status code - // Unavailable. + // - If the error implements DropRPC() bool, returning true, gRPC will + // terminate all RPCs with the code and message provided. If the error + // is not a status error, it will be converted by gRPC to a status error + // with code Unknown. // - // - Any other errors terminate all RPCs with the code and message - // provided. If the error is not a status error, it will be converted by - // gRPC to a status error with code Unknown. + // - For all other errors, wait for ready RPCs will wait, but non-wait for + // ready RPCs will be terminated with this error's Error() string and + // status code Unavailable. Pick(info PickInfo) (PickResult, error) } @@ -346,34 +307,36 @@ type V2Picker interface { // // It also generates and updates the Picker used by gRPC to pick SubConns for RPCs. // -// HandleSubConnectionStateChange, HandleResolvedAddrs and Close are guaranteed -// to be called synchronously from the same goroutine. -// There's no guarantee on picker.Pick, it may be called anytime. +// UpdateClientConnState, ResolverError, UpdateSubConnState, and Close are +// guaranteed to be called synchronously from the same goroutine. There's no +// guarantee on picker.Pick, it may be called anytime. type Balancer interface { - // HandleSubConnStateChange is called by gRPC when the connectivity state - // of sc has changed. - // Balancer is expected to aggregate all the state of SubConn and report - // that back to gRPC. - // Balancer should also generate and update Pickers when its internal state has - // been changed by the new state. - // - // Deprecated: if V2Balancer is implemented by the Balancer, - // UpdateSubConnState will be called instead. - HandleSubConnStateChange(sc SubConn, state connectivity.State) - // HandleResolvedAddrs is called by gRPC to send updated resolved addresses to - // balancers. - // Balancer can create new SubConn or remove SubConn with the addresses. - // An empty address slice and a non-nil error will be passed if the resolver returns - // non-nil error to gRPC. - // - // Deprecated: if V2Balancer is implemented by the Balancer, - // UpdateClientConnState will be called instead. - HandleResolvedAddrs([]resolver.Address, error) + // UpdateClientConnState is called by gRPC when the state of the ClientConn + // 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) // Close closes the balancer. The balancer is not required to call // ClientConn.RemoveSubConn for its existing SubConns. Close() } +// V2Balancer is temporarily defined for backward compatibility reasons. +// +// Deprecated: use Balancer directly instead. +type V2Balancer = Balancer + +// V2Picker is temporarily defined for backward compatibility reasons. +// +// Deprecated: use Picker directly instead. +type V2Picker = Picker + // SubConnState describes the state of a SubConn. type SubConnState struct { // ConnectivityState is the connectivity state of the SubConn. @@ -396,27 +359,6 @@ type ClientConnState struct { // 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. 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) - // Close closes the balancer. The balancer is not required to call - // ClientConn.RemoveSubConn for its existing SubConns. - Close() -} - // ConnectivityStateEvaluator takes the connectivity states of multiple SubConns // and returns one aggregated connectivity state. // diff --git a/balancer/base/balancer.go b/balancer/base/balancer.go index 80559b80ace..d62b4b6069a 100644 --- a/balancer/base/balancer.go +++ b/balancer/base/balancer.go @@ -19,7 +19,6 @@ package base import ( - "context" "errors" "fmt" @@ -30,17 +29,15 @@ import ( ) type baseBuilder struct { - name string - pickerBuilder PickerBuilder - v2PickerBuilder V2PickerBuilder - config Config + name string + pickerBuilder PickerBuilder + config Config } func (bb *baseBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) balancer.Balancer { bal := &baseBalancer{ - cc: cc, - pickerBuilder: bb.pickerBuilder, - v2PickerBuilder: bb.v2PickerBuilder, + cc: cc, + pickerBuilder: bb.pickerBuilder, subConns: make(map[resolver.Address]balancer.SubConn), scStates: make(map[balancer.SubConn]connectivity.State), @@ -50,11 +47,7 @@ func (bb *baseBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) // Initialize picker to a picker that always returns // ErrNoSubConnAvailable, because when state of a SubConn changes, we // may call UpdateState with this picker. - if bb.pickerBuilder != nil { - bal.picker = NewErrPicker(balancer.ErrNoSubConnAvailable) - } else { - bal.v2Picker = NewErrPickerV2(balancer.ErrNoSubConnAvailable) - } + bal.picker = NewErrPicker(balancer.ErrNoSubConnAvailable) return bal } @@ -62,12 +55,9 @@ 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 - v2PickerBuilder V2PickerBuilder + cc balancer.ClientConn + pickerBuilder PickerBuilder csEvltr *balancer.ConnectivityStateEvaluator state connectivity.State @@ -75,40 +65,31 @@ type baseBalancer struct { subConns map[resolver.Address]balancer.SubConn scStates map[balancer.SubConn]connectivity.State picker balancer.Picker - v2Picker balancer.V2Picker config Config resolverErr error // the last error reported by the resolver; cleared on successful resolution connErr error // the last connection error; cleared upon leaving TransientFailure } -func (b *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { - panic("not implemented") -} - func (b *baseBalancer) ResolverError(err error) { b.resolverErr = err if len(b.subConns) == 0 { b.state = connectivity.TransientFailure } + if b.state != connectivity.TransientFailure { // The picker will not change since the balancer does not currently // report an error. return } b.regeneratePicker() - if b.picker != nil { - b.cc.UpdateBalancerState(b.state, b.picker) - } else { - b.cc.UpdateState(balancer.State{ - ConnectivityState: b.state, - Picker: b.v2Picker, - }) - } + b.cc.UpdateState(balancer.State{ + ConnectivityState: b.state, + Picker: b.picker, + }) } 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) { grpclog.Infoln("base.baseBalancer: got new ClientConn state: ", s) @@ -137,7 +118,7 @@ func (b *baseBalancer) UpdateClientConnState(s balancer.ClientConnState) error { b.cc.RemoveSubConn(sc) delete(b.subConns, a) // Keep the state of this sc in b.scStates until sc's state becomes Shutdown. - // The entry will be deleted in HandleSubConnStateChange. + // The entry will be deleted in UpdateSubConnState. } } // If resolver state contains no addresses, return an error so ClientConn @@ -171,38 +152,18 @@ func (b *baseBalancer) mergeErrors() error { // - built by the pickerBuilder with all READY SubConns otherwise. func (b *baseBalancer) regeneratePicker() { if b.state == connectivity.TransientFailure { - if b.pickerBuilder != nil { - b.picker = NewErrPicker(balancer.ErrTransientFailure) - } else { - b.v2Picker = NewErrPickerV2(balancer.TransientFailureError(b.mergeErrors())) - } + b.picker = NewErrPicker(b.mergeErrors()) return } - if b.pickerBuilder != nil { - readySCs := make(map[resolver.Address]balancer.SubConn) + readySCs := make(map[balancer.SubConn]SubConnInfo) - // Filter out all ready SCs from full subConn map. - for addr, sc := range b.subConns { - if st, ok := b.scStates[sc]; ok && st == connectivity.Ready { - readySCs[addr] = sc - } - } - b.picker = b.pickerBuilder.Build(readySCs) - } else { - readySCs := make(map[balancer.SubConn]SubConnInfo) - - // Filter out all ready SCs from full subConn map. - for addr, sc := range b.subConns { - if st, ok := b.scStates[sc]; ok && st == connectivity.Ready { - readySCs[sc] = SubConnInfo{Address: addr} - } + // Filter out all ready SCs from full subConn map. + for addr, sc := range b.subConns { + if st, ok := b.scStates[sc]; ok && st == connectivity.Ready { + readySCs[sc] = SubConnInfo{Address: addr} } - b.v2Picker = b.v2PickerBuilder.Build(PickerBuildInfo{ReadySCs: readySCs}) } -} - -func (b *baseBalancer) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - panic("not implemented") + b.picker = b.pickerBuilder.Build(PickerBuildInfo{ReadySCs: readySCs}) } func (b *baseBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { @@ -247,11 +208,7 @@ func (b *baseBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.Su b.regeneratePicker() } - if b.picker != nil { - b.cc.UpdateBalancerState(b.state, b.picker) - } else { - b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.v2Picker}) - } + b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker}) } // Close is a nop because base balancer doesn't have internal state to clean up, @@ -259,28 +216,20 @@ func (b *baseBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.Su func (b *baseBalancer) Close() { } -// NewErrPicker returns a picker that always returns err on Pick(). +// NewErrPicker returns a Picker that always returns err on Pick(). func NewErrPicker(err error) balancer.Picker { return &errPicker{err: err} } -type errPicker struct { - err error // Pick() always returns this err. -} +// NewErrPickerV2 is temporarily defined for backward compatibility reasons. +// +// Deprecated: use NewErrPicker instead. +var NewErrPickerV2 = NewErrPicker -func (p *errPicker) Pick(context.Context, balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) { - return nil, nil, p.err -} - -// NewErrPickerV2 returns a V2Picker that always returns err on Pick(). -func NewErrPickerV2(err error) balancer.V2Picker { - return &errPickerV2{err: err} -} - -type errPickerV2 struct { +type errPicker struct { err error // Pick() always returns this err. } -func (p *errPickerV2) Pick(info balancer.PickInfo) (balancer.PickResult, error) { +func (p *errPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { return balancer.PickResult{}, p.err } diff --git a/balancer/base/base.go b/balancer/base/base.go index 4192918b9e2..c4fc89111bf 100644 --- a/balancer/base/base.go +++ b/balancer/base/base.go @@ -37,15 +37,8 @@ import ( // PickerBuilder creates balancer.Picker. type PickerBuilder interface { - // Build takes a slice of ready SubConns, and returns a picker that will be - // used by gRPC to pick a SubConn. - Build(readySCs map[resolver.Address]balancer.SubConn) balancer.Picker -} - -// V2PickerBuilder creates balancer.V2Picker. -type V2PickerBuilder interface { // Build returns a picker that will be used by gRPC to pick a SubConn. - Build(info PickerBuildInfo) balancer.V2Picker + Build(info PickerBuildInfo) balancer.Picker } // PickerBuildInfo contains information needed by the picker builder to @@ -62,20 +55,14 @@ type SubConnInfo struct { Address resolver.Address // the address used to create this SubConn } -// NewBalancerBuilder returns a balancer builder. The balancers -// built by this builder will use the picker builder to build pickers. -func NewBalancerBuilder(name string, pb PickerBuilder) balancer.Builder { - return NewBalancerBuilderWithConfig(name, pb, Config{}) -} - // Config contains the config info about the base balancer builder. type Config struct { // HealthCheck indicates whether health checking should be enabled for this specific balancer. HealthCheck bool } -// NewBalancerBuilderWithConfig returns a base balancer builder configured by the provided config. -func NewBalancerBuilderWithConfig(name string, pb PickerBuilder, config Config) balancer.Builder { +// NewBalancerBuilder returns a base balancer builder configured by the provided config. +func NewBalancerBuilder(name string, pb PickerBuilder, config Config) balancer.Builder { return &baseBuilder{ name: name, pickerBuilder: pb, @@ -83,11 +70,13 @@ func NewBalancerBuilderWithConfig(name string, pb PickerBuilder, config Config) } } -// NewBalancerBuilderV2 returns a base balancer builder configured by the provided config. -func NewBalancerBuilderV2(name string, pb V2PickerBuilder, config Config) balancer.Builder { - return &baseBuilder{ - name: name, - v2PickerBuilder: pb, - config: config, - } -} +// NewBalancerBuilderV2 is temporarily defined for backward compatibility +// reasons. +// +// Deprecated: use NewBalancerBuilder instead. +var NewBalancerBuilderV2 = NewBalancerBuilder + +// V2PickerBuilder is temporarily defined for backward compatibility reasons. +// +// Deprecated: use PickerBuilder instead. +type V2PickerBuilder = PickerBuilder diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index 55ccd065a2b..193873b0fa1 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -159,8 +159,6 @@ 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 @@ -210,7 +208,7 @@ type lbBalancer struct { state connectivity.State subConns map[resolver.Address]balancer.SubConn // Used to new/remove SubConn. scStates map[balancer.SubConn]connectivity.State // Used to filter READY SubConns. - picker balancer.V2Picker + picker balancer.Picker // Support fallback to resolved backend addresses if there's no response // from remote balancer within fallbackTimeout. remoteBalancerConnected bool @@ -308,10 +306,6 @@ func (lb *lbBalancer) aggregateSubConnStates() connectivity.State { return connectivity.TransientFailure } -func (lb *lbBalancer) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - panic("not used") -} - func (lb *lbBalancer) UpdateSubConnState(sc balancer.SubConn, scs balancer.SubConnState) { s := scs.ConnectivityState if grpclog.V(2) { @@ -389,13 +383,6 @@ func (lb *lbBalancer) fallbackToBackendsAfter(fallbackTimeout time.Duration) { lb.mu.Unlock() } -// HandleResolvedAddrs sends the updated remoteLB addresses to remoteLB -// clientConn. The remoteLB clientConn will handle creating/removing remoteLB -// connections. -func (lb *lbBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { - panic("not used") -} - func (lb *lbBalancer) handleServiceConfig(gc *grpclbServiceConfig) { lb.mu.Lock() defer lb.mu.Unlock() diff --git a/balancer/grpclb/grpclb_picker.go b/balancer/grpclb/grpclb_picker.go index 39bc5cc71e8..59f871d7814 100644 --- a/balancer/grpclb/grpclb_picker.go +++ b/balancer/grpclb/grpclb_picker.go @@ -172,7 +172,7 @@ func (p *lbPicker) Pick(balancer.PickInfo) (balancer.PickResult, error) { // If it's a drop, return an error and fail the RPC. if s.Drop { p.stats.drop(s.LoadBalanceToken) - return balancer.PickResult{}, status.Errorf(codes.Unavailable, "request dropped by grpclb") + return balancer.PickResult{}, balancer.DropRPCError(status.Errorf(codes.Unavailable, "request dropped by grpclb")) } // If not a drop but there's no ready subConns. diff --git a/balancer/roundrobin/roundrobin.go b/balancer/roundrobin/roundrobin.go index d4d645501c1..a02b372cf20 100644 --- a/balancer/roundrobin/roundrobin.go +++ b/balancer/roundrobin/roundrobin.go @@ -35,7 +35,7 @@ const Name = "round_robin" // newBuilder creates a new roundrobin balancer builder. func newBuilder() balancer.Builder { - return base.NewBalancerBuilderV2(Name, &rrPickerBuilder{}, base.Config{HealthCheck: true}) + return base.NewBalancerBuilder(Name, &rrPickerBuilder{}, base.Config{HealthCheck: true}) } func init() { @@ -44,10 +44,10 @@ func init() { type rrPickerBuilder struct{} -func (*rrPickerBuilder) Build(info base.PickerBuildInfo) balancer.V2Picker { +func (*rrPickerBuilder) Build(info base.PickerBuildInfo) balancer.Picker { grpclog.Infof("roundrobinPicker: newPicker called with info: %v", info) if len(info.ReadySCs) == 0 { - return base.NewErrPickerV2(balancer.ErrNoSubConnAvailable) + return base.NewErrPicker(balancer.ErrNoSubConnAvailable) } var scs []balancer.SubConn for sc := range info.ReadySCs { diff --git a/balancer_conn_wrappers.go b/balancer_conn_wrappers.go index f8667a23f2c..807d1919777 100644 --- a/balancer_conn_wrappers.go +++ b/balancer_conn_wrappers.go @@ -74,11 +74,7 @@ func (ccb *ccBalancerWrapper) watcher() { } ccb.balancerMu.Lock() su := t.(*scStateUpdate) - if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { - ub.UpdateSubConnState(su.sc, balancer.SubConnState{ConnectivityState: su.state, ConnectionError: su.err}) - } else { - ccb.balancer.HandleSubConnStateChange(su.sc, su.state) - } + ccb.balancer.UpdateSubConnState(su.sc, balancer.SubConnState{ConnectivityState: su.state, ConnectionError: su.err}) ccb.balancerMu.Unlock() case <-ccb.done.Done(): } @@ -123,19 +119,13 @@ func (ccb *ccBalancerWrapper) handleSubConnStateChange(sc balancer.SubConn, s co func (ccb *ccBalancerWrapper) updateClientConnState(ccs *balancer.ClientConnState) error { 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 + return ccb.balancer.UpdateClientConnState(*ccs) } func (ccb *ccBalancerWrapper) resolverError(err error) { - if ub, ok := ccb.balancer.(balancer.V2Balancer); ok { - ccb.balancerMu.Lock() - ub.ResolverError(err) - ccb.balancerMu.Unlock() - } + ccb.balancerMu.Lock() + ccb.balancer.ResolverError(err) + ccb.balancerMu.Unlock() } func (ccb *ccBalancerWrapper) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) { @@ -173,21 +163,6 @@ func (ccb *ccBalancerWrapper) RemoveSubConn(sc balancer.SubConn) { ccb.cc.removeAddrConn(acbw.getAddrConn(), errConnDrain) } -func (ccb *ccBalancerWrapper) UpdateBalancerState(s connectivity.State, p balancer.Picker) { - ccb.mu.Lock() - defer ccb.mu.Unlock() - if ccb.subConns == nil { - return - } - // Update picker before updating state. Even though the ordering here does - // not matter, it can lead to multiple calls of Pick in the common start-up - // case where we wait for ready and then perform an RPC. If the picker is - // updated later, we could call the "connecting" picker when the state is - // updated, and then call the "ready" picker after the picker gets updated. - ccb.cc.blockingpicker.updatePicker(p) - ccb.cc.csMgr.updateState(s) -} - func (ccb *ccBalancerWrapper) UpdateState(s balancer.State) { ccb.mu.Lock() defer ccb.mu.Unlock() @@ -199,7 +174,7 @@ func (ccb *ccBalancerWrapper) UpdateState(s balancer.State) { // case where we wait for ready and then perform an RPC. If the picker is // updated later, we could call the "connecting" picker when the state is // updated, and then call the "ready" picker after the picker gets updated. - ccb.cc.blockingpicker.updatePickerV2(s.Picker) + ccb.cc.blockingpicker.updatePicker(s.Picker) ccb.cc.csMgr.updateState(s.ConnectivityState) } diff --git a/balancer_conn_wrappers_test.go b/balancer_conn_wrappers_test.go index 33a439f806d..935d11d1d39 100644 --- a/balancer_conn_wrappers_test.go +++ b/balancer_conn_wrappers_test.go @@ -25,50 +25,19 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/roundrobin" - "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/internal/balancer/stub" "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) {} -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 } - // 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) { // The test balancer will return ErrBadResolverState iff the // ClientConnState contains no addresses. - fb := &funcBalancer{ - updateClientConnState: func(s balancer.ClientConnState) error { + bf := stub.BalancerFuncs{ + UpdateClientConnState: func(_ *stub.BalancerData, s balancer.ClientConnState) error { if len(s.ResolverState.Addresses) == 0 { return balancer.ErrBadResolverState } @@ -76,7 +45,7 @@ func (s) TestBalancerErrorResolverPolling(t *testing.T) { }, } const balName = "BalancerErrorResolverPolling" - balancer.Register(&funcBalancerBuilder{name: balName, instance: fb}) + stub.Register(balName, bf) testResolverErrorPolling(t, func(r *manual.Resolver) { diff --git a/balancer_switching_test.go b/balancer_switching_test.go index 1ccbaba6cdc..f47754bfdfe 100644 --- a/balancer_switching_test.go +++ b/balancer_switching_test.go @@ -27,7 +27,6 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/roundrobin" - "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" @@ -48,9 +47,13 @@ func (b *magicalLB) Build(cc balancer.ClientConn, opts balancer.BuildOptions) ba return b } -func (b *magicalLB) HandleSubConnStateChange(balancer.SubConn, connectivity.State) {} +func (b *magicalLB) ResolverError(error) {} -func (b *magicalLB) HandleResolvedAddrs([]resolver.Address, error) {} +func (b *magicalLB) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) {} + +func (b *magicalLB) UpdateClientConnState(balancer.ClientConnState) error { + return nil +} func (b *magicalLB) Close() {} @@ -58,6 +61,21 @@ func init() { balancer.Register(&magicalLB{}) } +func startServers(t *testing.T, numServers int, maxStreams uint32) ([]*server, func()) { + var servers []*server + for i := 0; i < numServers; i++ { + s := newTestServer() + servers = append(servers, s) + go s.start(t, 0, maxStreams) + s.wait(t, 2*time.Second) + } + return servers, func() { + for i := 0; i < numServers; i++ { + servers[i].stop() + } + } +} + func checkPickFirst(cc *ClientConn, servers []*server) error { var ( req = "port" @@ -133,7 +151,7 @@ func (s) TestSwitchBalancer(t *testing.T) { defer rcleanup() const numServers = 2 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -165,7 +183,7 @@ func (s) TestBalancerDialOption(t *testing.T) { defer rcleanup() const numServers = 2 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{}), WithBalancerName(roundrobin.Name)) @@ -481,7 +499,7 @@ func (s) TestSwitchBalancerGRPCLBWithGRPCLBNotRegistered(t *testing.T) { defer rcleanup() const numServers = 3 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) diff --git a/balancer_test.go b/balancer_test.go deleted file mode 100644 index 524fd43a6f5..00000000000 --- a/balancer_test.go +++ /dev/null @@ -1,789 +0,0 @@ -/* - * - * Copyright 2016 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 ( - "context" - "fmt" - "math" - "strconv" - "sync" - "testing" - "time" - - "google.golang.org/grpc/codes" - "google.golang.org/grpc/naming" - "google.golang.org/grpc/status" -) - -func pickFirstBalancerV1(r naming.Resolver) Balancer { - return &pickFirst{&roundRobin{r: r}} -} - -type testWatcher struct { - // the channel to receives name resolution updates - update chan *naming.Update - // the side channel to get to know how many updates in a batch - side chan int - // the channel to notify update injector that the update reading is done - readDone chan int -} - -func (w *testWatcher) Next() (updates []*naming.Update, err error) { - n := <-w.side - if n == 0 { - return nil, fmt.Errorf("w.side is closed") - } - for i := 0; i < n; i++ { - u := <-w.update - if u != nil { - updates = append(updates, u) - } - } - w.readDone <- 0 - return -} - -func (w *testWatcher) Close() { - close(w.side) -} - -// Inject naming resolution updates to the testWatcher. -func (w *testWatcher) inject(updates []*naming.Update) { - w.side <- len(updates) - for _, u := range updates { - w.update <- u - } - <-w.readDone -} - -type testNameResolver struct { - w *testWatcher - addr string -} - -func (r *testNameResolver) Resolve(target string) (naming.Watcher, error) { - r.w = &testWatcher{ - update: make(chan *naming.Update, 1), - side: make(chan int, 1), - readDone: make(chan int), - } - r.w.side <- 1 - r.w.update <- &naming.Update{ - Op: naming.Add, - Addr: r.addr, - } - go func() { - <-r.w.readDone - }() - return r.w, nil -} - -func startServers(t *testing.T, numServers int, maxStreams uint32) ([]*server, *testNameResolver, func()) { - var servers []*server - for i := 0; i < numServers; i++ { - s := newTestServer() - servers = append(servers, s) - go s.start(t, 0, maxStreams) - s.wait(t, 2*time.Second) - } - // Point to server[0] - addr := "localhost:" + servers[0].port - return servers, &testNameResolver{ - addr: addr, - }, func() { - for i := 0; i < numServers; i++ { - servers[i].stop() - } - } -} - -func (s) TestNameDiscovery(t *testing.T) { - // Start 2 servers on 2 ports. - numServers := 2 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - req := "port" - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[0].port { - t.Fatalf("grpc.Invoke(_, _, _, _, _) = %v, want %s", err, servers[0].port) - } - // Inject the name resolution change to remove servers[0] and add servers[1]. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }) - updates = append(updates, &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - }) - r.w.inject(updates) - // Loop until the rpcs in flight talks to servers[1]. - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } -} - -func (s) TestEmptyAddrs(t *testing.T) { - servers, r, cleanup := startServers(t, 1, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply); err != nil || reply != expectedResponse { - t.Fatalf("grpc.Invoke(_, _, _, _, _) = %v, reply = %q, want %q, ", err, reply, expectedResponse) - } - // Inject name resolution change to remove the server so that there is no address - // available after that. - u := &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - } - r.w.inject([]*naming.Update{u}) - // Loop until the above updates apply. - for { - time.Sleep(10 * time.Millisecond) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - if err := cc.Invoke(ctx, "/foo/bar", &expectedRequest, &reply); err != nil { - cancel() - break - } - cancel() - } -} - -func (s) TestRoundRobin(t *testing.T) { - // Start 3 servers on 3 ports. - numServers := 3 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] to the service discovery. - u := &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - } - r.w.inject([]*naming.Update{u}) - req := "port" - var reply string - // Loop until servers[1] is up - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } - // Add server2[2] to the service discovery. - u = &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[2].port, - } - r.w.inject([]*naming.Update{u}) - // Loop until both servers[2] are up. - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[2].port { - break - } - time.Sleep(10 * time.Millisecond) - } - // Check the incoming RPCs served in a round-robin manner. - for i := 0; i < 10; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[i%numServers].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", i, err, servers[i%numServers].port) - } - } -} - -func (s) TestCloseWithPendingRPC(t *testing.T) { - servers, r, cleanup := startServers(t, 1, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err != nil { - t.Fatalf("grpc.Invoke(_, _, _, _, _) = %v, want %s", err, servers[0].port) - } - // Remove the server. - updates := []*naming.Update{{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }} - r.w.inject(updates) - // Loop until the above update applies. - for { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - if err := cc.Invoke(ctx, "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); status.Code(err) == codes.DeadlineExceeded { - cancel() - break - } - time.Sleep(10 * time.Millisecond) - cancel() - } - // Issue 2 RPCs which should be completed with error status once cc is closed. - var wg sync.WaitGroup - wg.Add(2) - go func() { - defer wg.Done() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err == nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want not nil", err) - } - }() - go func() { - defer wg.Done() - var reply string - time.Sleep(5 * time.Millisecond) - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err == nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want not nil", err) - } - }() - time.Sleep(5 * time.Millisecond) - cc.Close() - wg.Wait() -} - -func (s) TestGetOnWaitChannel(t *testing.T) { - servers, r, cleanup := startServers(t, 1, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Remove all servers so that all upcoming RPCs will block on waitCh. - updates := []*naming.Update{{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }} - r.w.inject(updates) - for { - var reply string - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - if err := cc.Invoke(ctx, "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); status.Code(err) == codes.DeadlineExceeded { - cancel() - break - } - cancel() - time.Sleep(10 * time.Millisecond) - } - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err != nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want ", err) - } - }() - // Add a connected server to get the above RPC through. - updates = []*naming.Update{{ - Op: naming.Add, - Addr: "localhost:" + servers[0].port, - }} - r.w.inject(updates) - // Wait until the above RPC succeeds. - wg.Wait() -} - -func (s) TestOneServerDown(t *testing.T) { - // Start 2 servers. - numServers := 2 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] to the service discovery. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - }) - r.w.inject(updates) - req := "port" - var reply string - // Loop until servers[1] is up - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } - - var wg sync.WaitGroup - numRPC := 100 - sleepDuration := 10 * time.Millisecond - wg.Add(1) - go func() { - time.Sleep(sleepDuration) - // After sleepDuration, kill server[0]. - servers[0].stop() - wg.Done() - }() - - // All non-failfast RPCs should not block because there's at least one connection available. - for i := 0; i < numRPC; i++ { - wg.Add(1) - go func() { - time.Sleep(sleepDuration) - // After sleepDuration, invoke RPC. - // server[0] is killed around the same time to make it racy between balancer and gRPC internals. - cc.Invoke(context.Background(), "/foo/bar", &req, &reply, WaitForReady(true)) - wg.Done() - }() - } - wg.Wait() -} - -func (s) TestOneAddressRemoval(t *testing.T) { - // Start 2 servers. - numServers := 2 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] to the service discovery. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - }) - r.w.inject(updates) - req := "port" - var reply string - // Loop until servers[1] is up - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } - - var wg sync.WaitGroup - numRPC := 100 - sleepDuration := 10 * time.Millisecond - wg.Add(1) - go func() { - time.Sleep(sleepDuration) - // After sleepDuration, delete server[0]. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }) - r.w.inject(updates) - wg.Done() - }() - - // All non-failfast RPCs should not fail because there's at least one connection available. - for i := 0; i < numRPC; i++ { - wg.Add(1) - go func() { - var reply string - time.Sleep(sleepDuration) - // After sleepDuration, invoke RPC. - // server[0] is removed around the same time to make it racy between balancer and gRPC internals. - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err != nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want nil", err) - } - wg.Done() - }() - } - wg.Wait() -} - -func checkServerUp(t *testing.T, currentServer *server) { - req := "port" - port := currentServer.port - cc, err := Dial("passthrough:///localhost:"+port, WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - var reply string - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == port { - break - } - time.Sleep(10 * time.Millisecond) - } -} - -func (s) TestPickFirstEmptyAddrs(t *testing.T) { - servers, r, cleanup := startServers(t, 1, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(pickFirstBalancerV1(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply); err != nil || reply != expectedResponse { - t.Fatalf("grpc.Invoke(_, _, _, _, _) = %v, reply = %q, want %q, ", err, reply, expectedResponse) - } - // Inject name resolution change to remove the server so that there is no address - // available after that. - u := &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - } - r.w.inject([]*naming.Update{u}) - // Loop until the above updates apply. - for { - time.Sleep(10 * time.Millisecond) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - if err := cc.Invoke(ctx, "/foo/bar", &expectedRequest, &reply); err != nil { - cancel() - break - } - cancel() - } -} - -func (s) TestPickFirstCloseWithPendingRPC(t *testing.T) { - servers, r, cleanup := startServers(t, 1, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(pickFirstBalancerV1(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err != nil { - t.Fatalf("grpc.Invoke(_, _, _, _, _) = %v, want %s", err, servers[0].port) - } - // Remove the server. - updates := []*naming.Update{{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }} - r.w.inject(updates) - // Loop until the above update applies. - for { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) - if err := cc.Invoke(ctx, "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); status.Code(err) == codes.DeadlineExceeded { - cancel() - break - } - time.Sleep(10 * time.Millisecond) - cancel() - } - // Issue 2 RPCs which should be completed with error status once cc is closed. - var wg sync.WaitGroup - wg.Add(2) - go func() { - defer wg.Done() - var reply string - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err == nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want not nil", err) - } - }() - go func() { - defer wg.Done() - var reply string - time.Sleep(5 * time.Millisecond) - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err == nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want not nil", err) - } - }() - time.Sleep(5 * time.Millisecond) - cc.Close() - wg.Wait() -} - -func (s) TestPickFirstOrderAllServerUp(t *testing.T) { - // Start 3 servers on 3 ports. - numServers := 3 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(pickFirstBalancerV1(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] and [2] to the service discovery. - u := &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - } - r.w.inject([]*naming.Update{u}) - - u = &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[2].port, - } - r.w.inject([]*naming.Update{u}) - - // Loop until all 3 servers are up - checkServerUp(t, servers[0]) - checkServerUp(t, servers[1]) - checkServerUp(t, servers[2]) - - // Check the incoming RPCs served in server[0] - req := "port" - var reply string - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[0].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 0, err, servers[0].port) - } - time.Sleep(10 * time.Millisecond) - } - - // Delete server[0] in the balancer, the incoming RPCs served in server[1] - // For test addrconn, close server[0] instead - u = &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - } - r.w.inject([]*naming.Update{u}) - // Loop until it changes to server[1] - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[1].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 1, err, servers[1].port) - } - time.Sleep(10 * time.Millisecond) - } - - // Add server[0] back to the balancer, the incoming RPCs served in server[1] - // Add is append operation, the order of Notify now is {server[1].port server[2].port server[0].port} - u = &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[0].port, - } - r.w.inject([]*naming.Update{u}) - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[1].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 1, err, servers[1].port) - } - time.Sleep(10 * time.Millisecond) - } - - // Delete server[1] in the balancer, the incoming RPCs served in server[2] - u = &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[1].port, - } - r.w.inject([]*naming.Update{u}) - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[2].port { - break - } - time.Sleep(1 * time.Second) - } - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[2].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 2, err, servers[2].port) - } - time.Sleep(10 * time.Millisecond) - } - - // Delete server[2] in the balancer, the incoming RPCs served in server[0] - u = &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[2].port, - } - r.w.inject([]*naming.Update{u}) - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[0].port { - break - } - time.Sleep(1 * time.Second) - } - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[0].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 0, err, servers[0].port) - } - time.Sleep(10 * time.Millisecond) - } -} - -func (s) TestPickFirstOrderOneServerDown(t *testing.T) { - // Start 3 servers on 3 ports. - numServers := 3 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(pickFirstBalancerV1(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] and [2] to the service discovery. - u := &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - } - r.w.inject([]*naming.Update{u}) - - u = &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[2].port, - } - r.w.inject([]*naming.Update{u}) - - // Loop until all 3 servers are up - checkServerUp(t, servers[0]) - checkServerUp(t, servers[1]) - checkServerUp(t, servers[2]) - - // Check the incoming RPCs served in server[0] - req := "port" - var reply string - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[0].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 0, err, servers[0].port) - } - time.Sleep(10 * time.Millisecond) - } - - // server[0] down, incoming RPCs served in server[1], but the order of Notify still remains - // {server[0] server[1] server[2]} - servers[0].stop() - // Loop until it changes to server[1] - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[1].port { - break - } - time.Sleep(10 * time.Millisecond) - } - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[1].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 1, err, servers[1].port) - } - time.Sleep(10 * time.Millisecond) - } - - // up the server[0] back, the incoming RPCs served in server[1] - p, _ := strconv.Atoi(servers[0].port) - servers[0] = newTestServer() - go servers[0].start(t, p, math.MaxUint32) - defer servers[0].stop() - servers[0].wait(t, 2*time.Second) - checkServerUp(t, servers[0]) - - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[1].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 1, err, servers[1].port) - } - time.Sleep(10 * time.Millisecond) - } - - // Delete server[1] in the balancer, the incoming RPCs served in server[0] - u = &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[1].port, - } - r.w.inject([]*naming.Update{u}) - for { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err != nil && errorDesc(err) == servers[0].port { - break - } - time.Sleep(1 * time.Second) - } - for i := 0; i < 20; i++ { - if err := cc.Invoke(context.Background(), "/foo/bar", &req, &reply); err == nil || errorDesc(err) != servers[0].port { - t.Fatalf("Index %d: Invoke(_, _, _, _, _) = %v, want %s", 0, err, servers[0].port) - } - time.Sleep(10 * time.Millisecond) - } -} - -func (s) TestPickFirstOneAddressRemoval(t *testing.T) { - // Start 2 servers. - numServers := 2 - servers, r, cleanup := startServers(t, numServers, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///localhost:"+servers[0].port, WithBalancer(pickFirstBalancerV1(r)), WithBlock(), WithInsecure(), WithCodec(testCodec{})) - if err != nil { - t.Fatalf("Failed to create ClientConn: %v", err) - } - defer cc.Close() - // Add servers[1] to the service discovery. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - }) - r.w.inject(updates) - - // Create a new cc to Loop until servers[1] is up - checkServerUp(t, servers[0]) - checkServerUp(t, servers[1]) - - var wg sync.WaitGroup - numRPC := 100 - sleepDuration := 10 * time.Millisecond - wg.Add(1) - go func() { - time.Sleep(sleepDuration) - // After sleepDuration, delete server[0]. - var updates []*naming.Update - updates = append(updates, &naming.Update{ - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }) - r.w.inject(updates) - wg.Done() - }() - - // All non-failfast RPCs should not fail because there's at least one connection available. - for i := 0; i < numRPC; i++ { - wg.Add(1) - go func() { - var reply string - time.Sleep(sleepDuration) - // After sleepDuration, invoke RPC. - // server[0] is removed around the same time to make it racy between balancer and gRPC internals. - if err := cc.Invoke(context.Background(), "/foo/bar", &expectedRequest, &reply, WaitForReady(true)); err != nil { - t.Errorf("grpc.Invoke(_, _, _, _, _) = %v, want nil", err) - } - wg.Done() - }() - } - wg.Wait() -} diff --git a/balancer_v1_wrapper.go b/balancer_v1_wrapper.go deleted file mode 100644 index db04b08b842..00000000000 --- a/balancer_v1_wrapper.go +++ /dev/null @@ -1,334 +0,0 @@ -/* - * - * Copyright 2017 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 ( - "sync" - - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/resolver" -) - -type balancerWrapperBuilder struct { - b Balancer // The v1 balancer. -} - -func (bwb *balancerWrapperBuilder) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { - bwb.b.Start(opts.Target.Endpoint, BalancerConfig{ - DialCreds: opts.DialCreds, - Dialer: opts.Dialer, - }) - _, pickfirst := bwb.b.(*pickFirst) - bw := &balancerWrapper{ - balancer: bwb.b, - pickfirst: pickfirst, - cc: cc, - targetAddr: opts.Target.Endpoint, - startCh: make(chan struct{}), - conns: make(map[resolver.Address]balancer.SubConn), - connSt: make(map[balancer.SubConn]*scState), - csEvltr: &balancer.ConnectivityStateEvaluator{}, - state: connectivity.Idle, - } - cc.UpdateState(balancer.State{ConnectivityState: connectivity.Idle, Picker: bw}) - go bw.lbWatcher() - return bw -} - -func (bwb *balancerWrapperBuilder) Name() string { - return "wrapper" -} - -type scState struct { - addr Address // The v1 address type. - s connectivity.State - down func(error) -} - -type balancerWrapper struct { - balancer Balancer // The v1 balancer. - pickfirst bool - - cc balancer.ClientConn - targetAddr string // Target without the scheme. - - mu sync.Mutex - conns map[resolver.Address]balancer.SubConn - connSt map[balancer.SubConn]*scState - // This channel is closed when handling the first resolver result. - // lbWatcher blocks until this is closed, to avoid race between - // - NewSubConn is created, cc wants to notify balancer of state changes; - // - Build hasn't return, cc doesn't have access to balancer. - startCh chan struct{} - - // To aggregate the connectivity state. - csEvltr *balancer.ConnectivityStateEvaluator - state connectivity.State -} - -// lbWatcher watches the Notify channel of the balancer and manages -// connections accordingly. -func (bw *balancerWrapper) lbWatcher() { - <-bw.startCh - notifyCh := bw.balancer.Notify() - if notifyCh == nil { - // There's no resolver in the balancer. Connect directly. - a := resolver.Address{ - Addr: bw.targetAddr, - Type: resolver.Backend, - } - sc, err := bw.cc.NewSubConn([]resolver.Address{a}, balancer.NewSubConnOptions{}) - if err != nil { - grpclog.Warningf("Error creating connection to %v. Err: %v", a, err) - } else { - bw.mu.Lock() - bw.conns[a] = sc - bw.connSt[sc] = &scState{ - addr: Address{Addr: bw.targetAddr}, - s: connectivity.Idle, - } - bw.mu.Unlock() - sc.Connect() - } - return - } - - for addrs := range notifyCh { - grpclog.Infof("balancerWrapper: got update addr from Notify: %v", addrs) - if bw.pickfirst { - var ( - oldA resolver.Address - oldSC balancer.SubConn - ) - bw.mu.Lock() - for oldA, oldSC = range bw.conns { - break - } - bw.mu.Unlock() - if len(addrs) <= 0 { - if oldSC != nil { - // Teardown old sc. - bw.mu.Lock() - delete(bw.conns, oldA) - delete(bw.connSt, oldSC) - bw.mu.Unlock() - bw.cc.RemoveSubConn(oldSC) - } - continue - } - - var newAddrs []resolver.Address - for _, a := range addrs { - newAddr := resolver.Address{ - Addr: a.Addr, - Type: resolver.Backend, // All addresses from balancer are all backends. - ServerName: "", - Metadata: a.Metadata, - } - newAddrs = append(newAddrs, newAddr) - } - if oldSC == nil { - // Create new sc. - sc, err := bw.cc.NewSubConn(newAddrs, balancer.NewSubConnOptions{}) - if err != nil { - grpclog.Warningf("Error creating connection to %v. Err: %v", newAddrs, err) - } else { - bw.mu.Lock() - // For pickfirst, there should be only one SubConn, so the - // address doesn't matter. All states updating (up and down) - // and picking should all happen on that only SubConn. - bw.conns[resolver.Address{}] = sc - bw.connSt[sc] = &scState{ - addr: addrs[0], // Use the first address. - s: connectivity.Idle, - } - bw.mu.Unlock() - sc.Connect() - } - } else { - bw.mu.Lock() - bw.connSt[oldSC].addr = addrs[0] - bw.mu.Unlock() - oldSC.UpdateAddresses(newAddrs) - } - } else { - var ( - add []resolver.Address // Addresses need to setup connections. - del []balancer.SubConn // Connections need to tear down. - ) - resAddrs := make(map[resolver.Address]Address) - for _, a := range addrs { - resAddrs[resolver.Address{ - Addr: a.Addr, - Type: resolver.Backend, // All addresses from balancer are all backends. - ServerName: "", - Metadata: a.Metadata, - }] = a - } - bw.mu.Lock() - for a := range resAddrs { - if _, ok := bw.conns[a]; !ok { - add = append(add, a) - } - } - for a, c := range bw.conns { - if _, ok := resAddrs[a]; !ok { - del = append(del, c) - delete(bw.conns, a) - // Keep the state of this sc in bw.connSt until its state becomes Shutdown. - } - } - bw.mu.Unlock() - for _, a := range add { - sc, err := bw.cc.NewSubConn([]resolver.Address{a}, balancer.NewSubConnOptions{}) - if err != nil { - grpclog.Warningf("Error creating connection to %v. Err: %v", a, err) - } else { - bw.mu.Lock() - bw.conns[a] = sc - bw.connSt[sc] = &scState{ - addr: resAddrs[a], - s: connectivity.Idle, - } - bw.mu.Unlock() - sc.Connect() - } - } - for _, c := range del { - bw.cc.RemoveSubConn(c) - } - } - } -} - -func (bw *balancerWrapper) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - bw.mu.Lock() - defer bw.mu.Unlock() - scSt, ok := bw.connSt[sc] - if !ok { - return - } - if s == connectivity.Idle { - sc.Connect() - } - oldS := scSt.s - scSt.s = s - if oldS != connectivity.Ready && s == connectivity.Ready { - scSt.down = bw.balancer.Up(scSt.addr) - } else if oldS == connectivity.Ready && s != connectivity.Ready { - if scSt.down != nil { - scSt.down(errConnClosing) - } - } - sa := bw.csEvltr.RecordTransition(oldS, s) - if bw.state != sa { - bw.state = sa - } - bw.cc.UpdateState(balancer.State{ConnectivityState: bw.state, Picker: bw}) - if s == connectivity.Shutdown { - // Remove state for this sc. - delete(bw.connSt, sc) - } -} - -func (bw *balancerWrapper) HandleResolvedAddrs([]resolver.Address, error) { - bw.mu.Lock() - defer bw.mu.Unlock() - select { - case <-bw.startCh: - default: - close(bw.startCh) - } - // There should be a resolver inside the balancer. - // All updates here, if any, are ignored. -} - -func (bw *balancerWrapper) Close() { - bw.mu.Lock() - defer bw.mu.Unlock() - select { - case <-bw.startCh: - default: - close(bw.startCh) - } - bw.balancer.Close() -} - -// The picker is the balancerWrapper itself. -// It either blocks or returns error, consistent with v1 balancer Get(). -func (bw *balancerWrapper) Pick(info balancer.PickInfo) (result balancer.PickResult, err error) { - failfast := true // Default failfast is true. - if ss, ok := rpcInfoFromContext(info.Ctx); ok { - failfast = ss.failfast - } - a, p, err := bw.balancer.Get(info.Ctx, BalancerGetOptions{BlockingWait: !failfast}) - if err != nil { - return balancer.PickResult{}, toRPCErr(err) - } - if p != nil { - result.Done = func(balancer.DoneInfo) { p() } - defer func() { - if err != nil { - p() - } - }() - } - - bw.mu.Lock() - defer bw.mu.Unlock() - if bw.pickfirst { - // Get the first sc in conns. - for _, result.SubConn = range bw.conns { - return result, nil - } - return balancer.PickResult{}, balancer.ErrNoSubConnAvailable - } - var ok1 bool - result.SubConn, ok1 = bw.conns[resolver.Address{ - Addr: a.Addr, - Type: resolver.Backend, - ServerName: "", - Metadata: a.Metadata, - }] - s, ok2 := bw.connSt[result.SubConn] - if !ok1 || !ok2 { - // This can only happen due to a race where Get() returned an address - // that was subsequently removed by Notify. In this case we should - // retry always. - return balancer.PickResult{}, balancer.ErrNoSubConnAvailable - } - switch s.s { - case connectivity.Ready, connectivity.Idle: - return result, nil - case connectivity.Shutdown, connectivity.TransientFailure: - // If the returned sc has been shut down or is in transient failure, - // return error, and this RPC will fail or wait for another picker (if - // non-failfast). - return balancer.PickResult{}, balancer.ErrTransientFailure - default: - // For other states (connecting or unknown), the v1 balancer would - // traditionally wait until ready and then issue the RPC. Returning - // ErrNoSubConnAvailable will be a slight improvement in that it will - // allow the balancer to choose another address in case others are - // connected. - return balancer.PickResult{}, balancer.ErrNoSubConnAvailable - } -} diff --git a/clientconn.go b/clientconn.go index 3e4b42b3dd4..71cae0ffe6a 100644 --- a/clientconn.go +++ b/clientconn.go @@ -68,8 +68,6 @@ var ( errConnDrain = errors.New("grpc: the connection is drained") // errConnClosing indicates that the connection is closing. errConnClosing = errors.New("grpc: the connection is closing") - // errBalancerClosed indicates that the balancer is closed. - errBalancerClosed = errors.New("grpc: balancer is closed") // invalidDefaultServiceConfigErrPrefix is used to prefix the json parsing error for the default // service config. invalidDefaultServiceConfigErrPrefix = "grpc: the provided default service config is invalid" diff --git a/clientconn_state_transition_test.go b/clientconn_state_transition_test.go index 0e9b1e75215..0c58131a1c6 100644 --- a/clientconn_state_transition_test.go +++ b/clientconn_state_transition_test.go @@ -449,9 +449,9 @@ type stateRecordingBalancer struct { balancer.Balancer } -func (b *stateRecordingBalancer) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - b.notifier <- s - b.Balancer.HandleSubConnStateChange(sc, s) +func (b *stateRecordingBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) { + b.notifier <- s.ConnectivityState + b.Balancer.UpdateSubConnState(sc, s) } func (b *stateRecordingBalancer) ResetNotifier(r chan<- connectivity.State) { diff --git a/clientconn_test.go b/clientconn_test.go index dd4a2b427bf..524b9736c1a 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -36,21 +36,11 @@ import ( internalbackoff "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/keepalive" - "google.golang.org/grpc/naming" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" "google.golang.org/grpc/testdata" ) -func assertState(wantState connectivity.State, cc *ClientConn) (connectivity.State, bool) { - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - defer cancel() - var state connectivity.State - for state = cc.GetState(); state != wantState && cc.WaitForStateChange(ctx, state); state = cc.GetState() { - } - return state, state == wantState -} - func (s) TestDialWithTimeout(t *testing.T) { lis, err := net.Listen("tcp", "localhost:0") if err != nil { @@ -361,42 +351,6 @@ func (s) TestBackoffWhenNoServerPrefaceReceived(t *testing.T) { } -func (s) TestConnectivityStates(t *testing.T) { - servers, resolver, cleanup := startServers(t, 2, math.MaxUint32) - defer cleanup() - cc, err := Dial("passthrough:///foo.bar.com", WithBalancer(RoundRobin(resolver)), WithInsecure()) - if err != nil { - t.Fatalf("Dial(\"foo.bar.com\", WithBalancer(_)) = _, %v, want _ ", err) - } - defer cc.Close() - wantState := connectivity.Ready - if state, ok := assertState(wantState, cc); !ok { - t.Fatalf("asserState(%s) = %s, false, want %s, true", wantState, state, wantState) - } - // Send an update to delete the server connection (tearDown addrConn). - update := []*naming.Update{ - { - Op: naming.Delete, - Addr: "localhost:" + servers[0].port, - }, - } - resolver.w.inject(update) - wantState = connectivity.TransientFailure - if state, ok := assertState(wantState, cc); !ok { - t.Fatalf("asserState(%s) = %s, false, want %s, true", wantState, state, wantState) - } - update[0] = &naming.Update{ - Op: naming.Add, - Addr: "localhost:" + servers[1].port, - } - resolver.w.inject(update) - wantState = connectivity.Ready - if state, ok := assertState(wantState, cc); !ok { - t.Fatalf("asserState(%s) = %s, false, want %s, true", wantState, state, wantState) - } - -} - func (s) TestWithTimeout(t *testing.T) { conn, err := Dial("passthrough:///Non-Existent.Server:80", WithTimeout(time.Millisecond), WithBlock(), WithInsecure()) if err == nil { @@ -592,43 +546,6 @@ func (s) TestDialContextFailFast(t *testing.T) { } } -// blockingBalancer mimics the behavior of balancers whose initialization takes a long time. -// In this test, reading from blockingBalancer.Notify() blocks forever. -type blockingBalancer struct { - ch chan []Address -} - -func newBlockingBalancer() Balancer { - return &blockingBalancer{ch: make(chan []Address)} -} -func (b *blockingBalancer) Start(target string, config BalancerConfig) error { - return nil -} -func (b *blockingBalancer) Up(addr Address) func(error) { - return nil -} -func (b *blockingBalancer) Get(ctx context.Context, opts BalancerGetOptions) (addr Address, put func(), err error) { - return Address{}, nil, nil -} -func (b *blockingBalancer) Notify() <-chan []Address { - return b.ch -} -func (b *blockingBalancer) Close() error { - close(b.ch) - return nil -} - -func (s) TestDialWithBlockingBalancer(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - dialDone := make(chan struct{}) - go func() { - DialContext(ctx, "Non-Existent.Server:80", WithBlock(), WithInsecure(), WithBalancer(newBlockingBalancer())) - close(dialDone) - }() - cancel() - <-dialDone -} - // securePerRPCCredentials always requires transport security. type securePerRPCCredentials struct{} @@ -724,50 +641,6 @@ func (s) TestConnectParamsWithMinConnectTimeout(t *testing.T) { } } -// emptyBalancer returns an empty set of servers. -type emptyBalancer struct { - ch chan []Address -} - -func newEmptyBalancer() Balancer { - return &emptyBalancer{ch: make(chan []Address, 1)} -} -func (b *emptyBalancer) Start(_ string, _ BalancerConfig) error { - b.ch <- nil - return nil -} -func (b *emptyBalancer) Up(_ Address) func(error) { - return nil -} -func (b *emptyBalancer) Get(_ context.Context, _ BalancerGetOptions) (Address, func(), error) { - return Address{}, nil, nil -} -func (b *emptyBalancer) Notify() <-chan []Address { - return b.ch -} -func (b *emptyBalancer) Close() error { - close(b.ch) - return nil -} - -func (s) TestNonblockingDialWithEmptyBalancer(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - dialDone := make(chan error) - go func() { - dialDone <- func() error { - conn, err := DialContext(ctx, "Non-Existent.Server:80", WithInsecure(), WithBalancer(newEmptyBalancer())) - if err != nil { - return err - } - return conn.Close() - }() - }() - if err := <-dialDone; err != nil { - t.Fatalf("unexpected error dialing connection: %s", err) - } -} - func (s) TestResolverServiceConfigBeforeAddressNotPanic(t *testing.T) { r, rcleanup := manual.GenerateAndRegisterManualResolver() defer rcleanup() diff --git a/dialoptions.go b/dialoptions.go index d33ec45276f..5fa6dec41f3 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -200,19 +200,6 @@ func WithDecompressor(dc Decompressor) DialOption { }) } -// WithBalancer returns a DialOption which sets a load balancer with the v1 API. -// Name resolver will be ignored if this DialOption is specified. -// -// Deprecated: use the new balancer APIs in balancer package and -// WithBalancerName. Will be removed in a future 1.x release. -func WithBalancer(b Balancer) DialOption { - return newFuncDialOption(func(o *dialOptions) { - o.balancerBuilder = &balancerWrapperBuilder{ - b: b, - } - }) -} - // WithBalancerName sets the balancer that the ClientConn will be initialized // with. Balancer registered with balancerName will be used. This function // panics if no balancer was registered by balancerName. diff --git a/picker_wrapper.go b/picker_wrapper.go index 00447894f07..a2e996d7295 100644 --- a/picker_wrapper.go +++ b/picker_wrapper.go @@ -20,7 +20,6 @@ package grpc import ( "context" - "fmt" "io" "sync" @@ -32,33 +31,13 @@ import ( "google.golang.org/grpc/status" ) -// v2PickerWrapper wraps a balancer.Picker while providing the -// balancer.V2Picker API. It requires a pickerWrapper to generate errors -// including the latest connectionError. To be deleted when balancer.Picker is -// updated to the balancer.V2Picker API. -type v2PickerWrapper struct { - picker balancer.Picker - connErr *connErr -} - -func (v *v2PickerWrapper) Pick(info balancer.PickInfo) (balancer.PickResult, error) { - sc, done, err := v.picker.Pick(info.Ctx, info) - if err != nil { - if err == balancer.ErrTransientFailure { - return balancer.PickResult{}, balancer.TransientFailureError(fmt.Errorf("%v, latest connection error: %v", err, v.connErr.connectionError())) - } - return balancer.PickResult{}, err - } - return balancer.PickResult{SubConn: sc, Done: done}, nil -} - // pickerWrapper is a wrapper of balancer.Picker. It blocks on certain pick // actions and unblock when there's a picker update. type pickerWrapper struct { mu sync.Mutex done bool blockingCh chan struct{} - picker balancer.V2Picker + picker balancer.Picker // The latest connection error. TODO: remove when V1 picker is deprecated; // balancer should be responsible for providing the error. @@ -89,11 +68,6 @@ func newPickerWrapper() *pickerWrapper { // updatePicker is called by UpdateBalancerState. It unblocks all blocked pick. func (pw *pickerWrapper) updatePicker(p balancer.Picker) { - pw.updatePickerV2(&v2PickerWrapper{picker: p, connErr: pw.connErr}) -} - -// updatePicker is called by UpdateBalancerState. It unblocks all blocked pick. -func (pw *pickerWrapper) updatePickerV2(p balancer.V2Picker) { pw.mu.Lock() if pw.done { pw.mu.Unlock() @@ -180,18 +154,18 @@ func (pw *pickerWrapper) pick(ctx context.Context, failfast bool, info balancer. if err == balancer.ErrNoSubConnAvailable { continue } - if tfe, ok := err.(interface{ IsTransientFailure() bool }); ok && tfe.IsTransientFailure() { - if !failfast { - lastPickErr = err - continue - } - return nil, nil, status.Error(codes.Unavailable, err.Error()) + if drop, ok := err.(interface{ DropRPC() bool }); ok && drop.DropRPC() { + // End the RPC unconditionally. If not a status error, Convert + // will transform it into one with code Unknown. + return nil, nil, status.Convert(err).Err() } - if _, ok := status.FromError(err); ok { - return nil, nil, err + // For all other errors, wait for ready RPCs should block and other + // RPCs should fail. + if !failfast { + lastPickErr = err + continue } - // err is some other error. - return nil, nil, status.Error(codes.Unknown, err.Error()) + return nil, nil, status.Error(codes.Unavailable, err.Error()) } acw, ok := pickResult.SubConn.(*acBalancerWrapper) diff --git a/picker_wrapper_test.go b/picker_wrapper_test.go index 50916a2dfde..5f786b28580 100644 --- a/picker_wrapper_test.go +++ b/picker_wrapper_test.go @@ -55,14 +55,14 @@ type testingPicker struct { maxCalled int64 } -func (p *testingPicker) Pick(ctx context.Context, info balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) { +func (p *testingPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { if atomic.AddInt64(&p.maxCalled, -1) < 0 { - return nil, nil, fmt.Errorf("pick called to many times (> goroutineCount)") + return balancer.PickResult{}, fmt.Errorf("pick called to many times (> goroutineCount)") } if p.err != nil { - return nil, nil, p.err + return balancer.PickResult{}, p.err } - return p.sc, nil, nil + return balancer.PickResult{SubConn: p.sc}, nil } func (s) TestBlockingPickTimeout(t *testing.T) { diff --git a/pickfirst.go b/pickfirst.go index c43dac9ad84..7ba90ae5ab1 100644 --- a/pickfirst.go +++ b/pickfirst.go @@ -25,7 +25,6 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/resolver" "google.golang.org/grpc/status" ) @@ -52,20 +51,6 @@ type pickfirstBalancer struct { sc balancer.SubConn } -var _ balancer.V2Balancer = &pickfirstBalancer{} // Assert we implement v2 - -func (b *pickfirstBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { - if err != nil { - b.ResolverError(err) - return - } - b.UpdateClientConnState(balancer.ClientConnState{ResolverState: resolver.State{Addresses: addrs}}) // Ignore error -} - -func (b *pickfirstBalancer) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - b.UpdateSubConnState(sc, balancer.SubConnState{ConnectivityState: s}) -} - func (b *pickfirstBalancer) ResolverError(err error) { switch b.state { case connectivity.TransientFailure, connectivity.Idle, connectivity.Connecting: @@ -129,15 +114,9 @@ func (b *pickfirstBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.S case connectivity.Connecting: b.cc.UpdateState(balancer.State{ConnectivityState: s.ConnectivityState, Picker: &picker{err: balancer.ErrNoSubConnAvailable}}) case connectivity.TransientFailure: - err := balancer.ErrTransientFailure - // TODO: this can be unconditional after the V1 API is removed, as - // SubConnState will always contain a connection error. - if s.ConnectionError != nil { - err = balancer.TransientFailureError(s.ConnectionError) - } b.cc.UpdateState(balancer.State{ ConnectivityState: s.ConnectivityState, - Picker: &picker{err: err}, + Picker: &picker{err: s.ConnectionError}, }) } } diff --git a/pickfirst_test.go b/pickfirst_test.go index 475eedfdf46..a69cec1c51d 100644 --- a/pickfirst_test.go +++ b/pickfirst_test.go @@ -43,7 +43,7 @@ func (s) TestOneBackendPickfirst(t *testing.T) { defer rcleanup() numServers := 1 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -76,7 +76,7 @@ func (s) TestBackendsPickfirst(t *testing.T) { defer rcleanup() numServers := 2 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -109,7 +109,7 @@ func (s) TestNewAddressWhileBlockingPickfirst(t *testing.T) { defer rcleanup() numServers := 1 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -145,7 +145,7 @@ func (s) TestCloseWithPendingRPCPickfirst(t *testing.T) { defer rcleanup() numServers := 1 - _, _, scleanup := startServers(t, numServers, math.MaxInt32) + _, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -181,7 +181,7 @@ func (s) TestOneServerDownPickfirst(t *testing.T) { defer rcleanup() numServers := 2 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -222,7 +222,7 @@ func (s) TestAllServersDownPickfirst(t *testing.T) { defer rcleanup() numServers := 2 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) @@ -265,7 +265,7 @@ func (s) TestAddressesRemovedPickfirst(t *testing.T) { defer rcleanup() numServers := 3 - servers, _, scleanup := startServers(t, numServers, math.MaxInt32) + servers, scleanup := startServers(t, numServers, math.MaxInt32) defer scleanup() cc, err := Dial(r.Scheme()+":///test.server", WithInsecure(), WithCodec(testCodec{})) diff --git a/resolver_conn_wrapper_test.go b/resolver_conn_wrapper_test.go index 605043f5f5d..9f22c8b90f6 100644 --- a/resolver_conn_wrapper_test.go +++ b/resolver_conn_wrapper_test.go @@ -29,6 +29,7 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/codes" + "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" "google.golang.org/grpc/serviceconfig" @@ -130,12 +131,12 @@ const happyBalancerName = "happy balancer" func init() { // Register a balancer that never returns an error from // UpdateClientConnState, and doesn't do anything else either. - fb := &funcBalancer{ - updateClientConnState: func(s balancer.ClientConnState) error { + bf := stub.BalancerFuncs{ + UpdateClientConnState: func(*stub.BalancerData, balancer.ClientConnState) error { return nil }, } - balancer.Register(&funcBalancerBuilder{name: happyBalancerName, instance: fb}) + stub.Register(happyBalancerName, bf) } // TestResolverErrorPolling injects resolver errors and verifies ResolveNow is diff --git a/test/balancer_test.go b/test/balancer_test.go index 7db9635a897..3cd4e0e91fb 100644 --- a/test/balancer_test.go +++ b/test/balancer_test.go @@ -20,6 +20,7 @@ package test import ( "context" + "errors" "fmt" "net" "reflect" @@ -30,12 +31,14 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/balancer/stub" "google.golang.org/grpc/internal/balancerload" + "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/metadata" "google.golang.org/grpc/resolver" @@ -69,37 +72,43 @@ func (*testBalancer) Name() string { return testBalancerName } -func (b *testBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { +func (*testBalancer) ResolverError(err error) { + panic("not implemented") +} + +func (b *testBalancer) UpdateClientConnState(state balancer.ClientConnState) error { // Only create a subconn at the first time. - if err == nil && b.sc == nil { - b.sc, err = b.cc.NewSubConn(addrs, b.newSubConnOptions) + if b.sc == nil { + var err error + b.sc, err = b.cc.NewSubConn(state.ResolverState.Addresses, b.newSubConnOptions) if err != nil { grpclog.Errorf("testBalancer: failed to NewSubConn: %v", err) - return + return nil } b.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Connecting, Picker: &picker{sc: b.sc, bal: b}}) b.sc.Connect() } + return nil } -func (b *testBalancer) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { - grpclog.Infof("testBalancer: HandleSubConnStateChange: %p, %v", sc, s) +func (b *testBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) { + grpclog.Infof("testBalancer: UpdateSubConnState: %p, %v", sc, s) if b.sc != sc { grpclog.Infof("testBalancer: ignored state change because sc is not recognized") return } - if s == connectivity.Shutdown { + if s.ConnectivityState == connectivity.Shutdown { b.sc = nil return } - switch s { + switch s.ConnectivityState { case connectivity.Ready, connectivity.Idle: - b.cc.UpdateState(balancer.State{ConnectivityState: s, Picker: &picker{sc: sc, bal: b}}) + b.cc.UpdateState(balancer.State{ConnectivityState: s.ConnectivityState, Picker: &picker{sc: sc, bal: b}}) case connectivity.Connecting: - b.cc.UpdateState(balancer.State{ConnectivityState: s, Picker: &picker{err: balancer.ErrNoSubConnAvailable, bal: b}}) + b.cc.UpdateState(balancer.State{ConnectivityState: s.ConnectivityState, Picker: &picker{err: balancer.ErrNoSubConnAvailable, bal: b}}) case connectivity.TransientFailure: - b.cc.UpdateState(balancer.State{ConnectivityState: s, Picker: &picker{err: balancer.ErrTransientFailure, bal: b}}) + b.cc.UpdateState(balancer.State{ConnectivityState: s.ConnectivityState, Picker: &picker{err: balancer.ErrTransientFailure, bal: b}}) } } @@ -285,6 +294,10 @@ func newTestBalancerKeepAddresses() *testBalancerKeepAddresses { } } +func (testBalancerKeepAddresses) ResolverError(err error) { + panic("not implemented") +} + func (b *testBalancerKeepAddresses) Build(cc balancer.ClientConn, opt balancer.BuildOptions) balancer.Balancer { return b } @@ -293,11 +306,12 @@ func (*testBalancerKeepAddresses) Name() string { return testBalancerKeepAddressesName } -func (b *testBalancerKeepAddresses) HandleResolvedAddrs(addrs []resolver.Address, err error) { - b.addrsChan <- addrs +func (b *testBalancerKeepAddresses) UpdateClientConnState(state balancer.ClientConnState) error { + b.addrsChan <- state.ResolverState.Addresses + return nil } -func (testBalancerKeepAddresses) HandleSubConnStateChange(sc balancer.SubConn, s connectivity.State) { +func (testBalancerKeepAddresses) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) { panic("not used") } @@ -472,3 +486,217 @@ func (s) TestAddressAttributesInNewSubConn(t *testing.T) { t.Fatalf("received attributes %v in creds, want %v", gotAttr, wantAttr) } } + +// TestServersSwap creates two servers and verifies the client switches between +// them when the name resolver reports the first and then the second. +func (s) TestServersSwap(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Initialize servers + reg := func(username string) (addr string, cleanup func()) { + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Error while listening. Err: %v", err) + } + s := grpc.NewServer() + ts := &funcServer{ + unaryCall: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{Username: username}, nil + }, + } + testpb.RegisterTestServiceServer(s, ts) + go s.Serve(lis) + return lis.Addr().String(), s.Stop + } + const one = "1" + addr1, cleanup := reg(one) + defer cleanup() + const two = "2" + addr2, cleanup := reg(two) + defer cleanup() + + // Initialize client + r, cleanup := manual.GenerateAndRegisterManualResolver() + defer cleanup() + r.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: addr1}}}) + cc, err := grpc.DialContext(ctx, r.Scheme()+":///", grpc.WithInsecure()) + if err != nil { + t.Fatalf("Error creating client: %v", err) + } + defer cc.Close() + client := testpb.NewTestServiceClient(cc) + + // Confirm we are connected to the first server + if res, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil || res.Username != one { + t.Fatalf("UnaryCall(_) = %v, %v; want {Username: %q}, nil", res, err, one) + } + + // Update resolver to report only the second server + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: addr2}}}) + + // Loop until new RPCs talk to server two. + for i := 0; i < 2000; i++ { + if res, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil { + t.Fatalf("UnaryCall(_) = _, %v; want _, nil", err) + } else if res.Username == two { + break // pass + } + time.Sleep(5 * time.Millisecond) + } +} + +// TestEmptyAddrs verifies client behavior when a working connection is +// removed. In pick first and round-robin, both will continue using the old +// connections. +func (s) TestEmptyAddrs(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Initialize server + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Error while listening. Err: %v", err) + } + s := grpc.NewServer() + defer s.Stop() + const one = "1" + ts := &funcServer{ + unaryCall: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{Username: one}, nil + }, + } + testpb.RegisterTestServiceServer(s, ts) + go s.Serve(lis) + + // Initialize pickfirst client + pfr, cleanup := manual.GenerateAndRegisterManualResolver() + defer cleanup() + pfrnCalled := grpcsync.NewEvent() + pfr.ResolveNowCallback = func(resolver.ResolveNowOptions) { + pfrnCalled.Fire() + } + pfr.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}}) + + pfcc, err := grpc.DialContext(ctx, pfr.Scheme()+":///", grpc.WithInsecure()) + if err != nil { + t.Fatalf("Error creating client: %v", err) + } + defer pfcc.Close() + pfclient := testpb.NewTestServiceClient(pfcc) + + // Confirm we are connected to the server + if res, err := pfclient.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil || res.Username != one { + t.Fatalf("UnaryCall(_) = %v, %v; want {Username: %q}, nil", res, err, one) + } + + // Remove all addresses. + pfr.UpdateState(resolver.State{}) + // Wait for a ResolveNow call on the pick first client's resolver. + <-pfrnCalled.Done() + + // Initialize roundrobin client + rrr, cleanup := manual.GenerateAndRegisterManualResolver() + defer cleanup() + rrrnCalled := grpcsync.NewEvent() + rrr.ResolveNowCallback = func(resolver.ResolveNowOptions) { + rrrnCalled.Fire() + } + rrr.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}}) + + rrcc, err := grpc.DialContext(ctx, rrr.Scheme()+":///", grpc.WithInsecure(), + grpc.WithDefaultServiceConfig(fmt.Sprintf(`{ "loadBalancingConfig": [{"%v": {}}] }`, roundrobin.Name))) + if err != nil { + t.Fatalf("Error creating client: %v", err) + } + defer rrcc.Close() + rrclient := testpb.NewTestServiceClient(rrcc) + + // Confirm we are connected to the server + if res, err := rrclient.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil || res.Username != one { + t.Fatalf("UnaryCall(_) = %v, %v; want {Username: %q}, nil", res, err, one) + } + + // Remove all addresses. + rrr.UpdateState(resolver.State{}) + // Wait for a ResolveNow call on the round robin client's resolver. + <-rrrnCalled.Done() + + // Confirm several new RPCs succeed on pick first. + for i := 0; i < 10; i++ { + if _, err := pfclient.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil { + t.Fatalf("UnaryCall(_) = _, %v; want _, nil", err) + } + time.Sleep(5 * time.Millisecond) + } + + // Confirm several new RPCs succeed on round robin. + for i := 0; i < 10; i++ { + if _, err := pfclient.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil { + t.Fatalf("UnaryCall(_) = _, %v; want _, nil", err) + } + time.Sleep(5 * time.Millisecond) + } +} + +func (s) TestWaitForReady(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Initialize server + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Error while listening. Err: %v", err) + } + s := grpc.NewServer() + defer s.Stop() + const one = "1" + ts := &funcServer{ + unaryCall: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{Username: one}, nil + }, + } + testpb.RegisterTestServiceServer(s, ts) + go s.Serve(lis) + + // Initialize client + r, cleanup := manual.GenerateAndRegisterManualResolver() + defer cleanup() + + cc, err := grpc.DialContext(ctx, r.Scheme()+":///", grpc.WithInsecure()) + if err != nil { + t.Fatalf("Error creating client: %v", err) + } + defer cc.Close() + client := testpb.NewTestServiceClient(cc) + + // Report an error so non-WFR RPCs will give up early. + r.CC.ReportError(errors.New("fake resolver error")) + + // Ensure the client is not connected to anything and fails non-WFR RPCs. + if res, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.Unavailable { + t.Fatalf("UnaryCall(_) = %v, %v; want _, Code()=%v", res, err, codes.Unavailable) + } + + errChan := make(chan error, 1) + go func() { + if res, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.WaitForReady(true)); err != nil || res.Username != one { + errChan <- fmt.Errorf("UnaryCall(_) = %v, %v; want {Username: %q}, nil", res, err, one) + } + close(errChan) + }() + + select { + case err := <-errChan: + t.Errorf("unexpected receive from errChan before addresses provided") + t.Fatal(err.Error()) + case <-time.After(5 * time.Millisecond): + } + + // Resolve the server. The WFR RPC should unblock and use it. + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}}) + + if err := <-errChan; err != nil { + t.Fatal(err.Error()) + } +} diff --git a/test/creds_test.go b/test/creds_test.go index 65d99c7555d..bc4dca17a78 100644 --- a/test/creds_test.go +++ b/test/creds_test.go @@ -75,7 +75,7 @@ func (c *testCredsBundle) NewWithMode(mode string) (credentials.Bundle, error) { } func (s) TestCredsBundleBoth(t *testing.T) { - te := newTest(t, env{name: "creds-bundle", network: "tcp", balancer: "v1", security: "empty"}) + te := newTest(t, env{name: "creds-bundle", network: "tcp", security: "empty"}) te.tapHandle = authHandle te.customDialOptions = []grpc.DialOption{ grpc.WithCredentialsBundle(&testCredsBundle{t: t}), @@ -98,7 +98,7 @@ func (s) TestCredsBundleBoth(t *testing.T) { } func (s) TestCredsBundleTransportCredentials(t *testing.T) { - te := newTest(t, env{name: "creds-bundle", network: "tcp", balancer: "v1", security: "empty"}) + te := newTest(t, env{name: "creds-bundle", network: "tcp", security: "empty"}) te.customDialOptions = []grpc.DialOption{ grpc.WithCredentialsBundle(&testCredsBundle{t: t, mode: bundleTLSOnly}), } @@ -120,7 +120,7 @@ func (s) TestCredsBundleTransportCredentials(t *testing.T) { } func (s) TestCredsBundlePerRPCCredentials(t *testing.T) { - te := newTest(t, env{name: "creds-bundle", network: "tcp", balancer: "v1", security: "empty"}) + te := newTest(t, env{name: "creds-bundle", network: "tcp", security: "empty"}) te.tapHandle = authHandle te.customDialOptions = []grpc.DialOption{ grpc.WithCredentialsBundle(&testCredsBundle{t: t, mode: bundlePerRPCOnly}), diff --git a/test/end2end_test.go b/test/end2end_test.go index fb95f4a2fee..7f628f9d914 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -47,7 +47,6 @@ import ( "golang.org/x/net/http2/hpack" spb "google.golang.org/genproto/googleapis/rpc/status" "google.golang.org/grpc" - "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" @@ -397,7 +396,7 @@ type env struct { network string // The type of network such as tcp, unix, etc. security string // The security protocol such as TLS, SSH, etc. httpHandler bool // whether to use the http.Handler ServerTransport; requires TLS - balancer string // One of "round_robin", "pick_first", "v1", or "". + balancer string // One of "round_robin", "pick_first", or "". customDialer func(string, string, time.Duration) (net.Conn, error) } @@ -416,8 +415,8 @@ func (e env) dialer(addr string, timeout time.Duration) (net.Conn, error) { } var ( - tcpClearEnv = env{name: "tcp-clear-v1-balancer", network: "tcp", balancer: "v1"} - tcpTLSEnv = env{name: "tcp-tls-v1-balancer", network: "tcp", security: "tls", balancer: "v1"} + tcpClearEnv = env{name: "tcp-clear-v1-balancer", network: "tcp"} + tcpTLSEnv = env{name: "tcp-tls-v1-balancer", network: "tcp", security: "tls"} tcpClearRREnv = env{name: "tcp-clear", network: "tcp", balancer: "round_robin"} tcpTLSRREnv = env{name: "tcp-tls", network: "tcp", security: "tls", balancer: "round_robin"} handlerEnv = env{name: "handler-tls", network: "tcp", security: "tls", httpHandler: true, balancer: "round_robin"} @@ -809,11 +808,8 @@ func (te *test) configDial(opts ...grpc.DialOption) ([]grpc.DialOption, string) } else { scheme = te.resolverScheme + ":///" } - switch te.e.balancer { - case "v1": - opts = append(opts, grpc.WithBalancer(grpc.RoundRobin(nil))) - case "round_robin": - opts = append(opts, grpc.WithBalancerName(roundrobin.Name)) + if te.e.balancer != "" { + opts = append(opts, grpc.WithBalancerName(te.e.balancer)) } if te.clientInitialWindowSize > 0 { opts = append(opts, grpc.WithInitialWindowSize(te.clientInitialWindowSize)) @@ -4712,6 +4708,251 @@ func testClientResourceExhaustedCancelFullDuplex(t *testing.T, e env) { } } +type clientTimeoutCreds struct { + timeoutReturned bool +} + +func (c *clientTimeoutCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + if !c.timeoutReturned { + c.timeoutReturned = true + return nil, nil, context.DeadlineExceeded + } + return rawConn, nil, nil +} +func (c *clientTimeoutCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + return rawConn, nil, nil +} +func (c *clientTimeoutCreds) Info() credentials.ProtocolInfo { + return credentials.ProtocolInfo{} +} +func (c *clientTimeoutCreds) Clone() credentials.TransportCredentials { + return nil +} +func (c *clientTimeoutCreds) OverrideServerName(s string) error { + return nil +} + +func (s) TestNonFailFastRPCSucceedOnTimeoutCreds(t *testing.T) { + te := newTest(t, env{name: "timeout-cred", network: "tcp", security: "clientTimeoutCreds"}) + te.userAgent = testAppUA + te.startServer(&testServer{security: te.e.security}) + defer te.tearDown() + + cc := te.clientConn() + tc := testpb.NewTestServiceClient(cc) + // This unary call should succeed, because ClientHandshake will succeed for the second time. + if _, err := tc.EmptyCall(context.Background(), &testpb.Empty{}, grpc.WaitForReady(true)); err != nil { + te.t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want ", err) + } +} + +type serverDispatchCred struct { + rawConnCh chan net.Conn +} + +func newServerDispatchCred() *serverDispatchCred { + return &serverDispatchCred{ + rawConnCh: make(chan net.Conn, 1), + } +} +func (c *serverDispatchCred) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + return rawConn, nil, nil +} +func (c *serverDispatchCred) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + select { + case c.rawConnCh <- rawConn: + default: + } + return nil, nil, credentials.ErrConnDispatched +} +func (c *serverDispatchCred) Info() credentials.ProtocolInfo { + return credentials.ProtocolInfo{} +} +func (c *serverDispatchCred) Clone() credentials.TransportCredentials { + return nil +} +func (c *serverDispatchCred) OverrideServerName(s string) error { + return nil +} +func (c *serverDispatchCred) getRawConn() net.Conn { + return <-c.rawConnCh +} + +func (s) TestServerCredsDispatch(t *testing.T) { + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Failed to listen: %v", err) + } + cred := newServerDispatchCred() + s := grpc.NewServer(grpc.Creds(cred)) + go s.Serve(lis) + defer s.Stop() + + cc, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(cred)) + if err != nil { + t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) + } + defer cc.Close() + + rawConn := cred.getRawConn() + // Give grpc a chance to see the error and potentially close the connection. + // And check that connection is not closed after that. + time.Sleep(100 * time.Millisecond) + // Check rawConn is not closed. + if n, err := rawConn.Write([]byte{0}); n <= 0 || err != nil { + t.Errorf("Read() = %v, %v; want n>0, ", n, err) + } +} + +type authorityCheckCreds struct { + got string +} + +func (c *authorityCheckCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + return rawConn, nil, nil +} +func (c *authorityCheckCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + c.got = authority + return rawConn, nil, nil +} +func (c *authorityCheckCreds) Info() credentials.ProtocolInfo { + return credentials.ProtocolInfo{} +} +func (c *authorityCheckCreds) Clone() credentials.TransportCredentials { + return c +} +func (c *authorityCheckCreds) OverrideServerName(s string) error { + return nil +} + +// This test makes sure that the authority client handshake gets is the endpoint +// in dial target, not the resolved ip address. +func (s) TestCredsHandshakeAuthority(t *testing.T) { + const testAuthority = "test.auth.ori.ty" + + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Failed to listen: %v", err) + } + cred := &authorityCheckCreds{} + s := grpc.NewServer() + go s.Serve(lis) + defer s.Stop() + + r, rcleanup := manual.GenerateAndRegisterManualResolver() + defer rcleanup() + + cc, err := grpc.Dial(r.Scheme()+":///"+testAuthority, grpc.WithTransportCredentials(cred)) + if err != nil { + t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) + } + defer cc.Close() + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}}) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + for { + s := cc.GetState() + if s == connectivity.Ready { + break + } + if !cc.WaitForStateChange(ctx, s) { + // ctx got timeout or canceled. + t.Fatalf("ClientConn is not ready after 100 ms") + } + } + + if cred.got != testAuthority { + t.Fatalf("client creds got authority: %q, want: %q", cred.got, testAuthority) + } +} + +// This test makes sure that the authority client handshake gets is the endpoint +// of the ServerName of the address when it is set. +func (s) TestCredsHandshakeServerNameAuthority(t *testing.T) { + const testAuthority = "test.auth.ori.ty" + const testServerName = "test.server.name" + + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Failed to listen: %v", err) + } + cred := &authorityCheckCreds{} + s := grpc.NewServer() + go s.Serve(lis) + defer s.Stop() + + r, rcleanup := manual.GenerateAndRegisterManualResolver() + defer rcleanup() + + cc, err := grpc.Dial(r.Scheme()+":///"+testAuthority, grpc.WithTransportCredentials(cred)) + if err != nil { + t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) + } + defer cc.Close() + r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String(), ServerName: testServerName}}}) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + for { + s := cc.GetState() + if s == connectivity.Ready { + break + } + if !cc.WaitForStateChange(ctx, s) { + // ctx got timeout or canceled. + t.Fatalf("ClientConn is not ready after 100 ms") + } + } + + if cred.got != testServerName { + t.Fatalf("client creds got authority: %q, want: %q", cred.got, testAuthority) + } +} + +type clientFailCreds struct{} + +func (c *clientFailCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + return rawConn, nil, nil +} +func (c *clientFailCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { + return nil, nil, fmt.Errorf("client handshake fails with fatal error") +} +func (c *clientFailCreds) Info() credentials.ProtocolInfo { + return credentials.ProtocolInfo{} +} +func (c *clientFailCreds) Clone() credentials.TransportCredentials { + return c +} +func (c *clientFailCreds) OverrideServerName(s string) error { + return nil +} + +// This test makes sure that failfast RPCs fail if client handshake fails with +// fatal errors. +func (s) TestFailfastRPCFailOnFatalHandshakeError(t *testing.T) { + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("Failed to listen: %v", err) + } + defer lis.Close() + + cc, err := grpc.Dial("passthrough:///"+lis.Addr().String(), grpc.WithTransportCredentials(&clientFailCreds{})) + if err != nil { + t.Fatalf("grpc.Dial(_) = %v", err) + } + defer cc.Close() + + tc := testpb.NewTestServiceClient(cc) + // This unary call should fail, but not timeout. + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + if _, err := tc.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(false)); status.Code(err) != codes.Unavailable { + t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want ", err) + } +} + +>>>>>>> c9ad8dff... balancer: move Balancer and Picker to V2; delete legacy API func (s) TestFlowControlLogicalRace(t *testing.T) { // Test for a regression of https://github.com/grpc/grpc-go/issues/632, // and other flow control bugs. diff --git a/vet.sh b/vet.sh index 84b18859eb9..493e67b511e 100755 --- a/vet.sh +++ b/vet.sh @@ -132,7 +132,6 @@ not grep -Fv '.CredsBundle .NewServiceConfig .Metadata is deprecated: use Attributes .Type is deprecated: use Attributes -.UpdateBalancerState balancer.Picker grpc.CallCustomCodec grpc.Code diff --git a/xds/internal/balancer/balancergroup/balancergroup.go b/xds/internal/balancer/balancergroup/balancergroup.go index d6307867207..fdc3f42ec5b 100644 --- a/xds/internal/balancer/balancergroup/balancergroup.go +++ b/xds/internal/balancer/balancergroup/balancergroup.go @@ -68,9 +68,6 @@ type subBalancerWithConfig struct { balancer balancer.Balancer } -func (sbc *subBalancerWithConfig) UpdateBalancerState(state connectivity.State, picker balancer.Picker) { -} - // UpdateState overrides balancer.ClientConn, to keep state and picker. func (sbc *subBalancerWithConfig) UpdateState(state balancer.State) { sbc.mu.Lock() @@ -97,11 +94,7 @@ func (sbc *subBalancerWithConfig) startBalancer() { b := sbc.builder.Build(sbc, balancer.BuildOptions{}) sbc.group.logger.Infof("Created child policy %p of type %v", b, sbc.builder.Name()) sbc.balancer = b - if ub, ok := b.(balancer.V2Balancer); ok { - ub.UpdateClientConnState(sbc.ccState) - return - } - b.HandleResolvedAddrs(sbc.ccState.ResolverState.Addresses, nil) + b.UpdateClientConnState(sbc.ccState) } func (sbc *subBalancerWithConfig) updateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { @@ -113,11 +106,7 @@ func (sbc *subBalancerWithConfig) updateSubConnState(sc balancer.SubConn, state // happen. return } - if ub, ok := b.(balancer.V2Balancer); ok { - ub.UpdateSubConnState(sc, state) - return - } - b.HandleSubConnStateChange(sc, state.ConnectivityState) + b.UpdateSubConnState(sc, state) } func (sbc *subBalancerWithConfig) updateClientConnState(s balancer.ClientConnState) error { @@ -134,10 +123,7 @@ func (sbc *subBalancerWithConfig) updateClientConnState(s balancer.ClientConnSta // it's the lower priority, but it can still get address updates. return nil } - if ub, ok := b.(balancer.V2Balancer); ok { - return ub.UpdateClientConnState(s) - } - b.HandleResolvedAddrs(s.ResolverState.Addresses, nil) + return ub.UpdateClientConnState(s) return nil } @@ -148,7 +134,7 @@ func (sbc *subBalancerWithConfig) stopBalancer() { type pickerState struct { weight uint32 - picker balancer.V2Picker + picker balancer.Picker state connectivity.State } @@ -583,7 +569,7 @@ func buildPickerAndState(m map[internal.LocalityID]*pickerState) balancer.State aggregatedState = connectivity.TransientFailure } if aggregatedState == connectivity.TransientFailure { - return balancer.State{ConnectivityState: aggregatedState, Picker: base.NewErrPickerV2(balancer.ErrTransientFailure)} + return balancer.State{ConnectivityState: aggregatedState, Picker: base.NewErrPicker(balancer.ErrTransientFailure)} } return balancer.State{ConnectivityState: aggregatedState, Picker: newPickerGroup(readyPickerWithWeights)} } @@ -620,7 +606,7 @@ func (pg *pickerGroup) Pick(info balancer.PickInfo) (balancer.PickResult, error) if pg.length <= 0 { return balancer.PickResult{}, balancer.ErrNoSubConnAvailable } - p := pg.w.Next().(balancer.V2Picker) + p := pg.w.Next().(balancer.Picker) return p.Pick(info) } @@ -630,13 +616,13 @@ const ( ) type loadReportPicker struct { - p balancer.V2Picker + p balancer.Picker id internal.LocalityID loadStore lrs.Store } -func newLoadReportPicker(p balancer.V2Picker, id internal.LocalityID, loadStore lrs.Store) *loadReportPicker { +func newLoadReportPicker(p balancer.Picker, id internal.LocalityID, loadStore lrs.Store) *loadReportPicker { return &loadReportPicker{ p: p, id: id, diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer.go b/xds/internal/balancer/cdsbalancer/cdsbalancer.go index bb1653868e4..31ab7d697b3 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer.go @@ -25,7 +25,6 @@ import ( "google.golang.org/grpc/attributes" "google.golang.org/grpc/balancer" - "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal/buffer" "google.golang.org/grpc/internal/grpclog" "google.golang.org/grpc/resolver" @@ -46,7 +45,7 @@ var ( // newEDSBalancer is a helper function to build a new edsBalancer and will be // overridden in unittests. - newEDSBalancer = func(cc balancer.ClientConn, opts balancer.BuildOptions) (balancer.V2Balancer, error) { + newEDSBalancer = func(cc balancer.ClientConn, opts balancer.BuildOptions) (balancer.Balancer, error) { builder := balancer.Get(edsName) if builder == nil { return nil, fmt.Errorf("xds: no balancer builder with name %v", edsName) @@ -54,7 +53,7 @@ var ( // We directly pass the parent clientConn to the // underlying edsBalancer because the cdsBalancer does // not deal with subConns. - return builder.Build(cc, opts).(balancer.V2Balancer), nil + return builder.Build(cc, opts), nil } ) @@ -151,7 +150,7 @@ type cdsBalancer struct { updateCh *buffer.Unbounded client xdsClientInterface cancelWatch func() - edsLB balancer.V2Balancer + edsLB balancer.Balancer clusterToWatch string logger *grpclog.PrefixLogger @@ -354,11 +353,3 @@ func (b *cdsBalancer) isClosed() bool { b.mu.Unlock() return closed } - -func (b *cdsBalancer) HandleSubConnStateChange(sc balancer.SubConn, state connectivity.State) { - b.logger.Errorf("UpdateSubConnState should be called instead of HandleSubConnStateChange") -} - -func (b *cdsBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { - b.logger.Errorf("UpdateClientConnState should be called instead of HandleResolvedAddrs") -} diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go index 2d5d305ab9f..683c40b89cb 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go @@ -219,11 +219,11 @@ func edsCCS(service string, enableLRS bool, xdsClient interface{}) balancer.Clie func setup() (*cdsBalancer, *testEDSBalancer, func()) { builder := cdsBB{} tcc := &testClientConn{} - cdsB := builder.Build(tcc, balancer.BuildOptions{}).(balancer.V2Balancer) + cdsB := builder.Build(tcc, balancer.BuildOptions{}) edsB := newTestEDSBalancer() oldEDSBalancerBuilder := newEDSBalancer - newEDSBalancer = func(cc balancer.ClientConn, opts balancer.BuildOptions) (balancer.V2Balancer, error) { + newEDSBalancer = func(cc balancer.ClientConn, opts balancer.BuildOptions) (balancer.Balancer, error) { return edsB, nil } diff --git a/xds/internal/balancer/edsbalancer/balancergroup_test.go b/xds/internal/balancer/edsbalancer/balancergroup_test.go new file mode 100644 index 00000000000..e680455be6d --- /dev/null +++ b/xds/internal/balancer/edsbalancer/balancergroup_test.go @@ -0,0 +1,796 @@ +/* + * 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 edsbalancer + +import ( + "fmt" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/roundrobin" + "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/resolver" + "google.golang.org/grpc/xds/internal" + orcapb "google.golang.org/grpc/xds/internal/proto/udpa/data/orca/v1" +) + +var ( + rrBuilder = balancer.Get(roundrobin.Name) + testBalancerIDs = []internal.Locality{{Region: "b1"}, {Region: "b2"}, {Region: "b3"}} + testBackendAddrs []resolver.Address +) + +const testBackendAddrsCount = 12 + +func init() { + for i := 0; i < testBackendAddrsCount; i++ { + testBackendAddrs = append(testBackendAddrs, resolver.Address{Addr: fmt.Sprintf("%d.%d.%d.%d:%d", i, i, i, i, i)}) + } + + // Disable caching for all tests. It will be re-enabled in caching specific + // tests. + defaultSubBalancerCloseTimeout = time.Millisecond +} + +func subConnFromPicker(p balancer.Picker) func() balancer.SubConn { + return func() balancer.SubConn { + scst, _ := p.Pick(balancer.PickInfo{}) + return scst.SubConn + } +} + +// 1 balancer, 1 backend -> 2 backends -> 1 backend. +func (s) TestBalancerGroup_OneRR_AddRemoveBackend(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add one balancer to group. + bg.add(testBalancerIDs[0], 1, rrBuilder) + // Send one resolved address. + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) + + // Send subconn state change. + sc1 := <-cc.newSubConnCh + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + + // Test pick with one backend. + p1 := <-cc.newPickerCh + for i := 0; i < 5; i++ { + gotSCSt, _ := p1.Pick(balancer.PickInfo{}) + if !cmp.Equal(gotSCSt.SubConn, sc1, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("picker.Pick, got %v, want SubConn=%v", gotSCSt, sc1) + } + } + + // Send two addresses. + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + // Expect one new subconn, send state update. + sc2 := <-cc.newSubConnCh + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + + // Test roundrobin pick. + p2 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc2} + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Remove the first address. + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[1:2]) + scToRemove := <-cc.removeSubConnCh + if !cmp.Equal(scToRemove, sc1, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("RemoveSubConn, want %v, got %v", sc1, scToRemove) + } + bg.handleSubConnStateChange(scToRemove, connectivity.Shutdown) + + // Test pick with only the second subconn. + p3 := <-cc.newPickerCh + for i := 0; i < 5; i++ { + gotSC, _ := p3.Pick(balancer.PickInfo{}) + if !cmp.Equal(gotSC.SubConn, sc2, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("picker.Pick, got %v, want SubConn=%v", gotSC, sc2) + } + } +} + +// 2 balancers, each with 1 backend. +func (s) TestBalancerGroup_TwoRR_OneBackend(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add two balancers to group and send one resolved address to both + // balancers. + bg.add(testBalancerIDs[0], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) + sc1 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[0:1]) + sc2 := <-cc.newSubConnCh + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc2} + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } +} + +// 2 balancers, each with more than 1 backends. +func (s) TestBalancerGroup_TwoRR_MoreBackends(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add two balancers to group and send one resolved address to both + // balancers. + bg.add(testBalancerIDs[0], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + sc1 := <-cc.newSubConnCh + sc2 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + sc3 := <-cc.newSubConnCh + sc4 := <-cc.newSubConnCh + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + bg.handleSubConnStateChange(sc3, connectivity.Connecting) + bg.handleSubConnStateChange(sc3, connectivity.Ready) + bg.handleSubConnStateChange(sc4, connectivity.Connecting) + bg.handleSubConnStateChange(sc4, connectivity.Ready) + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc2, sc3, sc4} + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Turn sc2's connection down, should be RR between balancers. + bg.handleSubConnStateChange(sc2, connectivity.TransientFailure) + p2 := <-cc.newPickerCh + // Expect two sc1's in the result, because balancer1 will be picked twice, + // but there's only one sc in it. + want = []balancer.SubConn{sc1, sc1, sc3, sc4} + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Remove sc3's addresses. + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[3:4]) + scToRemove := <-cc.removeSubConnCh + if !cmp.Equal(scToRemove, sc3, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("RemoveSubConn, want %v, got %v", sc3, scToRemove) + } + bg.handleSubConnStateChange(scToRemove, connectivity.Shutdown) + p3 := <-cc.newPickerCh + want = []balancer.SubConn{sc1, sc4} + if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Turn sc1's connection down. + bg.handleSubConnStateChange(sc1, connectivity.TransientFailure) + p4 := <-cc.newPickerCh + want = []balancer.SubConn{sc4} + if err := isRoundRobin(want, subConnFromPicker(p4)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Turn last connection to connecting. + bg.handleSubConnStateChange(sc4, connectivity.Connecting) + p5 := <-cc.newPickerCh + for i := 0; i < 5; i++ { + if _, err := p5.Pick(balancer.PickInfo{}); err != balancer.ErrNoSubConnAvailable { + t.Fatalf("want pick error %v, got %v", balancer.ErrNoSubConnAvailable, err) + } + } + + // Turn all connections down. + bg.handleSubConnStateChange(sc4, connectivity.TransientFailure) + p6 := <-cc.newPickerCh + for i := 0; i < 5; i++ { + if _, err := p6.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure { + t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err) + } + } +} + +// 2 balancers with different weights. +func (s) TestBalancerGroup_TwoRR_DifferentWeight_MoreBackends(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add two balancers to group and send two resolved addresses to both + // balancers. + bg.add(testBalancerIDs[0], 2, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + sc1 := <-cc.newSubConnCh + sc2 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + sc3 := <-cc.newSubConnCh + sc4 := <-cc.newSubConnCh + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + bg.handleSubConnStateChange(sc3, connectivity.Connecting) + bg.handleSubConnStateChange(sc3, connectivity.Ready) + bg.handleSubConnStateChange(sc4, connectivity.Connecting) + bg.handleSubConnStateChange(sc4, connectivity.Ready) + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc1, sc2, sc2, sc3, sc4} + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } +} + +// totally 3 balancers, add/remove balancer. +func (s) TestBalancerGroup_ThreeRR_RemoveBalancer(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add three balancers to group and send one resolved address to both + // balancers. + bg.add(testBalancerIDs[0], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) + sc1 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[1:2]) + sc2 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[2], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[2], testBackendAddrs[1:2]) + sc3 := <-cc.newSubConnCh + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + bg.handleSubConnStateChange(sc3, connectivity.Connecting) + bg.handleSubConnStateChange(sc3, connectivity.Ready) + + p1 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc2, sc3} + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // Remove the second balancer, while the others two are ready. + bg.remove(testBalancerIDs[1]) + scToRemove := <-cc.removeSubConnCh + if !cmp.Equal(scToRemove, sc2, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("RemoveSubConn, want %v, got %v", sc2, scToRemove) + } + p2 := <-cc.newPickerCh + want = []balancer.SubConn{sc1, sc3} + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + // move balancer 3 into transient failure. + bg.handleSubConnStateChange(sc3, connectivity.TransientFailure) + // Remove the first balancer, while the third is transient failure. + bg.remove(testBalancerIDs[0]) + scToRemove = <-cc.removeSubConnCh + if !cmp.Equal(scToRemove, sc1, cmp.AllowUnexported(testSubConn{})) { + t.Fatalf("RemoveSubConn, want %v, got %v", sc1, scToRemove) + } + p3 := <-cc.newPickerCh + for i := 0; i < 5; i++ { + if _, err := p3.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure { + t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err) + } + } +} + +// 2 balancers, change balancer weight. +func (s) TestBalancerGroup_TwoRR_ChangeWeight_MoreBackends(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + bg.start() + + // Add two balancers to group and send two resolved addresses to both + // balancers. + bg.add(testBalancerIDs[0], 2, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + sc1 := <-cc.newSubConnCh + sc2 := <-cc.newSubConnCh + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + sc3 := <-cc.newSubConnCh + sc4 := <-cc.newSubConnCh + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + bg.handleSubConnStateChange(sc3, connectivity.Connecting) + bg.handleSubConnStateChange(sc3, connectivity.Ready) + bg.handleSubConnStateChange(sc4, connectivity.Connecting) + bg.handleSubConnStateChange(sc4, connectivity.Ready) + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{sc1, sc1, sc2, sc2, sc3, sc4} + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + bg.changeWeight(testBalancerIDs[0], 3) + + // Test roundrobin with new weight. + p2 := <-cc.newPickerCh + want = []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc2, sc3, sc4} + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } +} + +func (s) TestBalancerGroup_LoadReport(t *testing.T) { + testLoadStore := newTestLoadStore() + + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, testLoadStore, nil) + bg.start() + + backendToBalancerID := make(map[balancer.SubConn]internal.Locality) + + // Add two balancers to group and send two resolved addresses to both + // balancers. + bg.add(testBalancerIDs[0], 2, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + sc1 := <-cc.newSubConnCh + sc2 := <-cc.newSubConnCh + backendToBalancerID[sc1] = testBalancerIDs[0] + backendToBalancerID[sc2] = testBalancerIDs[0] + + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + sc3 := <-cc.newSubConnCh + sc4 := <-cc.newSubConnCh + backendToBalancerID[sc3] = testBalancerIDs[1] + backendToBalancerID[sc4] = testBalancerIDs[1] + + // Send state changes for both subconns. + bg.handleSubConnStateChange(sc1, connectivity.Connecting) + bg.handleSubConnStateChange(sc1, connectivity.Ready) + bg.handleSubConnStateChange(sc2, connectivity.Connecting) + bg.handleSubConnStateChange(sc2, connectivity.Ready) + bg.handleSubConnStateChange(sc3, connectivity.Connecting) + bg.handleSubConnStateChange(sc3, connectivity.Ready) + bg.handleSubConnStateChange(sc4, connectivity.Connecting) + bg.handleSubConnStateChange(sc4, connectivity.Ready) + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + var ( + wantStart []internal.Locality + wantEnd []internal.Locality + wantCost []testServerLoad + ) + for i := 0; i < 10; i++ { + scst, _ := p1.Pick(balancer.PickInfo{}) + locality := backendToBalancerID[scst.SubConn] + wantStart = append(wantStart, locality) + if scst.Done != nil && scst.SubConn != sc1 { + scst.Done(balancer.DoneInfo{ + ServerLoad: &orcapb.OrcaLoadReport{ + CpuUtilization: 10, + MemUtilization: 5, + RequestCost: map[string]float64{"pic": 3.14}, + Utilization: map[string]float64{"piu": 3.14}, + }, + }) + wantEnd = append(wantEnd, locality) + wantCost = append(wantCost, + testServerLoad{name: serverLoadCPUName, d: 10}, + testServerLoad{name: serverLoadMemoryName, d: 5}, + testServerLoad{name: "pic", d: 3.14}, + testServerLoad{name: "piu", d: 3.14}) + } + } + + if !cmp.Equal(testLoadStore.callsStarted, wantStart) { + t.Fatalf("want started: %v, got: %v", testLoadStore.callsStarted, wantStart) + } + if !cmp.Equal(testLoadStore.callsEnded, wantEnd) { + t.Fatalf("want ended: %v, got: %v", testLoadStore.callsEnded, wantEnd) + } + if !cmp.Equal(testLoadStore.callsCost, wantCost, cmp.AllowUnexported(testServerLoad{})) { + t.Fatalf("want cost: %v, got: %v", testLoadStore.callsCost, wantCost) + } +} + +// Create a new balancer group, add balancer and backends, but not start. +// - b1, weight 2, backends [0,1] +// - b2, weight 1, backends [2,3] +// Start the balancer group and check behavior. +// +// Close the balancer group, call add/remove/change weight/change address. +// - b2, weight 3, backends [0,3] +// - b3, weight 1, backends [1,2] +// Start the balancer group again and check for behavior. +func (s) TestBalancerGroup_start_close(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + + // Add two balancers to group and send two resolved addresses to both + // balancers. + bg.add(testBalancerIDs[0], 2, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + + bg.start() + + m1 := make(map[resolver.Address]balancer.SubConn) + for i := 0; i < 4; i++ { + addrs := <-cc.newSubConnAddrsCh + sc := <-cc.newSubConnCh + m1[addrs[0]] = sc + bg.handleSubConnStateChange(sc, connectivity.Connecting) + bg.handleSubConnStateChange(sc, connectivity.Ready) + } + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{ + m1[testBackendAddrs[0]], m1[testBackendAddrs[0]], + m1[testBackendAddrs[1]], m1[testBackendAddrs[1]], + m1[testBackendAddrs[2]], m1[testBackendAddrs[3]], + } + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + bg.close() + for i := 0; i < 4; i++ { + bg.handleSubConnStateChange(<-cc.removeSubConnCh, connectivity.Shutdown) + } + + // Add b3, weight 1, backends [1,2]. + bg.add(testBalancerIDs[2], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[2], testBackendAddrs[1:3]) + + // Remove b1. + bg.remove(testBalancerIDs[0]) + + // Update b2 to weight 3, backends [0,3]. + bg.changeWeight(testBalancerIDs[1], 3) + bg.handleResolvedAddrs(testBalancerIDs[1], append([]resolver.Address(nil), testBackendAddrs[0], testBackendAddrs[3])) + + bg.start() + + m2 := make(map[resolver.Address]balancer.SubConn) + for i := 0; i < 4; i++ { + addrs := <-cc.newSubConnAddrsCh + sc := <-cc.newSubConnCh + m2[addrs[0]] = sc + bg.handleSubConnStateChange(sc, connectivity.Connecting) + bg.handleSubConnStateChange(sc, connectivity.Ready) + } + + // Test roundrobin on the last picker. + p2 := <-cc.newPickerCh + want = []balancer.SubConn{ + m2[testBackendAddrs[0]], m2[testBackendAddrs[0]], m2[testBackendAddrs[0]], + m2[testBackendAddrs[3]], m2[testBackendAddrs[3]], m2[testBackendAddrs[3]], + m2[testBackendAddrs[1]], m2[testBackendAddrs[2]], + } + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } +} + +// Test that balancer group start() doesn't deadlock if the balancer calls back +// into balancer group inline when it gets an update. +// +// The potential deadlock can happen if we +// - hold a lock and send updates to balancer (e.g. update resolved addresses) +// - the balancer calls back (NewSubConn or update picker) in line +// The callback will try to hold hte same lock again, which will cause a +// deadlock. +// +// This test starts the balancer group with a test balancer, will updates picker +// whenever it gets an address update. It's expected that start() doesn't block +// because of deadlock. +func (s) TestBalancerGroup_start_close_deadlock(t *testing.T) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + + bg.add(testBalancerIDs[0], 2, &testConstBalancerBuilder{}) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + bg.add(testBalancerIDs[1], 1, &testConstBalancerBuilder{}) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + + bg.start() +} + +func replaceDefaultSubBalancerCloseTimeout(n time.Duration) func() { + old := defaultSubBalancerCloseTimeout + defaultSubBalancerCloseTimeout = n + return func() { defaultSubBalancerCloseTimeout = old } +} + +// initBalancerGroupForCachingTest creates a balancer group, and initialize it +// to be ready for caching tests. +// +// Two rr balancers are added to bg, each with 2 ready subConns. A sub-balancer +// is removed later, so the balancer group returned has one sub-balancer in its +// own map, and one sub-balancer in cache. +func initBalancerGroupForCachingTest(t *testing.T) (*balancerGroup, *testClientConn, map[resolver.Address]balancer.SubConn) { + cc := newTestClientConn(t) + bg := newBalancerGroup(cc, nil, nil) + + // Add two balancers to group and send two resolved addresses to both + // balancers. + bg.add(testBalancerIDs[0], 2, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) + bg.add(testBalancerIDs[1], 1, rrBuilder) + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) + + bg.start() + + m1 := make(map[resolver.Address]balancer.SubConn) + for i := 0; i < 4; i++ { + addrs := <-cc.newSubConnAddrsCh + sc := <-cc.newSubConnCh + m1[addrs[0]] = sc + bg.handleSubConnStateChange(sc, connectivity.Connecting) + bg.handleSubConnStateChange(sc, connectivity.Ready) + } + + // Test roundrobin on the last picker. + p1 := <-cc.newPickerCh + want := []balancer.SubConn{ + m1[testBackendAddrs[0]], m1[testBackendAddrs[0]], + m1[testBackendAddrs[1]], m1[testBackendAddrs[1]], + m1[testBackendAddrs[2]], m1[testBackendAddrs[3]], + } + if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + bg.remove(testBalancerIDs[1]) + // Don't wait for SubConns to be removed after close, because they are only + // removed after close timeout. + for i := 0; i < 10; i++ { + select { + case <-cc.removeSubConnCh: + t.Fatalf("Got request to remove subconn, want no remove subconn (because subconns were still in cache)") + default: + } + time.Sleep(time.Millisecond) + } + // Test roundrobin on the with only sub-balancer0. + p2 := <-cc.newPickerCh + want = []balancer.SubConn{ + m1[testBackendAddrs[0]], m1[testBackendAddrs[1]], + } + if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + return bg, cc, m1 +} + +// Test that if a sub-balancer is removed, and re-added within close timeout, +// the subConns won't be re-created. +func (s) TestBalancerGroup_locality_caching(t *testing.T) { + defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() + bg, cc, addrToSC := initBalancerGroupForCachingTest(t) + + // Turn down subconn for addr2, shouldn't get picker update because + // sub-balancer1 was removed. + bg.handleSubConnStateChange(addrToSC[testBackendAddrs[2]], connectivity.TransientFailure) + for i := 0; i < 10; i++ { + select { + case <-cc.newPickerCh: + t.Fatalf("Got new picker, want no new picker (because the sub-balancer was removed)") + default: + } + time.Sleep(time.Millisecond) + } + + // Sleep, but sleep less then close timeout. + time.Sleep(time.Millisecond * 100) + + // Re-add sub-balancer-1, because subconns were in cache, no new subconns + // should be created. But a new picker will still be generated, with subconn + // states update to date. + bg.add(testBalancerIDs[1], 1, rrBuilder) + + p3 := <-cc.newPickerCh + want := []balancer.SubConn{ + addrToSC[testBackendAddrs[0]], addrToSC[testBackendAddrs[0]], + addrToSC[testBackendAddrs[1]], addrToSC[testBackendAddrs[1]], + // addr2 is down, b2 only has addr3 in READY state. + addrToSC[testBackendAddrs[3]], addrToSC[testBackendAddrs[3]], + } + if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } + + for i := 0; i < 10; i++ { + select { + case <-cc.newSubConnAddrsCh: + t.Fatalf("Got new subconn, want no new subconn (because subconns were still in cache)") + default: + } + time.Sleep(time.Millisecond * 10) + } +} + +// Sub-balancers are put in cache when they are removed. If balancer group is +// closed within close timeout, all subconns should still be rmeoved +// immediately. +func (s) TestBalancerGroup_locality_caching_close_group(t *testing.T) { + defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() + bg, cc, addrToSC := initBalancerGroupForCachingTest(t) + + bg.close() + // The balancer group is closed. The subconns should be removed immediately. + removeTimeout := time.After(time.Millisecond * 500) + scToRemove := map[balancer.SubConn]int{ + addrToSC[testBackendAddrs[0]]: 1, + addrToSC[testBackendAddrs[1]]: 1, + addrToSC[testBackendAddrs[2]]: 1, + addrToSC[testBackendAddrs[3]]: 1, + } + for i := 0; i < len(scToRemove); i++ { + select { + case sc := <-cc.removeSubConnCh: + c := scToRemove[sc] + if c == 0 { + t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) + } + scToRemove[sc] = c - 1 + case <-removeTimeout: + t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") + } + } +} + +// Sub-balancers in cache will be closed if not re-added within timeout, and +// subConns will be removed. +func (s) TestBalancerGroup_locality_caching_not_readd_within_timeout(t *testing.T) { + defer replaceDefaultSubBalancerCloseTimeout(time.Second)() + _, cc, addrToSC := initBalancerGroupForCachingTest(t) + + // The sub-balancer is not re-added withtin timeout. The subconns should be + // removed. + removeTimeout := time.After(defaultSubBalancerCloseTimeout) + scToRemove := map[balancer.SubConn]int{ + addrToSC[testBackendAddrs[2]]: 1, + addrToSC[testBackendAddrs[3]]: 1, + } + for i := 0; i < len(scToRemove); i++ { + select { + case sc := <-cc.removeSubConnCh: + c := scToRemove[sc] + if c == 0 { + t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) + } + scToRemove[sc] = c - 1 + case <-removeTimeout: + t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") + } + } +} + +// Wrap the rr builder, so it behaves the same, but has a different pointer. +type noopBalancerBuilderWrapper struct { + balancer.Builder +} + +// After removing a sub-balancer, re-add with same ID, but different balancer +// builder. Old subconns should be removed, and new subconns should be created. +func (s) TestBalancerGroup_locality_caching_readd_with_different_builder(t *testing.T) { + defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() + bg, cc, addrToSC := initBalancerGroupForCachingTest(t) + + // Re-add sub-balancer-1, but with a different balancer builder. The + // sub-balancer was still in cache, but cann't be reused. This should cause + // old sub-balancer's subconns to be removed immediately, and new subconns + // to be created. + bg.add(testBalancerIDs[1], 1, &noopBalancerBuilderWrapper{rrBuilder}) + + // The cached sub-balancer should be closed, and the subconns should be + // removed immediately. + removeTimeout := time.After(time.Millisecond * 500) + scToRemove := map[balancer.SubConn]int{ + addrToSC[testBackendAddrs[2]]: 1, + addrToSC[testBackendAddrs[3]]: 1, + } + for i := 0; i < len(scToRemove); i++ { + select { + case sc := <-cc.removeSubConnCh: + c := scToRemove[sc] + if c == 0 { + t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) + } + scToRemove[sc] = c - 1 + case <-removeTimeout: + t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") + } + } + + bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[4:6]) + + newSCTimeout := time.After(time.Millisecond * 500) + scToAdd := map[resolver.Address]int{ + testBackendAddrs[4]: 1, + testBackendAddrs[5]: 1, + } + for i := 0; i < len(scToAdd); i++ { + select { + case addr := <-cc.newSubConnAddrsCh: + c := scToAdd[addr[0]] + if c == 0 { + t.Fatalf("Got newSubConn for %v when there's %d new expected", addr, c) + } + scToAdd[addr[0]] = c - 1 + sc := <-cc.newSubConnCh + addrToSC[addr[0]] = sc + bg.handleSubConnStateChange(sc, connectivity.Connecting) + bg.handleSubConnStateChange(sc, connectivity.Ready) + case <-newSCTimeout: + t.Fatalf("timeout waiting for subConns (from new sub-balancer) to be newed") + } + } + + // Test roundrobin on the new picker. + p3 := <-cc.newPickerCh + want := []balancer.SubConn{ + addrToSC[testBackendAddrs[0]], addrToSC[testBackendAddrs[0]], + addrToSC[testBackendAddrs[1]], addrToSC[testBackendAddrs[1]], + addrToSC[testBackendAddrs[4]], addrToSC[testBackendAddrs[5]], + } + if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { + t.Fatalf("want %v, got %v", want, err) + } +} diff --git a/xds/internal/balancer/edsbalancer/eds.go b/xds/internal/balancer/edsbalancer/eds.go index d1578c32771..02ac314cd71 100644 --- a/xds/internal/balancer/edsbalancer/eds.go +++ b/xds/internal/balancer/edsbalancer/eds.go @@ -31,7 +31,6 @@ import ( "google.golang.org/grpc/connectivity" "google.golang.org/grpc/internal/buffer" "google.golang.org/grpc/internal/grpclog" - "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" "google.golang.org/grpc/xds/internal/balancer/lrs" xdsclient "google.golang.org/grpc/xds/internal/client" @@ -104,8 +103,6 @@ type edsBalancerImplInterface interface { close() } -var _ balancer.V2Balancer = (*edsBalancer)(nil) // Assert that we implement V2Balancer - // edsBalancer manages xdsClient and the actual EDS balancer implementation that // does load balancing. // @@ -210,14 +207,6 @@ type subConnStateUpdate struct { state balancer.SubConnState } -func (x *edsBalancer) HandleSubConnStateChange(sc balancer.SubConn, state connectivity.State) { - x.logger.Errorf("UpdateSubConnState should be called instead of HandleSubConnStateChange") -} - -func (x *edsBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { - x.logger.Errorf("UpdateClientConnState should be called instead of HandleResolvedAddrs") -} - func (x *edsBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { update := &subConnStateUpdate{ sc: sc, diff --git a/xds/internal/balancer/edsbalancer/eds_impl.go b/xds/internal/balancer/edsbalancer/eds_impl.go index 7dfa6f83505..27ec8a4147e 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl.go +++ b/xds/internal/balancer/edsbalancer/eds_impl.go @@ -389,10 +389,6 @@ type edsBalancerWrapperCC struct { func (ebwcc *edsBalancerWrapperCC) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) { return ebwcc.parent.newSubConn(ebwcc.priority, addrs, opts) } - -func (ebwcc *edsBalancerWrapperCC) UpdateBalancerState(state connectivity.State, picker balancer.Picker) { -} - func (ebwcc *edsBalancerWrapperCC) UpdateState(state balancer.State) { ebwcc.parent.enqueueChildBalancerStateUpdate(ebwcc.priority, state) } @@ -419,11 +415,11 @@ func (edsImpl *edsBalancerImpl) close() { type dropPicker struct { drops []*dropper - p balancer.V2Picker + p balancer.Picker loadStore lrs.Store } -func newDropPicker(p balancer.V2Picker, drops []*dropper, loadStore lrs.Store) *dropPicker { +func newDropPicker(p balancer.Picker, drops []*dropper, loadStore lrs.Store) *dropPicker { return &dropPicker{ drops: drops, p: p, @@ -447,7 +443,7 @@ func (d *dropPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { if d.loadStore != nil { d.loadStore.CallDropped(category) } - return balancer.PickResult{}, status.Errorf(codes.Unavailable, "RPC is dropped") + return balancer.PickResult{}, balancer.DropRPCError(status.Errorf(codes.Unavailable, "RPC is dropped")) } // TODO: (eds) don't drop unless the inner picker is READY. Similar to // https://github.com/grpc/grpc-go/issues/2622. diff --git a/xds/internal/balancer/edsbalancer/eds_impl_priority.go b/xds/internal/balancer/edsbalancer/eds_impl_priority.go index a7d65a13854..a279ddc6437 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl_priority.go +++ b/xds/internal/balancer/edsbalancer/eds_impl_priority.go @@ -46,7 +46,7 @@ func (edsImpl *edsBalancerImpl) handlePriorityChange() { // Everything was removed by EDS. if !edsImpl.priorityLowest.isSet() { edsImpl.priorityInUse = newPriorityTypeUnset() - edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, Picker: base.NewErrPickerV2(balancer.ErrTransientFailure)}) + edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, Picker: base.NewErrPicker(balancer.ErrTransientFailure)}) return } @@ -73,7 +73,7 @@ func (edsImpl *edsBalancerImpl) handlePriorityChange() { // We don't have an old state to send to parent, but we also don't // want parent to keep using picker from old_priorityInUse. Send an // update to trigger block picks until a new picker is ready. - edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Connecting, Picker: base.NewErrPickerV2(balancer.ErrNoSubConnAvailable)}) + edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Connecting, Picker: base.NewErrPicker(balancer.ErrNoSubConnAvailable)}) } return } diff --git a/xds/internal/balancer/edsbalancer/eds_impl_test.go b/xds/internal/balancer/edsbalancer/eds_impl_test.go index 550807c3f24..6fbffd234de 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl_test.go +++ b/xds/internal/balancer/edsbalancer/eds_impl_test.go @@ -27,7 +27,6 @@ import ( "google.golang.org/grpc/balancer" "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/balancergroup" xdsclient "google.golang.org/grpc/xds/internal/client" @@ -384,7 +383,58 @@ func (s) TestClose(t *testing.T) { edsb := newEDSBalancerImpl(nil, nil, nil, nil) // This is what could happen when switching between fallback and eds. This // make sure it doesn't panic. - edsb.close() + edsb.Close() +} + +func init() { + balancer.Register(&testConstBalancerBuilder{}) +} + +var errTestConstPicker = fmt.Errorf("const picker error") + +type testConstBalancerBuilder struct{} + +func (*testConstBalancerBuilder) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { + return &testConstBalancer{cc: cc} +} + +func (*testConstBalancerBuilder) Name() string { + return "test-const-balancer" +} + +type testConstBalancer struct { + cc balancer.ClientConn +} + +func (tb *testConstBalancer) ResolverError(error) { + panic("not implemented") +} + +func (tb *testConstBalancer) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) { + tb.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Ready, Picker: &testConstPicker{err: errTestConstPicker}}) +} + +func (tb *testConstBalancer) UpdateClientConnState(s balancer.ClientConnState) error { + a := s.ResolverState.Addresses + if len(a) > 0 { + tb.cc.NewSubConn(a, balancer.NewSubConnOptions{}) + } + return nil +} + +func (*testConstBalancer) Close() { +} + +type testConstPicker struct { + err error + sc balancer.SubConn +} + +func (tcp *testConstPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { + if tcp.err != nil { + return balancer.PickResult{}, tcp.err + } + return balancer.PickResult{SubConn: tcp.sc}, nil } // Create XDS balancer, and update sub-balancer before handling eds responses. @@ -504,16 +554,21 @@ type testInlineUpdateBalancer struct { cc balancer.ClientConn } -func (tb *testInlineUpdateBalancer) HandleSubConnStateChange(sc balancer.SubConn, state connectivity.State) { +func (tb *testInlineUpdateBalancer) ResolverError(error) { + panic("not implemented") +} + +func (tb *testInlineUpdateBalancer) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) { } var errTestInlineStateUpdate = fmt.Errorf("don't like addresses, empty or not") -func (tb *testInlineUpdateBalancer) HandleResolvedAddrs(a []resolver.Address, err error) { +func (tb *testInlineUpdateBalancer) UpdateClientConnState(balancer.ClientConnState) error { tb.cc.UpdateState(balancer.State{ ConnectivityState: connectivity.Ready, Picker: &testutils.TestConstPicker{Err: errTestInlineStateUpdate}, }) + return nil } func (*testInlineUpdateBalancer) Close() { diff --git a/xds/internal/balancer/edsbalancer/eds_test.go b/xds/internal/balancer/edsbalancer/eds_test.go index a9e9e663bde..eab7d137d87 100644 --- a/xds/internal/balancer/edsbalancer/eds_test.go +++ b/xds/internal/balancer/edsbalancer/eds_test.go @@ -288,11 +288,15 @@ type fakeBalancer struct { cc balancer.ClientConn } -func (b *fakeBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) { +func (b *fakeBalancer) ResolverError(error) { panic("implement me") } -func (b *fakeBalancer) HandleSubConnStateChange(sc balancer.SubConn, state connectivity.State) { +func (b *fakeBalancer) UpdateClientConnState(balancer.ClientConnState) error { + panic("implement me") +} + +func (b *fakeBalancer) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) { panic("implement me") } diff --git a/xds/internal/balancer/edsbalancer/test_util_test.go b/xds/internal/balancer/edsbalancer/test_util_test.go new file mode 100644 index 00000000000..e2b3c56a381 --- /dev/null +++ b/xds/internal/balancer/edsbalancer/test_util_test.go @@ -0,0 +1,346 @@ +/* + * 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 edsbalancer + +import ( + "context" + "fmt" + "testing" + + "google.golang.org/grpc" + "google.golang.org/grpc/balancer" + "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/resolver" + "google.golang.org/grpc/xds/internal" + + corepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" +) + +const testSubConnsCount = 16 + +var testSubConns []*testSubConn + +func init() { + for i := 0; i < testSubConnsCount; i++ { + testSubConns = append(testSubConns, &testSubConn{ + id: fmt.Sprintf("sc%d", i), + }) + } +} + +type testSubConn struct { + id string +} + +func (tsc *testSubConn) UpdateAddresses([]resolver.Address) { + panic("not implemented") +} + +func (tsc *testSubConn) Connect() { +} + +// Implement stringer to get human friendly error message. +func (tsc *testSubConn) String() string { + return tsc.id +} + +type testClientConn struct { + t *testing.T // For logging only. + + newSubConnAddrsCh chan []resolver.Address // The last 10 []Address to create subconn. + newSubConnCh chan balancer.SubConn // The last 10 subconn created. + removeSubConnCh chan balancer.SubConn // The last 10 subconn removed. + + newPickerCh chan balancer.Picker // The last picker updated. + newStateCh chan connectivity.State // The last state. + + subConnIdx int +} + +func newTestClientConn(t *testing.T) *testClientConn { + return &testClientConn{ + t: t, + + newSubConnAddrsCh: make(chan []resolver.Address, 10), + newSubConnCh: make(chan balancer.SubConn, 10), + removeSubConnCh: make(chan balancer.SubConn, 10), + + newPickerCh: make(chan balancer.Picker, 1), + newStateCh: make(chan connectivity.State, 1), + } +} + +func (tcc *testClientConn) NewSubConn(a []resolver.Address, o balancer.NewSubConnOptions) (balancer.SubConn, error) { + sc := testSubConns[tcc.subConnIdx] + tcc.subConnIdx++ + + tcc.t.Logf("testClientConn: NewSubConn(%v, %+v) => %s", a, o, sc) + select { + case tcc.newSubConnAddrsCh <- a: + default: + } + + select { + case tcc.newSubConnCh <- sc: + default: + } + + return sc, nil +} + +func (tcc *testClientConn) RemoveSubConn(sc balancer.SubConn) { + tcc.t.Logf("testClientCOnn: RemoveSubConn(%p)", sc) + select { + case tcc.removeSubConnCh <- sc: + default: + } +} + +func (tcc *testClientConn) UpdateState(bs balancer.State) { + tcc.t.Logf("testClientConn: UpdateState(%v)", bs) + select { + case <-tcc.newStateCh: + default: + } + tcc.newStateCh <- bs.ConnectivityState + + select { + case <-tcc.newPickerCh: + default: + } + tcc.newPickerCh <- bs.Picker +} + +func (tcc *testClientConn) ResolveNow(resolver.ResolveNowOptions) { + panic("not implemented") +} + +func (tcc *testClientConn) Target() string { + panic("not implemented") +} + +type testServerLoad struct { + name string + d float64 +} + +type testLoadStore struct { + callsStarted []internal.Locality + callsEnded []internal.Locality + callsCost []testServerLoad +} + +func newTestLoadStore() *testLoadStore { + return &testLoadStore{} +} + +func (*testLoadStore) CallDropped(category string) { + panic("not implemented") +} + +func (tls *testLoadStore) CallStarted(l internal.Locality) { + tls.callsStarted = append(tls.callsStarted, l) +} + +func (tls *testLoadStore) CallFinished(l internal.Locality, err error) { + tls.callsEnded = append(tls.callsEnded, l) +} + +func (tls *testLoadStore) CallServerLoad(l internal.Locality, name string, d float64) { + tls.callsCost = append(tls.callsCost, testServerLoad{name: name, d: d}) +} + +func (*testLoadStore) ReportTo(ctx context.Context, cc *grpc.ClientConn, clusterName string, node *corepb.Node) { + panic("not implemented") +} + +// isRoundRobin checks whether f's return value is roundrobin of elements from +// want. But it doesn't check for the order. Note that want can contain +// duplicate items, which makes it weight-round-robin. +// +// Step 1. the return values of f should form a permutation of all elements in +// want, but not necessary in the same order. E.g. if want is {a,a,b}, the check +// fails if f returns: +// - {a,a,a}: third a is returned before b +// - {a,b,b}: second b is returned before the second a +// +// If error is found in this step, the returned error contains only the first +// iteration until where it goes wrong. +// +// Step 2. the return values of f should be repetitions of the same permutation. +// E.g. if want is {a,a,b}, the check failes if f returns: +// - {a,b,a,b,a,a}: though it satisfies step 1, the second iteration is not +// repeating the first iteration. +// +// If error is found in this step, the returned error contains the first +// iteration + the second iteration until where it goes wrong. +func isRoundRobin(want []balancer.SubConn, f func() balancer.SubConn) error { + wantSet := make(map[balancer.SubConn]int) // SubConn -> count, for weighted RR. + for _, sc := range want { + wantSet[sc]++ + } + + // The first iteration: makes sure f's return values form a permutation of + // elements in want. + // + // Also keep the returns values in a slice, so we can compare the order in + // the second iteration. + gotSliceFirstIteration := make([]balancer.SubConn, 0, len(want)) + for range want { + got := f() + gotSliceFirstIteration = append(gotSliceFirstIteration, got) + wantSet[got]-- + if wantSet[got] < 0 { + return fmt.Errorf("non-roundrobin want: %v, result: %v", want, gotSliceFirstIteration) + } + } + + // The second iteration should repeat the first iteration. + var gotSliceSecondIteration []balancer.SubConn + for i := 0; i < 2; i++ { + for _, w := range gotSliceFirstIteration { + g := f() + gotSliceSecondIteration = append(gotSliceSecondIteration, g) + if w != g { + return fmt.Errorf("non-roundrobin, first iter: %v, second iter: %v", gotSliceFirstIteration, gotSliceSecondIteration) + } + } + } + + return nil +} + +// testClosure is a test util for TestIsRoundRobin. +type testClosure struct { + r []balancer.SubConn + i int +} + +func (tc *testClosure) next() balancer.SubConn { + ret := tc.r[tc.i] + tc.i = (tc.i + 1) % len(tc.r) + return ret +} + +func (s) TestIsRoundRobin(t *testing.T) { + var ( + sc1 = testSubConns[0] + sc2 = testSubConns[1] + sc3 = testSubConns[2] + ) + + testCases := []struct { + desc string + want []balancer.SubConn + got []balancer.SubConn + pass bool + }{ + { + desc: "0 element", + want: []balancer.SubConn{}, + got: []balancer.SubConn{}, + pass: true, + }, + { + desc: "1 element RR", + want: []balancer.SubConn{sc1}, + got: []balancer.SubConn{sc1, sc1, sc1, sc1}, + pass: true, + }, + { + desc: "1 element not RR", + want: []balancer.SubConn{sc1}, + got: []balancer.SubConn{sc1, sc2, sc1}, + pass: false, + }, + { + desc: "2 elements RR", + want: []balancer.SubConn{sc1, sc2}, + got: []balancer.SubConn{sc1, sc2, sc1, sc2, sc1, sc2}, + pass: true, + }, + { + desc: "2 elements RR different order from want", + want: []balancer.SubConn{sc2, sc1}, + got: []balancer.SubConn{sc1, sc2, sc1, sc2, sc1, sc2}, + pass: true, + }, + { + desc: "2 elements RR not RR, mistake in first iter", + want: []balancer.SubConn{sc1, sc2}, + got: []balancer.SubConn{sc1, sc1, sc1, sc2, sc1, sc2}, + pass: false, + }, + { + desc: "2 elements RR not RR, mistake in second iter", + want: []balancer.SubConn{sc1, sc2}, + got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc1, sc2}, + pass: false, + }, + { + desc: "2 elements weighted RR", + want: []balancer.SubConn{sc1, sc1, sc2}, + got: []balancer.SubConn{sc1, sc1, sc2, sc1, sc1, sc2}, + pass: true, + }, + { + desc: "2 elements weighted RR different order", + want: []balancer.SubConn{sc1, sc1, sc2}, + got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc2, sc1}, + pass: true, + }, + + { + desc: "3 elements RR", + want: []balancer.SubConn{sc1, sc2, sc3}, + got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc3, sc1, sc2, sc3}, + pass: true, + }, + { + desc: "3 elements RR different order", + want: []balancer.SubConn{sc1, sc2, sc3}, + got: []balancer.SubConn{sc3, sc2, sc1, sc3, sc2, sc1}, + pass: true, + }, + { + desc: "3 elements weighted RR", + want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, + got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc1, sc1, sc2, sc3, sc1, sc2, sc1}, + pass: true, + }, + { + desc: "3 elements weighted RR not RR, mistake in first iter", + want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, + got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc2, sc1, sc1, sc2, sc3, sc1, sc2, sc1}, + pass: false, + }, + { + desc: "3 elements weighted RR not RR, mistake in second iter", + want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, + got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc1, sc1, sc1, sc3, sc1, sc2, sc1}, + pass: false, + }, + } + for _, tC := range testCases { + t.Run(tC.desc, func(t *testing.T) { + err := isRoundRobin(tC.want, (&testClosure{r: tC.got}).next) + if err == nil != tC.pass { + t.Errorf("want pass %v, want %v, got err %v", tC.pass, tC.want, err) + } + }) + } +} From 236d7a063c7a8c2ad2f87990ed5fdeed6c3b8dbf Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Thu, 23 Apr 2020 12:35:05 -0700 Subject: [PATCH 2/3] Updates after rebase and review comments --- balancer/balancer.go | 23 +- balancer/grpclb/grpclb_remote_balancer.go | 2 +- balancer/rls/internal/cache/cache.go | 4 +- balancer/rls/internal/picker.go | 5 +- clientconn.go | 21 +- dialoptions.go | 3 +- examples/go.sum | 2 + internal/status/status.go | 24 +- naming/dns_resolver.go | 293 ------- naming/dns_resolver_test.go | 341 -------- naming/naming.go | 68 -- picker_wrapper.go | 26 +- pickfirst.go | 2 +- service_config.go | 2 +- status/status_test.go | 4 +- test/channelz_test.go | 3 +- test/creds_test.go | 4 +- test/end2end_test.go | 205 +---- vet.sh | 9 +- .../balancer/balancergroup/balancergroup.go | 3 +- .../balancergroup/balancergroup_test.go | 2 +- .../edsbalancer/balancergroup_test.go | 796 ------------------ .../balancer/edsbalancer/eds_impl_test.go | 53 +- xds/internal/balancer/edsbalancer/eds_test.go | 2 +- .../balancer/edsbalancer/test_util_test.go | 346 -------- xds/internal/testutils/balancer.go | 19 +- 26 files changed, 79 insertions(+), 2183 deletions(-) delete mode 100644 naming/dns_resolver.go delete mode 100644 naming/dns_resolver_test.go delete mode 100644 naming/naming.go delete mode 100644 xds/internal/balancer/edsbalancer/balancergroup_test.go delete mode 100644 xds/internal/balancer/edsbalancer/test_util_test.go diff --git a/balancer/balancer.go b/balancer/balancer.go index 11a6bab2864..dc75a957183 100644 --- a/balancer/balancer.go +++ b/balancer/balancer.go @@ -27,9 +27,11 @@ import ( "net" "strings" + "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal" + istatus "google.golang.org/grpc/internal/status" "google.golang.org/grpc/metadata" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" @@ -149,7 +151,7 @@ type ClientConn interface { // changed. // // gRPC will update the connectivity state of the ClientConn, and will call - // pick on the new picker to pick new SubConns. + // Pick on the new Picker to pick new SubConns. UpdateState(State) // ResolveNow is called by balancer to notify gRPC to do a name resolving. @@ -250,20 +252,23 @@ type PickResult struct { } type dropRPCError struct { - error - status *status.Status + *istatus.ErrorT } -func (e *dropRPCError) DropRPC() bool { return true } - -func (e *dropRPCError) GRPCStatus() *status.Status { return e.status } +func (e dropRPCError) DropRPC() bool { return true } // DropRPCError wraps err in an error implementing DropRPC() bool, returning // true. If err is not a status error, it will be converted to one with code -// Unknown and the message containing the err.Error() result. +// Unknown and the message containing the err.Error() result. DropRPCError +// should not be called with a nil error. func DropRPCError(err error) error { - st := status.Convert(err) - return &dropRPCError{error: st.Err(), status: st} + if err == nil { + return dropRPCError{ErrorT: status.Error(codes.Unknown, "nil error passed to DropRPCError").(*istatus.ErrorT)} + } + if se, ok := err.(*istatus.ErrorT); ok { + return dropRPCError{ErrorT: se} + } + return dropRPCError{ErrorT: status.Convert(err).Err().(*istatus.ErrorT)} } // TransientFailureError returns e. It exists for backward compatibility and diff --git a/balancer/grpclb/grpclb_remote_balancer.go b/balancer/grpclb/grpclb_remote_balancer.go index c6d555e4d8c..302d71316d5 100644 --- a/balancer/grpclb/grpclb_remote_balancer.go +++ b/balancer/grpclb/grpclb_remote_balancer.go @@ -192,7 +192,7 @@ func (lb *lbBalancer) refreshSubConns(backendAddrs []resolver.Address, fallback lb.cc.RemoveSubConn(sc) delete(lb.subConns, a) // Keep the state of this sc in b.scStates until sc's state becomes Shutdown. - // The entry will be deleted in HandleSubConnStateChange. + // The entry will be deleted in UpdateSubConnState. } } diff --git a/balancer/rls/internal/cache/cache.go b/balancer/rls/internal/cache/cache.go index c945d272ab1..dd03695e0e9 100644 --- a/balancer/rls/internal/cache/cache.go +++ b/balancer/rls/internal/cache/cache.go @@ -85,10 +85,10 @@ type Entry struct { // X-Google-RLS-Data header for matching RPCs. HeaderData string // ChildPicker is a very thin wrapper around the child policy wrapper. - // The type is declared as a V2Picker interface since the users of + // The type is declared as a Picker interface since the users of // the cache only care about the picker provided by the child policy, and // this makes it easy for testing. - ChildPicker balancer.V2Picker + ChildPicker balancer.Picker // size stores the size of this cache entry. Uses only a subset of the // fields. See `entrySize` for this is computed. diff --git a/balancer/rls/internal/picker.go b/balancer/rls/internal/picker.go index e823cf581b4..698185b1595 100644 --- a/balancer/rls/internal/picker.go +++ b/balancer/rls/internal/picker.go @@ -31,10 +31,7 @@ import ( "google.golang.org/grpc/metadata" ) -var errRLSThrottled = balancer.TransientFailureError(errors.New("RLS call throttled at client side")) - -// Compile time assert to ensure we implement V2Picker. -var _ balancer.V2Picker = (*rlsPicker)(nil) +var errRLSThrottled = errors.New("RLS call throttled at client side") // RLS rlsPicker selects the subConn to be used for a particular RPC. It does // not manage subConns directly and usually deletegates to pickers provided by diff --git a/clientconn.go b/clientconn.go index 71cae0ffe6a..ef327e8af4f 100644 --- a/clientconn.go +++ b/clientconn.go @@ -316,7 +316,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * if s == connectivity.Ready { break } else if cc.dopts.copts.FailOnNonTempDialError && s == connectivity.TransientFailure { - if err = cc.blockingpicker.connectionError(); err != nil { + if err = cc.connectionError(); err != nil { terr, ok := err.(interface { Temporary() bool }) @@ -327,7 +327,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } if !cc.WaitForStateChange(ctx, s) { // ctx got timeout or canceled. - if err = cc.blockingpicker.connectionError(); err != nil && cc.dopts.returnLastError { + if err = cc.connectionError(); err != nil && cc.dopts.returnLastError { return nil, err } return nil, ctx.Err() @@ -498,6 +498,9 @@ type ClientConn struct { channelzID int64 // channelz unique identification number czData *channelzData + + lceMu sync.Mutex // protects lastConnectionError + lastConnectionError error } // WaitForStateChange waits until the connectivity.State of ClientConn changes from sourceState or @@ -1207,7 +1210,7 @@ func (ac *addrConn) tryAllAddrs(addrs []resolver.Address, connectDeadline time.T if firstConnErr == nil { firstConnErr = err } - ac.cc.blockingpicker.updateConnectionError(err) + ac.cc.updateConnectionError(err) } // Couldn't connect to any address. @@ -1533,3 +1536,15 @@ func (cc *ClientConn) getResolver(scheme string) resolver.Builder { } return resolver.Get(scheme) } + +func (cc *ClientConn) updateConnectionError(err error) { + cc.lceMu.Lock() + cc.lastConnectionError = err + cc.lceMu.Unlock() +} + +func (cc *ClientConn) connectionError() error { + cc.lceMu.Lock() + defer cc.lceMu.Unlock() + return cc.lastConnectionError +} diff --git a/dialoptions.go b/dialoptions.go index 5fa6dec41f3..b5c810927b1 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -57,8 +57,7 @@ type dialOptions struct { authority string copts transport.ConnectOptions callOptions []CallOption - // This is used by v1 balancer dial option WithBalancer to support v1 - // balancer, and also by WithBalancerName dial option. + // This is used by WithBalancerName dial option. balancerBuilder balancer.Builder channelzParentID int64 disableServiceConfig bool diff --git a/examples/go.sum b/examples/go.sum index 0d649b1f5ae..7aa49f55984 100644 --- a/examples/go.sum +++ b/examples/go.sum @@ -63,6 +63,8 @@ google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55/go.mod h1:DMBHOl98 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg= google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQciAY= +google.golang.org/grpc v1.30.0-dev.1 h1:UPWdABFs9zu2kdq7GrCUcfnVgCT65hSpvHmy0RiKn0M= +google.golang.org/grpc v1.30.0-dev.1/go.mod h1:N36X2cJ7JwdamYAgDz+s+rVMFjt3numwzf/HckM8pak= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM= diff --git a/internal/status/status.go b/internal/status/status.go index 681260692e3..e5f4af1b090 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -97,7 +97,7 @@ func (s *Status) Err() error { if s.Code() == codes.OK { return nil } - return (*Error)(s.Proto()) + return (*ErrorT)(s.Proto()) } // WithDetails returns a new status with the provided details messages appended to the status. @@ -136,26 +136,26 @@ func (s *Status) Details() []interface{} { return details } -// Error is an alias of a status proto. It implements error and Status, -// and a nil Error should never be returned by this package. -type Error spb.Status +// ErrorT is an alias of a status proto. It implements error and Status, +// and a nil *ErrorT should never be returned by this package. +type ErrorT spb.Status -func (se *Error) Error() string { - p := (*spb.Status)(se) +func (e *ErrorT) Error() string { + p := (*spb.Status)(e) return fmt.Sprintf("rpc error: code = %s desc = %s", codes.Code(p.GetCode()), p.GetMessage()) } // GRPCStatus returns the Status represented by se. -func (se *Error) GRPCStatus() *Status { - return FromProto((*spb.Status)(se)) +func (e *ErrorT) GRPCStatus() *Status { + return FromProto((*spb.Status)(e)) } // Is implements future error.Is functionality. -// A Error is equivalent if the code and message are identical. -func (se *Error) Is(target error) bool { - tse, ok := target.(*Error) +// A ErrorT is equivalent if the code and message are identical. +func (e *ErrorT) Is(target error) bool { + tse, ok := target.(*ErrorT) if !ok { return false } - return proto.Equal((*spb.Status)(se), (*spb.Status)(tse)) + return proto.Equal((*spb.Status)(e), (*spb.Status)(tse)) } diff --git a/naming/dns_resolver.go b/naming/dns_resolver.go deleted file mode 100644 index c9f79dc5336..00000000000 --- a/naming/dns_resolver.go +++ /dev/null @@ -1,293 +0,0 @@ -/* - * - * Copyright 2017 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 naming - -import ( - "context" - "errors" - "fmt" - "net" - "strconv" - "time" - - "google.golang.org/grpc/grpclog" -) - -const ( - defaultPort = "443" - defaultFreq = time.Minute * 30 -) - -var ( - errMissingAddr = errors.New("missing address") - errWatcherClose = errors.New("watcher has been closed") - - lookupHost = net.DefaultResolver.LookupHost - lookupSRV = net.DefaultResolver.LookupSRV -) - -// NewDNSResolverWithFreq creates a DNS Resolver that can resolve DNS names, and -// create watchers that poll the DNS server using the frequency set by freq. -func NewDNSResolverWithFreq(freq time.Duration) (Resolver, error) { - return &dnsResolver{freq: freq}, nil -} - -// NewDNSResolver creates a DNS Resolver that can resolve DNS names, and create -// watchers that poll the DNS server using the default frequency defined by defaultFreq. -func NewDNSResolver() (Resolver, error) { - return NewDNSResolverWithFreq(defaultFreq) -} - -// dnsResolver handles name resolution for names following the DNS scheme -type dnsResolver struct { - // frequency of polling the DNS server that the watchers created by this resolver will use. - freq time.Duration -} - -// formatIP returns ok = false if addr is not a valid textual representation of an IP address. -// If addr is an IPv4 address, return the addr and ok = true. -// If addr is an IPv6 address, return the addr enclosed in square brackets and ok = true. -func formatIP(addr string) (addrIP string, ok bool) { - ip := net.ParseIP(addr) - if ip == nil { - return "", false - } - if ip.To4() != nil { - return addr, true - } - return "[" + addr + "]", true -} - -// parseTarget takes the user input target string, returns formatted host and port info. -// If target doesn't specify a port, set the port to be the defaultPort. -// If target is in IPv6 format and host-name is enclosed in square brackets, brackets -// are stripped when setting the host. -// examples: -// target: "www.google.com" returns host: "www.google.com", port: "443" -// target: "ipv4-host:80" returns host: "ipv4-host", port: "80" -// target: "[ipv6-host]" returns host: "ipv6-host", port: "443" -// target: ":80" returns host: "localhost", port: "80" -// target: ":" returns host: "localhost", port: "443" -func parseTarget(target string) (host, port string, err error) { - if target == "" { - return "", "", errMissingAddr - } - - if ip := net.ParseIP(target); ip != nil { - // target is an IPv4 or IPv6(without brackets) address - return target, defaultPort, nil - } - if host, port, err := net.SplitHostPort(target); err == nil { - // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port - if host == "" { - // Keep consistent with net.Dial(): If the host is empty, as in ":80", the local system is assumed. - host = "localhost" - } - if port == "" { - // If the port field is empty(target ends with colon), e.g. "[::1]:", defaultPort is used. - port = defaultPort - } - return host, port, nil - } - if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { - // target doesn't have port - return host, port, nil - } - return "", "", fmt.Errorf("invalid target address %v", target) -} - -// Resolve creates a watcher that watches the name resolution of the target. -func (r *dnsResolver) Resolve(target string) (Watcher, error) { - host, port, err := parseTarget(target) - if err != nil { - return nil, err - } - - if net.ParseIP(host) != nil { - ipWatcher := &ipWatcher{ - updateChan: make(chan *Update, 1), - } - host, _ = formatIP(host) - ipWatcher.updateChan <- &Update{Op: Add, Addr: host + ":" + port} - return ipWatcher, nil - } - - ctx, cancel := context.WithCancel(context.Background()) - return &dnsWatcher{ - r: r, - host: host, - port: port, - ctx: ctx, - cancel: cancel, - t: time.NewTimer(0), - }, nil -} - -// dnsWatcher watches for the name resolution update for a specific target -type dnsWatcher struct { - r *dnsResolver - host string - port string - // The latest resolved address set - curAddrs map[string]*Update - ctx context.Context - cancel context.CancelFunc - t *time.Timer -} - -// ipWatcher watches for the name resolution update for an IP address. -type ipWatcher struct { - updateChan chan *Update -} - -// Next returns the address resolution Update for the target. For IP address, -// the resolution is itself, thus polling name server is unnecessary. Therefore, -// Next() will return an Update the first time it is called, and will be blocked -// for all following calls as no Update exists until watcher is closed. -func (i *ipWatcher) Next() ([]*Update, error) { - u, ok := <-i.updateChan - if !ok { - return nil, errWatcherClose - } - return []*Update{u}, nil -} - -// Close closes the ipWatcher. -func (i *ipWatcher) Close() { - close(i.updateChan) -} - -// AddressType indicates the address type returned by name resolution. -type AddressType uint8 - -const ( - // Backend indicates the server is a backend server. - Backend AddressType = iota - // GRPCLB indicates the server is a grpclb load balancer. - GRPCLB -) - -// AddrMetadataGRPCLB contains the information the name resolver for grpclb should provide. The -// name resolver used by the grpclb balancer is required to provide this type of metadata in -// its address updates. -type AddrMetadataGRPCLB struct { - // AddrType is the type of server (grpc load balancer or backend). - AddrType AddressType - // ServerName is the name of the grpc load balancer. Used for authentication. - ServerName string -} - -// compileUpdate compares the old resolved addresses and newly resolved addresses, -// and generates an update list -func (w *dnsWatcher) compileUpdate(newAddrs map[string]*Update) []*Update { - var res []*Update - for a, u := range w.curAddrs { - if _, ok := newAddrs[a]; !ok { - u.Op = Delete - res = append(res, u) - } - } - for a, u := range newAddrs { - if _, ok := w.curAddrs[a]; !ok { - res = append(res, u) - } - } - return res -} - -func (w *dnsWatcher) lookupSRV() map[string]*Update { - newAddrs := make(map[string]*Update) - _, srvs, err := lookupSRV(w.ctx, "grpclb", "tcp", w.host) - if err != nil { - grpclog.Infof("grpc: failed dns SRV record lookup due to %v.\n", err) - return nil - } - for _, s := range srvs { - lbAddrs, err := lookupHost(w.ctx, s.Target) - if err != nil { - grpclog.Warningf("grpc: failed load balancer address dns lookup due to %v.\n", err) - continue - } - for _, a := range lbAddrs { - a, ok := formatIP(a) - if !ok { - grpclog.Errorf("grpc: failed IP parsing due to %v.\n", err) - continue - } - addr := a + ":" + strconv.Itoa(int(s.Port)) - newAddrs[addr] = &Update{Addr: addr, - Metadata: AddrMetadataGRPCLB{AddrType: GRPCLB, ServerName: s.Target}} - } - } - return newAddrs -} - -func (w *dnsWatcher) lookupHost() map[string]*Update { - newAddrs := make(map[string]*Update) - addrs, err := lookupHost(w.ctx, w.host) - if err != nil { - grpclog.Warningf("grpc: failed dns A record lookup due to %v.\n", err) - return nil - } - for _, a := range addrs { - a, ok := formatIP(a) - if !ok { - grpclog.Errorf("grpc: failed IP parsing due to %v.\n", err) - continue - } - addr := a + ":" + w.port - newAddrs[addr] = &Update{Addr: addr} - } - return newAddrs -} - -func (w *dnsWatcher) lookup() []*Update { - newAddrs := w.lookupSRV() - if newAddrs == nil { - // If failed to get any balancer address (either no corresponding SRV for the - // target, or caused by failure during resolution/parsing of the balancer target), - // return any A record info available. - newAddrs = w.lookupHost() - } - result := w.compileUpdate(newAddrs) - w.curAddrs = newAddrs - return result -} - -// Next returns the resolved address update(delta) for the target. If there's no -// change, it will sleep for 30 mins and try to resolve again after that. -func (w *dnsWatcher) Next() ([]*Update, error) { - for { - select { - case <-w.ctx.Done(): - return nil, errWatcherClose - case <-w.t.C: - } - result := w.lookup() - // Next lookup should happen after an interval defined by w.r.freq. - w.t.Reset(w.r.freq) - if len(result) > 0 { - return result, nil - } - } -} - -func (w *dnsWatcher) Close() { - w.cancel() -} diff --git a/naming/dns_resolver_test.go b/naming/dns_resolver_test.go deleted file mode 100644 index a7eff2d4038..00000000000 --- a/naming/dns_resolver_test.go +++ /dev/null @@ -1,341 +0,0 @@ -/* - * - * Copyright 2017 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 naming - -import ( - "context" - "fmt" - "net" - "reflect" - "sync" - "testing" - "time" - - "google.golang.org/grpc/internal/grpctest" -) - -type s struct { - grpctest.Tester -} - -func Test(t *testing.T) { - grpctest.RunSubTests(t, s{}) -} - -func newUpdateWithMD(op Operation, addr, lb string) *Update { - return &Update{ - Op: op, - Addr: addr, - Metadata: AddrMetadataGRPCLB{AddrType: GRPCLB, ServerName: lb}, - } -} - -func toMap(u []*Update) map[string]*Update { - m := make(map[string]*Update) - for _, v := range u { - m[v.Addr] = v - } - return m -} - -func (s) TestCompileUpdate(t *testing.T) { - tests := []struct { - oldAddrs []string - newAddrs []string - want []*Update - }{ - { - []string{}, - []string{"1.0.0.1"}, - []*Update{{Op: Add, Addr: "1.0.0.1"}}, - }, - { - []string{"1.0.0.1"}, - []string{"1.0.0.1"}, - []*Update{}, - }, - { - []string{"1.0.0.0"}, - []string{"1.0.0.1"}, - []*Update{{Op: Delete, Addr: "1.0.0.0"}, {Op: Add, Addr: "1.0.0.1"}}, - }, - { - []string{"1.0.0.1"}, - []string{"1.0.0.0"}, - []*Update{{Op: Add, Addr: "1.0.0.0"}, {Op: Delete, Addr: "1.0.0.1"}}, - }, - { - []string{"1.0.0.1"}, - []string{"1.0.0.1", "1.0.0.2", "1.0.0.3"}, - []*Update{{Op: Add, Addr: "1.0.0.2"}, {Op: Add, Addr: "1.0.0.3"}}, - }, - { - []string{"1.0.0.1", "1.0.0.2", "1.0.0.3"}, - []string{"1.0.0.0"}, - []*Update{{Op: Add, Addr: "1.0.0.0"}, {Op: Delete, Addr: "1.0.0.1"}, {Op: Delete, Addr: "1.0.0.2"}, {Op: Delete, Addr: "1.0.0.3"}}, - }, - { - []string{"1.0.0.1", "1.0.0.3", "1.0.0.5"}, - []string{"1.0.0.2", "1.0.0.3", "1.0.0.6"}, - []*Update{{Op: Delete, Addr: "1.0.0.1"}, {Op: Add, Addr: "1.0.0.2"}, {Op: Delete, Addr: "1.0.0.5"}, {Op: Add, Addr: "1.0.0.6"}}, - }, - { - []string{"1.0.0.1", "1.0.0.1", "1.0.0.2"}, - []string{"1.0.0.1"}, - []*Update{{Op: Delete, Addr: "1.0.0.2"}}, - }, - } - - var w dnsWatcher - for _, c := range tests { - w.curAddrs = make(map[string]*Update) - newUpdates := make(map[string]*Update) - for _, a := range c.oldAddrs { - w.curAddrs[a] = &Update{Addr: a} - } - for _, a := range c.newAddrs { - newUpdates[a] = &Update{Addr: a} - } - r := w.compileUpdate(newUpdates) - if !reflect.DeepEqual(toMap(c.want), toMap(r)) { - t.Errorf("w(%+v).compileUpdate(%+v) = %+v, want %+v", c.oldAddrs, c.newAddrs, updatesToSlice(r), updatesToSlice(c.want)) - } - } -} - -func (s) TestResolveFunc(t *testing.T) { - tests := []struct { - addr string - want error - }{ - // TODO(yuxuanli): More false cases? - {"www.google.com", nil}, - {"foo.bar:12345", nil}, - {"127.0.0.1", nil}, - {"127.0.0.1:12345", nil}, - {"[::1]:80", nil}, - {"[2001:db8:a0b:12f0::1]:21", nil}, - {":80", nil}, - {"127.0.0...1:12345", nil}, - {"[fe80::1%lo0]:80", nil}, - {"golang.org:http", nil}, - {"[2001:db8::1]:http", nil}, - {":", nil}, - {"", errMissingAddr}, - {"[2001:db8:a0b:12f0::1", fmt.Errorf("invalid target address %v", "[2001:db8:a0b:12f0::1")}, - } - - r, err := NewDNSResolver() - if err != nil { - t.Errorf("%v", err) - } - for _, v := range tests { - _, err := r.Resolve(v.addr) - if !reflect.DeepEqual(err, v.want) { - t.Errorf("Resolve(%q) = %v, want %v", v.addr, err, v.want) - } - } -} - -var hostLookupTbl = map[string][]string{ - "foo.bar.com": {"1.2.3.4", "5.6.7.8"}, - "ipv4.single.fake": {"1.2.3.4"}, - "ipv4.multi.fake": {"1.2.3.4", "5.6.7.8", "9.10.11.12"}, - "ipv6.single.fake": {"2607:f8b0:400a:801::1001"}, - "ipv6.multi.fake": {"2607:f8b0:400a:801::1001", "2607:f8b0:400a:801::1002", "2607:f8b0:400a:801::1003"}, -} - -func hostLookup(host string) ([]string, error) { - if addrs, ok := hostLookupTbl[host]; ok { - return addrs, nil - } - return nil, fmt.Errorf("failed to lookup host:%s resolution in hostLookupTbl", host) -} - -var srvLookupTbl = map[string][]*net.SRV{ - "_grpclb._tcp.srv.ipv4.single.fake": {&net.SRV{Target: "ipv4.single.fake", Port: 1234}}, - "_grpclb._tcp.srv.ipv4.multi.fake": {&net.SRV{Target: "ipv4.multi.fake", Port: 1234}}, - "_grpclb._tcp.srv.ipv6.single.fake": {&net.SRV{Target: "ipv6.single.fake", Port: 1234}}, - "_grpclb._tcp.srv.ipv6.multi.fake": {&net.SRV{Target: "ipv6.multi.fake", Port: 1234}}, -} - -func srvLookup(service, proto, name string) (string, []*net.SRV, error) { - cname := "_" + service + "._" + proto + "." + name - if srvs, ok := srvLookupTbl[cname]; ok { - return cname, srvs, nil - } - return "", nil, fmt.Errorf("failed to lookup srv record for %s in srvLookupTbl", cname) -} - -func updatesToSlice(updates []*Update) []Update { - res := make([]Update, len(updates)) - for i, u := range updates { - res[i] = *u - } - return res -} - -func testResolver(t *testing.T, freq time.Duration, slp time.Duration) { - tests := []struct { - target string - want []*Update - }{ - { - "foo.bar.com", - []*Update{{Op: Add, Addr: "1.2.3.4" + colonDefaultPort}, {Op: Add, Addr: "5.6.7.8" + colonDefaultPort}}, - }, - { - "foo.bar.com:1234", - []*Update{{Op: Add, Addr: "1.2.3.4:1234"}, {Op: Add, Addr: "5.6.7.8:1234"}}, - }, - { - "srv.ipv4.single.fake", - []*Update{newUpdateWithMD(Add, "1.2.3.4:1234", "ipv4.single.fake")}, - }, - { - "srv.ipv4.multi.fake", - []*Update{ - newUpdateWithMD(Add, "1.2.3.4:1234", "ipv4.multi.fake"), - newUpdateWithMD(Add, "5.6.7.8:1234", "ipv4.multi.fake"), - newUpdateWithMD(Add, "9.10.11.12:1234", "ipv4.multi.fake")}, - }, - { - "srv.ipv6.single.fake", - []*Update{newUpdateWithMD(Add, "[2607:f8b0:400a:801::1001]:1234", "ipv6.single.fake")}, - }, - { - "srv.ipv6.multi.fake", - []*Update{ - newUpdateWithMD(Add, "[2607:f8b0:400a:801::1001]:1234", "ipv6.multi.fake"), - newUpdateWithMD(Add, "[2607:f8b0:400a:801::1002]:1234", "ipv6.multi.fake"), - newUpdateWithMD(Add, "[2607:f8b0:400a:801::1003]:1234", "ipv6.multi.fake"), - }, - }, - } - - for _, a := range tests { - r, err := NewDNSResolverWithFreq(freq) - if err != nil { - t.Fatalf("%v\n", err) - } - w, err := r.Resolve(a.target) - if err != nil { - t.Fatalf("%v\n", err) - } - updates, err := w.Next() - if err != nil { - t.Fatalf("%v\n", err) - } - if !reflect.DeepEqual(toMap(a.want), toMap(updates)) { - t.Errorf("Resolve(%q) = %+v, want %+v\n", a.target, updatesToSlice(updates), updatesToSlice(a.want)) - } - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - for { - _, err := w.Next() - if err != nil { - return - } - t.Error("Execution shouldn't reach here, since w.Next() should be blocked until close happen.") - } - }() - // Sleep for sometime to let watcher do more than one lookup - time.Sleep(slp) - w.Close() - wg.Wait() - } -} - -func replaceNetFunc() func() { - oldLookupHost := lookupHost - oldLookupSRV := lookupSRV - lookupHost = func(ctx context.Context, host string) ([]string, error) { - return hostLookup(host) - } - lookupSRV = func(ctx context.Context, service, proto, name string) (string, []*net.SRV, error) { - return srvLookup(service, proto, name) - } - return func() { - lookupHost = oldLookupHost - lookupSRV = oldLookupSRV - } -} - -func (s) TestResolve(t *testing.T) { - defer replaceNetFunc()() - testResolver(t, time.Millisecond*5, time.Millisecond*10) -} - -const colonDefaultPort = ":" + defaultPort - -func (s) TestIPWatcher(t *testing.T) { - tests := []struct { - target string - want []*Update - }{ - {"127.0.0.1", []*Update{{Op: Add, Addr: "127.0.0.1" + colonDefaultPort}}}, - {"127.0.0.1:12345", []*Update{{Op: Add, Addr: "127.0.0.1:12345"}}}, - {"::1", []*Update{{Op: Add, Addr: "[::1]" + colonDefaultPort}}}, - {"[::1]:12345", []*Update{{Op: Add, Addr: "[::1]:12345"}}}, - {"[::1]:", []*Update{{Op: Add, Addr: "[::1]:443"}}}, - {"2001:db8:85a3::8a2e:370:7334", []*Update{{Op: Add, Addr: "[2001:db8:85a3::8a2e:370:7334]" + colonDefaultPort}}}, - {"[2001:db8:85a3::8a2e:370:7334]", []*Update{{Op: Add, Addr: "[2001:db8:85a3::8a2e:370:7334]" + colonDefaultPort}}}, - {"[2001:db8:85a3::8a2e:370:7334]:12345", []*Update{{Op: Add, Addr: "[2001:db8:85a3::8a2e:370:7334]:12345"}}}, - {"[2001:db8::1]:http", []*Update{{Op: Add, Addr: "[2001:db8::1]:http"}}}, - // TODO(yuxuanli): zone support? - } - - for _, v := range tests { - r, err := NewDNSResolverWithFreq(time.Millisecond * 5) - if err != nil { - t.Fatalf("%v\n", err) - } - w, err := r.Resolve(v.target) - if err != nil { - t.Fatalf("%v\n", err) - } - var updates []*Update - var wg sync.WaitGroup - wg.Add(1) - count := 0 - go func() { - defer wg.Done() - for { - u, err := w.Next() - if err != nil { - return - } - updates = u - count++ - } - }() - // Sleep for sometime to let watcher do more than one lookup - time.Sleep(time.Millisecond * 10) - w.Close() - wg.Wait() - if !reflect.DeepEqual(v.want, updates) { - t.Errorf("Resolve(%q) = %v, want %+v\n", v.target, updatesToSlice(updates), updatesToSlice(v.want)) - } - if count != 1 { - t.Errorf("IPWatcher Next() should return only once, not %d times\n", count) - } - } -} diff --git a/naming/naming.go b/naming/naming.go deleted file mode 100644 index f4c1c8b6894..00000000000 --- a/naming/naming.go +++ /dev/null @@ -1,68 +0,0 @@ -/* - * - * Copyright 2014 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 naming defines the naming API and related data structures for gRPC. -// -// This package is deprecated: please use package resolver instead. -package naming - -// Operation defines the corresponding operations for a name resolution change. -// -// Deprecated: please use package resolver. -type Operation uint8 - -const ( - // Add indicates a new address is added. - Add Operation = iota - // Delete indicates an existing address is deleted. - Delete -) - -// Update defines a name resolution update. Notice that it is not valid having both -// empty string Addr and nil Metadata in an Update. -// -// Deprecated: please use package resolver. -type Update struct { - // Op indicates the operation of the update. - Op Operation - // Addr is the updated address. It is empty string if there is no address update. - Addr string - // Metadata is the updated metadata. It is nil if there is no metadata update. - // Metadata is not required for a custom naming implementation. - Metadata interface{} -} - -// Resolver creates a Watcher for a target to track its resolution changes. -// -// Deprecated: please use package resolver. -type Resolver interface { - // Resolve creates a Watcher for target. - Resolve(target string) (Watcher, error) -} - -// Watcher watches for the updates on the specified target. -// -// Deprecated: please use package resolver. -type Watcher interface { - // Next blocks until an update or error happens. It may return one or more - // updates. The first call should get the full set of the results. It should - // return an error if and only if Watcher cannot recover. - Next() ([]*Update, error) - // Close closes the Watcher. - Close() -} diff --git a/picker_wrapper.go b/picker_wrapper.go index a2e996d7295..364fe96caac 100644 --- a/picker_wrapper.go +++ b/picker_wrapper.go @@ -38,32 +38,10 @@ type pickerWrapper struct { done bool blockingCh chan struct{} picker balancer.Picker - - // The latest connection error. TODO: remove when V1 picker is deprecated; - // balancer should be responsible for providing the error. - *connErr -} - -type connErr struct { - mu sync.Mutex - err error -} - -func (c *connErr) updateConnectionError(err error) { - c.mu.Lock() - c.err = err - c.mu.Unlock() -} - -func (c *connErr) connectionError() error { - c.mu.Lock() - err := c.err - c.mu.Unlock() - return err } func newPickerWrapper() *pickerWrapper { - return &pickerWrapper{blockingCh: make(chan struct{}), connErr: &connErr{}} + return &pickerWrapper{blockingCh: make(chan struct{})} } // updatePicker is called by UpdateBalancerState. It unblocks all blocked pick. @@ -128,8 +106,6 @@ func (pw *pickerWrapper) pick(ctx context.Context, failfast bool, info balancer. var errStr string if lastPickErr != nil { errStr = "latest balancer error: " + lastPickErr.Error() - } else if connectionErr := pw.connectionError(); connectionErr != nil { - errStr = "latest connection error: " + connectionErr.Error() } else { errStr = ctx.Err().Error() } diff --git a/pickfirst.go b/pickfirst.go index 7ba90ae5ab1..95791a56554 100644 --- a/pickfirst.go +++ b/pickfirst.go @@ -94,7 +94,7 @@ func (b *pickfirstBalancer) UpdateClientConnState(cs balancer.ClientConnState) e func (b *pickfirstBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) { if grpclog.V(2) { - grpclog.Infof("pickfirstBalancer: HandleSubConnStateChange: %p, %v", sc, s) + grpclog.Infof("pickfirstBalancer: UpdateSubConnState: %p, %v", sc, s) } if b.sc != sc { if grpclog.V(2) { diff --git a/service_config.go b/service_config.go index c8267fc8eb3..37d4a58f122 100644 --- a/service_config.go +++ b/service_config.go @@ -79,7 +79,7 @@ type ServiceConfig struct { serviceconfig.Config // LB is the load balancer the service providers recommends. The balancer - // specified via grpc.WithBalancer will override this. This is deprecated; + // specified via grpc.WithBalancerName will override this. This is deprecated; // lbConfigs is preferred. If lbConfig and LB are both present, lbConfig // will be used. LB *string diff --git a/status/status_test.go b/status/status_test.go index 839a3c390ed..1f8578e27a6 100644 --- a/status/status_test.go +++ b/status/status_test.go @@ -64,7 +64,7 @@ func (s) TestErrorsWithSameParameters(t *testing.T) { e1 := Errorf(codes.AlreadyExists, description) e2 := Errorf(codes.AlreadyExists, description) if e1 == e2 || !errEqual(e1, e2) { - t.Fatalf("Errors should be equivalent but unique - e1: %v, %v e2: %p, %v", e1.(*status.Error), e1, e2.(*status.Error), e2) + t.Fatalf("Errors should be equivalent but unique - e1: %v, %v e2: %p, %v", e1.(*status.ErrorT), e1, e2.(*status.ErrorT), e2) } } @@ -116,7 +116,7 @@ func (s) TestError(t *testing.T) { func (s) TestErrorOK(t *testing.T) { err := Error(codes.OK, "foo") if err != nil { - t.Fatalf("Error(codes.OK, _) = %p; want nil", err.(*status.Error)) + t.Fatalf("Error(codes.OK, _) = %p; want nil", err.(*status.ErrorT)) } } diff --git a/test/channelz_test.go b/test/channelz_test.go index 75846640e22..c69e0cec2e6 100644 --- a/test/channelz_test.go +++ b/test/channelz_test.go @@ -1880,7 +1880,8 @@ func (s) TestCZTraceOverwriteChannelDeletion(t *testing.T) { czCleanup := channelz.NewChannelzStorage() defer czCleanupWrapper(czCleanup, t) e := tcpClearRREnv - // avoid newTest using WithBalancer, which would override service config's change of balancer below. + // avoid newTest using WithBalancerName, which would override service + // config's change of balancer below. e.balancer = "" te := newTest(t, e) channelz.SetMaxTraceEntry(1) diff --git a/test/creds_test.go b/test/creds_test.go index bc4dca17a78..7b607ef0371 100644 --- a/test/creds_test.go +++ b/test/creds_test.go @@ -157,7 +157,7 @@ func (c *clientTimeoutCreds) Clone() credentials.TransportCredentials { } func (s) TestNonFailFastRPCSucceedOnTimeoutCreds(t *testing.T) { - te := newTest(t, env{name: "timeout-cred", network: "tcp", security: "empty", balancer: "v1"}) + te := newTest(t, env{name: "timeout-cred", network: "tcp", security: "empty"}) te.userAgent = testAppUA te.startServer(&testServer{security: te.e.security}) defer te.tearDown() @@ -181,7 +181,7 @@ func (m *methodTestCreds) RequireTransportSecurity() bool { return false } func (s) TestGRPCMethodAccessibleToCredsViaContextRequestInfo(t *testing.T) { const wantMethod = "/grpc.testing.TestService/EmptyCall" - te := newTest(t, env{name: "context-request-info", network: "tcp", balancer: "v1"}) + te := newTest(t, env{name: "context-request-info", network: "tcp"}) te.userAgent = testAppUA te.startServer(&testServer{security: te.e.security}) defer te.tearDown() diff --git a/test/end2end_test.go b/test/end2end_test.go index 7f628f9d914..f3a60de5a96 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -567,6 +567,7 @@ func newTest(t *testing.T, e env) *test { } func (te *test) listenAndServe(ts testpb.TestServiceServer, listen func(network, address string) (net.Listener, error)) net.Listener { + te.t.Helper() te.t.Logf("Running test in %s environment...", te.e.name) sopts := []grpc.ServerOption{grpc.MaxConcurrentStreams(te.maxStream)} if te.maxServerMsgSize != nil { @@ -698,6 +699,7 @@ func (te *test) startServerWithConnControl(ts testpb.TestServiceServer) *listene // startServer starts a gRPC server exposing the provided TestService // implementation. Callers should defer a call to te.tearDown to clean up func (te *test) startServer(ts testpb.TestServiceServer) { + te.t.Helper() te.listenAndServe(ts, net.Listen) } @@ -4708,208 +4710,6 @@ func testClientResourceExhaustedCancelFullDuplex(t *testing.T, e env) { } } -type clientTimeoutCreds struct { - timeoutReturned bool -} - -func (c *clientTimeoutCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - if !c.timeoutReturned { - c.timeoutReturned = true - return nil, nil, context.DeadlineExceeded - } - return rawConn, nil, nil -} -func (c *clientTimeoutCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - return rawConn, nil, nil -} -func (c *clientTimeoutCreds) Info() credentials.ProtocolInfo { - return credentials.ProtocolInfo{} -} -func (c *clientTimeoutCreds) Clone() credentials.TransportCredentials { - return nil -} -func (c *clientTimeoutCreds) OverrideServerName(s string) error { - return nil -} - -func (s) TestNonFailFastRPCSucceedOnTimeoutCreds(t *testing.T) { - te := newTest(t, env{name: "timeout-cred", network: "tcp", security: "clientTimeoutCreds"}) - te.userAgent = testAppUA - te.startServer(&testServer{security: te.e.security}) - defer te.tearDown() - - cc := te.clientConn() - tc := testpb.NewTestServiceClient(cc) - // This unary call should succeed, because ClientHandshake will succeed for the second time. - if _, err := tc.EmptyCall(context.Background(), &testpb.Empty{}, grpc.WaitForReady(true)); err != nil { - te.t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want ", err) - } -} - -type serverDispatchCred struct { - rawConnCh chan net.Conn -} - -func newServerDispatchCred() *serverDispatchCred { - return &serverDispatchCred{ - rawConnCh: make(chan net.Conn, 1), - } -} -func (c *serverDispatchCred) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - return rawConn, nil, nil -} -func (c *serverDispatchCred) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - select { - case c.rawConnCh <- rawConn: - default: - } - return nil, nil, credentials.ErrConnDispatched -} -func (c *serverDispatchCred) Info() credentials.ProtocolInfo { - return credentials.ProtocolInfo{} -} -func (c *serverDispatchCred) Clone() credentials.TransportCredentials { - return nil -} -func (c *serverDispatchCred) OverrideServerName(s string) error { - return nil -} -func (c *serverDispatchCred) getRawConn() net.Conn { - return <-c.rawConnCh -} - -func (s) TestServerCredsDispatch(t *testing.T) { - lis, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatalf("Failed to listen: %v", err) - } - cred := newServerDispatchCred() - s := grpc.NewServer(grpc.Creds(cred)) - go s.Serve(lis) - defer s.Stop() - - cc, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(cred)) - if err != nil { - t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) - } - defer cc.Close() - - rawConn := cred.getRawConn() - // Give grpc a chance to see the error and potentially close the connection. - // And check that connection is not closed after that. - time.Sleep(100 * time.Millisecond) - // Check rawConn is not closed. - if n, err := rawConn.Write([]byte{0}); n <= 0 || err != nil { - t.Errorf("Read() = %v, %v; want n>0, ", n, err) - } -} - -type authorityCheckCreds struct { - got string -} - -func (c *authorityCheckCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - return rawConn, nil, nil -} -func (c *authorityCheckCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { - c.got = authority - return rawConn, nil, nil -} -func (c *authorityCheckCreds) Info() credentials.ProtocolInfo { - return credentials.ProtocolInfo{} -} -func (c *authorityCheckCreds) Clone() credentials.TransportCredentials { - return c -} -func (c *authorityCheckCreds) OverrideServerName(s string) error { - return nil -} - -// This test makes sure that the authority client handshake gets is the endpoint -// in dial target, not the resolved ip address. -func (s) TestCredsHandshakeAuthority(t *testing.T) { - const testAuthority = "test.auth.ori.ty" - - lis, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatalf("Failed to listen: %v", err) - } - cred := &authorityCheckCreds{} - s := grpc.NewServer() - go s.Serve(lis) - defer s.Stop() - - r, rcleanup := manual.GenerateAndRegisterManualResolver() - defer rcleanup() - - cc, err := grpc.Dial(r.Scheme()+":///"+testAuthority, grpc.WithTransportCredentials(cred)) - if err != nil { - t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) - } - defer cc.Close() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String()}}}) - - ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) - defer cancel() - for { - s := cc.GetState() - if s == connectivity.Ready { - break - } - if !cc.WaitForStateChange(ctx, s) { - // ctx got timeout or canceled. - t.Fatalf("ClientConn is not ready after 100 ms") - } - } - - if cred.got != testAuthority { - t.Fatalf("client creds got authority: %q, want: %q", cred.got, testAuthority) - } -} - -// This test makes sure that the authority client handshake gets is the endpoint -// of the ServerName of the address when it is set. -func (s) TestCredsHandshakeServerNameAuthority(t *testing.T) { - const testAuthority = "test.auth.ori.ty" - const testServerName = "test.server.name" - - lis, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatalf("Failed to listen: %v", err) - } - cred := &authorityCheckCreds{} - s := grpc.NewServer() - go s.Serve(lis) - defer s.Stop() - - r, rcleanup := manual.GenerateAndRegisterManualResolver() - defer rcleanup() - - cc, err := grpc.Dial(r.Scheme()+":///"+testAuthority, grpc.WithTransportCredentials(cred)) - if err != nil { - t.Fatalf("grpc.Dial(%q) = %v", lis.Addr().String(), err) - } - defer cc.Close() - r.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: lis.Addr().String(), ServerName: testServerName}}}) - - ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) - defer cancel() - for { - s := cc.GetState() - if s == connectivity.Ready { - break - } - if !cc.WaitForStateChange(ctx, s) { - // ctx got timeout or canceled. - t.Fatalf("ClientConn is not ready after 100 ms") - } - } - - if cred.got != testServerName { - t.Fatalf("client creds got authority: %q, want: %q", cred.got, testAuthority) - } -} - type clientFailCreds struct{} func (c *clientFailCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { @@ -4952,7 +4752,6 @@ func (s) TestFailfastRPCFailOnFatalHandshakeError(t *testing.T) { } } ->>>>>>> c9ad8dff... balancer: move Balancer and Picker to V2; delete legacy API func (s) TestFlowControlLogicalRace(t *testing.T) { // Test for a regression of https://github.com/grpc/grpc-go/issues/632, // and other flow control bugs. diff --git a/vet.sh b/vet.sh index 493e67b511e..2fa14169b37 100755 --- a/vet.sh +++ b/vet.sh @@ -125,12 +125,10 @@ staticcheck -go 1.9 -checks 'inherit,-ST1015' ./... > "${SC_OUT}" || true not grep -v "is deprecated:.*SA1019" "${SC_OUT}" # Only ignore the following deprecated types/fields/functions. not grep -Fv '.CredsBundle -.HandleResolvedAddrs -.HandleSubConnStateChange .HeaderMap +.Metadata is deprecated: use Attributes .NewAddress .NewServiceConfig -.Metadata is deprecated: use Attributes .Type is deprecated: use Attributes balancer.Picker grpc.CallCustomCodec @@ -143,9 +141,7 @@ grpc.NewGZIPCompressor grpc.NewGZIPDecompressor grpc.RPCCompressor grpc.RPCDecompressor -grpc.RoundRobin grpc.ServiceConfig -grpc.WithBalancer grpc.WithBalancerName grpc.WithCompressor grpc.WithDecompressor @@ -155,9 +151,6 @@ grpc.WithServiceConfig grpc.WithTimeout http.CloseNotifier info.SecurityVersion -naming.Resolver -naming.Update -naming.Watcher resolver.Backend resolver.GRPCLB' "${SC_OUT}" diff --git a/xds/internal/balancer/balancergroup/balancergroup.go b/xds/internal/balancer/balancergroup/balancergroup.go index fdc3f42ec5b..d6428afb96a 100644 --- a/xds/internal/balancer/balancergroup/balancergroup.go +++ b/xds/internal/balancer/balancergroup/balancergroup.go @@ -123,8 +123,7 @@ func (sbc *subBalancerWithConfig) updateClientConnState(s balancer.ClientConnSta // it's the lower priority, but it can still get address updates. return nil } - return ub.UpdateClientConnState(s) - return nil + return b.UpdateClientConnState(s) } func (sbc *subBalancerWithConfig) stopBalancer() { diff --git a/xds/internal/balancer/balancergroup/balancergroup_test.go b/xds/internal/balancer/balancergroup/balancergroup_test.go index f2804abf50e..8c211c04161 100644 --- a/xds/internal/balancer/balancergroup/balancergroup_test.go +++ b/xds/internal/balancer/balancergroup/balancergroup_test.go @@ -49,7 +49,7 @@ func init() { DefaultSubBalancerCloseTimeout = time.Millisecond } -func subConnFromPicker(p balancer.V2Picker) func() balancer.SubConn { +func subConnFromPicker(p balancer.Picker) func() balancer.SubConn { return func() balancer.SubConn { scst, _ := p.Pick(balancer.PickInfo{}) return scst.SubConn diff --git a/xds/internal/balancer/edsbalancer/balancergroup_test.go b/xds/internal/balancer/edsbalancer/balancergroup_test.go deleted file mode 100644 index e680455be6d..00000000000 --- a/xds/internal/balancer/edsbalancer/balancergroup_test.go +++ /dev/null @@ -1,796 +0,0 @@ -/* - * 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 edsbalancer - -import ( - "fmt" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/balancer/roundrobin" - "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/resolver" - "google.golang.org/grpc/xds/internal" - orcapb "google.golang.org/grpc/xds/internal/proto/udpa/data/orca/v1" -) - -var ( - rrBuilder = balancer.Get(roundrobin.Name) - testBalancerIDs = []internal.Locality{{Region: "b1"}, {Region: "b2"}, {Region: "b3"}} - testBackendAddrs []resolver.Address -) - -const testBackendAddrsCount = 12 - -func init() { - for i := 0; i < testBackendAddrsCount; i++ { - testBackendAddrs = append(testBackendAddrs, resolver.Address{Addr: fmt.Sprintf("%d.%d.%d.%d:%d", i, i, i, i, i)}) - } - - // Disable caching for all tests. It will be re-enabled in caching specific - // tests. - defaultSubBalancerCloseTimeout = time.Millisecond -} - -func subConnFromPicker(p balancer.Picker) func() balancer.SubConn { - return func() balancer.SubConn { - scst, _ := p.Pick(balancer.PickInfo{}) - return scst.SubConn - } -} - -// 1 balancer, 1 backend -> 2 backends -> 1 backend. -func (s) TestBalancerGroup_OneRR_AddRemoveBackend(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add one balancer to group. - bg.add(testBalancerIDs[0], 1, rrBuilder) - // Send one resolved address. - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) - - // Send subconn state change. - sc1 := <-cc.newSubConnCh - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - - // Test pick with one backend. - p1 := <-cc.newPickerCh - for i := 0; i < 5; i++ { - gotSCSt, _ := p1.Pick(balancer.PickInfo{}) - if !cmp.Equal(gotSCSt.SubConn, sc1, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("picker.Pick, got %v, want SubConn=%v", gotSCSt, sc1) - } - } - - // Send two addresses. - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - // Expect one new subconn, send state update. - sc2 := <-cc.newSubConnCh - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - - // Test roundrobin pick. - p2 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc2} - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Remove the first address. - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[1:2]) - scToRemove := <-cc.removeSubConnCh - if !cmp.Equal(scToRemove, sc1, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("RemoveSubConn, want %v, got %v", sc1, scToRemove) - } - bg.handleSubConnStateChange(scToRemove, connectivity.Shutdown) - - // Test pick with only the second subconn. - p3 := <-cc.newPickerCh - for i := 0; i < 5; i++ { - gotSC, _ := p3.Pick(balancer.PickInfo{}) - if !cmp.Equal(gotSC.SubConn, sc2, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("picker.Pick, got %v, want SubConn=%v", gotSC, sc2) - } - } -} - -// 2 balancers, each with 1 backend. -func (s) TestBalancerGroup_TwoRR_OneBackend(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add two balancers to group and send one resolved address to both - // balancers. - bg.add(testBalancerIDs[0], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) - sc1 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[0:1]) - sc2 := <-cc.newSubConnCh - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc2} - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } -} - -// 2 balancers, each with more than 1 backends. -func (s) TestBalancerGroup_TwoRR_MoreBackends(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add two balancers to group and send one resolved address to both - // balancers. - bg.add(testBalancerIDs[0], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - sc1 := <-cc.newSubConnCh - sc2 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - sc3 := <-cc.newSubConnCh - sc4 := <-cc.newSubConnCh - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - bg.handleSubConnStateChange(sc3, connectivity.Connecting) - bg.handleSubConnStateChange(sc3, connectivity.Ready) - bg.handleSubConnStateChange(sc4, connectivity.Connecting) - bg.handleSubConnStateChange(sc4, connectivity.Ready) - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc2, sc3, sc4} - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Turn sc2's connection down, should be RR between balancers. - bg.handleSubConnStateChange(sc2, connectivity.TransientFailure) - p2 := <-cc.newPickerCh - // Expect two sc1's in the result, because balancer1 will be picked twice, - // but there's only one sc in it. - want = []balancer.SubConn{sc1, sc1, sc3, sc4} - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Remove sc3's addresses. - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[3:4]) - scToRemove := <-cc.removeSubConnCh - if !cmp.Equal(scToRemove, sc3, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("RemoveSubConn, want %v, got %v", sc3, scToRemove) - } - bg.handleSubConnStateChange(scToRemove, connectivity.Shutdown) - p3 := <-cc.newPickerCh - want = []balancer.SubConn{sc1, sc4} - if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Turn sc1's connection down. - bg.handleSubConnStateChange(sc1, connectivity.TransientFailure) - p4 := <-cc.newPickerCh - want = []balancer.SubConn{sc4} - if err := isRoundRobin(want, subConnFromPicker(p4)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Turn last connection to connecting. - bg.handleSubConnStateChange(sc4, connectivity.Connecting) - p5 := <-cc.newPickerCh - for i := 0; i < 5; i++ { - if _, err := p5.Pick(balancer.PickInfo{}); err != balancer.ErrNoSubConnAvailable { - t.Fatalf("want pick error %v, got %v", balancer.ErrNoSubConnAvailable, err) - } - } - - // Turn all connections down. - bg.handleSubConnStateChange(sc4, connectivity.TransientFailure) - p6 := <-cc.newPickerCh - for i := 0; i < 5; i++ { - if _, err := p6.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure { - t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err) - } - } -} - -// 2 balancers with different weights. -func (s) TestBalancerGroup_TwoRR_DifferentWeight_MoreBackends(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add two balancers to group and send two resolved addresses to both - // balancers. - bg.add(testBalancerIDs[0], 2, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - sc1 := <-cc.newSubConnCh - sc2 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - sc3 := <-cc.newSubConnCh - sc4 := <-cc.newSubConnCh - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - bg.handleSubConnStateChange(sc3, connectivity.Connecting) - bg.handleSubConnStateChange(sc3, connectivity.Ready) - bg.handleSubConnStateChange(sc4, connectivity.Connecting) - bg.handleSubConnStateChange(sc4, connectivity.Ready) - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc1, sc2, sc2, sc3, sc4} - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } -} - -// totally 3 balancers, add/remove balancer. -func (s) TestBalancerGroup_ThreeRR_RemoveBalancer(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add three balancers to group and send one resolved address to both - // balancers. - bg.add(testBalancerIDs[0], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:1]) - sc1 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[1:2]) - sc2 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[2], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[2], testBackendAddrs[1:2]) - sc3 := <-cc.newSubConnCh - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - bg.handleSubConnStateChange(sc3, connectivity.Connecting) - bg.handleSubConnStateChange(sc3, connectivity.Ready) - - p1 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc2, sc3} - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // Remove the second balancer, while the others two are ready. - bg.remove(testBalancerIDs[1]) - scToRemove := <-cc.removeSubConnCh - if !cmp.Equal(scToRemove, sc2, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("RemoveSubConn, want %v, got %v", sc2, scToRemove) - } - p2 := <-cc.newPickerCh - want = []balancer.SubConn{sc1, sc3} - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - // move balancer 3 into transient failure. - bg.handleSubConnStateChange(sc3, connectivity.TransientFailure) - // Remove the first balancer, while the third is transient failure. - bg.remove(testBalancerIDs[0]) - scToRemove = <-cc.removeSubConnCh - if !cmp.Equal(scToRemove, sc1, cmp.AllowUnexported(testSubConn{})) { - t.Fatalf("RemoveSubConn, want %v, got %v", sc1, scToRemove) - } - p3 := <-cc.newPickerCh - for i := 0; i < 5; i++ { - if _, err := p3.Pick(balancer.PickInfo{}); err != balancer.ErrTransientFailure { - t.Fatalf("want pick error %v, got %v", balancer.ErrTransientFailure, err) - } - } -} - -// 2 balancers, change balancer weight. -func (s) TestBalancerGroup_TwoRR_ChangeWeight_MoreBackends(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - bg.start() - - // Add two balancers to group and send two resolved addresses to both - // balancers. - bg.add(testBalancerIDs[0], 2, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - sc1 := <-cc.newSubConnCh - sc2 := <-cc.newSubConnCh - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - sc3 := <-cc.newSubConnCh - sc4 := <-cc.newSubConnCh - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - bg.handleSubConnStateChange(sc3, connectivity.Connecting) - bg.handleSubConnStateChange(sc3, connectivity.Ready) - bg.handleSubConnStateChange(sc4, connectivity.Connecting) - bg.handleSubConnStateChange(sc4, connectivity.Ready) - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{sc1, sc1, sc2, sc2, sc3, sc4} - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - bg.changeWeight(testBalancerIDs[0], 3) - - // Test roundrobin with new weight. - p2 := <-cc.newPickerCh - want = []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc2, sc3, sc4} - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } -} - -func (s) TestBalancerGroup_LoadReport(t *testing.T) { - testLoadStore := newTestLoadStore() - - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, testLoadStore, nil) - bg.start() - - backendToBalancerID := make(map[balancer.SubConn]internal.Locality) - - // Add two balancers to group and send two resolved addresses to both - // balancers. - bg.add(testBalancerIDs[0], 2, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - sc1 := <-cc.newSubConnCh - sc2 := <-cc.newSubConnCh - backendToBalancerID[sc1] = testBalancerIDs[0] - backendToBalancerID[sc2] = testBalancerIDs[0] - - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - sc3 := <-cc.newSubConnCh - sc4 := <-cc.newSubConnCh - backendToBalancerID[sc3] = testBalancerIDs[1] - backendToBalancerID[sc4] = testBalancerIDs[1] - - // Send state changes for both subconns. - bg.handleSubConnStateChange(sc1, connectivity.Connecting) - bg.handleSubConnStateChange(sc1, connectivity.Ready) - bg.handleSubConnStateChange(sc2, connectivity.Connecting) - bg.handleSubConnStateChange(sc2, connectivity.Ready) - bg.handleSubConnStateChange(sc3, connectivity.Connecting) - bg.handleSubConnStateChange(sc3, connectivity.Ready) - bg.handleSubConnStateChange(sc4, connectivity.Connecting) - bg.handleSubConnStateChange(sc4, connectivity.Ready) - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - var ( - wantStart []internal.Locality - wantEnd []internal.Locality - wantCost []testServerLoad - ) - for i := 0; i < 10; i++ { - scst, _ := p1.Pick(balancer.PickInfo{}) - locality := backendToBalancerID[scst.SubConn] - wantStart = append(wantStart, locality) - if scst.Done != nil && scst.SubConn != sc1 { - scst.Done(balancer.DoneInfo{ - ServerLoad: &orcapb.OrcaLoadReport{ - CpuUtilization: 10, - MemUtilization: 5, - RequestCost: map[string]float64{"pic": 3.14}, - Utilization: map[string]float64{"piu": 3.14}, - }, - }) - wantEnd = append(wantEnd, locality) - wantCost = append(wantCost, - testServerLoad{name: serverLoadCPUName, d: 10}, - testServerLoad{name: serverLoadMemoryName, d: 5}, - testServerLoad{name: "pic", d: 3.14}, - testServerLoad{name: "piu", d: 3.14}) - } - } - - if !cmp.Equal(testLoadStore.callsStarted, wantStart) { - t.Fatalf("want started: %v, got: %v", testLoadStore.callsStarted, wantStart) - } - if !cmp.Equal(testLoadStore.callsEnded, wantEnd) { - t.Fatalf("want ended: %v, got: %v", testLoadStore.callsEnded, wantEnd) - } - if !cmp.Equal(testLoadStore.callsCost, wantCost, cmp.AllowUnexported(testServerLoad{})) { - t.Fatalf("want cost: %v, got: %v", testLoadStore.callsCost, wantCost) - } -} - -// Create a new balancer group, add balancer and backends, but not start. -// - b1, weight 2, backends [0,1] -// - b2, weight 1, backends [2,3] -// Start the balancer group and check behavior. -// -// Close the balancer group, call add/remove/change weight/change address. -// - b2, weight 3, backends [0,3] -// - b3, weight 1, backends [1,2] -// Start the balancer group again and check for behavior. -func (s) TestBalancerGroup_start_close(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - - // Add two balancers to group and send two resolved addresses to both - // balancers. - bg.add(testBalancerIDs[0], 2, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - - bg.start() - - m1 := make(map[resolver.Address]balancer.SubConn) - for i := 0; i < 4; i++ { - addrs := <-cc.newSubConnAddrsCh - sc := <-cc.newSubConnCh - m1[addrs[0]] = sc - bg.handleSubConnStateChange(sc, connectivity.Connecting) - bg.handleSubConnStateChange(sc, connectivity.Ready) - } - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{ - m1[testBackendAddrs[0]], m1[testBackendAddrs[0]], - m1[testBackendAddrs[1]], m1[testBackendAddrs[1]], - m1[testBackendAddrs[2]], m1[testBackendAddrs[3]], - } - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - bg.close() - for i := 0; i < 4; i++ { - bg.handleSubConnStateChange(<-cc.removeSubConnCh, connectivity.Shutdown) - } - - // Add b3, weight 1, backends [1,2]. - bg.add(testBalancerIDs[2], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[2], testBackendAddrs[1:3]) - - // Remove b1. - bg.remove(testBalancerIDs[0]) - - // Update b2 to weight 3, backends [0,3]. - bg.changeWeight(testBalancerIDs[1], 3) - bg.handleResolvedAddrs(testBalancerIDs[1], append([]resolver.Address(nil), testBackendAddrs[0], testBackendAddrs[3])) - - bg.start() - - m2 := make(map[resolver.Address]balancer.SubConn) - for i := 0; i < 4; i++ { - addrs := <-cc.newSubConnAddrsCh - sc := <-cc.newSubConnCh - m2[addrs[0]] = sc - bg.handleSubConnStateChange(sc, connectivity.Connecting) - bg.handleSubConnStateChange(sc, connectivity.Ready) - } - - // Test roundrobin on the last picker. - p2 := <-cc.newPickerCh - want = []balancer.SubConn{ - m2[testBackendAddrs[0]], m2[testBackendAddrs[0]], m2[testBackendAddrs[0]], - m2[testBackendAddrs[3]], m2[testBackendAddrs[3]], m2[testBackendAddrs[3]], - m2[testBackendAddrs[1]], m2[testBackendAddrs[2]], - } - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } -} - -// Test that balancer group start() doesn't deadlock if the balancer calls back -// into balancer group inline when it gets an update. -// -// The potential deadlock can happen if we -// - hold a lock and send updates to balancer (e.g. update resolved addresses) -// - the balancer calls back (NewSubConn or update picker) in line -// The callback will try to hold hte same lock again, which will cause a -// deadlock. -// -// This test starts the balancer group with a test balancer, will updates picker -// whenever it gets an address update. It's expected that start() doesn't block -// because of deadlock. -func (s) TestBalancerGroup_start_close_deadlock(t *testing.T) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - - bg.add(testBalancerIDs[0], 2, &testConstBalancerBuilder{}) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - bg.add(testBalancerIDs[1], 1, &testConstBalancerBuilder{}) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - - bg.start() -} - -func replaceDefaultSubBalancerCloseTimeout(n time.Duration) func() { - old := defaultSubBalancerCloseTimeout - defaultSubBalancerCloseTimeout = n - return func() { defaultSubBalancerCloseTimeout = old } -} - -// initBalancerGroupForCachingTest creates a balancer group, and initialize it -// to be ready for caching tests. -// -// Two rr balancers are added to bg, each with 2 ready subConns. A sub-balancer -// is removed later, so the balancer group returned has one sub-balancer in its -// own map, and one sub-balancer in cache. -func initBalancerGroupForCachingTest(t *testing.T) (*balancerGroup, *testClientConn, map[resolver.Address]balancer.SubConn) { - cc := newTestClientConn(t) - bg := newBalancerGroup(cc, nil, nil) - - // Add two balancers to group and send two resolved addresses to both - // balancers. - bg.add(testBalancerIDs[0], 2, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[0], testBackendAddrs[0:2]) - bg.add(testBalancerIDs[1], 1, rrBuilder) - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[2:4]) - - bg.start() - - m1 := make(map[resolver.Address]balancer.SubConn) - for i := 0; i < 4; i++ { - addrs := <-cc.newSubConnAddrsCh - sc := <-cc.newSubConnCh - m1[addrs[0]] = sc - bg.handleSubConnStateChange(sc, connectivity.Connecting) - bg.handleSubConnStateChange(sc, connectivity.Ready) - } - - // Test roundrobin on the last picker. - p1 := <-cc.newPickerCh - want := []balancer.SubConn{ - m1[testBackendAddrs[0]], m1[testBackendAddrs[0]], - m1[testBackendAddrs[1]], m1[testBackendAddrs[1]], - m1[testBackendAddrs[2]], m1[testBackendAddrs[3]], - } - if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - bg.remove(testBalancerIDs[1]) - // Don't wait for SubConns to be removed after close, because they are only - // removed after close timeout. - for i := 0; i < 10; i++ { - select { - case <-cc.removeSubConnCh: - t.Fatalf("Got request to remove subconn, want no remove subconn (because subconns were still in cache)") - default: - } - time.Sleep(time.Millisecond) - } - // Test roundrobin on the with only sub-balancer0. - p2 := <-cc.newPickerCh - want = []balancer.SubConn{ - m1[testBackendAddrs[0]], m1[testBackendAddrs[1]], - } - if err := isRoundRobin(want, subConnFromPicker(p2)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - return bg, cc, m1 -} - -// Test that if a sub-balancer is removed, and re-added within close timeout, -// the subConns won't be re-created. -func (s) TestBalancerGroup_locality_caching(t *testing.T) { - defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() - bg, cc, addrToSC := initBalancerGroupForCachingTest(t) - - // Turn down subconn for addr2, shouldn't get picker update because - // sub-balancer1 was removed. - bg.handleSubConnStateChange(addrToSC[testBackendAddrs[2]], connectivity.TransientFailure) - for i := 0; i < 10; i++ { - select { - case <-cc.newPickerCh: - t.Fatalf("Got new picker, want no new picker (because the sub-balancer was removed)") - default: - } - time.Sleep(time.Millisecond) - } - - // Sleep, but sleep less then close timeout. - time.Sleep(time.Millisecond * 100) - - // Re-add sub-balancer-1, because subconns were in cache, no new subconns - // should be created. But a new picker will still be generated, with subconn - // states update to date. - bg.add(testBalancerIDs[1], 1, rrBuilder) - - p3 := <-cc.newPickerCh - want := []balancer.SubConn{ - addrToSC[testBackendAddrs[0]], addrToSC[testBackendAddrs[0]], - addrToSC[testBackendAddrs[1]], addrToSC[testBackendAddrs[1]], - // addr2 is down, b2 only has addr3 in READY state. - addrToSC[testBackendAddrs[3]], addrToSC[testBackendAddrs[3]], - } - if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } - - for i := 0; i < 10; i++ { - select { - case <-cc.newSubConnAddrsCh: - t.Fatalf("Got new subconn, want no new subconn (because subconns were still in cache)") - default: - } - time.Sleep(time.Millisecond * 10) - } -} - -// Sub-balancers are put in cache when they are removed. If balancer group is -// closed within close timeout, all subconns should still be rmeoved -// immediately. -func (s) TestBalancerGroup_locality_caching_close_group(t *testing.T) { - defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() - bg, cc, addrToSC := initBalancerGroupForCachingTest(t) - - bg.close() - // The balancer group is closed. The subconns should be removed immediately. - removeTimeout := time.After(time.Millisecond * 500) - scToRemove := map[balancer.SubConn]int{ - addrToSC[testBackendAddrs[0]]: 1, - addrToSC[testBackendAddrs[1]]: 1, - addrToSC[testBackendAddrs[2]]: 1, - addrToSC[testBackendAddrs[3]]: 1, - } - for i := 0; i < len(scToRemove); i++ { - select { - case sc := <-cc.removeSubConnCh: - c := scToRemove[sc] - if c == 0 { - t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) - } - scToRemove[sc] = c - 1 - case <-removeTimeout: - t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") - } - } -} - -// Sub-balancers in cache will be closed if not re-added within timeout, and -// subConns will be removed. -func (s) TestBalancerGroup_locality_caching_not_readd_within_timeout(t *testing.T) { - defer replaceDefaultSubBalancerCloseTimeout(time.Second)() - _, cc, addrToSC := initBalancerGroupForCachingTest(t) - - // The sub-balancer is not re-added withtin timeout. The subconns should be - // removed. - removeTimeout := time.After(defaultSubBalancerCloseTimeout) - scToRemove := map[balancer.SubConn]int{ - addrToSC[testBackendAddrs[2]]: 1, - addrToSC[testBackendAddrs[3]]: 1, - } - for i := 0; i < len(scToRemove); i++ { - select { - case sc := <-cc.removeSubConnCh: - c := scToRemove[sc] - if c == 0 { - t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) - } - scToRemove[sc] = c - 1 - case <-removeTimeout: - t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") - } - } -} - -// Wrap the rr builder, so it behaves the same, but has a different pointer. -type noopBalancerBuilderWrapper struct { - balancer.Builder -} - -// After removing a sub-balancer, re-add with same ID, but different balancer -// builder. Old subconns should be removed, and new subconns should be created. -func (s) TestBalancerGroup_locality_caching_readd_with_different_builder(t *testing.T) { - defer replaceDefaultSubBalancerCloseTimeout(10 * time.Second)() - bg, cc, addrToSC := initBalancerGroupForCachingTest(t) - - // Re-add sub-balancer-1, but with a different balancer builder. The - // sub-balancer was still in cache, but cann't be reused. This should cause - // old sub-balancer's subconns to be removed immediately, and new subconns - // to be created. - bg.add(testBalancerIDs[1], 1, &noopBalancerBuilderWrapper{rrBuilder}) - - // The cached sub-balancer should be closed, and the subconns should be - // removed immediately. - removeTimeout := time.After(time.Millisecond * 500) - scToRemove := map[balancer.SubConn]int{ - addrToSC[testBackendAddrs[2]]: 1, - addrToSC[testBackendAddrs[3]]: 1, - } - for i := 0; i < len(scToRemove); i++ { - select { - case sc := <-cc.removeSubConnCh: - c := scToRemove[sc] - if c == 0 { - t.Fatalf("Got removeSubConn for %v when there's %d remove expected", sc, c) - } - scToRemove[sc] = c - 1 - case <-removeTimeout: - t.Fatalf("timeout waiting for subConns (from balancer in cache) to be removed") - } - } - - bg.handleResolvedAddrs(testBalancerIDs[1], testBackendAddrs[4:6]) - - newSCTimeout := time.After(time.Millisecond * 500) - scToAdd := map[resolver.Address]int{ - testBackendAddrs[4]: 1, - testBackendAddrs[5]: 1, - } - for i := 0; i < len(scToAdd); i++ { - select { - case addr := <-cc.newSubConnAddrsCh: - c := scToAdd[addr[0]] - if c == 0 { - t.Fatalf("Got newSubConn for %v when there's %d new expected", addr, c) - } - scToAdd[addr[0]] = c - 1 - sc := <-cc.newSubConnCh - addrToSC[addr[0]] = sc - bg.handleSubConnStateChange(sc, connectivity.Connecting) - bg.handleSubConnStateChange(sc, connectivity.Ready) - case <-newSCTimeout: - t.Fatalf("timeout waiting for subConns (from new sub-balancer) to be newed") - } - } - - // Test roundrobin on the new picker. - p3 := <-cc.newPickerCh - want := []balancer.SubConn{ - addrToSC[testBackendAddrs[0]], addrToSC[testBackendAddrs[0]], - addrToSC[testBackendAddrs[1]], addrToSC[testBackendAddrs[1]], - addrToSC[testBackendAddrs[4]], addrToSC[testBackendAddrs[5]], - } - if err := isRoundRobin(want, subConnFromPicker(p3)); err != nil { - t.Fatalf("want %v, got %v", want, err) - } -} diff --git a/xds/internal/balancer/edsbalancer/eds_impl_test.go b/xds/internal/balancer/edsbalancer/eds_impl_test.go index 6fbffd234de..1e449430697 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl_test.go +++ b/xds/internal/balancer/edsbalancer/eds_impl_test.go @@ -383,58 +383,7 @@ func (s) TestClose(t *testing.T) { edsb := newEDSBalancerImpl(nil, nil, nil, nil) // This is what could happen when switching between fallback and eds. This // make sure it doesn't panic. - edsb.Close() -} - -func init() { - balancer.Register(&testConstBalancerBuilder{}) -} - -var errTestConstPicker = fmt.Errorf("const picker error") - -type testConstBalancerBuilder struct{} - -func (*testConstBalancerBuilder) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { - return &testConstBalancer{cc: cc} -} - -func (*testConstBalancerBuilder) Name() string { - return "test-const-balancer" -} - -type testConstBalancer struct { - cc balancer.ClientConn -} - -func (tb *testConstBalancer) ResolverError(error) { - panic("not implemented") -} - -func (tb *testConstBalancer) UpdateSubConnState(balancer.SubConn, balancer.SubConnState) { - tb.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Ready, Picker: &testConstPicker{err: errTestConstPicker}}) -} - -func (tb *testConstBalancer) UpdateClientConnState(s balancer.ClientConnState) error { - a := s.ResolverState.Addresses - if len(a) > 0 { - tb.cc.NewSubConn(a, balancer.NewSubConnOptions{}) - } - return nil -} - -func (*testConstBalancer) Close() { -} - -type testConstPicker struct { - err error - sc balancer.SubConn -} - -func (tcp *testConstPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { - if tcp.err != nil { - return balancer.PickResult{}, tcp.err - } - return balancer.PickResult{SubConn: tcp.sc}, nil + edsb.close() } // Create XDS balancer, and update sub-balancer before handling eds responses. diff --git a/xds/internal/balancer/edsbalancer/eds_test.go b/xds/internal/balancer/edsbalancer/eds_test.go index eab7d137d87..5f8b04d6820 100644 --- a/xds/internal/balancer/edsbalancer/eds_test.go +++ b/xds/internal/balancer/edsbalancer/eds_test.go @@ -55,7 +55,7 @@ func init() { } } -func subConnFromPicker(p balancer.V2Picker) func() balancer.SubConn { +func subConnFromPicker(p balancer.Picker) func() balancer.SubConn { return func() balancer.SubConn { scst, _ := p.Pick(balancer.PickInfo{}) return scst.SubConn diff --git a/xds/internal/balancer/edsbalancer/test_util_test.go b/xds/internal/balancer/edsbalancer/test_util_test.go deleted file mode 100644 index e2b3c56a381..00000000000 --- a/xds/internal/balancer/edsbalancer/test_util_test.go +++ /dev/null @@ -1,346 +0,0 @@ -/* - * 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 edsbalancer - -import ( - "context" - "fmt" - "testing" - - "google.golang.org/grpc" - "google.golang.org/grpc/balancer" - "google.golang.org/grpc/connectivity" - "google.golang.org/grpc/resolver" - "google.golang.org/grpc/xds/internal" - - corepb "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" -) - -const testSubConnsCount = 16 - -var testSubConns []*testSubConn - -func init() { - for i := 0; i < testSubConnsCount; i++ { - testSubConns = append(testSubConns, &testSubConn{ - id: fmt.Sprintf("sc%d", i), - }) - } -} - -type testSubConn struct { - id string -} - -func (tsc *testSubConn) UpdateAddresses([]resolver.Address) { - panic("not implemented") -} - -func (tsc *testSubConn) Connect() { -} - -// Implement stringer to get human friendly error message. -func (tsc *testSubConn) String() string { - return tsc.id -} - -type testClientConn struct { - t *testing.T // For logging only. - - newSubConnAddrsCh chan []resolver.Address // The last 10 []Address to create subconn. - newSubConnCh chan balancer.SubConn // The last 10 subconn created. - removeSubConnCh chan balancer.SubConn // The last 10 subconn removed. - - newPickerCh chan balancer.Picker // The last picker updated. - newStateCh chan connectivity.State // The last state. - - subConnIdx int -} - -func newTestClientConn(t *testing.T) *testClientConn { - return &testClientConn{ - t: t, - - newSubConnAddrsCh: make(chan []resolver.Address, 10), - newSubConnCh: make(chan balancer.SubConn, 10), - removeSubConnCh: make(chan balancer.SubConn, 10), - - newPickerCh: make(chan balancer.Picker, 1), - newStateCh: make(chan connectivity.State, 1), - } -} - -func (tcc *testClientConn) NewSubConn(a []resolver.Address, o balancer.NewSubConnOptions) (balancer.SubConn, error) { - sc := testSubConns[tcc.subConnIdx] - tcc.subConnIdx++ - - tcc.t.Logf("testClientConn: NewSubConn(%v, %+v) => %s", a, o, sc) - select { - case tcc.newSubConnAddrsCh <- a: - default: - } - - select { - case tcc.newSubConnCh <- sc: - default: - } - - return sc, nil -} - -func (tcc *testClientConn) RemoveSubConn(sc balancer.SubConn) { - tcc.t.Logf("testClientCOnn: RemoveSubConn(%p)", sc) - select { - case tcc.removeSubConnCh <- sc: - default: - } -} - -func (tcc *testClientConn) UpdateState(bs balancer.State) { - tcc.t.Logf("testClientConn: UpdateState(%v)", bs) - select { - case <-tcc.newStateCh: - default: - } - tcc.newStateCh <- bs.ConnectivityState - - select { - case <-tcc.newPickerCh: - default: - } - tcc.newPickerCh <- bs.Picker -} - -func (tcc *testClientConn) ResolveNow(resolver.ResolveNowOptions) { - panic("not implemented") -} - -func (tcc *testClientConn) Target() string { - panic("not implemented") -} - -type testServerLoad struct { - name string - d float64 -} - -type testLoadStore struct { - callsStarted []internal.Locality - callsEnded []internal.Locality - callsCost []testServerLoad -} - -func newTestLoadStore() *testLoadStore { - return &testLoadStore{} -} - -func (*testLoadStore) CallDropped(category string) { - panic("not implemented") -} - -func (tls *testLoadStore) CallStarted(l internal.Locality) { - tls.callsStarted = append(tls.callsStarted, l) -} - -func (tls *testLoadStore) CallFinished(l internal.Locality, err error) { - tls.callsEnded = append(tls.callsEnded, l) -} - -func (tls *testLoadStore) CallServerLoad(l internal.Locality, name string, d float64) { - tls.callsCost = append(tls.callsCost, testServerLoad{name: name, d: d}) -} - -func (*testLoadStore) ReportTo(ctx context.Context, cc *grpc.ClientConn, clusterName string, node *corepb.Node) { - panic("not implemented") -} - -// isRoundRobin checks whether f's return value is roundrobin of elements from -// want. But it doesn't check for the order. Note that want can contain -// duplicate items, which makes it weight-round-robin. -// -// Step 1. the return values of f should form a permutation of all elements in -// want, but not necessary in the same order. E.g. if want is {a,a,b}, the check -// fails if f returns: -// - {a,a,a}: third a is returned before b -// - {a,b,b}: second b is returned before the second a -// -// If error is found in this step, the returned error contains only the first -// iteration until where it goes wrong. -// -// Step 2. the return values of f should be repetitions of the same permutation. -// E.g. if want is {a,a,b}, the check failes if f returns: -// - {a,b,a,b,a,a}: though it satisfies step 1, the second iteration is not -// repeating the first iteration. -// -// If error is found in this step, the returned error contains the first -// iteration + the second iteration until where it goes wrong. -func isRoundRobin(want []balancer.SubConn, f func() balancer.SubConn) error { - wantSet := make(map[balancer.SubConn]int) // SubConn -> count, for weighted RR. - for _, sc := range want { - wantSet[sc]++ - } - - // The first iteration: makes sure f's return values form a permutation of - // elements in want. - // - // Also keep the returns values in a slice, so we can compare the order in - // the second iteration. - gotSliceFirstIteration := make([]balancer.SubConn, 0, len(want)) - for range want { - got := f() - gotSliceFirstIteration = append(gotSliceFirstIteration, got) - wantSet[got]-- - if wantSet[got] < 0 { - return fmt.Errorf("non-roundrobin want: %v, result: %v", want, gotSliceFirstIteration) - } - } - - // The second iteration should repeat the first iteration. - var gotSliceSecondIteration []balancer.SubConn - for i := 0; i < 2; i++ { - for _, w := range gotSliceFirstIteration { - g := f() - gotSliceSecondIteration = append(gotSliceSecondIteration, g) - if w != g { - return fmt.Errorf("non-roundrobin, first iter: %v, second iter: %v", gotSliceFirstIteration, gotSliceSecondIteration) - } - } - } - - return nil -} - -// testClosure is a test util for TestIsRoundRobin. -type testClosure struct { - r []balancer.SubConn - i int -} - -func (tc *testClosure) next() balancer.SubConn { - ret := tc.r[tc.i] - tc.i = (tc.i + 1) % len(tc.r) - return ret -} - -func (s) TestIsRoundRobin(t *testing.T) { - var ( - sc1 = testSubConns[0] - sc2 = testSubConns[1] - sc3 = testSubConns[2] - ) - - testCases := []struct { - desc string - want []balancer.SubConn - got []balancer.SubConn - pass bool - }{ - { - desc: "0 element", - want: []balancer.SubConn{}, - got: []balancer.SubConn{}, - pass: true, - }, - { - desc: "1 element RR", - want: []balancer.SubConn{sc1}, - got: []balancer.SubConn{sc1, sc1, sc1, sc1}, - pass: true, - }, - { - desc: "1 element not RR", - want: []balancer.SubConn{sc1}, - got: []balancer.SubConn{sc1, sc2, sc1}, - pass: false, - }, - { - desc: "2 elements RR", - want: []balancer.SubConn{sc1, sc2}, - got: []balancer.SubConn{sc1, sc2, sc1, sc2, sc1, sc2}, - pass: true, - }, - { - desc: "2 elements RR different order from want", - want: []balancer.SubConn{sc2, sc1}, - got: []balancer.SubConn{sc1, sc2, sc1, sc2, sc1, sc2}, - pass: true, - }, - { - desc: "2 elements RR not RR, mistake in first iter", - want: []balancer.SubConn{sc1, sc2}, - got: []balancer.SubConn{sc1, sc1, sc1, sc2, sc1, sc2}, - pass: false, - }, - { - desc: "2 elements RR not RR, mistake in second iter", - want: []balancer.SubConn{sc1, sc2}, - got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc1, sc2}, - pass: false, - }, - { - desc: "2 elements weighted RR", - want: []balancer.SubConn{sc1, sc1, sc2}, - got: []balancer.SubConn{sc1, sc1, sc2, sc1, sc1, sc2}, - pass: true, - }, - { - desc: "2 elements weighted RR different order", - want: []balancer.SubConn{sc1, sc1, sc2}, - got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc2, sc1}, - pass: true, - }, - - { - desc: "3 elements RR", - want: []balancer.SubConn{sc1, sc2, sc3}, - got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc3, sc1, sc2, sc3}, - pass: true, - }, - { - desc: "3 elements RR different order", - want: []balancer.SubConn{sc1, sc2, sc3}, - got: []balancer.SubConn{sc3, sc2, sc1, sc3, sc2, sc1}, - pass: true, - }, - { - desc: "3 elements weighted RR", - want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, - got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc1, sc1, sc2, sc3, sc1, sc2, sc1}, - pass: true, - }, - { - desc: "3 elements weighted RR not RR, mistake in first iter", - want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, - got: []balancer.SubConn{sc1, sc2, sc1, sc1, sc2, sc1, sc1, sc2, sc3, sc1, sc2, sc1}, - pass: false, - }, - { - desc: "3 elements weighted RR not RR, mistake in second iter", - want: []balancer.SubConn{sc1, sc1, sc1, sc2, sc2, sc3}, - got: []balancer.SubConn{sc1, sc2, sc3, sc1, sc2, sc1, sc1, sc1, sc3, sc1, sc2, sc1}, - pass: false, - }, - } - for _, tC := range testCases { - t.Run(tC.desc, func(t *testing.T) { - err := isRoundRobin(tC.want, (&testClosure{r: tC.got}).next) - if err == nil != tC.pass { - t.Errorf("want pass %v, want %v, got err %v", tC.pass, tC.want, err) - } - }) - } -} diff --git a/xds/internal/testutils/balancer.go b/xds/internal/testutils/balancer.go index 604417c8211..ec3c4b5c6e9 100644 --- a/xds/internal/testutils/balancer.go +++ b/xds/internal/testutils/balancer.go @@ -70,7 +70,7 @@ type TestClientConn struct { NewSubConnCh chan balancer.SubConn // the last 10 subconn created. RemoveSubConnCh chan balancer.SubConn // the last 10 subconn removed. - NewPickerCh chan balancer.V2Picker // the last picker updated. + NewPickerCh chan balancer.Picker // the last picker updated. NewStateCh chan connectivity.State // the last state. subConnIdx int @@ -85,7 +85,7 @@ func NewTestClientConn(t *testing.T) *TestClientConn { NewSubConnCh: make(chan balancer.SubConn, 10), RemoveSubConnCh: make(chan balancer.SubConn, 10), - NewPickerCh: make(chan balancer.V2Picker, 1), + NewPickerCh: make(chan balancer.Picker, 1), NewStateCh: make(chan connectivity.State, 1), } } @@ -285,15 +285,20 @@ type testConstBalancer struct { cc balancer.ClientConn } -func (tb *testConstBalancer) HandleSubConnStateChange(sc balancer.SubConn, state connectivity.State) { +func (tb *testConstBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { tb.cc.UpdateState(balancer.State{ConnectivityState: connectivity.Ready, Picker: &TestConstPicker{Err: ErrTestConstPicker}}) } -func (tb *testConstBalancer) HandleResolvedAddrs(a []resolver.Address, err error) { - if len(a) == 0 { - return +func (tb *testConstBalancer) ResolverError(error) { + panic("not implemented") +} + +func (tb *testConstBalancer) UpdateClientConnState(s balancer.ClientConnState) error { + if len(s.ResolverState.Addresses) == 0 { + return nil } - tb.cc.NewSubConn(a, balancer.NewSubConnOptions{}) + tb.cc.NewSubConn(s.ResolverState.Addresses, balancer.NewSubConnOptions{}) + return nil } func (*testConstBalancer) Close() { From 79449f3b30d8b58ee01fe297f9c531c9cb3583ec Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 27 Apr 2020 14:10:03 -0700 Subject: [PATCH 3/3] remove DropRPCError and treat all RPC status errors as dropping --- balancer/balancer.go | 34 ++++--------------- balancer/grpclb/grpclb_picker.go | 2 +- internal/status/status.go | 18 +++++----- picker_wrapper.go | 9 +++-- pickfirst.go | 11 +++--- status/status_test.go | 4 +-- vet.sh | 1 + xds/internal/balancer/edsbalancer/eds_impl.go | 2 +- 8 files changed, 30 insertions(+), 51 deletions(-) diff --git a/balancer/balancer.go b/balancer/balancer.go index dc75a957183..e75b2843604 100644 --- a/balancer/balancer.go +++ b/balancer/balancer.go @@ -27,15 +27,12 @@ import ( "net" "strings" - "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal" - istatus "google.golang.org/grpc/internal/status" "google.golang.org/grpc/metadata" "google.golang.org/grpc/resolver" "google.golang.org/grpc/serviceconfig" - "google.golang.org/grpc/status" ) var ( @@ -233,6 +230,10 @@ var ( ErrNoSubConnAvailable = errors.New("no SubConn is available") // ErrTransientFailure indicates all SubConns are in TransientFailure. // WaitForReady RPCs will block, non-WaitForReady RPCs will fail. + // + // Deprecated: return an appropriate error based on the last resolution or + // connection attempt instead. The behavior is the same for any non-gRPC + // status error. ErrTransientFailure = errors.New("all SubConns are in TransientFailure") ) @@ -251,26 +252,6 @@ type PickResult struct { Done func(DoneInfo) } -type dropRPCError struct { - *istatus.ErrorT -} - -func (e dropRPCError) DropRPC() bool { return true } - -// DropRPCError wraps err in an error implementing DropRPC() bool, returning -// true. If err is not a status error, it will be converted to one with code -// Unknown and the message containing the err.Error() result. DropRPCError -// should not be called with a nil error. -func DropRPCError(err error) error { - if err == nil { - return dropRPCError{ErrorT: status.Error(codes.Unknown, "nil error passed to DropRPCError").(*istatus.ErrorT)} - } - if se, ok := err.(*istatus.ErrorT); ok { - return dropRPCError{ErrorT: se} - } - return dropRPCError{ErrorT: status.Convert(err).Err().(*istatus.ErrorT)} -} - // TransientFailureError returns e. It exists for backward compatibility and // will be deleted soon. // @@ -296,10 +277,9 @@ type Picker interface { // - If the error is ErrNoSubConnAvailable, gRPC will block until a new // Picker is provided by the balancer (using ClientConn.UpdateState). // - // - If the error implements DropRPC() bool, returning true, gRPC will - // terminate all RPCs with the code and message provided. If the error - // is not a status error, it will be converted by gRPC to a status error - // with code Unknown. + // - If the error is a status error (implemented by the grpc/status + // package), gRPC will terminate the RPC with the code and message + // provided. // // - For all other errors, wait for ready RPCs will wait, but non-wait for // ready RPCs will be terminated with this error's Error() string and diff --git a/balancer/grpclb/grpclb_picker.go b/balancer/grpclb/grpclb_picker.go index 59f871d7814..39bc5cc71e8 100644 --- a/balancer/grpclb/grpclb_picker.go +++ b/balancer/grpclb/grpclb_picker.go @@ -172,7 +172,7 @@ func (p *lbPicker) Pick(balancer.PickInfo) (balancer.PickResult, error) { // If it's a drop, return an error and fail the RPC. if s.Drop { p.stats.drop(s.LoadBalanceToken) - return balancer.PickResult{}, balancer.DropRPCError(status.Errorf(codes.Unavailable, "request dropped by grpclb")) + return balancer.PickResult{}, status.Errorf(codes.Unavailable, "request dropped by grpclb") } // If not a drop but there's no ready subConns. diff --git a/internal/status/status.go b/internal/status/status.go index e5f4af1b090..981c265b4c1 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -97,7 +97,7 @@ func (s *Status) Err() error { if s.Code() == codes.OK { return nil } - return (*ErrorT)(s.Proto()) + return (*Error)(s.Proto()) } // WithDetails returns a new status with the provided details messages appended to the status. @@ -136,24 +136,24 @@ func (s *Status) Details() []interface{} { return details } -// ErrorT is an alias of a status proto. It implements error and Status, -// and a nil *ErrorT should never be returned by this package. -type ErrorT spb.Status +// Error is an alias of a status proto. It implements error and Status, +// and a nil *Error should never be returned by this package. +type Error spb.Status -func (e *ErrorT) Error() string { +func (e *Error) Error() string { p := (*spb.Status)(e) return fmt.Sprintf("rpc error: code = %s desc = %s", codes.Code(p.GetCode()), p.GetMessage()) } // GRPCStatus returns the Status represented by se. -func (e *ErrorT) GRPCStatus() *Status { +func (e *Error) GRPCStatus() *Status { return FromProto((*spb.Status)(e)) } // Is implements future error.Is functionality. -// A ErrorT is equivalent if the code and message are identical. -func (e *ErrorT) Is(target error) bool { - tse, ok := target.(*ErrorT) +// A Error is equivalent if the code and message are identical. +func (e *Error) Is(target error) bool { + tse, ok := target.(*Error) if !ok { return false } diff --git a/picker_wrapper.go b/picker_wrapper.go index 364fe96caac..7f3edaaedc6 100644 --- a/picker_wrapper.go +++ b/picker_wrapper.go @@ -130,13 +130,12 @@ func (pw *pickerWrapper) pick(ctx context.Context, failfast bool, info balancer. if err == balancer.ErrNoSubConnAvailable { continue } - if drop, ok := err.(interface{ DropRPC() bool }); ok && drop.DropRPC() { - // End the RPC unconditionally. If not a status error, Convert - // will transform it into one with code Unknown. - return nil, nil, status.Convert(err).Err() + if _, ok := status.FromError(err); ok { + // Status error: end the RPC unconditionally with this status. + return nil, nil, err } // For all other errors, wait for ready RPCs should block and other - // RPCs should fail. + // RPCs should fail with unavailable. if !failfast { lastPickErr = err continue diff --git a/pickfirst.go b/pickfirst.go index 95791a56554..4b7340ad3ec 100644 --- a/pickfirst.go +++ b/pickfirst.go @@ -20,12 +20,11 @@ package grpc import ( "errors" + "fmt" "google.golang.org/grpc/balancer" - "google.golang.org/grpc/codes" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/grpclog" - "google.golang.org/grpc/status" ) // PickFirstBalancerName is the name of the pick_first balancer. @@ -56,8 +55,8 @@ func (b *pickfirstBalancer) ResolverError(err error) { case connectivity.TransientFailure, connectivity.Idle, connectivity.Connecting: // Set a failing picker if we don't have a good picker. b.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, - Picker: &picker{err: status.Errorf(codes.Unavailable, "name resolver error: %v", err)}}, - ) + Picker: &picker{err: fmt.Errorf("name resolver error: %v", err)}, + }) } if grpclog.V(2) { grpclog.Infof("pickfirstBalancer: ResolverError called with error %v", err) @@ -78,8 +77,8 @@ func (b *pickfirstBalancer) UpdateClientConnState(cs balancer.ClientConnState) e } b.state = connectivity.TransientFailure b.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, - Picker: &picker{err: status.Errorf(codes.Unavailable, "error creating connection: %v", err)}}, - ) + Picker: &picker{err: fmt.Errorf("error creating connection: %v", err)}, + }) return balancer.ErrBadResolverState } b.state = connectivity.Idle diff --git a/status/status_test.go b/status/status_test.go index 1f8578e27a6..839a3c390ed 100644 --- a/status/status_test.go +++ b/status/status_test.go @@ -64,7 +64,7 @@ func (s) TestErrorsWithSameParameters(t *testing.T) { e1 := Errorf(codes.AlreadyExists, description) e2 := Errorf(codes.AlreadyExists, description) if e1 == e2 || !errEqual(e1, e2) { - t.Fatalf("Errors should be equivalent but unique - e1: %v, %v e2: %p, %v", e1.(*status.ErrorT), e1, e2.(*status.ErrorT), e2) + t.Fatalf("Errors should be equivalent but unique - e1: %v, %v e2: %p, %v", e1.(*status.Error), e1, e2.(*status.Error), e2) } } @@ -116,7 +116,7 @@ func (s) TestError(t *testing.T) { func (s) TestErrorOK(t *testing.T) { err := Error(codes.OK, "foo") if err != nil { - t.Fatalf("Error(codes.OK, _) = %p; want nil", err.(*status.ErrorT)) + t.Fatalf("Error(codes.OK, _) = %p; want nil", err.(*status.Error)) } } diff --git a/vet.sh b/vet.sh index 2fa14169b37..f0a67298a5b 100755 --- a/vet.sh +++ b/vet.sh @@ -130,6 +130,7 @@ not grep -Fv '.CredsBundle .NewAddress .NewServiceConfig .Type is deprecated: use Attributes +balancer.ErrTransientFailure balancer.Picker grpc.CallCustomCodec grpc.Code diff --git a/xds/internal/balancer/edsbalancer/eds_impl.go b/xds/internal/balancer/edsbalancer/eds_impl.go index 27ec8a4147e..0bc98de3eca 100644 --- a/xds/internal/balancer/edsbalancer/eds_impl.go +++ b/xds/internal/balancer/edsbalancer/eds_impl.go @@ -443,7 +443,7 @@ func (d *dropPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { if d.loadStore != nil { d.loadStore.CallDropped(category) } - return balancer.PickResult{}, balancer.DropRPCError(status.Errorf(codes.Unavailable, "RPC is dropped")) + return balancer.PickResult{}, status.Errorf(codes.Unavailable, "RPC is dropped") } // TODO: (eds) don't drop unless the inner picker is READY. Similar to // https://github.com/grpc/grpc-go/issues/2622.