Skip to content

Commit

Permalink
xds: fix test race in cluster_resolver (#4555)
Browse files Browse the repository at this point in the history 
There's a race between update sub-balancer and the first EDS resp. If
sub-balancer is updated after the first EDS resp, the old balancers
(round_robin) will create two lingering SubConns that are not handled,
which will mess up the following SubConn state updates.
  • Loading branch information
@menghanl
menghanl committed Jun 17, 2021
1 parent 151c8b7 commit 1c1e3f8
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 25 deletions.
28 changes: 13 additions & 15 deletions xds/internal/balancer/clusterresolver/eds_impl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func init() {
balancergroup.DefaultSubBalancerCloseTimeout = time.Millisecond * 100
}

func setupTestEDS(t *testing.T) (balancer.Balancer, *testutils.TestClientConn, *fakeclient.Client, func()) {
func setupTestEDS(t *testing.T, initChild *loadBalancingConfig) (balancer.Balancer, *testutils.TestClientConn, *fakeclient.Client, func()) {
xdsC := fakeclient.NewClientWithName(testBalancerNameFooBar)
cc := testutils.NewTestClientConn(t)
builder := balancer.Get(Name)
Expand All @@ -70,8 +70,11 @@ func setupTestEDS(t *testing.T) (balancer.Balancer, *testutils.TestClientConn, *
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if err := edsb.UpdateClientConnState(balancer.ClientConnState{
ResolverState: xdsclient.SetClient(resolver.State{}, xdsC),
BalancerConfig: &EDSConfig{ClusterName: testClusterName},
ResolverState: xdsclient.SetClient(resolver.State{}, xdsC),
BalancerConfig: &EDSConfig{
ClusterName: testClusterName,
ChildPolicy: initChild,
},
}); err != nil {
edsb.Close()
xdsC.Close()
Expand All @@ -94,7 +97,7 @@ func setupTestEDS(t *testing.T) (balancer.Balancer, *testutils.TestClientConn, *
// - replace backend
// - change drop rate
func (s) TestEDS_OneLocality(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

// One locality with one backend.
Expand Down Expand Up @@ -200,7 +203,7 @@ func (s) TestEDS_OneLocality(t *testing.T) {
// - address change for the <not-the-first> locality
// - update locality weight
func (s) TestEDS_TwoLocalities(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

// Two localities, each with one backend.
Expand Down Expand Up @@ -321,7 +324,7 @@ func (s) TestEDS_TwoLocalities(t *testing.T) {
// The EDS balancer gets EDS resp with unhealthy endpoints. Test that only
// healthy ones are used.
func (s) TestEDS_EndpointsHealth(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

// Two localities, each 3 backend, one Healthy, one Unhealthy, one Unknown.
Expand Down Expand Up @@ -394,7 +397,7 @@ func (s) TestEDS_EndpointsHealth(t *testing.T) {
//
// It should send an error picker with transient failure to the parent.
func (s) TestEDS_EmptyUpdate(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

const cacheTimeout = 100 * time.Microsecond
Expand Down Expand Up @@ -473,15 +476,10 @@ func (s) TestEDS_UpdateSubBalancerName(t *testing.T) {
},
})

edsb, cc, xdsC, cleanup := setupTestEDS(t)
t.Logf("initialize with sub-balancer: stub-balancer")
edsb, cc, xdsC, cleanup := setupTestEDS(t, &loadBalancingConfig{Name: balancerName})
defer cleanup()

t.Logf("update sub-balancer to stub-balancer")
if err := edsb.UpdateClientConnState(balancer.ClientConnState{
BalancerConfig: &EDSConfig{ClusterName: testClusterName, ChildPolicy: &loadBalancingConfig{Name: balancerName}},
}); err != nil {
t.Fatal(err)
}
// Two localities, each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
clab1.AddLocality(testSubZones[0], 1, 0, testEndpointAddrs[:1], nil)
Expand Down Expand Up @@ -565,7 +563,7 @@ func (s) TestEDS_UpdateSubBalancerName(t *testing.T) {
}

func (s) TestEDS_CircuitBreaking(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

var maxRequests uint32 = 50
Expand Down
20 changes: 10 additions & 10 deletions xds/internal/balancer/clusterresolver/priority_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import (
//
// Init 0 and 1; 0 is up, use 0; add 2, use 0; remove 2, use 0.
func (s) TestEDSPriority_HighPriorityReady(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

// Two localities, with priorities [0, 1], each with one backend.
Expand Down Expand Up @@ -100,7 +100,7 @@ func (s) TestEDSPriority_HighPriorityReady(t *testing.T) {
// Init 0 and 1; 0 is up, use 0; 0 is down, 1 is up, use 1; add 2, use 1; 1 is
// down, use 2; remove 2, use 1.
func (s) TestEDSPriority_SwitchPriority(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()

// Two localities, with priorities [0, 1], each with one backend.
Expand Down Expand Up @@ -194,7 +194,7 @@ func (s) TestEDSPriority_SwitchPriority(t *testing.T) {
//
// Init 0 and 1; 0 and 1 both down; add 2, use 2.
func (s) TestEDSPriority_HigherDownWhileAddingLower(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with different priorities, each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -247,7 +247,7 @@ func (s) TestEDSPriority_HigherDownWhileAddingLower(t *testing.T) {
//
// Init 0,1,2; 0 and 1 down, use 2; 0 up, close 1 and 2.
func (s) TestEDSPriority_HigherReadyCloseAllLower(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with priorities [0,1,2], each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -338,7 +338,7 @@ func (s) TestEDSPriority_InitTimeout(t *testing.T) {
}
}()()

edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with different priorities, each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -381,7 +381,7 @@ func (s) TestEDSPriority_InitTimeout(t *testing.T) {
// - start with 2 locality with p0 and p1
// - add localities to existing p0 and p1
func (s) TestEDSPriority_MultipleLocalities(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with different priorities, each with one backend.
clab0 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -478,7 +478,7 @@ func (s) TestEDSPriority_RemovesAllLocalities(t *testing.T) {
}
}()()

edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with different priorities, each with one backend.
clab0 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -585,7 +585,7 @@ func (s) TestEDSPriority_RemovesAllLocalities(t *testing.T) {
// Test the case where the high priority contains no backends. The low priority
// will be used.
func (s) TestEDSPriority_HighPriorityNoEndpoints(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with priorities [0, 1], each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -636,7 +636,7 @@ func (s) TestEDSPriority_HighPriorityNoEndpoints(t *testing.T) {
// Test the case where the high priority contains no healthy backends. The low
// priority will be used.
func (s) TestEDSPriority_HighPriorityAllUnhealthy(t *testing.T) {
edsb, cc, xdsC, cleanup := setupTestEDS(t)
edsb, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// Two localities, with priorities [0, 1], each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down Expand Up @@ -697,7 +697,7 @@ func (s) TestEDSPriority_FirstPriorityRemoved(t *testing.T) {
}
}()()

_, cc, xdsC, cleanup := setupTestEDS(t)
_, cc, xdsC, cleanup := setupTestEDS(t, nil)
defer cleanup()
// One localities, with priorities [0], each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
Expand Down

0 comments on commit 1c1e3f8

Please sign in to comment.