diff --git a/balancer/xds/edsbalancer/balancergroup.go b/balancer/xds/edsbalancer/balancergroup.go index 1ae931b8..ebd2aa37 100644 --- a/balancer/xds/edsbalancer/balancergroup.go +++ b/balancer/xds/edsbalancer/balancergroup.go @@ -81,7 +81,13 @@ func newBalancerGroup(cc balancer.ClientConn, loadStore lrs.Store) *balancerGrou } // add adds a balancer built by builder to the group, with given id and weight. +// +// weight should never be zero. func (bg *balancerGroup) add(id internal.Locality, weight uint32, builder balancer.Builder) { + if weight == 0 { + grpclog.Errorf("balancerGroup.add called with weight 0, locality: %v. Locality is not added to balancer group", id) + return + } bg.mu.Lock() if _, ok := bg.idToBalancer[id]; ok { bg.mu.Unlock() @@ -140,10 +146,16 @@ func (bg *balancerGroup) remove(id internal.Locality) { // changeWeight changes the weight of the balancer. // +// newWeight should never be zero. +// // NOTE: It always results in a picker update now. This probably isn't // necessary. But it seems better to do the update because it's a change in the // picker (which is balancer's snapshot). func (bg *balancerGroup) changeWeight(id internal.Locality, newWeight uint32) { + if newWeight == 0 { + grpclog.Errorf("balancerGroup.changeWeight called with newWeight 0. Weight is not changed") + return + } bg.pickerMu.Lock() defer bg.pickerMu.Unlock() pState, ok := bg.idToPickerState[id] diff --git a/balancer/xds/edsbalancer/edsbalancer.go b/balancer/xds/edsbalancer/edsbalancer.go index 7a68129b..7ffa21ec 100644 --- a/balancer/xds/edsbalancer/edsbalancer.go +++ b/balancer/xds/edsbalancer/edsbalancer.go @@ -29,6 +29,7 @@ import ( "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/balancer/xds/internal" edspb "google.golang.org/grpc/balancer/xds/internal/proto/envoy/api/v2/eds" + endpointpb "google.golang.org/grpc/balancer/xds/internal/proto/envoy/api/v2/endpoint/endpoint" percentpb "google.golang.org/grpc/balancer/xds/internal/proto/envoy/type/percent" "google.golang.org/grpc/balancer/xds/lrs" "google.golang.org/grpc/codes" @@ -187,10 +188,27 @@ func (xdsB *EDSBalancer) HandleEDSResponse(edsResp *edspb.ClusterLoadAssignment) xdsB.updateDrops(edsResp.GetPolicy().GetDropOverloads()) + // Filter out all localities with weight 0. + // + // Locality weighted load balancer can be enabled by setting an option in + // CDS, and the weight of each locality. Currently, without the guarantee + // that CDS is always sent, we assume locality weighted load balance is + // always enabled, and ignore all weight 0 localities. + // + // In the future, we should look at the config in CDS response and decide + // whether locality weight matters. + newEndpoints := make([]*endpointpb.LocalityLbEndpoints, 0, len(edsResp.Endpoints)) + for _, locality := range edsResp.Endpoints { + if locality.GetLoadBalancingWeight().GetValue() == 0 { + continue + } + newEndpoints = append(newEndpoints, locality) + } + // newLocalitiesSet contains all names of localitis in the new EDS response. // It's used to delete localities that are removed in the new EDS response. newLocalitiesSet := make(map[internal.Locality]struct{}) - for _, locality := range edsResp.Endpoints { + for _, locality := range newEndpoints { // One balancer for each locality. l := locality.GetLocality() @@ -206,11 +224,6 @@ func (xdsB *EDSBalancer) HandleEDSResponse(edsResp *edspb.ClusterLoadAssignment) newLocalitiesSet[lid] = struct{}{} newWeight := locality.GetLoadBalancingWeight().GetValue() - if newWeight == 0 { - // Weight can never be 0. - newWeight = 1 - } - var newAddrs []resolver.Address for _, lbEndpoint := range locality.GetLbEndpoints() { socketAddress := lbEndpoint.GetEndpoint().GetAddress().GetSocketAddress() diff --git a/balancer/xds/edsbalancer/edsbalancer_test.go b/balancer/xds/edsbalancer/edsbalancer_test.go index 2fbcbd32..788ccb29 100644 --- a/balancer/xds/edsbalancer/edsbalancer_test.go +++ b/balancer/xds/edsbalancer/edsbalancer_test.go @@ -334,6 +334,24 @@ func TestEDS_TwoLocalities(t *testing.T) { }); err != nil { t.Fatalf("want %v, got %v", want, err) } + + // Change weight of the locality[1] to 0, it should never be picked. + clab6 := newClusterLoadAssignmentBuilder(testClusterNames[0], nil) + clab6.addLocality(testSubZones[1], 0, testEndpointAddrs[1:2]) + clab6.addLocality(testSubZones[2], 1, testEndpointAddrs[2:4]) + edsb.HandleEDSResponse(clab6.build()) + + // Test pick with two subconns different locality weight. + p6 := <-cc.newPickerCh + // Locality-1 will be not be picked, and locality-2 will be picked. + // Locality-2 contains sc3 and sc4. So expect sc3, sc4. + want = []balancer.SubConn{sc3, sc4} + if err := isRoundRobin(want, func() balancer.SubConn { + sc, _, _ := p6.Pick(context.Background(), balancer.PickOptions{}) + return sc + }); err != nil { + t.Fatalf("want %v, got %v", want, err) + } } func TestClose(t *testing.T) {