From 11791fad6a9c39f4c71224f7d068a2b09116605b Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 25 Sep 2026 07:30:42 +0800 Subject: [PATCH] filer: resolve the collection a bucket delete drops (#11439) * filer: resolve the collection a bucket delete drops A bucket delete dropped the collection named after the bucket, which assumes bucket name is collection name. With a collection rule the write path honors, deleting the bucket either orphaned its collection or, when a bucket was named after a shared collection, removed volumes other buckets still write to. Resolve the collection through the same rule chain the write path uses and drop it only when no other bucket resolves there too. A listing failure keeps the collection, the safe side of an unknown. * filer: prove collection exclusivity across all paths before dropping it The sibling-bucket scan missed every non-bucket writer: a broad rule like '/' or '/buckets/', a rule under a surviving bucket, or a rule on an unrelated path can route into the same collection. Check every storage rule's prefix instead, and mirror the grouped gateway's explicit _ collection, which otherwise resolves a rule-named collection the bucket never wrote to. * s3: let the filer own the collection decision on bucket delete Both entry points deleted a name-derived collection around the filer's own resolved delete, bypassing its exclusivity check and wiping sibling data. The filer now resolves the collection a bucket actually used, including the grouped form. * filer: keep a collection the default write route also uses Rule-less writes outside buckets land in the filer's default collection, so a bucket resolving there shares it with them. --- weed/filer/filer_delete_collection_test.go | 206 ++++++++++++++++++++- weed/filer/filer_delete_entry.go | 92 ++++++++- weed/s3api/s3api_bucket_handlers.go | 57 +----- weed/shell/command_s3_bucket_delete.go | 12 -- 4 files changed, 296 insertions(+), 71 deletions(-) diff --git a/weed/filer/filer_delete_collection_test.go b/weed/filer/filer_delete_collection_test.go index ac7458ce5..9701f3871 100644 --- a/weed/filer/filer_delete_collection_test.go +++ b/weed/filer/filer_delete_collection_test.go @@ -13,6 +13,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/cluster" "github.com/seaweedfs/seaweedfs/weed/pb" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" "github.com/seaweedfs/seaweedfs/weed/util" "github.com/seaweedfs/seaweedfs/weed/util/log_buffer" @@ -96,7 +97,7 @@ func newFilerWithFakeMaster(t *testing.T) (*Filer, *hookedStore, *collectionDele mc := wdclient.NewMasterClient( grpc.WithTransportCredentials(insecure.NewCredentials()), - "test", cluster.FilerType, pb.ServerAddress("localhost:0"), "", "", + "", cluster.FilerType, pb.ServerAddress("localhost:0"), "", "", *pb.NewServiceDiscoveryFromMap(map[string]pb.ServerAddress{"m": masterAddress}), ) @@ -181,3 +182,206 @@ func TestDeleteEntryMetaAndDataDeletesCollectionWhenTheRequestIsCancelledMidDele t.Error("the bucket entry survived the delete") } } + +func seedBucket(t *testing.T, store *hookedStore, path util.FullPath) { + t.Helper() + if err := store.InsertEntry(context.Background(), &Entry{ + FullPath: path, + Attr: Attr{Mode: os.ModeDir | 0755}, + }); err != nil { + t.Fatalf("seed bucket %s: %v", path, err) + } +} + +// Two buckets resolving to one collection: deleting either must leave the +// collection for the other. Previously the delete dropped the collection +// named after the bucket regardless of where its data actually lived. +func TestDeleteBucketKeepsSharedCollection(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets", + Collection: "shared", + }) + seedBucket(t, store, util.FullPath("/buckets/a")) + seedBucket(t, store, util.FullPath("/buckets/b")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/a", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("shared collection was deleted: %q", call.name) + default: + } + if store.getEntry("/buckets/b") == nil { + t.Error("the surviving bucket's entry is gone") + } +} + +// A bucket named after a collection other buckets resolve to is still just a +// bucket: deleting it must not take the shared collection down with it. +func TestDeleteBucketNamedAfterSharedCollection(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets", + Collection: "shared", + }) + seedBucket(t, store, util.FullPath("/buckets/shared")) + seedBucket(t, store, util.FullPath("/buckets/other")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/shared", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("collection backing other buckets was deleted: %q", call.name) + default: + } +} + +// A rule pointing a non-bucket path at the same collection keeps it: the +// collection serves files the bucket delete must not orphan. +func TestDeleteBucketKeepsCollectionUsedByNonBucketPath(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/a", + Collection: "cold", + }) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/archives", + Collection: "cold", + }) + seedBucket(t, store, util.FullPath("/buckets/a")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/a", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("collection used by /archives was deleted: %q", call.name) + default: + } +} + +// A broad prefix rule covering the whole tree keeps the collection even for a +// lone bucket: the same collection backs non-bucket paths too. +func TestDeleteBucketKeepsCollectionFromBroadRule(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/", + Collection: "everything", + }) + seedBucket(t, store, util.FullPath("/buckets/a")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/a", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("collection from a / rule was deleted: %q", call.name) + default: + } +} + +// A bucket resolving to the filer's default collection keeps it: rule-less +// writes outside buckets land there too, so it is never one bucket's alone. +func TestDeleteBucketKeepsDefaultCollection(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.metaLogCollection = "everything" + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/a", + Collection: "everything", + }) + seedBucket(t, store, util.FullPath("/buckets/a")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/a", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("the filer's default collection was deleted: %q", call.name) + default: + } +} + +// A rule nested under a surviving bucket keeps the collection: the other +// bucket resolves elsewhere at its root, but objects deeper inside it still +// land in the shared collection. +func TestDeleteBucketKeepsCollectionFromNestedSiblingRule(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/a", + Collection: "shared", + }) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/b/deep", + Collection: "shared", + }) + seedBucket(t, store, util.FullPath("/buckets/a")) + seedBucket(t, store, util.FullPath("/buckets/b")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/a", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + t.Fatalf("collection used under /buckets/b/deep was deleted: %q", call.name) + default: + } +} + +// A grouped gateway writes to _ regardless of the storage +// rules, so that is the collection the delete must drop -- and a rule-named +// collection the bucket never used must survive. +func TestDeleteBucketUnderFilerGroup(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.MasterClient.FilerGroup = "tenant1" + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/photos", + Collection: "archive", + }) + seedBucket(t, store, util.FullPath("/buckets/photos")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/photos", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + if call.name != "tenant1_photos" { + t.Fatalf("CollectionDelete = %q, want %q", call.name, "tenant1_photos") + } + case <-time.After(20 * time.Second): + t.Fatal("CollectionDelete never reached the master") + } +} + +// A collection only the deleted bucket resolves to is dropped under its real +// name, so a dedicated custom collection does not leak its volumes. +func TestDeleteBucketDeletesResolvedCollection(t *testing.T) { + f, store, master := newFilerWithFakeMaster(t) + f.FilerConf.SetLocationConf(&filer_pb.FilerConf_PathConf{ + LocationPrefix: "/buckets/only", + Collection: "custom", + }) + seedBucket(t, store, util.FullPath("/buckets/only")) + + if err := f.DeleteEntryMetaAndData(context.Background(), "/buckets/only", true, false, true, false, nil, 0); err != nil { + t.Fatalf("DeleteEntryMetaAndData: %v", err) + } + + select { + case call := <-master.calls: + if call.name != "custom" { + t.Fatalf("CollectionDelete = %q, want %q", call.name, "custom") + } + case <-time.After(20 * time.Second): + t.Fatal("CollectionDelete never reached the master") + } +} diff --git a/weed/filer/filer_delete_entry.go b/weed/filer/filer_delete_entry.go index 1cc2019df..6c86b2a4f 100644 --- a/weed/filer/filer_delete_entry.go +++ b/weed/filer/filer_delete_entry.go @@ -68,6 +68,10 @@ func (f *Filer) DeleteEntryMetaAndData(ctx context.Context, p util.FullPath, isR return nil } isDeleteCollection := f.IsBucket(entry) + collectionName := "" + if isDeleteCollection { + collectionName = f.bucketCollection(ctx, entry.Name()) + } if entry.IsDirectory() { // delete the folder children, not including the folder itself err = f.doBatchDeleteFolderMetaAndData(ctx, entry, isRecursive, ignoreRecursiveError, shouldDeleteChunks && !isDeleteCollection, isDeleteCollection, isFromOtherCluster, signatures, func(hardLinkIds []HardLinkId) error { @@ -102,17 +106,18 @@ func (f *Filer) DeleteEntryMetaAndData(ctx context.Context, p util.FullPath, isR } if isDeleteCollection { - collectionName := entry.Name() - // the entry is already gone: a caller that hung up must not leave the - // collection behind, so this cleanup outlives the request -- bounded all - // the same, or a master that is down parks this handler indefinitely and - // every client retry behind it parks another - collectionCtx, cancelCollection := context.WithTimeout(context.WithoutCancel(ctx), collectionDeleteTimeout) - f.DoDeleteCollection(collectionCtx, collectionName) - cancelCollection() + if collectionName != "" { + // the entry is already gone: a caller that hung up must not leave the + // collection behind, so this cleanup outlives the request -- bounded all + // the same, or a master that is down parks this handler indefinitely and + // every client retry behind it parks another + collectionCtx, cancelCollection := context.WithTimeout(context.WithoutCancel(ctx), collectionDeleteTimeout) + f.DoDeleteCollection(collectionCtx, collectionName) + cancelCollection() + } // drop bucket-labeled series held by this process; the S3 gateway // only cleans its own registry - stats.DeleteBucketMetrics(collectionName) + stats.DeleteBucketMetrics(entry.Name()) } return nil @@ -221,6 +226,75 @@ func (f *Filer) doDeleteEntryMetaAndData(ctx context.Context, entry *Entry, shou // DeleteCollection, still has retry budget left. const collectionDeleteTimeout = 15 * time.Second +// bucketCollection resolves the collection a bucket's objects land in +// through the same chain the write path uses -- a grouped gateway's explicit +// collection, then the storage rules, then the bucket name -- and reports it +// only when nothing outside the bucket can still route into it. A shared +// collection must survive the bucket delete: dropping it removes volumes +// other paths still write to. A listing failure keeps the collection, the +// safe side of an unknown. +func (f *Filer) bucketCollection(ctx context.Context, bucket string) (collection string) { + bucketDir := f.DirBucketsPath + "/" + bucket + "/" + resolve := func(dir, name string) string { + if f.MasterClient != nil { + if group := f.MasterClient.FilerGroup; group != "" { + return group + "_" + name + } + } + return util.Nvl(f.FilerConf.MatchStorageRule(dir).Collection, name) + } + collection = resolve(bucketDir, bucket) + + // Rule-less writes outside buckets fall back to the filer's default + // collection, so a bucket resolving there shares it with them. + if collection == f.metaLogCollection { + return "" + } + + // A rule whose prefix escapes the bucket can route other paths into the + // same collection, including prefixes nested under surviving buckets. + for _, rule := range f.FilerConf.ToProto().Locations { + prefix := strings.TrimSuffix(rule.LocationPrefix, "/") + "/" + if strings.HasPrefix(prefix, bucketDir) { + continue + } + if f.FilerConf.MatchStorageRule(prefix).Collection == collection { + return "" + } + } + + siblings, err := f.listBuckets(ctx) + if err != nil { + glog.ErrorfCtx(ctx, "list buckets for collection check: %v", err) + return "" + } + for _, sibling := range siblings { + if sibling != bucket && resolve(f.DirBucketsPath+"/"+sibling+"/", sibling) == collection { + return "" + } + } + return collection +} + +func (f *Filer) listBuckets(ctx context.Context) (buckets []string, err error) { + lastFileName := "" + for { + entries, _, listErr := f.ListDirectoryEntries(ctx, util.FullPath(f.DirBucketsPath), lastFileName, false, PaginationSize, "", "", "") + if listErr != nil { + return nil, listErr + } + for _, entry := range entries { + lastFileName = entry.Name() + if f.IsBucket(entry) { + buckets = append(buckets, entry.Name()) + } + } + if len(entries) < PaginationSize { + return buckets, nil + } + } +} + func (f *Filer) DoDeleteCollection(ctx context.Context, collectionName string) (err error) { return f.MasterClient.WithClient(ctx, false, func(client master_pb.SeaweedClient) error { diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 2e970a936..59888e255 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -34,20 +34,12 @@ import ( util_http "github.com/seaweedfs/seaweedfs/weed/util/http" ) -// A bucket creation lists collections, and a bucket deletion deletes one. -// Neither RPC carried a deadline, so a transient failure anywhere down the chain -// -- gateway to filer, filer to master, master to volume server -- held the S3 -// request open until the client gave up on it. Both budgets are taken outside -// the filer failover walk, so they cover the whole walk rather than granting -// each filer a fresh one. -// -// The delete is the shorter of the two: the filer has already spent its own -// budget on this collection, under the bucket entry's delete inside s3a.rm, and -// this call is the follow-up for when that did not happen. -const ( - collectionListTimeout = 15 * time.Second - collectionDeleteTimeout = 10 * time.Second -) +// A bucket creation lists collections. The RPC carried no deadline, so a +// transient failure anywhere down the chain -- gateway to filer, filer to +// master, master to volume server -- held the S3 request open until the client +// gave up on it. The budget is taken outside the filer failover walk, so it +// covers the whole walk rather than granting each filer a fresh one. +const collectionListTimeout = 15 * time.Second func (s3a *S3ApiServer) ListBucketsHandler(w http.ResponseWriter, r *http.Request) { @@ -479,11 +471,8 @@ func (s3a *S3ApiServer) DeleteBucketHandler(w http.ResponseWriter, r *http.Reque } } - // Delete bucket directory first, then collection. This order ensures that if - // collection deletion fails, the bucket directory is already gone, preventing - // the "collection exists but bucket directory missing" inconsistency that blocks - // bucket recreation. An orphaned collection is harmless and will be cleaned up - // or reused when the bucket is recreated. + // The filer resolves and drops the bucket's collection inside the delete; + // it keeps a shared one rather than risk another bucket's volumes. err := s3a.rm(r.Context(), s3a.option.BucketsPath, bucket, false, true) if err != nil { s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) @@ -496,36 +485,6 @@ func (s3a *S3ApiServer) DeleteBucketHandler(w http.ResponseWriter, r *http.Reque } } - // Bounded on a background context: the bucket directory is already gone, so - // this follow-up must survive a client disconnect, but it must not outlive the - // client by an unbounded amount either. - deleteCtx, cancelDelete := context.WithTimeout(context.Background(), collectionDeleteTimeout) - err = s3a.withFilerClient(deleteCtx, false, func(client filer_pb.SeaweedFilerClient) error { - deleteCollectionRequest := &filer_pb.DeleteCollectionRequest{ - Collection: s3a.getCollectionName(bucket), - } - - glog.V(1).Infof("delete collection: %v", deleteCollectionRequest) - if _, err := client.DeleteCollection(deleteCtx, deleteCollectionRequest); err != nil { - return fmt.Errorf("delete collection %s: %v", bucket, err) - } - - return nil - }) - timedOut := deleteCtx.Err() != nil - cancelDelete() - - if err != nil { - // Log but don't fail — the bucket directory is already removed, so the bucket - // is effectively deleted. The orphaned collection will be cleaned up or reused. - if timedOut { - // Our own budget, not a refusal: the master carries on deleting once asked. - glog.Warningf("DeleteBucketHandler: stopped waiting for the collection delete for bucket %s: %v", bucket, err) - } else { - glog.Errorf("DeleteBucketHandler: failed to delete collection for bucket %s: %v", bucket, err) - } - } - // Clean up bucket-related caches, locks, and metrics after successful deletion s3a.invalidateBucketConfigCache(bucket) stats_collect.DeleteBucketMetrics(bucket) diff --git a/weed/shell/command_s3_bucket_delete.go b/weed/shell/command_s3_bucket_delete.go index 19ea4f329..026e671d1 100644 --- a/weed/shell/command_s3_bucket_delete.go +++ b/weed/shell/command_s3_bucket_delete.go @@ -7,7 +7,6 @@ import ( "io" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" - "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_objectlock" ) @@ -65,17 +64,6 @@ func (c *commandS3BucketDelete) Do(args []string, commandEnv *CommandEnv, writer return err } - // delete the collection directly first - err = commandEnv.MasterClient.WithClient(ctx, false, func(client master_pb.SeaweedClient) error { - _, err = client.CollectionDelete(ctx, &master_pb.CollectionDeleteRequest{ - Name: getCollectionName(commandEnv, *bucketName), - }) - return err - }) - if err != nil { - return - } - return filer_pb.Remove(ctx, commandEnv, filerBucketsPath, *bucketName, false, true, true, false, nil) }