diff --git a/core/metadata/snapshot.go b/core/metadata/snapshot.go index d22cc36169..132de60ec7 100644 --- a/core/metadata/snapshot.go +++ b/core/metadata/snapshot.go @@ -31,7 +31,6 @@ import ( "github.com/containerd/containerd/v2/pkg/filters" "github.com/containerd/containerd/v2/pkg/labels" "github.com/containerd/containerd/v2/pkg/namespaces" - "github.com/containerd/containerd/v2/pkg/timeout" "github.com/containerd/errdefs" "github.com/containerd/log" bolt "go.etcd.io/bbolt" @@ -42,20 +41,6 @@ const ( inheritedLabelsPrefix = "containerd.io/snapshot/" ) -// gcSnapshotterRemoveTimeoutKey bounds each Snapshotter.Remove during garbage -// collection, which holds the snapshotter write lock: an unresponsive -// snapshotter (e.g. a hung proxy plugin RPC) would otherwise block all -// snapshot operations forever. On timeout the rest of the pass is abandoned; -// remaining snapshots stay orphaned and are retried on the next GC pass. -const ( - gcSnapshotterRemoveTimeoutKey = "io.containerd.timeout.gc.snapshotter.remove" - defaultGCSnapshotterRemoveTimeout = 30 * time.Minute -) - -func init() { - timeout.Set(gcSnapshotterRemoveTimeoutKey, defaultGCSnapshotterRemoveTimeout) -} - type snapshotter struct { snapshots.Snapshotter name string @@ -1004,10 +989,7 @@ func (s *snapshotter) pruneBranch(ctx context.Context, node *treeNode) error { if node.remove { logger := log.G(ctx).WithField("snapshotter", s.name) - removeCtx, cancel := timeout.WithContext(ctx, gcSnapshotterRemoveTimeoutKey) - err := s.Snapshotter.Remove(removeCtx, node.info.Name) - cancel() - if err != nil { + if err := s.Snapshotter.Remove(ctx, node.info.Name); err != nil { if !errdefs.IsFailedPrecondition(err) { return err } diff --git a/core/metadata/snapshot_test.go b/core/metadata/snapshot_test.go index f82a277bc5..ae818aebed 100644 --- a/core/metadata/snapshot_test.go +++ b/core/metadata/snapshot_test.go @@ -18,14 +18,11 @@ package metadata import ( "context" - "errors" "fmt" "maps" "reflect" "sync" - "sync/atomic" "testing" - "testing/synctest" "time" "github.com/containerd/containerd/v2/core/leases" @@ -460,94 +457,3 @@ func (s *tmpSnapshotter) Walk(ctx context.Context, fn snapshots.WalkFunc, fs ... func (s *tmpSnapshotter) Close() error { return nil } - -type hangingRemoveSnapshotter struct { - snapshots.Snapshotter - removes atomic.Int32 - cleanups atomic.Int32 -} - -func (s *hangingRemoveSnapshotter) Remove(ctx context.Context, key string) error { - s.removes.Add(1) - <-ctx.Done() - return ctx.Err() -} - -// Cleanup makes the wrapper a snapshots.Cleaner so the test can assert GC does -// not call it after abandoning a pass. This stub returns immediately; a real -// wedged snapshotter would hang here just like it does in Remove. -func (s *hangingRemoveSnapshotter) Cleanup(ctx context.Context) error { - s.cleanups.Add(1) - return nil -} - -func TestGarbageCollectHangingRemove(t *testing.T) { - synctest.Test(t, func(t *testing.T) { - ctx, bdb := testEnv(t) - - sn := &hangingRemoveSnapshotter{Snapshotter: NewTmpSnapshotter()} - db := NewDB(bdb, nil, map[string]snapshots.Snapshotter{"tmp": sn}) - if err := db.Init(ctx); err != nil { - t.Fatal(err) - } - - // Orphan snapshots: exist in the backend but not in metadata, so GC removes them. - for _, key := range []string{"orphan-1", "orphan-2"} { - if _, err := sn.Snapshotter.Prepare(ctx, key, ""); err != nil { - t.Fatal(err) - } - } - - // The fake clock reaches the removal deadline as soon as Remove blocks, - // so this runs against the real default instead of a shortened one. An - // unbounded Remove would deadlock the bubble rather than pass. - msn := db.Snapshotter("tmp").(*snapshotter) - _, err := msn.garbageCollect(ctx) - if !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("expected the bounded Remove to fail with DeadlineExceeded, got %v", err) - } - - // The pass is abandoned after the first timed-out removal. - if n := sn.removes.Load(); n != 1 { - t.Fatalf("expected 1 remove attempt before abandoning the pass, got %d", n) - } - // Cleanup must be skipped after an abandoned pass: the snapshotter just - // failed to answer a bounded Remove, and Cleanup has no bound. - if n := sn.cleanups.Load(); n != 0 { - t.Fatalf("expected no Cleanup call after abandoned pass, got %d", n) - } - // Skipped snapshots stay orphaned for the next GC pass. - for _, key := range []string{"orphan-1", "orphan-2"} { - if _, err := sn.Snapshotter.Stat(ctx, key); err != nil { - t.Fatalf("orphan %q should survive for the next GC pass: %v", key, err) - } - } - }) -} - -type failingRemoveSnapshotter struct { - snapshots.Snapshotter -} - -func (s *failingRemoveSnapshotter) Remove(ctx context.Context, key string) error { - return errdefs.ErrFailedPrecondition -} - -func TestGarbageCollectFailedPreconditionRemove(t *testing.T) { - ctx, bdb := testEnv(t) - - sn := &failingRemoveSnapshotter{Snapshotter: NewTmpSnapshotter()} - db := NewDB(bdb, nil, map[string]snapshots.Snapshotter{"tmp": sn}) - if err := db.Init(ctx); err != nil { - t.Fatal(err) - } - - if _, err := sn.Snapshotter.Prepare(ctx, "orphan", ""); err != nil { - t.Fatal(err) - } - - msn := db.Snapshotter("tmp").(*snapshotter) - if _, err := msn.garbageCollect(ctx); err != nil { - t.Fatalf("FailedPrecondition on Remove must not fail garbage collection: %v", err) - } -} diff --git a/docs/man/containerd-config.toml.5.md b/docs/man/containerd-config.toml.5.md index 79e39d9390..d5f86acbf5 100644 --- a/docs/man/containerd-config.toml.5.md +++ b/docs/man/containerd-config.toml.5.md @@ -178,7 +178,6 @@ documentation. "io.containerd.timeout.shim.cleanup" = "5s" "io.containerd.timeout.shim.load" = "5s" "io.containerd.timeout.shim.shutdown" = "3s" - "io.containerd.timeout.gc.snapshotter.remove" = "30m" "io.containerd.timeout.task.state" = "2s" --> **imports**