Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eds: fix priority timeout failure when EDS removes all priorities #3830

Merged
merged 4 commits into from Aug 24, 2020
Merged

eds: fix priority timeout failure when EDS removes all priorities #3830

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e2a3bf
[xds_priority_unset] eds: fix priority timeout failure when EDS remov…
menghanl Aug 20, 2020
0778b78
[xds_priority_unset] c1
menghanl Aug 22, 2020
6969452
[xds_priority_unset] priority equal for unset
menghanl Aug 24, 2020
8f807dc
[xds_priority_unset] defer
menghanl Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion xds/internal/balancer/edsbalancer/eds_impl_priority.go
View file
Open in desktop
Expand Up @@ -48,6 +48,12 @@ func (edsImpl *edsBalancerImpl) handlePriorityChange() {
// Everything was removed by EDS.
if !edsImpl.priorityLowest.isSet() {
edsImpl.priorityInUse = newPriorityTypeUnset()
// Stop the init timer. This can happen if the only priority is removed
// shortly after it's added.
if timer := edsImpl.priorityInitTimer; timer != nil {
timer.Stop()
edsImpl.priorityInitTimer = nil
}
edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, Picker: base.NewErrPicker(errAllPrioritiesRemoved)})
return
}
Expand Down Expand Up @@ -116,7 +122,7 @@ func (edsImpl *edsBalancerImpl) startPriority(priority priorityType) {
edsImpl.priorityInitTimer = time.AfterFunc(defaultPriorityInitTimeout, func() {
edsImpl.priorityMu.Lock()
defer edsImpl.priorityMu.Unlock()
if !edsImpl.priorityInUse.equal(priority) {
if !edsImpl.priorityInUse.isSet() || !edsImpl.priorityInUse.equal(priority) {
return
}
edsImpl.priorityInitTimer = nil
Expand Down
28 changes: 28 additions & 0 deletions xds/internal/balancer/edsbalancer/eds_impl_priority_test.go
View file
Open in desktop
Expand Up @@ -774,3 +774,31 @@ func (s) TestEDSPriority_HighPriorityAllUnhealthy(t *testing.T) {
t.Fatalf("want %v, got %v", want, err)
}
}

// Test the case where the first and only priority is removed.
func (s) TestEDSPriority_FirstPriorityUnavailable(t *testing.T) {
const testPriorityInitTimeout = time.Second
defer func() func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow .... never seen this before defer func() func().
Cool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't much different from: assign new value, then defer func() to reassign old value back.

The only advantage is, it limits the scope of old, so you can just name it old, instead of oldPriorityInitTimeout..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

defer func(t time.Duration) {
  defaultPriorityInitTimeout = t
}(defaultPriorityInitTimeout)
defaultPriorityInitTimeout = testPriorityInitTimeout

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

old := defaultPriorityInitTimeout
defaultPriorityInitTimeout = testPriorityInitTimeout
return func() {
defaultPriorityInitTimeout = old
}
}()()

cc := testutils.NewTestClientConn(t)
edsb := newEDSBalancerImpl(cc, nil, nil, nil)
edsb.enqueueChildBalancerStateUpdate = edsb.updateState

// One localities, with priorities [0], each with one backend.
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
clab1.AddLocality(testSubZones[0], 1, 0, testEndpointAddrs[:1], nil)
edsb.handleEDSResponse(parseEDSRespProtoForTesting(clab1.Build()))

// Remove the only localities.
clab2 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
edsb.handleEDSResponse(parseEDSRespProtoForTesting(clab2.Build()))

// Wait after the init timer timeout, to ensure it doesn't fail.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe wait for 2 times the value of testPriorityInitTimeout to be double sure that the timeout has fired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

time.Sleep(time.Second)
}