-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix multiple overflow errors in exponential backoff (#18200)
We use capped exponential backoff in several places in the code when handling failures. The code we've copy-and-pasted all over has a check to see if the backoff is greater than the limit, but this check happens after the bitshift and we always increment the number of attempts. This causes an overflow with a fairly small number of failures (ex. at one place I tested it occurs after only 24 iterations), resulting in a negative backoff which then never recovers. The backoff becomes a tight loop consuming resources and/or DoS'ing a Nomad RPC handler or an external API such as Vault. Note this doesn't occur in places where we cap the number of iterations so the loop breaks (usually to return an error), so long as the number of iterations is reasonable. Introduce a helper with a check on the cap before the bitshift to avoid overflow in all places this can occur. Fixes: #18199 Co-authored-by: stswidwinski <stan.swidwinski@gmail.com>
- Loading branch information
1 parent
6329d19
commit 60641af
Showing
12 changed files
with
162 additions
and
75 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ```release-note:bug | ||
| core: Fixed a bug where exponential backoff could result in excessive CPU usage | ||
| ``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Copyright (c) HashiCorp, Inc. | ||
| // SPDX-License-Identifier: BUSL-1.1 | ||
|
|
||
| package helper | ||
|
|
||
| import ( | ||
| "time" | ||
| ) | ||
|
|
||
| func Backoff(backoffBase time.Duration, backoffLimit time.Duration, attempt uint64) time.Duration { | ||
| const MaxUint = ^uint64(0) | ||
| const MaxInt = int64(MaxUint >> 1) | ||
|
|
||
| // Ensure lack of non-positive backoffs since these make no sense | ||
| if backoffBase.Nanoseconds() <= 0 { | ||
| return max(backoffBase, 0*time.Second) | ||
| } | ||
|
|
||
| // Ensure that a large attempt will not cause an overflow | ||
| if attempt > 62 || MaxInt/backoffBase.Nanoseconds() < (1<<attempt) { | ||
| return backoffLimit | ||
| } | ||
|
|
||
| // Compute deadline and clamp it to backoffLimit | ||
| deadline := 1 << attempt * backoffBase | ||
| if deadline > backoffLimit { | ||
| deadline = backoffLimit | ||
| } | ||
|
|
||
| return deadline | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| // Copyright (c) HashiCorp, Inc. | ||
| // SPDX-License-Identifier: BUSL-1.1 | ||
|
|
||
| package helper | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/shoenig/test/must" | ||
| ) | ||
|
|
||
| func Test_Backoff(t *testing.T) { | ||
| const MaxUint = ^uint64(0) | ||
| const MaxInt = int64(MaxUint >> 1) | ||
|
|
||
| cases := []struct { | ||
| name string | ||
| backoffBase time.Duration | ||
| backoffLimit time.Duration | ||
| attempt uint64 | ||
| expectedResult time.Duration | ||
| }{ | ||
| { | ||
| name: "backoff limit clamps for high base", | ||
| backoffBase: time.Hour, | ||
| backoffLimit: time.Minute, | ||
| attempt: 1, | ||
| expectedResult: time.Minute, | ||
| }, | ||
| { | ||
| name: "backoff limit clamps for boundary attempt", | ||
| backoffBase: time.Hour, | ||
| backoffLimit: time.Minute, | ||
| attempt: 63, | ||
| expectedResult: time.Minute, | ||
| }, | ||
| { | ||
| name: "small retry value", | ||
| backoffBase: time.Minute, | ||
| backoffLimit: time.Hour, | ||
| attempt: 0, | ||
| expectedResult: time.Minute, | ||
| }, | ||
| { | ||
| name: "first retry value", | ||
| backoffBase: time.Minute, | ||
| backoffLimit: time.Hour, | ||
| attempt: 1, | ||
| expectedResult: 2 * time.Minute, | ||
| }, | ||
| { | ||
| name: "fifth retry value", | ||
| backoffBase: time.Minute, | ||
| backoffLimit: time.Hour, | ||
| attempt: 5, | ||
| expectedResult: 32 * time.Minute, | ||
| }, | ||
| { | ||
| name: "sixth retry value", | ||
| backoffBase: time.Minute, | ||
| backoffLimit: time.Hour, | ||
| attempt: 6, | ||
| expectedResult: time.Hour, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range cases { | ||
| result := Backoff(tc.backoffBase, tc.backoffLimit, tc.attempt) | ||
| must.Eq(t, tc.expectedResult, result) | ||
| } | ||
| } |
Oops, something went wrong.