diff --git a/weed/admin/dash/admin_server.go b/weed/admin/dash/admin_server.go index ab1e7cd26..97f443ed7 100644 --- a/weed/admin/dash/admin_server.go +++ b/weed/admin/dash/admin_server.go @@ -1233,14 +1233,8 @@ func (s *AdminServer) DeleteS3Bucket(bucketName string) error { // Then delete bucket directory recursively from filer // Use same parameters as s3.bucket.delete shell command and S3 API return s.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: filerConfig.BucketsPath, - Name: bucketName, - IsDeleteData: false, // Collection already deleted, just remove metadata - IsRecursive: true, - IgnoreRecursiveError: true, // Same as S3 API and shell command - }) - if err != nil { + // The collection is already gone, so this only has to drop the metadata. + if err := filer_pb.DoRemove(ctx, client, filerConfig.BucketsPath, bucketName, false, true, true, false, nil); err != nil { return fmt.Errorf("failed to delete bucket: %w", err) } diff --git a/weed/admin/dash/topic_retention.go b/weed/admin/dash/topic_retention.go index 1a9140368..22e118184 100644 --- a/weed/admin/dash/topic_retention.go +++ b/weed/admin/dash/topic_retention.go @@ -277,11 +277,7 @@ func (p *TopicRetentionPurger) deleteDirectoryRecursively(client filer_pb.Seawee } } else { // Delete file - _, err = client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: dirPath, - Name: resp.Entry.Name, - }) - if err != nil { + if err := filer_pb.DoRemove(context.Background(), client, dirPath, resp.Entry.Name, false, false, false, false, nil); err != nil { return fmt.Errorf("failed to delete file %s: %v", entryPath, err) } } @@ -291,11 +287,7 @@ func (p *TopicRetentionPurger) deleteDirectoryRecursively(client filer_pb.Seawee parentDir := path.Dir(dirPath) dirName := path.Base(dirPath) - _, err = client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: parentDir, - Name: dirName, - }) - if err != nil { + if err := filer_pb.DoRemove(context.Background(), client, parentDir, dirName, false, false, false, false, nil); err != nil { return fmt.Errorf("failed to delete directory %s: %v", dirPath, err) } diff --git a/weed/admin/handlers/file_browser_handlers.go b/weed/admin/handlers/file_browser_handlers.go index 929f55687..15d7a24b0 100644 --- a/weed/admin/handlers/file_browser_handlers.go +++ b/weed/admin/handlers/file_browser_handlers.go @@ -109,14 +109,7 @@ func (h *FileBrowserHandlers) DeleteFile(w http.ResponseWriter, r *http.Request) // Delete file via filer err := h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: path.Dir(request.Path), - Name: path.Base(request.Path), - IsDeleteData: true, - IsRecursive: true, - IgnoreRecursiveError: false, - }) - return err + return filer_pb.DoRemove(context.Background(), client, path.Dir(request.Path), path.Base(request.Path), true, true, false, false, nil) }) if err != nil { writeJSONError(w, http.StatusInternalServerError, "Failed to delete file: "+err.Error()) @@ -156,14 +149,7 @@ func (h *FileBrowserHandlers) DeleteMultipleFiles(w http.ResponseWriter, r *http // Delete each file/folder for _, p := range request.Paths { err := h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: path.Dir(p), - Name: path.Base(p), - IsDeleteData: true, - IsRecursive: true, - IgnoreRecursiveError: false, - }) - return err + return filer_pb.DoRemove(context.Background(), client, path.Dir(p), path.Base(p), true, true, false, false, nil) }) if err != nil { diff --git a/weed/credential/filer_etc/filer_etc_identity.go b/weed/credential/filer_etc/filer_etc_identity.go index 518b299b5..b2a83ec5c 100644 --- a/weed/credential/filer_etc/filer_etc_identity.go +++ b/weed/credential/filer_etc/filer_etc_identity.go @@ -209,10 +209,7 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p for _, entry := range entries { if !entry.IsDirectory && !validNames[entry.Name] { // Delete obsolete identity file - if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: entry.Name, - }); err != nil { + if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil { glog.Warningf("Failed to delete obsolete identity file %s: %v", entry.Name, err) } } @@ -240,10 +237,7 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p for _, entry := range entries { if !entry.IsDirectory && !validNames[entry.Name] { - if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: entry.Name, - }); err != nil { + if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil { glog.Warningf("Failed to delete obsolete service account file %s: %v", entry.Name, err) } } @@ -271,14 +265,8 @@ func (store *FilerEtcStore) SaveConfiguration(ctx context.Context, config *iam_p for _, entry := range entries { if !entry.IsDirectory && !validNames[entry.Name] { - resp, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: entry.Name, - }) - if err != nil { + if err := filer_pb.DoRemove(ctx, client, dir, entry.Name, false, false, false, false, nil); err != nil { glog.Warningf("Failed to delete obsolete group file %s: %v", entry.Name, err) - } else if resp != nil && resp.Error != "" { - glog.Warningf("Failed to delete obsolete group file %s: %s", entry.Name, resp.Error) } } } @@ -341,7 +329,7 @@ func (store *FilerEtcStore) DeleteUser(ctx context.Context, username string) err } return store.withFilerClient(func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ + resp, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ Directory: filer.IamConfigDirectory + "/" + IamIdentitiesDirectory, Name: username + ".json", }) @@ -351,6 +339,12 @@ func (store *FilerEtcStore) DeleteUser(ctx context.Context, username string) err } return err } + if resp != nil && resp.Error != "" { + if strings.Contains(resp.Error, filer_pb.ErrNotFound.Error()) { + return credential.ErrUserNotFound + } + return fmt.Errorf("delete user %s: %s", username, resp.Error) + } return nil }) } diff --git a/weed/credential/filer_etc/filer_etc_policy.go b/weed/credential/filer_etc/filer_etc_policy.go index 9bf2e98ff..00dbe3540 100644 --- a/weed/credential/filer_etc/filer_etc_policy.go +++ b/weed/credential/filer_etc/filer_etc_policy.go @@ -263,14 +263,7 @@ func (store *FilerEtcStore) DeletePolicy(ctx context.Context, name string) error } if err := store.withFilerClient(func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: filer.IamConfigDirectory + "/" + IamPoliciesDirectory, - Name: name + ".json", - }) - if err != nil && !strings.Contains(err.Error(), filer_pb.ErrNotFound.Error()) { - return err - } - return nil + return filer_pb.DoRemove(ctx, client, filer.IamConfigDirectory+"/"+IamPoliciesDirectory, name+".json", false, false, false, false, nil) }); err != nil { return err } diff --git a/weed/iam/integration/session_revocation.go b/weed/iam/integration/session_revocation.go index 4b584de9a..5faffefa5 100644 --- a/weed/iam/integration/session_revocation.go +++ b/weed/iam/integration/session_revocation.go @@ -235,11 +235,7 @@ func (f *FilerSessionRevocationStore) Purge(ctx context.Context, filerAddress st if entry.ExpiresAt.IsZero() || entry.ExpiresAt.After(before) { continue } - if _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: f.basePath, - Name: resp.Entry.Name, - IsDeleteData: true, - }); err == nil { + if err := filer_pb.DoRemove(ctx, client, f.basePath, resp.Entry.Name, true, false, false, false, nil); err == nil { count++ } } diff --git a/weed/mount/weedfs_stream_helpers.go b/weed/mount/weedfs_stream_helpers.go index ae4b25c48..1498df4de 100644 --- a/weed/mount/weedfs_stream_helpers.go +++ b/weed/mount/weedfs_stream_helpers.go @@ -3,6 +3,7 @@ package mount import ( "context" "errors" + "syscall" "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" @@ -60,7 +61,17 @@ func (wfs *WFS) streamDeleteEntry(ctx context.Context, req *filer_pb.DeleteEntry err := wfs.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { var err error resp, err = client.DeleteEntry(ctx, req) - return err + if err != nil { + return err + } + // A refused delete arrives in the response, as the stream branch above. + if resp.Error != "" { + return &streamMutateError{msg: resp.Error, errno: syscall.EIO} + } + return nil }) - return resp, err + if err != nil { + return nil, err + } + return resp, nil } diff --git a/weed/mq/kafka/consumer_offset/filer_storage.go b/weed/mq/kafka/consumer_offset/filer_storage.go index 967982683..19735d0f4 100644 --- a/weed/mq/kafka/consumer_offset/filer_storage.go +++ b/weed/mq/kafka/consumer_offset/filer_storage.go @@ -296,13 +296,6 @@ func (f *FilerStorage) deleteDirectory(path string) error { dir, name := fullPath.DirAndName() return f.fca.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - _, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: name, - IsDeleteData: true, - IsRecursive: true, - IgnoreRecursiveError: true, - }) - return err + return filer_pb.DoRemove(context.Background(), client, dir, name, true, true, true, false, nil) }) } diff --git a/weed/mq/kafka/gateway/coordinator_registry.go b/weed/mq/kafka/gateway/coordinator_registry.go index 152157463..278279b70 100644 --- a/weed/mq/kafka/gateway/coordinator_registry.go +++ b/weed/mq/kafka/gateway/coordinator_registry.go @@ -751,12 +751,7 @@ func (cr *CoordinatorRegistry) deleteCoordinatorAssignment(consumerGroup string) fileName := fmt.Sprintf("%s_assignments.json", consumerGroup) filePath := fmt.Sprintf("%s/%s", CoordinatorAssignmentsDir, fileName) - _, err := client.DeleteEntry(context.Background(), &filer_pb.DeleteEntryRequest{ - Directory: CoordinatorAssignmentsDir, - Name: fileName, - }) - - if err != nil { + if err := filer_pb.DoRemove(context.Background(), client, CoordinatorAssignmentsDir, fileName, false, false, false, false, nil); err != nil { return fmt.Errorf("failed to delete assignment file %s: %w", filePath, err) } diff --git a/weed/s3api/s3tables/filer_ops.go b/weed/s3api/s3tables/filer_ops.go index c67b60c36..87ead2af4 100644 --- a/weed/s3api/s3tables/filer_ops.go +++ b/weed/s3api/s3tables/filer_ops.go @@ -295,17 +295,9 @@ func (h *S3TablesHandler) deleteExtendedAttribute(ctx context.Context, client fi } // deleteDirectory deletes a directory and all its contents -// Note: DeleteEntry RPC response doesn't have an Error field, so we only check the RPC err func (h *S3TablesHandler) deleteDirectory(ctx context.Context, client filer_pb.SeaweedFilerClient, path string) error { dir, name := splitPath(path) - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: name, - IsDeleteData: true, - IsRecursive: true, - IgnoreRecursiveError: true, - }) - return err + return filer_pb.DoRemove(ctx, client, dir, name, true, true, true, false, nil) } // entryExists checks if an entry exists at the given path diff --git a/weed/s3api/s3tables/filer_ops_test.go b/weed/s3api/s3tables/filer_ops_test.go new file mode 100644 index 000000000..22f3e0c8f --- /dev/null +++ b/weed/s3api/s3tables/filer_ops_test.go @@ -0,0 +1,102 @@ +package s3tables + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// deleteEntryStub answers DeleteEntry the way the filer does: a rejected delete +// comes back as a nil transport error with the reason in the response. +type deleteEntryStub struct { + filer_pb.SeaweedFilerClient + resp *filer_pb.DeleteEntryResponse + err error + req *filer_pb.DeleteEntryRequest +} + +func (s *deleteEntryStub) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest, _ ...grpc.CallOption) (*filer_pb.DeleteEntryResponse, error) { + s.req = req + return s.resp, s.err +} + +func TestDeleteDirectoryReportsRejectedDelete(t *testing.T) { + stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{Error: "fail to delete non-empty folder"}} + + err := (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t") + + require.Error(t, err, "a delete the filer rejected must not be reported as success") + assert.Contains(t, err.Error(), "fail to delete non-empty folder") +} + +func TestDeleteDirectoryToleratesMissingEntry(t *testing.T) { + t.Run("reported in the response", func(t *testing.T) { + stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{Error: filer_pb.ErrNotFound.Error()}} + assert.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t")) + }) + t.Run("reported as a transport error", func(t *testing.T) { + stub := &deleteEntryStub{err: status.Error(codes.NotFound, filer_pb.ErrNotFound.Error())} + assert.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t")) + }) +} + +func TestDeleteDirectoryPropagatesTransportError(t *testing.T) { + stub := &deleteEntryStub{err: errors.New("filer unreachable")} + + err := (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t") + + require.Error(t, err) + assert.Contains(t, err.Error(), "filer unreachable") +} + +// deleteDirectory drops a whole subtree, so the delete has to stay recursive and +// take the data with it. +func TestDeleteDirectoryDeletesTheSubtree(t *testing.T) { + stub := &deleteEntryStub{resp: &filer_pb.DeleteEntryResponse{}} + + require.NoError(t, (&S3TablesHandler{}).deleteDirectory(context.Background(), stub, "/buckets/b/ns/t")) + + require.NotNil(t, stub.req) + assert.Equal(t, "/buckets/b/ns", stub.req.Directory) + assert.Equal(t, "t", stub.req.Name) + assert.True(t, stub.req.IsDeleteData) + assert.True(t, stub.req.IsRecursive) + assert.True(t, stub.req.IgnoreRecursiveError) +} + +// The bucket is the directory, so DeleteTableBucket must not answer success +// when the filer refused to remove it. +func TestDeleteTableBucketReportsRejectedDirectoryDelete(t *testing.T) { + fs := s3tablestest.Start(t) + bucketMeta, err := json.Marshal(tableBucketMetadata{Name: renameTestBucket, OwnerAccountID: DefaultAccountID}) + require.NoError(t, err) + fs.Put(TablesPath, renameTestBucket, map[string][]byte{ + ExtendedKeyTableBucket: []byte("{}"), + ExtendedKeyMetadata: bucketMeta, + }) + + bucketPath := GetTableBucketPath(renameTestBucket) + fs.RejectDelete = func(dir, name string) string { + if dir+"/"+name == bucketPath { + return "fail to delete non-empty folder" + } + return "" + } + + m := NewManager() + m.SetTrusted(true) + err = m.Execute(context.Background(), NewManagerClient(fs.Client), "DeleteTableBucket", + &DeleteTableBucketRequest{TableBucketARN: mustBucketARN(t)}, nil, "") + + require.Error(t, err, "a bucket the filer refused to delete must not be reported as deleted") + assert.NotNil(t, fs.Get(TablesPath, renameTestBucket), "the bucket is still there") +} diff --git a/weed/s3api/s3tables/handler_bucket_get_list_delete.go b/weed/s3api/s3tables/handler_bucket_get_list_delete.go index 1cfdea623..c457fd72c 100644 --- a/weed/s3api/s3tables/handler_bucket_get_list_delete.go +++ b/weed/s3api/s3tables/handler_bucket_get_list_delete.go @@ -347,22 +347,13 @@ func (h *S3TablesHandler) handleDeleteTableBucket(w http.ResponseWriter, r *http // Delete the bucket err = filerClient.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - // Delete table object entry first, then directory - // This ensures we clean up the leaf entry even if directory deletion fails - tableObjErr := h.deleteEntryIfExists(r.Context(), client, GetTableObjectBucketPath(bucketName)) - dirErr := h.deleteDirectory(r.Context(), client, bucketPath) - - // Log any errors but don't fail if one succeeds - if tableObjErr != nil && dirErr != nil { - return fmt.Errorf("delete table object failed: %w, delete directory failed: %w", tableObjErr, dirErr) - } - if tableObjErr != nil { + // Drop the leaf entry first, so it does not outlive the directory. + if tableObjErr := h.deleteEntryIfExists(r.Context(), client, GetTableObjectBucketPath(bucketName)); tableObjErr != nil { glog.V(1).Infof("failed to delete table object for %s: %v", bucketName, tableObjErr) } - if dirErr != nil { - glog.V(1).Infof("failed to delete table bucket dir for %s: %v", bucketName, dirErr) - } - return nil + // The bucket is the directory, so a refused delete leaves it in place + // and the caller must hear about it. + return h.deleteDirectory(r.Context(), client, bucketPath) }) if err != nil { diff --git a/weed/s3api/s3tables/s3tablestest/memfiler.go b/weed/s3api/s3tables/s3tablestest/memfiler.go index 1102439a8..5d4540771 100644 --- a/weed/s3api/s3tables/s3tablestest/memfiler.go +++ b/weed/s3api/s3tables/s3tablestest/memfiler.go @@ -34,6 +34,9 @@ type MemFiler struct { // BeforeUpdate runs once, at the start of the next UpdateEntry, so a test // can land a competing write in a handler's read-to-write window. BeforeUpdate func() + // RejectDelete returns the reason DeleteEntry refuses a path, answered the + // way the filer answers one: no transport error, the reason in the response. + RejectDelete func(dir, name string) string } func newMemFiler() *MemFiler { @@ -167,6 +170,11 @@ func (f *MemFiler) UpdateEntry(_ context.Context, req *filer_pb.UpdateEntryReque } func (f *MemFiler) DeleteEntry(_ context.Context, req *filer_pb.DeleteEntryRequest) (*filer_pb.DeleteEntryResponse, error) { + if reject := f.RejectDelete; reject != nil { + if reason := reject(req.Directory, req.Name); reason != "" { + return &filer_pb.DeleteEntryResponse{Error: reason}, nil + } + } f.mu.Lock() defer f.mu.Unlock() if d, ok := f.entries[req.Directory]; ok { diff --git a/weed/shell/command_remote_cache.go b/weed/shell/command_remote_cache.go index ca78f2c2c..69ff9bcfe 100644 --- a/weed/shell/command_remote_cache.go +++ b/weed/shell/command_remote_cache.go @@ -230,14 +230,7 @@ func (c *commandRemoteCache) doComprehensiveSync(commandEnv *CommandEnv, writer fmt.Fprintf(writer, "Deleting %s... ", pathToDelete) dir, name := util.FullPath(pathToDelete).DirAndName() - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: dir, - Name: name, - IgnoreRecursiveError: false, - IsDeleteData: true, - IsRecursive: false, - IsFromOtherCluster: false, - }) + err := filer_pb.DoRemove(ctx, client, dir, name, true, false, false, false, nil) if err != nil { fmt.Fprintf(writer, "failed: %v\n", err) return err diff --git a/weed/shell/command_remote_configure.go b/weed/shell/command_remote_configure.go index 2189a408f..8f754f0e6 100644 --- a/weed/shell/command_remote_configure.go +++ b/weed/shell/command_remote_configure.go @@ -179,16 +179,8 @@ func (c *commandRemoteConfigure) deleteRemoteStorage(commandEnv *CommandEnv, wri return commandEnv.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { - request := &filer_pb.DeleteEntryRequest{ - Directory: filer.DirectoryEtcRemote, - Name: storageName + filer.REMOTE_STORAGE_CONF_SUFFIX, - IgnoreRecursiveError: false, - IsDeleteData: true, - IsRecursive: true, - IsFromOtherCluster: false, - Signatures: nil, - } - _, err := client.DeleteEntry(context.Background(), request) + name := storageName + filer.REMOTE_STORAGE_CONF_SUFFIX + err := filer_pb.DoRemove(context.Background(), client, filer.DirectoryEtcRemote, name, true, true, false, false, nil) if err == nil { fmt.Fprintf(writer, "removed: %s\n", storageName) diff --git a/weed/shell/command_remote_meta_sync.go b/weed/shell/command_remote_meta_sync.go index 2f270b435..b1ce83140 100644 --- a/weed/shell/command_remote_meta_sync.go +++ b/weed/shell/command_remote_meta_sync.go @@ -326,12 +326,7 @@ func deleteRemoteBackedEntry(ctx context.Context, client filer_pb.SeaweedFilerCl } func deleteLocalEntry(ctx context.Context, client filer_pb.SeaweedFilerClient, localDir util.FullPath, name string) error { - _, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{ - Directory: string(localDir), - Name: name, - IsDeleteData: true, - }) - return err + return filer_pb.DoRemove(ctx, client, string(localDir), name, true, false, false, false, nil) } func isLocalDirectoryEmpty(ctx context.Context, client filer_pb.SeaweedFilerClient, dir util.FullPath) (bool, error) {