diff --git a/CHANGELOG.md b/CHANGELOG.md index a131a3c24..274135c7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,8 @@ Native restore: than one pool no longer hang, stay in progress or change the restored version ([#289](https://github.com/pgsty/silo/issues/289)), and a failed ordinary multipart completion stays usable ([#327](https://github.com/pgsty/silo/issues/327)). + A failed or canceled restore returns at once and clears its marker in the + background, instead of waiting up to a minute for the object lock. Locking, conditional writes and storage: diff --git a/cmd/restore-attempt.go b/cmd/restore-attempt.go index ec2d01ecb..aa144edd9 100644 --- a/cmd/restore-attempt.go +++ b/cmd/restore-attempt.go @@ -43,6 +43,11 @@ const ( // restoreTakeoverWait bounds how long a POST waits for the restore lock // of its version. A running restore holds it for its whole run. restoreTakeoverWait = 2 * time.Second + // restoreResetAttemptTimeout bounds the synchronous cleanup attempt made + // after a restore fails. A caller whose context has ended must not wait for + // another object operation to release the restore lock; the recovery loop + // below retries the cleanup after this attempt gives up. + restoreResetAttemptTimeout = 100 * time.Millisecond // restoreCleanupTimeout bounds each status reset and convergence pass. restoreCleanupTimeout = time.Minute ) @@ -639,6 +644,10 @@ func (z *erasureServerPools) recoverRestoreStatus(bucket, object string, opts Ob func (z *erasureServerPools) resetRestoreStatus(bucket, object string, opts ObjectOptions) error { ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout) defer cancel() + return z.resetRestoreStatusWithContext(ctx, bucket, object, opts) +} + +func (z *erasureServerPools) resetRestoreStatusWithContext(ctx context.Context, bucket, object string, opts ObjectOptions) error { lk := z.restoreLock(bucket, object) lkctx, err := lk.GetLock(ctx, globalOperationTimeout) if err != nil { diff --git a/cmd/restore-native-isolation_test.go b/cmd/restore-native-isolation_test.go index 2690aefea..048d810fd 100644 --- a/cmd/restore-native-isolation_test.go +++ b/cmd/restore-native-isolation_test.go @@ -455,7 +455,13 @@ func TestRestoreRecoversStrandedMarker(t *testing.T) { t.Fatal(lerr) } ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + start := time.Now() err = r.z.RestoreTransitionedObject(ctx, r.bucket, r.object, nativeRestoreOptions(r, oi.VersionID)) + // The canceled caller must not wait for the minute-long + // background cleanup deadline before releasing its lock. + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("canceled restore blocked on marker cleanup for %s", elapsed) + } cancel() lk.Unlock(lkctx) } else { diff --git a/cmd/restore-native_test.go b/cmd/restore-native_test.go index 61733e094..6af9b9be3 100644 --- a/cmd/restore-native_test.go +++ b/cmd/restore-native_test.go @@ -263,7 +263,13 @@ func TestNativeRestoreWithoutOuterLock(t *testing.T) { before := nativeRestoreSnapshot(t, r) ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) defer cancel() + start := time.Now() err = er.RestoreTransitionedObject(ctx, r.bucket, r.object, nativeRestoreOptions(r, oi.VersionID)) + // Failure cleanup must return while this caller still holds the + // object lock, leaving marker recovery to the background retry. + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("restore blocked on failure cleanup for %s", elapsed) + } if err == nil { t.Error("restore wrote through another caller's lock") } diff --git a/cmd/restore-stage.go b/cmd/restore-stage.go index c1c851cad..e9cc76a52 100644 --- a/cmd/restore-stage.go +++ b/cmd/restore-stage.go @@ -793,22 +793,47 @@ func (er erasureObjects) RestoreTransitionedObject(ctx context.Context, bucket, return lkctx, func() { lk.Unlock(lkctx) }, nil } clearMarker := func() { - ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout) - defer cancel() - lkctx, unlock, err := lock(ctx) - if err != nil { - storageLogIf(ctx, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, opts.VersionID, err)) + tryClear := func(ctx context.Context) error { + lkctx, unlock, err := lock(ctx) + if err != nil { + return err + } + defer unlock() + versionID := opts.VersionID + if a := opts.Transition.attempt; a != nil { + versionID = a.VersionID + } + err = er.clearRestoreStatus(lkctx.Context(), bucket, object, versionID, opts.Transition.attempt) + if isErrObjectNotFound(err) || isErrVersionNotFound(err) || isErrMethodNotAllowed(err) { + return nil + } + return err + } + + ctx, cancel := context.WithTimeout(GlobalContext, restoreResetAttemptTimeout) + err := tryClear(ctx) + cancel() + if err == nil { return } - defer unlock() - versionID := opts.VersionID - if a := opts.Transition.attempt; a != nil { - versionID = a.VersionID - } - if err := er.clearRestoreStatus(lkctx.Context(), bucket, object, versionID, opts.Transition.attempt); err != nil && - !isErrObjectNotFound(err) && !isErrVersionNotFound(err) && !isErrMethodNotAllowed(err) { - storageLogIf(ctx, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, versionID, err)) - } + + go func() { + var lastErr error + for _, delay := range restoreRecoveryDelays { + select { + case <-GlobalContext.Done(): + return + case <-time.After(delay): + } + ctx, cancel := context.WithTimeout(GlobalContext, restoreCleanupTimeout) + lastErr = tryClear(ctx) + cancel() + if lastErr == nil { + return + } + } + storageLogIf(GlobalContext, fmt.Errorf("Unable to clear the restore status of %s/%s(%s): %w", bucket, object, opts.VersionID, lastErr)) + }() } return er.restoreVersion(ctx, bucket, object, opts, restoreHooks{lock: lock, clear: clearMarker}) } @@ -835,7 +860,10 @@ func (z *erasureServerPools) RestoreTransitionedObject(ctx context.Context, buck opts.SkipDecommissioned = true opts.NoLock = true clearMarker := func() { - if err := z.resetRestoreStatus(bucket, object, opts); err != nil { + ctx, cancel := context.WithTimeout(GlobalContext, restoreResetAttemptTimeout) + err := z.resetRestoreStatusWithContext(ctx, bucket, object, opts) + cancel() + if err != nil { z.recoverRestoreStatus(bucket, object, opts) } }