From 60641afe75ecde095d524756587ab2a84e9888de Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 15 Aug 2023 14:38:18 -0400 Subject: [PATCH] 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 --- .changelog/18200.txt | 3 + client/allocrunner/taskrunner/stats_hook.go | 13 ++-- client/allocrunner/taskrunner/vault_hook.go | 13 ++-- client/devicemanager/instance.go | 11 ++- .../pluginmanager/drivermanager/instance.go | 16 ++--- command/agent/consul/version_checker.go | 14 ++-- drivers/docker/driver.go | 26 +++---- drivers/docker/stats.go | 10 +-- helper/backoff.go | 31 ++++++++ helper/backoff_test.go | 72 +++++++++++++++++++ nomad/state/state_store.go | 11 ++- nomad/worker.go | 17 ++--- 12 files changed, 162 insertions(+), 75 deletions(-) create mode 100644 .changelog/18200.txt create mode 100644 helper/backoff.go create mode 100644 helper/backoff_test.go diff --git a/.changelog/18200.txt b/.changelog/18200.txt new file mode 100644 index 000000000000..378043a657a4 --- /dev/null +++ b/.changelog/18200.txt @@ -0,0 +1,3 @@ +```release-note:bug +core: Fixed a bug where exponential backoff could result in excessive CPU usage +``` diff --git a/client/allocrunner/taskrunner/stats_hook.go b/client/allocrunner/taskrunner/stats_hook.go index 93359b4fe385..482679c61af1 100644 --- a/client/allocrunner/taskrunner/stats_hook.go +++ b/client/allocrunner/taskrunner/stats_hook.go @@ -8,6 +8,7 @@ import ( hclog "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/client/allocrunner/interfaces" cstructs "github.com/hashicorp/nomad/client/structs" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" bstructs "github.com/hashicorp/nomad/plugins/base/structs" ) @@ -124,7 +125,9 @@ MAIN: // // It logs the errors with appropriate log levels; don't log returned error func (h *statsHook) callStatsWithRetry(ctx context.Context, handle interfaces.DriverStats) (<-chan *cstructs.TaskResourceUsage, error) { - var retry int + var retry uint64 + var backoff time.Duration + limit := time.Second * 5 MAIN: if ctx.Err() != nil { @@ -159,13 +162,7 @@ MAIN: h.logger.Error("failed to start stats collection for task", "error", err) } - limit := time.Second * 5 - backoff := 1 << (2 * uint64(retry)) * time.Second - if backoff > limit || retry > 5 { - backoff = limit - } - - // Increment retry counter + backoff = helper.Backoff(time.Second, limit, retry) retry++ time.Sleep(backoff) diff --git a/client/allocrunner/taskrunner/vault_hook.go b/client/allocrunner/taskrunner/vault_hook.go index 87c76653ffd2..9b8700d41ed0 100644 --- a/client/allocrunner/taskrunner/vault_hook.go +++ b/client/allocrunner/taskrunner/vault_hook.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/nomad/client/allocrunner/interfaces" ti "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" "github.com/hashicorp/nomad/client/vaultclient" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" ) @@ -295,7 +296,8 @@ OUTER: // deriveVaultToken derives the Vault token using exponential backoffs. It // returns the Vault token and whether the manager should exit. func (h *vaultHook) deriveVaultToken() (token string, exit bool) { - attempts := 0 + var attempts uint64 + var backoff time.Duration for { tokens, err := h.client.DeriveToken(h.alloc, []string{h.taskName}) if err == nil { @@ -323,14 +325,11 @@ func (h *vaultHook) deriveVaultToken() (token string, exit bool) { } // Handle the retry case - backoff := (1 << (2 * uint64(attempts))) * vaultBackoffBaseline - if backoff > vaultBackoffLimit { - backoff = vaultBackoffLimit - } - h.logger.Error("failed to derive Vault token", "error", err, "recoverable", true, "backoff", backoff) - + backoff = helper.Backoff(vaultBackoffBaseline, vaultBackoffLimit, attempts) attempts++ + h.logger.Error("failed to derive Vault token", "error", err, "recoverable", true, "backoff", backoff) + // Wait till retrying select { case <-h.ctx.Done(): diff --git a/client/devicemanager/instance.go b/client/devicemanager/instance.go index b7d8a84648f2..d9da430cf532 100644 --- a/client/devicemanager/instance.go +++ b/client/devicemanager/instance.go @@ -8,6 +8,7 @@ import ( log "github.com/hashicorp/go-hclog" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/helper/pluginutils/singleton" "github.com/hashicorp/nomad/nomad/structs" @@ -447,7 +448,8 @@ func (i *instanceManager) handleFingerprint(f *device.FingerprintResponse) error // collectStats is a long lived goroutine for collecting device statistics. It // handles errors by backing off exponentially and retrying. func (i *instanceManager) collectStats() { - attempt := 0 + var attempt uint64 + var backoff time.Duration START: // Get a device plugin @@ -492,10 +494,7 @@ START: } // Retry with an exponential backoff - backoff := (1 << (2 * uint64(attempt))) * statsBackoffBaseline - if backoff > statsBackoffLimit { - backoff = statsBackoffLimit - } + backoff = helper.Backoff(statsBackoffBaseline, statsBackoffLimit, attempt) attempt++ i.logger.Error("stats returned an error", "error", err, "retry", backoff) @@ -508,7 +507,7 @@ START: } } - // Reset the attempt since we got statistics + // Reset the attempts since we got statistics attempt = 0 // Store the new stats diff --git a/client/pluginmanager/drivermanager/instance.go b/client/pluginmanager/drivermanager/instance.go index 68e6a5c351ec..77bb8d8ccaa4 100644 --- a/client/pluginmanager/drivermanager/instance.go +++ b/client/pluginmanager/drivermanager/instance.go @@ -7,6 +7,7 @@ import ( "time" log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/helper/pluginutils/singleton" "github.com/hashicorp/nomad/nomad/structs" @@ -287,7 +288,7 @@ func (i *instanceManager) fingerprint() { // backoff and retry used if the RPC is closed by the other end var backoff time.Duration - var retry int + var retry uint64 for { if backoff > 0 { select { @@ -326,11 +327,7 @@ func (i *instanceManager) fingerprint() { i.handleFingerprintError() // Calculate the new backoff - backoff = (1 << (2 * uint64(retry))) * driverFPBackoffBaseline - if backoff > driverFPBackoffLimit { - backoff = driverFPBackoffLimit - } - // Increment retry counter + backoff = helper.Backoff(driverFPBackoffBaseline, driverFPBackoffLimit, retry) retry++ continue } @@ -423,7 +420,7 @@ func (i *instanceManager) handleEvents() { } var backoff time.Duration - var retry int + var retry uint64 for { if backoff > 0 { select { @@ -450,10 +447,7 @@ func (i *instanceManager) handleEvents() { i.logger.Warn("failed to receive task events, retrying", "error", err, "retry", retry) // Calculate the new backoff - backoff = (1 << (2 * uint64(retry))) * driverFPBackoffBaseline - if backoff > driverFPBackoffLimit { - backoff = driverFPBackoffLimit - } + backoff = helper.Backoff(driverFPBackoffBaseline, driverFPBackoffLimit, retry) retry++ continue } diff --git a/command/agent/consul/version_checker.go b/command/agent/consul/version_checker.go index f31f016eaefc..3a7a950111b2 100644 --- a/command/agent/consul/version_checker.go +++ b/command/agent/consul/version_checker.go @@ -20,11 +20,12 @@ func checkConsulTLSSkipVerify(ctx context.Context, logger log.Logger, client Age defer close(done) - i := uint64(0) - timer, stop := helper.NewSafeTimer(limit) defer stop() + var attempts uint64 + var backoff time.Duration + for { self, err := client.Self() if err == nil { @@ -37,13 +38,8 @@ func checkConsulTLSSkipVerify(ctx context.Context, logger log.Logger, client Age return } - backoff := (1 << (2 * i)) * baseline - if backoff > limit { - backoff = limit - } else { - i++ - } - + backoff = helper.Backoff(baseline, limit, attempts) + attempts++ timer.Reset(backoff) select { diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 0ea2f09f9555..3f95a61c34f5 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -480,7 +480,9 @@ type createContainerClient interface { func (d *Driver) createContainer(client createContainerClient, config docker.CreateContainerOptions, image string) (*docker.Container, error) { // Create a container - attempted := 0 + var attempted uint64 + var backoff time.Duration + CREATE: container, createErr := client.CreateContainer(config) if createErr == nil { @@ -530,16 +532,19 @@ CREATE: if attempted < 5 { attempted++ - time.Sleep(nextBackoff(attempted)) + backoff = helper.Backoff(50*time.Millisecond, time.Minute, attempted) + time.Sleep(backoff) goto CREATE } + } else if strings.Contains(strings.ToLower(createErr.Error()), "no such image") { // There is still a very small chance this is possible even with the // coordinator so retry. return nil, nstructs.NewRecoverableError(createErr, true) } else if isDockerTransientError(createErr) && attempted < 5 { attempted++ - time.Sleep(nextBackoff(attempted)) + backoff = helper.Backoff(50*time.Millisecond, time.Minute, attempted) + time.Sleep(backoff) goto CREATE } @@ -554,8 +559,9 @@ func (d *Driver) startContainer(c *docker.Container) error { return err } - // Start a container - attempted := 0 + var attempted uint64 + var backoff time.Duration + START: startErr := dockerClient.StartContainer(c.ID, c.HostConfig) if startErr == nil || strings.Contains(startErr.Error(), "Container already running") { @@ -567,7 +573,8 @@ START: if isDockerTransientError(startErr) { if attempted < 5 { attempted++ - time.Sleep(nextBackoff(attempted)) + backoff = helper.Backoff(50*time.Millisecond, time.Minute, attempted) + time.Sleep(backoff) goto START } return nstructs.NewRecoverableError(startErr, true) @@ -576,13 +583,6 @@ START: return recoverableErrTimeouts(startErr) } -// nextBackoff returns appropriate docker backoff durations after attempted attempts. -func nextBackoff(attempted int) time.Duration { - // attempts in 200ms, 800ms, 3.2s, 12.8s, 51.2s - // TODO: add randomization factor and extract to a helper - return 1 << (2 * uint64(attempted)) * 50 * time.Millisecond -} - // createImage creates a docker image either by pulling it from a registry or by // loading it from the file system func (d *Driver) createImage(task *drivers.TaskConfig, driverConfig *TaskConfig, client *docker.Client) (string, error) { diff --git a/drivers/docker/stats.go b/drivers/docker/stats.go index 729283b1dc4d..14204af79f21 100644 --- a/drivers/docker/stats.go +++ b/drivers/docker/stats.go @@ -92,8 +92,8 @@ func (h *taskHandle) collectStats(ctx context.Context, destCh *usageSender, inte defer destCh.close() // backoff and retry used if the docker stats API returns an error - var backoff time.Duration = 0 - var retry int + var backoff time.Duration + var retry uint64 // create an interval timer timer, stop := helper.NewSafeTimer(backoff) @@ -133,11 +133,7 @@ func (h *taskHandle) collectStats(ctx context.Context, destCh *usageSender, inte h.logger.Debug("error collecting stats from container", "error", err) // Calculate the new backoff - backoff = (1 << (2 * uint64(retry))) * statsCollectorBackoffBaseline - if backoff > statsCollectorBackoffLimit { - backoff = statsCollectorBackoffLimit - } - // Increment retry counter + backoff = helper.Backoff(statsCollectorBackoffBaseline, statsCollectorBackoffLimit, retry) retry++ continue } diff --git a/helper/backoff.go b/helper/backoff.go new file mode 100644 index 000000000000..e25e1f1c2b95 --- /dev/null +++ b/helper/backoff.go @@ -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< backoffLimit { + deadline = backoffLimit + } + + return deadline +} diff --git a/helper/backoff_test.go b/helper/backoff_test.go new file mode 100644 index 000000000000..29af33f7b1ec --- /dev/null +++ b/helper/backoff_test.go @@ -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) + } +} diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index b9092601b00e..09ea4dbb655d 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/go-memdb" "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-set" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/nomad/stream" "github.com/hashicorp/nomad/nomad/structs" @@ -233,8 +234,9 @@ func (s *StateStore) SnapshotMinIndex(ctx context.Context, index uint64) (*State const backoffBase = 20 * time.Millisecond const backoffLimit = 1 * time.Second - var retries uint + var retries uint64 var retryTimer *time.Timer + var deadline time.Duration // XXX: Potential optimization is to set up a watch on the state // store's index table and only unblock via a trigger rather than @@ -252,16 +254,13 @@ func (s *StateStore) SnapshotMinIndex(ctx context.Context, index uint64) (*State } // Exponential back off - retries++ if retryTimer == nil { // First retry, start at baseline retryTimer = time.NewTimer(backoffBase) } else { // Subsequent retry, reset timer - deadline := 1 << (2 * retries) * backoffBase - if deadline > backoffLimit { - deadline = backoffLimit - } + deadline = helper.Backoff(backoffBase, backoffLimit, retries) + retries++ retryTimer.Reset(deadline) } diff --git a/nomad/worker.go b/nomad/worker.go index 8a15e0fdd107..9b3542a5ea18 100644 --- a/nomad/worker.go +++ b/nomad/worker.go @@ -13,6 +13,7 @@ import ( log "github.com/hashicorp/go-hclog" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/go-version" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/state" "github.com/hashicorp/nomad/nomad/structs" @@ -106,8 +107,9 @@ type Worker struct { // failures is the count of errors encountered while dequeueing evaluations // and is used to calculate backoff. - failures uint - evalToken string + failures uint64 + failureBackoff time.Duration + evalToken string // snapshotIndex is the index of the snapshot in which the scheduler was // first invoked. It is used to mark the SnapshotIndex of evaluations @@ -130,6 +132,7 @@ func newWorker(ctx context.Context, srv *Server, args SchedulerWorkerPoolArgs) * start: time.Now(), status: WorkerStarting, enabledSchedulers: make([]string, len(args.EnabledSchedulers)), + failureBackoff: time.Duration(0), } copy(w.enabledSchedulers, args.EnabledSchedulers) @@ -871,12 +874,10 @@ func (w *Worker) shouldResubmit(err error) bool { // backoff if the server or the worker is shutdown. func (w *Worker) backoffErr(base, limit time.Duration) bool { w.setWorkloadStatus(WorkloadBackoff) - backoff := (1 << (2 * w.failures)) * base - if backoff > limit { - backoff = limit - } else { - w.failures++ - } + + backoff := helper.Backoff(base, limit, w.failures) + w.failures++ + select { case <-time.After(backoff): return false