From 48d7ce8a4515473c43c541747d1485dc4718df64 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Mon, 11 May 2026 17:10:48 +1000 Subject: [PATCH] feat(viperblockd): retry LoadState on transient backend errors (siv-25 medium-term) (#210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(viperblockd): retry LoadState on transient backend errors ebs.mount used to call vb.LoadState once and treat any failure as fatal. When predastore was still warming up after a coordinated daemon restart, the backend Read came back with a network/timeout error, viperblock folded it into "block size is 0 in both local and backend state", and the daemon's recovery path then ran the cleanup branch — destroying DeleteOnTermination=true volumes (mulga-siv-25). Wrap LoadState in retryLoadState: 5 attempts, 200ms * 1.5^n backoff (total worst-case ~3.7s, well inside the 30s ebs.mount NATS deadline). Retry only on viperblock.ErrStateBackendUnavailable; ErrStateNotFound and any unclassified error fail fast so genuine problems are still visible. Retry loop is parameterised over sleep and loadFn so the retry policy itself is unit-tested without spinning up a real VB. * chore: bump viperblock to v1.2.2-0.20260511064619-1aa54f0dc1f6 Pulls in the ErrStateNotFound / ErrStateBackendUnavailable sentinels introduced by mulgadc/viperblock#11 (merged on dev). Required for the retry wrapper added in the previous commit to compile against the public viperblock module. --- go.mod | 3 +- go.sum | 4 +- spinifex/services/viperblockd/viperblockd.go | 53 ++++++++++++- .../services/viperblockd/viperblockd_test.go | 75 +++++++++++++++++++ 4 files changed, 129 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index bc398e870..f74c615c5 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/kdomanski/iso9660 v0.4.0 github.com/klauspost/cpuid/v2 v2.3.0 github.com/mulgadc/predastore v1.2.1 - github.com/mulgadc/viperblock v1.2.1 + github.com/mulgadc/viperblock v1.2.2-0.20260511064619-1aa54f0dc1f6 github.com/nats-io/nats-server/v2 v2.14.0 github.com/nats-io/nats.go v1.51.0 github.com/ovn-kubernetes/libovsdb v0.8.1 @@ -120,7 +120,6 @@ require ( go.opentelemetry.io/otel/trace v1.42.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - golang.org/x/exp v0.0.0-20231110203233-9a3e6036ecaa // indirect golang.org/x/mod v0.34.0 // indirect golang.org/x/net v0.53.0 // indirect golang.org/x/sync v0.20.0 // indirect diff --git a/go.sum b/go.sum index fcd641c3f..11a96d3c4 100644 --- a/go.sum +++ b/go.sum @@ -288,8 +288,8 @@ github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= github.com/mulgadc/predastore v1.2.1 h1:Os+CTjjs/IneRMWd/+m/Hi24sHI4M+vVxcBSRF2Pr10= github.com/mulgadc/predastore v1.2.1/go.mod h1:WGfbbzk0ExniHXkXsvlmIILSfONakrb7h9yDSxFktaY= -github.com/mulgadc/viperblock v1.2.1 h1:G3cSgh/yspZd7Wdg3C9QGqkqzYbEfYpusd6uMddSDmg= -github.com/mulgadc/viperblock v1.2.1/go.mod h1:CisHy13Uam0gUg5dxV0rKryJF2yGbgfDv09FYApRaNk= +github.com/mulgadc/viperblock v1.2.2-0.20260511064619-1aa54f0dc1f6 h1:PngaxM0orWymcmDMR97JbeedXlgpxoLiykrjSSmPNEU= +github.com/mulgadc/viperblock v1.2.2-0.20260511064619-1aa54f0dc1f6/go.mod h1:WCP9h+u2DvN8/+ccai5oMOa48hOJPUzSbh7Lia0Q8nY= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= diff --git a/spinifex/services/viperblockd/viperblockd.go b/spinifex/services/viperblockd/viperblockd.go index 87375dd2e..15f76ea44 100644 --- a/spinifex/services/viperblockd/viperblockd.go +++ b/spinifex/services/viperblockd/viperblockd.go @@ -2,6 +2,7 @@ package viperblockd import ( "encoding/json" + "errors" "fmt" "log/slog" "os" @@ -22,6 +23,52 @@ import ( "github.com/nats-io/nats.go" ) +// loadStateRetryAttempts and loadStateRetryBaseDelay tune the mount-time +// retry loop for transient backend errors (predastore restarting, NATS +// disconnect, etc.). 5 attempts at 200ms * 1.5^n caps total wait at ~3.7s, +// well under the daemon's 30s ebs.mount NATS timeout. See mulga-siv-25. +const ( + loadStateRetryAttempts = 5 + loadStateRetryBaseDelay = 200 * time.Millisecond +) + +// retryLoadState invokes loadFn up to attempts times, retrying with +// exponential backoff (delay * 3/2 each step) only when the returned error +// is viperblock.ErrStateBackendUnavailable. Success, ErrStateNotFound, and +// any unclassified error return immediately so callers see them. Extracted +// for unit testability — see loadStateWithRetry for the production caller. +func retryLoadState(volume string, attempts int, baseDelay time.Duration, sleep func(time.Duration), loadFn func() error) error { + delay := baseDelay + var err error + for attempt := 1; attempt <= attempts; attempt++ { + err = loadFn() + if err == nil { + if attempt > 1 { + slog.Info("LoadState succeeded after retry", + "volume", volume, "attempt", attempt) + } + return nil + } + if !errors.Is(err, viperblock.ErrStateBackendUnavailable) { + return err + } + if attempt == attempts { + break + } + slog.Warn("LoadState transient failure, retrying", + "volume", volume, "attempt", attempt, "delay", delay, "err", err) + sleep(delay) + delay = delay * 3 / 2 + } + return fmt.Errorf("LoadState exhausted %d retries: %w", attempts, err) +} + +// loadStateWithRetry calls vb.LoadState with the production retry budget +// (loadStateRetryAttempts / loadStateRetryBaseDelay). +func loadStateWithRetry(vb *viperblock.VB, volume string) error { + return retryLoadState(volume, loadStateRetryAttempts, loadStateRetryBaseDelay, time.Sleep, vb.LoadState) +} + var serviceName = "viperblock" type MountedVolume struct { @@ -463,8 +510,10 @@ func launchService(cfg *Config) (err error) { } // Next, connect to the volume and confirm the state - // First, fetch the state from the remote backend - err = vb.LoadState() + // First, fetch the state from the remote backend. Retry on transient + // backend errors (predastore restart races, NATS hiccups) so daemon + // recovery doesn't tip a healthy volume into the cleanup path. + err = loadStateWithRetry(vb, ebsRequest.Name) if err != nil { ebsResponse.Error = err.Error() diff --git a/spinifex/services/viperblockd/viperblockd_test.go b/spinifex/services/viperblockd/viperblockd_test.go index 25edfd9cc..093a084d2 100644 --- a/spinifex/services/viperblockd/viperblockd_test.go +++ b/spinifex/services/viperblockd/viperblockd_test.go @@ -1,12 +1,16 @@ package viperblockd import ( + "errors" + "fmt" "os" "sync" "testing" "time" + "github.com/mulgadc/viperblock/viperblock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestNew tests the service constructor @@ -439,3 +443,74 @@ func TestMountedVolumePortRange(t *testing.T) { assert.True(t, vol.Port <= 65535) } } + +// TestRetryLoadState pins the retry policy used by ebs.mount: transient +// backend errors retry with backoff, ErrStateNotFound fails fast, and the +// retry budget is honoured. Required by mulga-siv-25 medium-term fix. +func TestRetryLoadState(t *testing.T) { + t.Run("success_first_attempt_no_sleep", func(t *testing.T) { + calls := 0 + sleeps := 0 + err := retryLoadState("vol-x", 5, 10*time.Millisecond, + func(time.Duration) { sleeps++ }, + func() error { calls++; return nil }) + require.NoError(t, err) + assert.Equal(t, 1, calls) + assert.Equal(t, 0, sleeps) + }) + + t.Run("transient_recovers_within_budget", func(t *testing.T) { + calls := 0 + sleeps := 0 + err := retryLoadState("vol-x", 5, 10*time.Millisecond, + func(time.Duration) { sleeps++ }, + func() error { + calls++ + if calls < 3 { + return fmt.Errorf("wrap: %w", viperblock.ErrStateBackendUnavailable) + } + return nil + }) + require.NoError(t, err) + assert.Equal(t, 3, calls) + assert.Equal(t, 2, sleeps) + }) + + t.Run("not_found_fails_fast", func(t *testing.T) { + calls := 0 + sleeps := 0 + err := retryLoadState("vol-x", 5, 10*time.Millisecond, + func(time.Duration) { sleeps++ }, + func() error { calls++; return viperblock.ErrStateNotFound }) + require.Error(t, err) + assert.ErrorIs(t, err, viperblock.ErrStateNotFound) + assert.Equal(t, 1, calls, "ErrStateNotFound must not be retried") + assert.Equal(t, 0, sleeps) + }) + + t.Run("unclassified_fails_fast", func(t *testing.T) { + calls := 0 + sentinel := errors.New("permission denied") + err := retryLoadState("vol-x", 5, 10*time.Millisecond, + func(time.Duration) {}, + func() error { calls++; return sentinel }) + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) + assert.Equal(t, 1, calls) + }) + + t.Run("exhausts_budget_on_persistent_transient", func(t *testing.T) { + calls := 0 + sleeps := 0 + var lastDelay time.Duration + err := retryLoadState("vol-x", 4, 100*time.Millisecond, + func(d time.Duration) { sleeps++; lastDelay = d }, + func() error { calls++; return viperblock.ErrStateBackendUnavailable }) + require.Error(t, err) + assert.ErrorIs(t, err, viperblock.ErrStateBackendUnavailable) + assert.Equal(t, 4, calls) + assert.Equal(t, 3, sleeps, "sleep happens between attempts, not after the final one") + // Backoff multiplier is 1.5: 100ms, 150ms, 225ms. + assert.Equal(t, 225*time.Millisecond, lastDelay) + }) +}