Skip to content

Commit 410880d

Browse files
authored
eds: fix priority timeout failure when EDS removes all priorities (#3830)
Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.
1 parent 0e72e09 commit 410880d

File tree

2 files changed

+78
-2
lines changed

2 files changed

+78
-2
lines changed

xds/internal/balancer/edsbalancer/eds_impl_priority.go

+13-2
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,12 @@ func (edsImpl *edsBalancerImpl) handlePriorityChange() {
4848
// Everything was removed by EDS.
4949
if !edsImpl.priorityLowest.isSet() {
5050
edsImpl.priorityInUse = newPriorityTypeUnset()
51+
// Stop the init timer. This can happen if the only priority is removed
52+
// shortly after it's added.
53+
if timer := edsImpl.priorityInitTimer; timer != nil {
54+
timer.Stop()
55+
edsImpl.priorityInitTimer = nil
56+
}
5157
edsImpl.cc.UpdateState(balancer.State{ConnectivityState: connectivity.TransientFailure, Picker: base.NewErrPicker(errAllPrioritiesRemoved)})
5258
return
5359
}
@@ -116,7 +122,7 @@ func (edsImpl *edsBalancerImpl) startPriority(priority priorityType) {
116122
edsImpl.priorityInitTimer = time.AfterFunc(defaultPriorityInitTimeout, func() {
117123
edsImpl.priorityMu.Lock()
118124
defer edsImpl.priorityMu.Unlock()
119-
if !edsImpl.priorityInUse.equal(priority) {
125+
if !edsImpl.priorityInUse.isSet() || !edsImpl.priorityInUse.equal(priority) {
120126
return
121127
}
122128
edsImpl.priorityInitTimer = nil
@@ -309,21 +315,26 @@ func (p priorityType) isSet() bool {
309315
}
310316

311317
func (p priorityType) equal(p2 priorityType) bool {
318+
if !p.isSet() && !p2.isSet() {
319+
return true
320+
}
312321
if !p.isSet() || !p2.isSet() {
313-
panic("priority unset")
322+
return false
314323
}
315324
return p == p2
316325
}
317326

318327
func (p priorityType) higherThan(p2 priorityType) bool {
319328
if !p.isSet() || !p2.isSet() {
329+
// TODO(menghanl): return an appropriate value instead of panic.
320330
panic("priority unset")
321331
}
322332
return p.p < p2.p
323333
}
324334

325335
func (p priorityType) lowerThan(p2 priorityType) bool {
326336
if !p.isSet() || !p2.isSet() {
337+
// TODO(menghanl): return an appropriate value instead of panic.
327338
panic("priority unset")
328339
}
329340
return p.p > p2.p

xds/internal/balancer/edsbalancer/eds_impl_priority_test.go

+65
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,46 @@ func (s) TestPriorityType(t *testing.T) {
655655
}
656656
}
657657

658+
func (s) TestPriorityTypeEqual(t *testing.T) {
659+
tests := []struct {
660+
name string
661+
p1, p2 priorityType
662+
want bool
663+
}{
664+
{
665+
name: "equal",
666+
p1: newPriorityType(12),
667+
p2: newPriorityType(12),
668+
want: true,
669+
},
670+
{
671+
name: "not equal",
672+
p1: newPriorityType(12),
673+
p2: newPriorityType(34),
674+
want: false,
675+
},
676+
{
677+
name: "one not set",
678+
p1: newPriorityType(1),
679+
p2: newPriorityTypeUnset(),
680+
want: false,
681+
},
682+
{
683+
name: "both not set",
684+
p1: newPriorityTypeUnset(),
685+
p2: newPriorityTypeUnset(),
686+
want: true,
687+
},
688+
}
689+
for _, tt := range tests {
690+
t.Run(tt.name, func(t *testing.T) {
691+
if got := tt.p1.equal(tt.p2); got != tt.want {
692+
t.Errorf("equal() = %v, want %v", got, tt.want)
693+
}
694+
})
695+
}
696+
}
697+
658698
// Test the case where the high priority contains no backends. The low priority
659699
// will be used.
660700
func (s) TestEDSPriority_HighPriorityNoEndpoints(t *testing.T) {
@@ -774,3 +814,28 @@ func (s) TestEDSPriority_HighPriorityAllUnhealthy(t *testing.T) {
774814
t.Fatalf("want %v, got %v", want, err)
775815
}
776816
}
817+
818+
// Test the case where the first and only priority is removed.
819+
func (s) TestEDSPriority_FirstPriorityUnavailable(t *testing.T) {
820+
const testPriorityInitTimeout = time.Second
821+
defer func(t time.Duration) {
822+
defaultPriorityInitTimeout = t
823+
}(defaultPriorityInitTimeout)
824+
defaultPriorityInitTimeout = testPriorityInitTimeout
825+
826+
cc := testutils.NewTestClientConn(t)
827+
edsb := newEDSBalancerImpl(cc, nil, nil, nil)
828+
edsb.enqueueChildBalancerStateUpdate = edsb.updateState
829+
830+
// One localities, with priorities [0], each with one backend.
831+
clab1 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
832+
clab1.AddLocality(testSubZones[0], 1, 0, testEndpointAddrs[:1], nil)
833+
edsb.handleEDSResponse(parseEDSRespProtoForTesting(clab1.Build()))
834+
835+
// Remove the only localities.
836+
clab2 := testutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil)
837+
edsb.handleEDSResponse(parseEDSRespProtoForTesting(clab2.Build()))
838+
839+
// Wait after double the init timer timeout, to ensure it doesn't fail.
840+
time.Sleep(testPriorityInitTimeout * 2)
841+
}

0 commit comments

Comments
 (0)