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.

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

Automated cherry pick of #90476: fix backoff manager timer initialization race #90495

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 20 additions & 7 deletions staging/src/k8s.io/apimachinery/pkg/util/wait/wait.go
Expand Up @@ -286,8 +286,9 @@ func contextForChannel(parentCh <-chan struct{}) (context.Context, context.Cance
}

// BackoffManager manages backoff with a particular scheme based on its underlying implementation. It provides
// an interface to return a timer for backoff, and caller shall backoff until Timer.C returns. If the second Backoff()
// is called before the timer from the first Backoff() call finishes, the first timer will NOT be drained.
// an interface to return a timer for backoff, and caller shall backoff until Timer.C() drains. If the second Backoff()
// is called before the timer from the first Backoff() call finishes, the first timer will NOT be drained and result in
// undetermined behavior.
// The BackoffManager is supposed to be called in a single-threaded environment.
type BackoffManager interface {
Backoff() clock.Timer
Expand Down Expand Up @@ -317,7 +318,7 @@ func NewExponentialBackoffManager(initBackoff, maxBackoff, resetDuration time.Du
Steps: math.MaxInt32,
Cap: maxBackoff,
},
backoffTimer: c.NewTimer(0),
backoffTimer: nil,
initialBackoff: initBackoff,
lastBackoffStart: c.Now(),
backoffResetDuration: resetDuration,
Expand All @@ -334,9 +335,14 @@ func (b *exponentialBackoffManagerImpl) getNextBackoff() time.Duration {
return b.backoff.Step()
}

// Backoff implements BackoffManager.Backoff, it returns a timer so caller can block on the timer for backoff.
// Backoff implements BackoffManager.Backoff, it returns a timer so caller can block on the timer for exponential backoff.
// The returned timer must be drained before calling Backoff() the second time
func (b *exponentialBackoffManagerImpl) Backoff() clock.Timer {
b.backoffTimer.Reset(b.getNextBackoff())
if b.backoffTimer == nil {
b.backoffTimer = b.clock.NewTimer(b.getNextBackoff())
} else {
b.backoffTimer.Reset(b.getNextBackoff())
}
return b.backoffTimer
}

Expand All @@ -354,7 +360,7 @@ func NewJitteredBackoffManager(duration time.Duration, jitter float64, c clock.C
clock: c,
duration: duration,
jitter: jitter,
backoffTimer: c.NewTimer(0),
backoffTimer: nil,
}
}

Expand All @@ -366,8 +372,15 @@ func (j *jitteredBackoffManagerImpl) getNextBackoff() time.Duration {
return jitteredPeriod
}

// Backoff implements BackoffManager.Backoff, it returns a timer so caller can block on the timer for jittered backoff.
// The returned timer must be drained before calling Backoff() the second time
func (j *jitteredBackoffManagerImpl) Backoff() clock.Timer {
j.backoffTimer.Reset(j.getNextBackoff())
backoff := j.getNextBackoff()
if j.backoffTimer == nil {
j.backoffTimer = j.clock.NewTimer(backoff)
} else {
j.backoffTimer.Reset(backoff)
}
return j.backoffTimer
}

Expand Down
27 changes: 27 additions & 0 deletions staging/src/k8s.io/apimachinery/pkg/util/wait/wait_test.go
Expand Up @@ -731,3 +731,30 @@ func TestJitteredBackoffManagerGetNextBackoff(t *testing.T) {
t.Errorf("backoff should be 1, but got %d", backoff)
}
}

func TestJitterBackoffManagerWithRealClock(t *testing.T) {
backoffMgr := NewJitteredBackoffManager(1*time.Millisecond, 0, &clock.RealClock{})
for i := 0; i < 5; i++ {
start := time.Now()
<-backoffMgr.Backoff().C()
passed := time.Now().Sub(start)
if passed < 1*time.Millisecond {
t.Errorf("backoff should be at least 1ms, but got %s", passed.String())
}
}
}

func TestExponentialBackoffManagerWithRealClock(t *testing.T) {
// backoff at least 1ms, 2ms, 4ms, 8ms, 10ms, 10ms, 10ms
durationFactors := []time.Duration{1, 2, 4, 8, 10, 10, 10}
backoffMgr := NewExponentialBackoffManager(1*time.Millisecond, 10*time.Millisecond, 1*time.Hour, 2.0, 0.0, &clock.RealClock{})

for i := range durationFactors {
start := time.Now()
<-backoffMgr.Backoff().C()
passed := time.Now().Sub(start)
if passed < durationFactors[i]*time.Millisecond {
t.Errorf("backoff should be at least %d ms, but got %s", durationFactors[i], passed.String())
}
}
}