From cfa8afec92fbcddeb8f8980ee656cafcf832f02d Mon Sep 17 00:00:00 2001 From: Eliah Rusin Date: Sat, 5 Sep 2026 01:12:27 -0500 Subject: [PATCH] filer: guard FoundationDB 100KB value limit and pack earlier (#11161) * filer: guard the FoundationDB value size limit, not the transaction limit An entry's whole chunk list is one FoundationDB value, and FDB caps a value at 100,000 bytes while a transaction may reach 10MB. UpdateEntry checked the transaction limit, so every entry between the two limits passed the guard and was rejected by FDB itself with error 2103 (Value length exceeds limit). The failure surfaced inside the store rather than at the guard, so the S3 layer dropped the connection and clients saw a network fault instead of an error. Check the value limit in UpdateEntry and KvPut instead, after gzip and before the transaction, with an error that names the limit it hit. The removed transaction-size constant guarded nothing else: DeleteFolderChildren batches by entry count. Refs #11158 * filer: fold at 500 chunks in the foundationdb build Manifest packing is what keeps a large file's entry small, but it only ran once a flat chunk list reached 10000 chunks. A FoundationDB value stops at 100,000 bytes and an entry's whole chunk list is one value, which at ~100 bytes per chunk record is about 1000 chunks -- so on FDB the write always failed before packing could help: a 3.3 GiB PutObject at the default -maxMB=4 was already past the limit. FoundationDB support is its own build (`go build -tags foundationdb`, shipped as its own image), so the batch is a build-time choice and needs no negotiation at run time. The tagged build folds at 500, every other build keeps 10000 and is untouched. 500 is not arbitrary: a single fold level leaves (chunks/batch) manifest pointers plus up to (batch-1) unfolded chunks in the entry, so the reachable chunk count is highest when the two terms are near equal. For a 100,000-byte budget that optimum is 500, which holds an entry inside the limit up to ~250,000 chunks -- ~1 TB at -maxMB=4, against ~4 GB before. Larger files need nested packing, which no batch size substitutes for. One binary serves every role in that image, so the filer and each client that folds -- S3, mount, WebDAV, weed shell, filer.copy -- agree on the batch by construction. A binary built with the tag but pointed at another store folds earlier than that store requires, costing one manifest blob per 500 chunks and one read to resolve it. Fixes #11158 * filer: fold with rollback inside MaybeManifestize, not beside it A fold that fails midway has already uploaded manifest blobs for its earlier batches, and returns only the data chunks -- dropping the manifests it had separated out of the caller's list. Both were wrong in ways that mattered: - AppendToEntry assigned that shortened list straight to entry.Chunks and created the entry, so an append to an already-folded file whose fold failed lost every previously folded chunk. weed mount had the same shape. - cleanupChunks logged the error as "not good, but should be ok" and then returned it through a named result, failing the whole CreateEntry or UpdateEntry, while the blobs it had written stayed behind referenced by nothing. The S3 path was alone in handling this, through a private helper beside MaybeManifestize. A second entry point next to the one everything else calls just means the wrong one gets used, so the behaviour moves inside MaybeManifestize: on failure it returns inputChunks as it received them, and hands the blobs it saved to a deleteChunks callback. The filer, S3 and filer.copy pass their existing deleters -- filer.copy already cleans up this way after a failed upload -- and mount, WebDAV and weed shell pass nil, which reports the blobs rather than collecting them, as before. Each caller keeps its own error policy: the filer HTTP PUT path and filer.copy still fail the request, the rest still continue with the flat list, which is a correct entry. --------- Co-authored-by: Chris Lu --- weed/command/filer_copy.go | 17 +- weed/filer/filechunk_manifest.go | 40 ++++- .../filer/filechunk_manifest_rollback_test.go | 147 ++++++++++++++++++ weed/filer/foundationdb/foundationdb_store.go | 12 +- weed/filer/foundationdb/value_size.go | 16 ++ weed/filer/foundationdb/value_size_test.go | 32 ++++ weed/filer/manifest_batch.go | 10 ++ weed/filer/manifest_batch_foundationdb.go | 24 +++ weed/mount/weedfs_file_sync.go | 3 +- weed/mount/weedfs_metadata_flush.go | 3 +- weed/s3api/s3api_chunk_manifest.go | 22 +-- weed/s3api/s3api_chunk_manifest_test.go | 96 ------------ weed/server/filer_grpc_server.go | 16 +- .../filer_server_handlers_write_autochunk.go | 2 +- weed/server/webdav_server.go | 3 +- weed/shell/command_fs_distribute_chunks.go | 3 +- 16 files changed, 305 insertions(+), 141 deletions(-) create mode 100644 weed/filer/filechunk_manifest_rollback_test.go create mode 100644 weed/filer/foundationdb/value_size.go create mode 100644 weed/filer/foundationdb/value_size_test.go create mode 100644 weed/filer/manifest_batch.go create mode 100644 weed/filer/manifest_batch_foundationdb.go diff --git a/weed/command/filer_copy.go b/weed/command/filer_copy.go index 4b814116d..5525ebddd 100644 --- a/weed/command/filer_copy.go +++ b/weed/command/filer_copy.go @@ -491,7 +491,22 @@ func (worker *FileCopyWorker) uploadFileInChunks(task FileCopyTask, f *os.File, return uploadError } - manifestedChunks, manifestErr := filer.MaybeManifestize(worker.saveDataAsChunk, chunks) + // A fold that fails midway aborts the copy, so the blobs its earlier + // batches wrote go the same way as the chunks of a failed upload. + deleteManifestChunks := func(saved []*filer_pb.FileChunk) { + if len(worker.options.masters) == 0 { + return + } + var fileIds []string + for _, chunk := range saved { + fileIds = append(fileIds, chunk.GetFileIdString()) + } + operation.DeleteFileIds(func(_ context.Context) pb.ServerAddress { + return pb.ServerAddress(worker.options.masters[0]) + }, false, worker.options.grpcDialOption, fileIds) + } + + manifestedChunks, manifestErr := filer.MaybeManifestize(worker.saveDataAsChunk, deleteManifestChunks, chunks) if manifestErr != nil { return fmt.Errorf("create manifest: %w", manifestErr) } diff --git a/weed/filer/filechunk_manifest.go b/weed/filer/filechunk_manifest.go index f7c69766a..73dc79751 100644 --- a/weed/filer/filechunk_manifest.go +++ b/weed/filer/filechunk_manifest.go @@ -19,10 +19,6 @@ import ( util_http "github.com/seaweedfs/seaweedfs/weed/util/http" ) -const ( - ManifestBatch = 10000 -) - var bytesBufferPool = sync.Pool{ New: func() interface{} { return new(bytes.Buffer) @@ -228,14 +224,42 @@ func retriedStreamFetchChunkData(ctx context.Context, writer io.Writer, urlStrin } -func MaybeManifestize(saveFunc SaveDataAsChunkFunctionType, inputChunks []*filer_pb.FileChunk) (chunks []*filer_pb.FileChunk, err error) { +// MaybeManifestize folds a flat chunk list into manifest chunks once it passes +// ManifestBatch, so an entry's chunk list stays within what the metadata store +// will hold. A fold that fails partway returns inputChunks unchanged -- never +// the half-folded list, which drops the manifests the caller came in with -- +// and hands the blobs it had already saved to deleteChunks, since the flat list +// it returns references none of them. deleteChunks may be nil where the caller +// has no deleter to offer; then the blobs are only named in the log. +func MaybeManifestize(saveFunc SaveDataAsChunkFunctionType, deleteChunks func([]*filer_pb.FileChunk), inputChunks []*filer_pb.FileChunk) (chunks []*filer_pb.FileChunk, err error) { // Don't manifestize SSE-encrypted chunks to preserve per-chunk metadata for _, chunk := range inputChunks { if chunk.GetSseType() != 0 { // Any SSE type (SSE-C or SSE-KMS) return inputChunks, nil } } - return doMaybeManifestize(saveFunc, inputChunks, ManifestBatch, mergeIntoManifest) + + var saved []*filer_pb.FileChunk + record := func(reader io.Reader, name string, offset int64, tsNs int64, expectedDataSize uint64) (*filer_pb.FileChunk, error) { + chunk, saveErr := saveFunc(reader, name, offset, tsNs, expectedDataSize) + if saveErr == nil { + saved = append(saved, chunk) + } + return chunk, saveErr + } + + chunks, err = doMaybeManifestize(record, inputChunks, ManifestBatch, mergeIntoManifest) + if err == nil { + return chunks, nil + } + if len(saved) > 0 { + if deleteChunks != nil { + deleteChunks(saved) + } else { + glog.V(0).Infof("manifestize failed, %d manifest blobs left unreferenced: %v", len(saved), err) + } + } + return inputChunks, err } func doMaybeManifestize(saveFunc SaveDataAsChunkFunctionType, inputChunks []*filer_pb.FileChunk, mergeFactor int, mergefn func(saveFunc SaveDataAsChunkFunctionType, dataChunks []*filer_pb.FileChunk) (manifestChunk *filer_pb.FileChunk, err error)) (chunks []*filer_pb.FileChunk, err error) { @@ -253,7 +277,9 @@ func doMaybeManifestize(saveFunc SaveDataAsChunkFunctionType, inputChunks []*fil for i := 0; i+mergeFactor <= len(dataChunks); i += mergeFactor { chunk, err := mergefn(saveFunc, dataChunks[i:i+mergeFactor]) if err != nil { - return dataChunks, err + // dataChunks is what is left after the manifests the caller + // already had were separated out; returning it would drop them + return inputChunks, err } chunks = append(chunks, chunk) remaining -= mergeFactor diff --git a/weed/filer/filechunk_manifest_rollback_test.go b/weed/filer/filechunk_manifest_rollback_test.go new file mode 100644 index 000000000..61c899480 --- /dev/null +++ b/weed/filer/filechunk_manifest_rollback_test.go @@ -0,0 +1,147 @@ +package filer + +import ( + "fmt" + "io" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" +) + +func flatTestChunks(n int) []*filer_pb.FileChunk { + chunks := make([]*filer_pb.FileChunk, n) + for i := range chunks { + chunks[i] = &filer_pb.FileChunk{ + FileId: fmt.Sprintf("1,%x", i+1), + Offset: int64(i) * 8, + Size: 8, + } + } + return chunks +} + +type fakeManifestStore struct { + saves int + failOn int // 1-based save call to fail on; 0 = never + deleted []*filer_pb.FileChunk +} + +func (s *fakeManifestStore) save(reader io.Reader, name string, offset int64, tsNs int64, expectedDataSize uint64) (*filer_pb.FileChunk, error) { + s.saves++ + if s.saves == s.failOn { + return nil, fmt.Errorf("save %d failed", s.saves) + } + if _, err := io.Copy(io.Discard, reader); err != nil { + return nil, err + } + return &filer_pb.FileChunk{FileId: fmt.Sprintf("2,%x", s.saves), Offset: offset}, nil +} + +func (s *fakeManifestStore) delete(chunks []*filer_pb.FileChunk) { + s.deleted = append(s.deleted, chunks...) +} + +func TestMaybeManifestizeFolds(t *testing.T) { + store := &fakeManifestStore{} + chunks := flatTestChunks(ManifestBatch + 50) + + result, err := MaybeManifestize(store.save, store.delete, chunks) + if err != nil { + t.Fatalf("MaybeManifestize: %v", err) + } + + manifests, data := SeparateManifestChunks(result) + if len(manifests) != 1 || len(data) != 50 { + t.Fatalf("expected 1 manifest + 50 flat chunks, got %d + %d", len(manifests), len(data)) + } + if store.saves != 1 || len(store.deleted) != 0 { + t.Errorf("expected 1 save and no deletes, got %d saves, %d deleted", store.saves, len(store.deleted)) + } + if manifests[0].Offset != 0 || manifests[0].Size != uint64(ManifestBatch*8) { + t.Errorf("manifest span [%d,%d) wrong", manifests[0].Offset, manifests[0].Size) + } +} + +func TestMaybeManifestizeBelowThreshold(t *testing.T) { + store := &fakeManifestStore{} + chunks := flatTestChunks(ManifestBatch - 1) + + result, err := MaybeManifestize(store.save, store.delete, chunks) + if err != nil { + t.Fatalf("MaybeManifestize: %v", err) + } + if len(result) != len(chunks) || store.saves != 0 { + t.Fatalf("expected untouched flat list, got %d chunks, %d saves", len(result), store.saves) + } +} + +func TestMaybeManifestizeSkipsSse(t *testing.T) { + store := &fakeManifestStore{} + chunks := flatTestChunks(ManifestBatch + 50) + chunks[0].SseType = filer_pb.SSEType_SSE_S3 + + result, err := MaybeManifestize(store.save, store.delete, chunks) + if err != nil { + t.Fatalf("MaybeManifestize: %v", err) + } + if len(result) != len(chunks) || store.saves != 0 { + t.Fatalf("SSE chunks must not be folded, got %d chunks, %d saves", len(result), store.saves) + } +} + +// A fold that fails midway must hand back the caller's own list and delete the +// manifest blobs its earlier batches already uploaded. +func TestMaybeManifestizeRollsBackPartialFold(t *testing.T) { + store := &fakeManifestStore{failOn: 2} + chunks := flatTestChunks(2*ManifestBatch + 50) + + result, err := MaybeManifestize(store.save, store.delete, chunks) + if err == nil { + t.Fatal("expected the failed save to be reported") + } + if len(result) != len(chunks) { + t.Fatalf("expected fallback to %d flat chunks, got %d", len(chunks), len(result)) + } + if HasChunkManifest(result) { + t.Error("fallback list must not contain manifest chunks") + } + if len(store.deleted) != 1 || store.deleted[0].FileId != "2,1" { + t.Fatalf("expected the first saved blob deleted, got %+v", store.deleted) + } +} + +// The manifests a caller came in with are not re-folded, and must survive a +// fold that fails on the flat remainder: returning only the data chunks would +// lose everything they cover. +func TestMaybeManifestizeKeepsExistingManifestsOnFailure(t *testing.T) { + store := &fakeManifestStore{failOn: 2} + existing := &filer_pb.FileChunk{FileId: "m,1", IsChunkManifest: true, Offset: 0, Size: 24} + chunks := append([]*filer_pb.FileChunk{existing}, flatTestChunks(2*ManifestBatch+50)...) + + result, _ := MaybeManifestize(store.save, store.delete, chunks) + + if len(result) != len(chunks) { + t.Fatalf("expected original %d chunks, got %d", len(chunks), len(result)) + } + if result[0].FileId != "m,1" || !result[0].IsChunkManifest { + t.Fatalf("existing manifest dropped: %+v", result[0]) + } + if len(store.deleted) != 1 || store.deleted[0].FileId != "2,1" { + t.Fatalf("expected the first saved blob deleted, got %+v", store.deleted) + } +} + +// Without a deleter the fold still falls back to the caller's list; the blobs +// it saved are only reported. +func TestMaybeManifestizeRollbackWithoutDeleter(t *testing.T) { + store := &fakeManifestStore{failOn: 2} + chunks := flatTestChunks(2*ManifestBatch + 50) + + result, err := MaybeManifestize(store.save, nil, chunks) + if err == nil { + t.Fatal("expected the failed save to be reported") + } + if len(result) != len(chunks) || HasChunkManifest(result) { + t.Fatalf("expected fallback to %d flat chunks, got %d", len(chunks), len(result)) + } +} diff --git a/weed/filer/foundationdb/foundationdb_store.go b/weed/filer/foundationdb/foundationdb_store.go index b413d260f..52c74677a 100644 --- a/weed/filer/foundationdb/foundationdb_store.go +++ b/weed/filer/foundationdb/foundationdb_store.go @@ -35,8 +35,6 @@ import ( ) const ( - // FoundationDB transaction size limit is 10MB - FDB_TRANSACTION_SIZE_LIMIT = 10 * 1024 * 1024 // Safe limit for batch size (leave margin for FDB overhead) FDB_BATCH_SIZE_LIMIT = 8 * 1024 * 1024 // Maximum number of entries to return in a single directory listing @@ -396,10 +394,8 @@ func (store *FoundationDBStore) UpdateEntry(ctx context.Context, entry *filer.En value = util.MaybeGzipData(value) } - // Check transaction size limit - if len(value) > FDB_TRANSACTION_SIZE_LIMIT { - return fmt.Errorf("entry %s exceeds FoundationDB transaction size limit (%d > %d bytes)", - entry.FullPath, len(value), FDB_TRANSACTION_SIZE_LIMIT) + if err := errIfValueTooLarge(string(entry.FullPath), value); err != nil { + return err } // Check if there's a transaction in context @@ -696,6 +692,10 @@ func (store *FoundationDBStore) ListDirectoryPrefixedEntries(ctx context.Context func (store *FoundationDBStore) KvPut(ctx context.Context, key []byte, value []byte) error { fdbKey := store.kvDir.Pack(tuple.Tuple{key}) + if err := errIfValueTooLarge(string(key), value); err != nil { + return err + } + // Check if there's a transaction in context if tx, exists := store.getTransactionFromContext(ctx); exists { tx.Set(fdbKey, value) diff --git a/weed/filer/foundationdb/value_size.go b/weed/filer/foundationdb/value_size.go new file mode 100644 index 000000000..7e07234d6 --- /dev/null +++ b/weed/filer/foundationdb/value_size.go @@ -0,0 +1,16 @@ +package foundationdb + +import "fmt" + +// FDB_VALUE_SIZE_LIMIT is Apple's documented maximum value size: 100,000 +// bytes, not 100 KiB. A value over it is rejected by FoundationDB itself with +// error 2103, so the store checks it and names it. +const FDB_VALUE_SIZE_LIMIT = 100 * 1000 + +func errIfValueTooLarge(name string, value []byte) error { + if len(value) > FDB_VALUE_SIZE_LIMIT { + return fmt.Errorf("entry %s exceeds FoundationDB value size limit (%d > %d bytes)", + name, len(value), FDB_VALUE_SIZE_LIMIT) + } + return nil +} diff --git a/weed/filer/foundationdb/value_size_test.go b/weed/filer/foundationdb/value_size_test.go new file mode 100644 index 000000000..27256cf38 --- /dev/null +++ b/weed/filer/foundationdb/value_size_test.go @@ -0,0 +1,32 @@ +package foundationdb + +import ( + "strings" + "testing" +) + +func TestFDBValueSizeLimitConstant(t *testing.T) { + if FDB_VALUE_SIZE_LIMIT != 100*1000 { + t.Fatalf("FDB_VALUE_SIZE_LIMIT = %d, want 100000", FDB_VALUE_SIZE_LIMIT) + } +} + +func TestErrIfValueTooLarge(t *testing.T) { + if err := errIfValueTooLarge("/p", make([]byte, FDB_VALUE_SIZE_LIMIT)); err != nil { + t.Fatalf("100000-byte value should pass, got %v", err) + } + err := errIfValueTooLarge("/buckets/b/o", make([]byte, FDB_VALUE_SIZE_LIMIT+1)) + if err == nil { + t.Fatal("100001-byte value should fail") + } + msg := err.Error() + if !strings.Contains(msg, "value size limit") { + t.Fatalf("error should name value size limit, got %q", msg) + } + if strings.Contains(msg, "transaction size limit") { + t.Fatalf("error must not mention transaction size limit, got %q", msg) + } + if !strings.Contains(msg, "100001") || !strings.Contains(msg, "100000") { + t.Fatalf("error should include both sizes, got %q", msg) + } +} diff --git a/weed/filer/manifest_batch.go b/weed/filer/manifest_batch.go new file mode 100644 index 000000000..95c36aba3 --- /dev/null +++ b/weed/filer/manifest_batch.go @@ -0,0 +1,10 @@ +//go:build !foundationdb + +package filer + +// ManifestBatch is how many data chunks are folded into one manifest chunk, so +// that an entry's chunk list stays within the largest value its metadata store +// will accept. Stores reached by this build hold values far larger than the +// list 10000 chunks encode to; see manifest_batch_foundationdb.go for the +// FoundationDB build, whose values stop at 100,000 bytes. +const ManifestBatch = 10000 diff --git a/weed/filer/manifest_batch_foundationdb.go b/weed/filer/manifest_batch_foundationdb.go new file mode 100644 index 000000000..7b39795dc --- /dev/null +++ b/weed/filer/manifest_batch_foundationdb.go @@ -0,0 +1,24 @@ +//go:build foundationdb + +package filer + +// ManifestBatch is how many data chunks are folded into one manifest chunk. A +// FoundationDB value stops at 100,000 bytes (foundationdb.FDB_VALUE_SIZE_LIMIT) +// and an entry's whole chunk list is one value, which at ~100 bytes per chunk +// record is about 1000 chunks -- so folding at 10000 never ran before the write +// was already too large to store. +// +// A single fold level leaves (chunks/batch) manifest pointers plus up to +// (batch-1) unfolded chunks in the entry, so the reachable chunk count is +// highest when the two terms are near equal. 500 is that optimum for a +// 100,000-byte budget: it keeps an entry inside the limit up to ~250,000 +// chunks, ~1 TB at the default -maxMB=4, against ~4 GB before. Larger files +// need nested packing, which no batch size substitutes for. +// +// This is a property of the build rather than of the configured store: the +// FoundationDB image builds one binary for the filer and for every client that +// folds (S3, mount, WebDAV, weed shell, filer.copy), and each of them has to +// agree on a list the filer's store can hold. A binary built with this tag but +// pointed at another store folds earlier than that store requires, which costs +// one manifest blob per 500 chunks and one read to resolve it. +const ManifestBatch = 500 diff --git a/weed/mount/weedfs_file_sync.go b/weed/mount/weedfs_file_sync.go index 2e7790b6a..834e3827c 100644 --- a/weed/mount/weedfs_file_sync.go +++ b/weed/mount/weedfs_file_sync.go @@ -247,7 +247,8 @@ func (wfs *WFS) flushMetadataToFiler(ctx context.Context, fh *FileHandle, dir, n manifestChunks = nil } - chunks, manifestErr := filer.MaybeManifestize(wfs.saveDataAsChunk(fileFullPath), chunks) + // no chunk deleter here: a failed fold reports the blobs it saved + chunks, manifestErr := filer.MaybeManifestize(wfs.saveDataAsChunk(fileFullPath), nil, chunks) if manifestErr != nil { // not good, but should be ok glog.V(0).Infof("MaybeManifestize: %v", manifestErr) diff --git a/weed/mount/weedfs_metadata_flush.go b/weed/mount/weedfs_metadata_flush.go index 3ff2fa9f7..2a4f3eee9 100644 --- a/weed/mount/weedfs_metadata_flush.go +++ b/weed/mount/weedfs_metadata_flush.go @@ -139,7 +139,8 @@ func (wfs *WFS) flushFileMetadata(fh *FileHandle) error { compactedChunks, _ := filer.CompactFileChunks(context.Background(), wfs.LookupFn(), nonManifestChunks) // Try to create manifest chunks for large files - compactedChunks, manifestErr := filer.MaybeManifestize(wfs.saveDataAsChunk(fileFullPath), compactedChunks) + // no chunk deleter here: a failed fold reports the blobs it saved + compactedChunks, manifestErr := filer.MaybeManifestize(wfs.saveDataAsChunk(fileFullPath), nil, compactedChunks) if manifestErr != nil { glog.V(0).Infof("flushFileMetadata MaybeManifestize: %v", manifestErr) } diff --git a/weed/s3api/s3api_chunk_manifest.go b/weed/s3api/s3api_chunk_manifest.go index d8eaaa9e8..c60d39ed7 100644 --- a/weed/s3api/s3api_chunk_manifest.go +++ b/weed/s3api/s3api_chunk_manifest.go @@ -33,29 +33,13 @@ func (s3a *S3ApiServer) saveManifestChunk(filePath string, bucket string, ttlSec } // manifestizeChunks folds a large flat chunk list into manifest chunks. A -// failed fold falls back to the flat list, deleting any blobs it saved. +// failed fold falls back to the flat list, which is still a correct object. func (s3a *S3ApiServer) manifestizeChunks(filePath string, bucket string, ttlSec int32, chunks []*filer_pb.FileChunk) []*filer_pb.FileChunk { - return manifestizeOrKeepFlat(s3a.saveManifestChunk(filePath, bucket, ttlSec), s3a.deleteOrphanedChunks, filePath, chunks) -} - -func manifestizeOrKeepFlat(save filer.SaveDataAsChunkFunctionType, deleteChunks func([]*filer_pb.FileChunk), filePath string, chunks []*filer_pb.FileChunk) []*filer_pb.FileChunk { - var saved []*filer_pb.FileChunk - record := func(reader io.Reader, name string, offset int64, tsNs int64, expectedDataSize uint64) (*filer_pb.FileChunk, error) { - chunk, err := save(reader, name, offset, tsNs, expectedDataSize) - if err == nil { - saved = append(saved, chunk) - } - return chunk, err - } - manifested, err := filer.MaybeManifestize(record, chunks) + folded, err := filer.MaybeManifestize(s3a.saveManifestChunk(filePath, bucket, ttlSec), s3a.deleteOrphanedChunks, chunks) if err != nil { glog.V(0).Infof("MaybeManifestize %s: %v", filePath, err) - if len(saved) > 0 { - deleteChunks(saved) - } - return chunks } - return manifested + return folded } // flattenManifestChunks resolves manifest chunks into flat data chunks, diff --git a/weed/s3api/s3api_chunk_manifest_test.go b/weed/s3api/s3api_chunk_manifest_test.go index 8c3f373f5..5035b5278 100644 --- a/weed/s3api/s3api_chunk_manifest_test.go +++ b/weed/s3api/s3api_chunk_manifest_test.go @@ -1,107 +1,11 @@ package s3api import ( - "fmt" - "io" "testing" - "github.com/seaweedfs/seaweedfs/weed/filer" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" ) -func flatChunks(n int) []*filer_pb.FileChunk { - chunks := make([]*filer_pb.FileChunk, n) - for i := range chunks { - chunks[i] = &filer_pb.FileChunk{ - FileId: fmt.Sprintf("1,%x", i+1), - Offset: int64(i) * 8, - Size: 8, - } - } - return chunks -} - -type fakeManifestStore struct { - saves int - failOn int // 1-based save call to fail on; 0 = never - deleted []*filer_pb.FileChunk -} - -func (s *fakeManifestStore) save(reader io.Reader, name string, offset int64, tsNs int64, expectedDataSize uint64) (*filer_pb.FileChunk, error) { - s.saves++ - if s.saves == s.failOn { - return nil, fmt.Errorf("save %d failed", s.saves) - } - if _, err := io.Copy(io.Discard, reader); err != nil { - return nil, err - } - return &filer_pb.FileChunk{FileId: fmt.Sprintf("2,%x", s.saves), Offset: offset}, nil -} - -func (s *fakeManifestStore) delete(chunks []*filer_pb.FileChunk) { - s.deleted = append(s.deleted, chunks...) -} - -func TestManifestizeOrKeepFlatFolds(t *testing.T) { - store := &fakeManifestStore{} - chunks := flatChunks(filer.ManifestBatch + 50) - - result := manifestizeOrKeepFlat(store.save, store.delete, "/buckets/b/o", chunks) - - manifests, data := filer.SeparateManifestChunks(result) - if len(manifests) != 1 || len(data) != 50 { - t.Fatalf("expected 1 manifest + 50 flat chunks, got %d + %d", len(manifests), len(data)) - } - if store.saves != 1 || len(store.deleted) != 0 { - t.Errorf("expected 1 save and no deletes, got %d saves, %d deleted", store.saves, len(store.deleted)) - } - if manifests[0].Offset != 0 || manifests[0].Size != uint64(filer.ManifestBatch*8) { - t.Errorf("manifest span [%d,%d) wrong", manifests[0].Offset, manifests[0].Size) - } -} - -func TestManifestizeOrKeepFlatBelowThreshold(t *testing.T) { - store := &fakeManifestStore{} - chunks := flatChunks(filer.ManifestBatch - 1) - - result := manifestizeOrKeepFlat(store.save, store.delete, "/buckets/b/o", chunks) - - if len(result) != len(chunks) || store.saves != 0 { - t.Fatalf("expected untouched flat list, got %d chunks, %d saves", len(result), store.saves) - } -} - -func TestManifestizeOrKeepFlatSkipsSse(t *testing.T) { - store := &fakeManifestStore{} - chunks := flatChunks(filer.ManifestBatch + 50) - chunks[0].SseType = filer_pb.SSEType_SSE_S3 - - result := manifestizeOrKeepFlat(store.save, store.delete, "/buckets/b/o", chunks) - - if len(result) != len(chunks) || store.saves != 0 { - t.Fatalf("SSE chunks must not be folded, got %d chunks, %d saves", len(result), store.saves) - } -} - -// A fold that fails midway must fall back to the flat list and delete the -// manifest blobs its earlier batches already uploaded. -func TestManifestizeOrKeepFlatRollsBackPartialFold(t *testing.T) { - store := &fakeManifestStore{failOn: 2} - chunks := flatChunks(2*filer.ManifestBatch + 50) - - result := manifestizeOrKeepFlat(store.save, store.delete, "/buckets/b/o", chunks) - - if len(result) != len(chunks) { - t.Fatalf("expected fallback to %d flat chunks, got %d", len(chunks), len(result)) - } - if filer.HasChunkManifest(result) { - t.Error("fallback list must not contain manifest chunks") - } - if len(store.deleted) != 1 || store.deleted[0].FileId != "2,1" { - t.Fatalf("expected the first saved blob deleted, got %+v", store.deleted) - } -} - func TestPartRange(t *testing.T) { chunks := []*filer_pb.FileChunk{ {FileId: "1,a", Offset: 0, Size: 8}, diff --git a/weed/server/filer_grpc_server.go b/weed/server/filer_grpc_server.go index b86d82475..b0cf88345 100644 --- a/weed/server/filer_grpc_server.go +++ b/weed/server/filer_grpc_server.go @@ -769,11 +769,13 @@ func (fs *FilerServer) cleanupChunks(ctx context.Context, fullpath string, exist "", "", ) // ignore readonly error for capacity needed to manifestize - chunks, err = filer.MaybeManifestize(fs.saveAsChunk(ctx, so), chunks) - if err != nil { - // not good, but should be ok - glog.V(0).InfofCtx(ctx, "MaybeManifestize: %v", err) + // A failed fold leaves a flat list, which is still a correct entry -- + // and the store rejects it by name if it cannot hold one this size. + folded, manifestErr := filer.MaybeManifestize(fs.saveAsChunk(ctx, so), fs.filer.DeleteChunksNotRecursive, chunks) + if manifestErr != nil { + glog.V(0).InfofCtx(ctx, "MaybeManifestize %s: %v", fullpath, manifestErr) } + chunks = folded } chunks = append(manifestChunks, chunks...) @@ -824,10 +826,10 @@ func (fs *FilerServer) AppendToEntry(ctx context.Context, req *filer_pb.AppendTo glog.WarningfCtx(ctx, "applyStorageDefaultsToEntry: %v", err) return &filer_pb.AppendToEntryResponse{}, err } - entry.Chunks, err = filer.MaybeManifestize(fs.saveAsChunk(ctx, so), entry.GetChunks()) + entry.Chunks, err = filer.MaybeManifestize(fs.saveAsChunk(ctx, so), fs.filer.DeleteChunksNotRecursive, entry.GetChunks()) if err != nil { - // not good, but should be ok - glog.V(0).InfofCtx(ctx, "MaybeManifestize: %v", err) + // the append is still correct with the flat list + glog.V(0).InfofCtx(ctx, "MaybeManifestize %s: %v", fullpath, err) } err = fs.filer.CreateEntry(context.Background(), entry, nil, false, false, nil, false, fs.filer.MaxFilenameLength) diff --git a/weed/server/filer_server_handlers_write_autochunk.go b/weed/server/filer_server_handlers_write_autochunk.go index 34a70d3ef..28b14893e 100644 --- a/weed/server/filer_server_handlers_write_autochunk.go +++ b/weed/server/filer_server_handlers_write_autochunk.go @@ -306,7 +306,7 @@ func (fs *FilerServer) saveMetaData(ctx context.Context, r *http.Request, fileNa } // maybe compact entry chunks - mergedChunks, replyerr = filer.MaybeManifestize(fs.saveAsChunk(ctx, so), mergedChunks) + mergedChunks, replyerr = filer.MaybeManifestize(fs.saveAsChunk(ctx, so), fs.filer.DeleteChunksNotRecursive, mergedChunks) if replyerr != nil { glog.V(0).InfofCtx(ctx, "manifestize %s: %v", r.RequestURI, replyerr) return diff --git a/weed/server/webdav_server.go b/weed/server/webdav_server.go index 31d138508..7acac1d2f 100644 --- a/weed/server/webdav_server.go +++ b/weed/server/webdav_server.go @@ -495,7 +495,8 @@ func (f *WebDavFile) Write(buf []byte) (int, error) { } f.bufWriter.CloseFunc = func() error { - manifestedChunks, manifestErr := filer.MaybeManifestize(f.saveDataAsChunk, f.entry.GetChunks()) + // no chunk deleter here: a failed fold reports the blobs it saved + manifestedChunks, manifestErr := filer.MaybeManifestize(f.saveDataAsChunk, nil, f.entry.GetChunks()) if manifestErr != nil { // not good, but should be ok glog.V(0).Infof("file %s close MaybeManifestize: %v", f.name, manifestErr) diff --git a/weed/shell/command_fs_distribute_chunks.go b/weed/shell/command_fs_distribute_chunks.go index 96baaa740..17c6f4fdd 100644 --- a/weed/shell/command_fs_distribute_chunks.go +++ b/weed/shell/command_fs_distribute_chunks.go @@ -191,7 +191,8 @@ func (c *commandFsDistributeChunks) Do(args []string, commandEnv *CommandEnv, wr finalChunks := chunks if hadManifest { - remanifested, manErr := filer.MaybeManifestize(newShellSaveAsChunk(commandEnv), chunks) + // no chunk deleter here: a failed fold reports the blobs it saved + remanifested, manErr := filer.MaybeManifestize(newShellSaveAsChunk(commandEnv), nil, chunks) if manErr != nil { fmt.Fprintf(writer, "WARNING: re-manifestize failed: %v. Writing flat chunk list.\n", manErr) } else {