diff --git a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go index 22aad0b4b..d7acb84d0 100644 --- a/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go +++ b/weed/filer/empty_folder_cleanup/empty_folder_cleaner.go @@ -239,6 +239,16 @@ func (efc *EmptyFolderCleaner) OnCreateEvent(directory string, entryName string, if efc.cleanupQueue.Remove(directory) { glog.V(3).Infof("EmptyFolderCleaner: cancelled cleanup for %s due to new entry", directory) } + + // A directory that has just been created is a new incarnation, so a cleanup + // queued against the one it replaces would delete it rather than the folder + // that was found empty. + if isDirectory { + recreated := string(util.NewFullPath(directory, entryName)) + if efc.cleanupQueue.Remove(recreated) { + glog.V(3).Infof("EmptyFolderCleaner: cancelled cleanup for %s, recreated", recreated) + } + } } // cleanupProcessor runs in background and processes the cleanup queue @@ -529,6 +539,17 @@ func (efc *EmptyFolderCleaner) executeCleanup(folder string, triggeredBy string) return } + // An S3 Tables catalog entry - a namespace, a table whose files a rename left + // behind at the old path, a view that never had any - is the directory itself, + // so its being empty says nothing about whether the catalog still names it. + if extended, err := efc.filer.GetEntryAttributes(ctx, util.FullPath(folder)); err != nil { + glog.V(2).Infof("EmptyFolderCleaner: error reading attributes of %s: %v", folder, err) + return + } else if isCatalogEntry(extended) { + glog.V(3).Infof("EmptyFolderCleaner: skipping %s (triggered by %s), s3tables catalog entry", folder, triggeredBy) + return + } + // Observe it before the delete rather than after. A delete can fail partway and // still leave the folder gone - the redis stores remove the folder before their // parent-list member - so a failure return is not proof that it is still there. @@ -617,6 +638,16 @@ func (efc *EmptyFolderCleaner) getBucketCleanupPolicy(ctx context.Context, folde return bucketPath, autoRemove, "filer", attrValue, nil } +// isCatalogEntry reports whether the directory is an s3tables catalog record. +func isCatalogEntry(attrs map[string][]byte) bool { + for key := range attrs { + if strings.HasPrefix(key, s3_constants.ExtS3TablesPrefix) { + return true + } + } + return false +} + func autoRemoveEmptyFoldersEnabled(attrs map[string][]byte) (bool, string) { if attrs == nil { return true, "" diff --git a/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go b/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go index d37305b5e..e163aba13 100644 --- a/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go +++ b/weed/filer/empty_folder_cleanup/empty_folder_cleaner_test.go @@ -1141,6 +1141,85 @@ func TestEmptyFolderCleaner_executeCleanup_directoryMarker(t *testing.T) { } } +func TestEmptyFolderCleaner_OnCreateEvent_cancelsCleanupForRecreatedDirectory(t *testing.T) { + lockRing := lock_manager.NewLockRing(5 * time.Second) + lockRing.SetSnapshot([]pb.ServerAddress{"filer1:8888"}, 0) + + cleaner := &EmptyFolderCleaner{ + filer: &mockFilerOps{}, + lockRing: lockRing, + host: "filer1:8888", + bucketPath: "/buckets", + enabled: true, + folderCounts: make(map[string]*folderState), + deleted: make(map[string]*deletedFolder), + cleanupQueue: NewCleanupQueue(1000, time.Minute), + maxCountCheck: 1000, + cacheExpiry: time.Minute, + processorSleep: time.Second, + stopCh: make(chan struct{}), + } + + // Dropping the table queues the folder its children were removed from, then a + // rename puts a new table back under the same name. + cleaner.OnDeleteEvent("/buckets/warehouse/pedsnet/person", "metadata", true, time.Now()) + if cleaner.GetPendingCleanupCount() != 1 { + t.Fatalf("expected the emptied folder to be queued, got %d", cleaner.GetPendingCleanupCount()) + } + + cleaner.OnCreateEvent("/buckets/warehouse/pedsnet", "person", true) + if cleaner.GetPendingCleanupCount() != 0 { + t.Fatalf("expected the recreated folder to leave the queue, got %d", cleaner.GetPendingCleanupCount()) + } +} + +func TestEmptyFolderCleaner_executeCleanup_skipsCatalogEntry(t *testing.T) { + lockRing := lock_manager.NewLockRing(5 * time.Second) + lockRing.SetSnapshot([]pb.ServerAddress{"filer1:8888"}, 0) + + const namespace = "/buckets/warehouse/pedsnet" + const table = namespace + "/person" + + var deleted []string + mock := &mockFilerOps{ + countFn: func(_ util.FullPath) (int, error) { + return 0, nil + }, + deleteFn: func(path util.FullPath) error { + deleted = append(deleted, string(path)) + return nil + }, + attrsFn: func(path util.FullPath) (map[string][]byte, error) { + if path == namespace || path == table { + return map[string][]byte{s3_constants.ExtS3TablesPrefix + "metadata": []byte("{}")}, nil + } + return nil, nil + }, + } + + cleaner := &EmptyFolderCleaner{ + filer: mock, + lockRing: lockRing, + host: "filer1:8888", + bucketPath: "/buckets", + enabled: true, + folderCounts: make(map[string]*folderState), + cleanupQueue: NewCleanupQueue(1000, time.Minute), + maxCountCheck: 1000, + cacheExpiry: time.Minute, + processorSleep: time.Second, + stopCh: make(chan struct{}), + } + + // The dropped table left its metadata folder empty; the cascade up from it must + // stop at the table the rename put back, and at the namespace above it. + cleaner.executeCleanup(table+"/metadata", "v2.metadata.json") + + if len(deleted) != 1 || deleted[0] != table+"/metadata" { + t.Fatalf("expected only %s to be deleted, got %v", table+"/metadata", deleted) + } +} + func TestEmptyFolderCleaner_restoreIfWrittenTo(t *testing.T) { lockRing := lock_manager.NewLockRing(5 * time.Second) lockRing.SetSnapshot([]pb.ServerAddress{"filer1:8888"}, 0) diff --git a/weed/s3api/s3_constants/extend_key.go b/weed/s3api/s3_constants/extend_key.go index 978d021e0..959a96247 100644 --- a/weed/s3api/s3_constants/extend_key.go +++ b/weed/s3api/s3_constants/extend_key.go @@ -49,6 +49,10 @@ const ( // Bucket Policy ExtBucketPolicyKey = "Seaweed-X-Amz-Bucket-Policy" + // Every attribute the s3tables catalog stores on a table bucket, namespace, + // table or view directory entry starts with this. + ExtS3TablesPrefix = "s3tables." + // Object Retention and Legal Hold ExtObjectLockModeKey = "Seaweed-X-Amz-Object-Lock-Mode" ExtRetentionUntilDateKey = "Seaweed-X-Amz-Retention-Until-Date" diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index 9e813660e..b9d3501c0 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -21,17 +21,17 @@ const ( DefaultRegion = "us-east-1" // Extended entry attributes for metadata storage - ExtendedKeyTableBucket = "s3tables.tableBucket" - ExtendedKeyMetadata = "s3tables.metadata" - ExtendedKeyMetadataVersion = "s3tables.metadataVersion" - ExtendedKeyPolicy = "s3tables.policy" - ExtendedKeyTags = "s3tables.tags" - ExtendedKeyMaintenance = "s3tables.maintenance" + ExtendedKeyTableBucket = s3_constants.ExtS3TablesPrefix + "tableBucket" + ExtendedKeyMetadata = s3_constants.ExtS3TablesPrefix + "metadata" + ExtendedKeyMetadataVersion = s3_constants.ExtS3TablesPrefix + "metadataVersion" + ExtendedKeyPolicy = s3_constants.ExtS3TablesPrefix + "policy" + ExtendedKeyTags = s3_constants.ExtS3TablesPrefix + "tags" + ExtendedKeyMaintenance = s3_constants.ExtS3TablesPrefix + "maintenance" // Written by the maintenance worker, read by GetTableMaintenanceJobStatus. // Separate from ExtendedKeyMaintenance so worker and operator writes do not // contend on the same attribute. - ExtendedKeyMaintenanceStatus = "s3tables.maintenanceStatus" - ExtendedKeyEntryType = "s3tables.entryType" + ExtendedKeyMaintenanceStatus = s3_constants.ExtS3TablesPrefix + "maintenanceStatus" + ExtendedKeyEntryType = s3_constants.ExtS3TablesPrefix + "entryType" // Entry-type marker values for ExtendedKeyEntryType. Absent or "table" means // a table; views are stored like tables but tagged "view".