diff --git a/go.mod b/go.mod index da606aee1..4129b606d 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,7 @@ require ( github.com/mulgadc/bluebottle v1.17.0 github.com/mulgadc/northstar v1.17.0 github.com/mulgadc/predastore v1.17.0 - github.com/mulgadc/viperblock v1.17.0 + github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481 github.com/nats-io/nats-server/v2 v2.14.5 github.com/nats-io/nats.go v1.53.1 github.com/opencontainers/runtime-spec v1.3.0 diff --git a/go.sum b/go.sum index c58f70c1b..0d14efa9d 100644 --- a/go.sum +++ b/go.sum @@ -1335,6 +1335,8 @@ github.com/mulgadc/predastore v1.17.0 h1:1xMFrzQU7dBaC5Q6qiAJx5ipj/yDubjtOJY7rjp github.com/mulgadc/predastore v1.17.0/go.mod h1:RM3hLSVsJAfwPJnmqbmcDgfdnP/RntXe+r1GOTD+S9c= github.com/mulgadc/viperblock v1.17.0 h1:ybcjcBf10jA1SiUkH5PTGFUZ9ZhGMljBff7iV06Vkg8= github.com/mulgadc/viperblock v1.17.0/go.mod h1:XYRmXrJ+k1P5nCxxTihcBUVM/4Srxc8zkFQp+a7ALNk= +github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481 h1:kWiCPxfLURj6D06CA1rsaF2LP9EpwbJyqqTy/IdCCug= +github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481/go.mod h1:XYRmXrJ+k1P5nCxxTihcBUVM/4Srxc8zkFQp+a7ALNk= 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/provider_handlers.go b/spinifex/services/viperblockd/provider_handlers.go index 75c588185..7289f180c 100644 --- a/spinifex/services/viperblockd/provider_handlers.go +++ b/spinifex/services/viperblockd/provider_handlers.go @@ -1161,7 +1161,7 @@ func snapshotVolumeEngine(ctx context.Context, cfg *Config, volumeID, snapshotID if err := vb.Backend.InitCtx(ctx); err != nil { return nil, fmt.Errorf("backend init: %w", err) } - if err := loadStateWithRetry(ctx, vb, volumeID); err != nil { + if err := loadStateWithRetry(ctx, cfg, vb, volumeID); err != nil { return nil, fmt.Errorf("load state: %w", err) } if err := checkSnapshotIDFree(vb, volumeID, snapshotID); err != nil { @@ -1314,7 +1314,7 @@ func constructMountedVB(ctx context.Context, cfg *Config, volumeName string) (*v } // Retry on transient backend errors so daemon recovery doesn't tip a healthy volume into cleanup. - if err := loadStateWithRetry(ctx, vb, volumeName); err != nil { + if err := loadStateWithRetry(ctx, cfg, vb, volumeName); err != nil { return nil, 0, err } diff --git a/spinifex/services/viperblockd/viperblockd.go b/spinifex/services/viperblockd/viperblockd.go index 8ff7fe1f2..af3719be4 100644 --- a/spinifex/services/viperblockd/viperblockd.go +++ b/spinifex/services/viperblockd/viperblockd.go @@ -40,11 +40,11 @@ func endSpanWithResponseError(span trace.Span, respErr string) { span.End() } -// loadStateRetryAttempts / loadStateRetryBaseDelay tune the mount-time retry -// loop (5 attempts at 200ms * 1.5^n ≈ 3.7s; well under the 30s NATS timeout). +// The default mount retry budget is five attempts over about 1.6 seconds of +// backoff. Config fields override these values for isolated tests. const ( - loadStateRetryAttempts = 5 - loadStateRetryBaseDelay = 200 * time.Millisecond + defaultLoadStateRetryAttempts = 5 + defaultLoadStateRetryBaseDelay = 200 * time.Millisecond ) // retryLoadState invokes loadFn with exponential backoff (delay * 3/2 each step) @@ -75,13 +75,12 @@ func retryLoadState(volume string, attempts int, baseDelay time.Duration, sleep return fmt.Errorf("LoadState exhausted %d retries: %w", attempts, err) } -// loadStateWithRetry calls vb.LoadStateCtx with the production retry budget -// (loadStateRetryAttempts / loadStateRetryBaseDelay). The context matters -// beyond cancellation: the S3 backend only emits a span per request when the -// request carries one, so the no-context variant makes this time invisible. -func loadStateWithRetry(ctx context.Context, vb *viperblock.VB, volume string) error { +// loadStateWithRetry keeps the caller's context on every backend request so +// cancellation and tracing span all attempts. +func loadStateWithRetry(ctx context.Context, cfg *Config, vb *viperblock.VB, volume string) error { + attempts, baseDelay := cfg.loadStateRetryPolicy() load := func() error { return vb.LoadStateCtx(ctx) } - return retryLoadState(volume, loadStateRetryAttempts, loadStateRetryBaseDelay, time.Sleep, load) + return retryLoadState(volume, attempts, baseDelay, time.Sleep, load) } var serviceName = "viperblock" @@ -160,11 +159,15 @@ type Config struct { masterKey *masterkey.Key // s3HTTPClient is shared by every viperblock backend this service builds. - // One client per volume would be one connection pool per volume, so every - // engine open and state read would pay its own TLS handshake. + // One client per volume would create a connection pool per volume. s3HTTPClientOnce sync.Once s3HTTPClient *http.Client + // Per-service retry knobs avoid mutable package state in tests. Zero values + // select the production defaults. + loadStateRetryAttempts int + loadStateRetryBaseDelay time.Duration + // sealVolume overrides how a detached volume is sealed to predastore. // Nil means sealVolumeVB, the real seal. Tests that need a seal to FAIL // inject here rather than pointing S3Host at an unreachable endpoint: the @@ -219,6 +222,18 @@ func (cfg *Config) buildVB(ctx context.Context, volumeName string) (*viperblock. return vb, cacheSize, lease, nil } +func (cfg *Config) loadStateRetryPolicy() (int, time.Duration) { + attempts := cfg.loadStateRetryAttempts + if attempts <= 0 { + attempts = defaultLoadStateRetryAttempts + } + baseDelay := cfg.loadStateRetryBaseDelay + if baseDelay <= 0 { + baseDelay = defaultLoadStateRetryBaseDelay + } + return attempts, baseDelay +} + type Service struct { Config *Config } @@ -331,7 +346,8 @@ func readVolumeState(ctx context.Context, cfg *Config, volumeName string) (viper state, rerr = vb.ReadStateCtx(ctx) return rerr } - if err := retryLoadState(volumeName, loadStateRetryAttempts, loadStateRetryBaseDelay, time.Sleep, read); err != nil { + attempts, baseDelay := cfg.loadStateRetryPolicy() + if err := retryLoadState(volumeName, attempts, baseDelay, time.Sleep, read); err != nil { return viperblock.VBState{}, fmt.Errorf("read state: %w", err) } return state, nil @@ -373,7 +389,7 @@ func openVolumeVB(ctx context.Context, cfg *Config, volumeName string) (*viperbl if err := vb.Backend.InitCtx(ctx); err != nil { return nil, nil, fmt.Errorf("backend init: %w", err) } - if err := loadStateWithRetry(ctx, vb, volumeName); err != nil { + if err := loadStateWithRetry(ctx, cfg, vb, volumeName); err != nil { return nil, nil, fmt.Errorf("load state: %w", err) } diff --git a/spinifex/services/viperblockd/viperblockd_test.go b/spinifex/services/viperblockd/viperblockd_test.go index eb21fc969..af62a7d6c 100644 --- a/spinifex/services/viperblockd/viperblockd_test.go +++ b/spinifex/services/viperblockd/viperblockd_test.go @@ -1,14 +1,19 @@ package viperblockd import ( + "context" "errors" "fmt" + "net/http" + "net/http/httptest" "os" "path/filepath" "sync" + "sync/atomic" "testing" "time" + vbtypes "github.com/mulgadc/viperblock/types" "github.com/mulgadc/viperblock/viperblock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -446,6 +451,46 @@ func TestMountedVolumePortRange(t *testing.T) { } } +func TestConfigLoadStateRetryPolicy(t *testing.T) { + attempts, delay := (&Config{}).loadStateRetryPolicy() + assert.Equal(t, defaultLoadStateRetryAttempts, attempts) + assert.Equal(t, defaultLoadStateRetryBaseDelay, delay) + + cfg := &Config{loadStateRetryAttempts: 2, loadStateRetryBaseDelay: time.Nanosecond} + attempts, delay = cfg.loadStateRetryPolicy() + assert.Equal(t, 2, attempts) + assert.Equal(t, time.Nanosecond, delay) +} + +// TestReadVolumeState_BadRequestFailsFast covers all three retry layers with +// the production budgets: one HTTP 400 must produce one prompt request. +func TestReadVolumeState_BadRequestFailsFast(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusBadRequest) + })) + t.Cleanup(srv.Close) + + cfg := &Config{ + S3Host: srv.URL, + Bucket: "bucket", + Region: "us-east-1", + AccessKey: "access-key", + SecretKey: "secret-key", + BaseDir: t.TempDir(), + } + + start := time.Now() + _, err := readVolumeState(context.Background(), cfg, "vol-rejected") + elapsed := time.Since(start) + + require.Error(t, err) + assert.ErrorIs(t, err, vbtypes.ErrBackendNonRetryable) + assert.Equal(t, int32(1), requests.Load()) + assert.Less(t, elapsed, time.Second) +} + // TestRetryLoadState pins the retry policy: transient errors retry with backoff, // ErrStateNotFound fails fast, and the retry budget is honoured. func TestRetryLoadState(t *testing.T) {