Skip to content

Commit 644142c

Browse files
authored
eds: fix priority timeout failure when EDS removes all priorities (cherry pick of #3830) (#3839)
Without this fix, when the EDS response removes all priorities, after the timeout, the priority check panics because priority is unset.
1 parent f3c2c5e commit 644142c

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
@@ -656,6 +656,46 @@ func (s) TestPriorityType(t *testing.T) {
656656
}
657657
}
658658

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

0 commit comments

Comments
 (0)