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) }