mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-10-07 14:57:48 +02:00
s3: give each prefix its own hidden-prefix probe budget (#11626)
* s3: give each prefix its own hidden-prefix probe budget The shared cursor.probedEntries counter in dirHoldsOnlyHiddenEntries was exhausted by one large all-deleted subtree, causing every later prefix in the same request to be treated as visible. The fix allocates a fresh budget (hiddenProbePerPrefixBudget, default 1000) for each top-level call and threads it down to recursive calls via a pointer, so cross-prefix budget bleed is impossible. Fixes #10847. * s3: keep 10000 probe budget, now per prefix Restore hiddenProbeBudget = 10000 as a package const (not a mutable var) applied per-prefix instead of per-request. The override hook moves to an unexported probeBudget field on ListingCursor; zero means use the package default. Tests set probeBudget: 3 on the cursor, keeping them fast without touching package state. Also revert the unrelated uint32 cast on the ListEntries Limit field. * s3: cap total hidden-prefix probe work per listing request The per-prefix budget resets for every candidate prefix, but deleted prefixes do not spend maxKeys, so a page can walk an unbounded number of them. Keep a request-wide probedEntries ceiling (hiddenProbeTotalBudget, 10x the per-prefix budget) so total probe work stays bounded. * s3api: pin the probe-budget cutoff and the request-wide ceiling --------- Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
This commit is contained in:
1 parent
0bcebf708c
commit
079a7d8ba3
2 files changed
+115
-9
No files matched your search
@@ -780,6 +780,12 @@ type ListingCursor struct {
|
||||
// something to find once a bucket has version history to leave behind.
|
||||
hideDeletedPrefixes bool
|
||||
probedEntries int
|
||||
// probeBudget overrides hiddenProbeBudget for the per-prefix probe; 0 means use
|
||||
// the package default. Tests set this to a small value to stay fast.
|
||||
probeBudget int
|
||||
// probeTotalBudget overrides hiddenProbeTotalBudget for the request-wide probe
|
||||
// ceiling; 0 means use the package default.
|
||||
probeTotalBudget int
|
||||
// retractEntry undoes the listing of a base-path null object once its .versions
|
||||
// sibling reveals that the current version is a delete marker.
|
||||
retractEntry func(dir, name string)
|
||||
@@ -1161,12 +1167,16 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb.
|
||||
}
|
||||
}
|
||||
|
||||
// hiddenProbePageSize is the window one probe request asks the filer for, and
|
||||
// hiddenProbeBudget caps how many entries a single list request may look at while
|
||||
// deciding which directories still stand for a prefix.
|
||||
// hiddenProbePageSize is the window one probe request asks the filer for.
|
||||
// hiddenProbeBudget caps how many entries one prefix's probe may scan; it
|
||||
// resets for every candidate prefix so one large all-deleted subtree cannot
|
||||
// exhaust the allowance for later prefixes. hiddenProbeTotalBudget caps the
|
||||
// scanned entries a single listing request spends on probing overall, since a
|
||||
// page may still walk an unbounded number of all-deleted prefixes.
|
||||
const (
|
||||
hiddenProbePageSize = 64
|
||||
hiddenProbeBudget = 10000
|
||||
hiddenProbePageSize = 64
|
||||
hiddenProbeBudget = 10000
|
||||
hiddenProbeTotalBudget = 100000
|
||||
)
|
||||
|
||||
// dirHoldsOnlyHiddenEntries reports whether dir holds entries but none that a
|
||||
@@ -1179,13 +1189,29 @@ const (
|
||||
//
|
||||
// The scan stops at the first key it finds, so a populated prefix costs one ListEntries
|
||||
// answered by its first entry. A subtree that is entirely delete-marked costs a walk of
|
||||
// that subtree, bounded by the request's probe budget; once the budget is spent the
|
||||
// prefix is reported, as it was before this check existed.
|
||||
// that subtree, bounded by hiddenProbeBudget entries per prefix. The budget resets for
|
||||
// each candidate prefix, so a large all-deleted subtree cannot exhaust the allowance
|
||||
// for later prefixes in the same request.
|
||||
func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client filer_pb.SeaweedFilerClient, bucket, dir string, cursor *ListingCursor) bool {
|
||||
if !cursor.hideDeletedPrefixes {
|
||||
return false
|
||||
}
|
||||
budget := hiddenProbeBudget
|
||||
if cursor.probeBudget > 0 {
|
||||
budget = cursor.probeBudget
|
||||
}
|
||||
totalBudget := hiddenProbeTotalBudget
|
||||
if cursor.probeTotalBudget > 0 {
|
||||
totalBudget = cursor.probeTotalBudget
|
||||
}
|
||||
return s3a.dirHoldsOnlyHiddenEntriesInner(ctx, client, bucket, dir, &budget, totalBudget, cursor)
|
||||
}
|
||||
|
||||
// dirHoldsOnlyHiddenEntriesInner is the recursive body of dirHoldsOnlyHiddenEntries.
|
||||
// budget is shared across the recursive descent for one prefix but reset by the
|
||||
// caller for each top-level prefix, preventing cross-prefix budget exhaustion.
|
||||
// cursor.probedEntries bounds the total work one listing request spends probing.
|
||||
func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntriesInner(ctx context.Context, client filer_pb.SeaweedFilerClient, bucket, dir string, budget *int, totalBudget int, cursor *ListingCursor) bool {
|
||||
ctx, cancel := context.WithCancel(ctx)
|
||||
defer cancel()
|
||||
|
||||
@@ -1227,8 +1253,9 @@ func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client fi
|
||||
startFrom = entry.Name
|
||||
sawEntry = true
|
||||
|
||||
*budget--
|
||||
cursor.probedEntries++
|
||||
if cursor.probedEntries > hiddenProbeBudget {
|
||||
if *budget < 0 || cursor.probedEntries > totalBudget {
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -1269,7 +1296,7 @@ func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client fi
|
||||
if entry.IsDirectoryKeyObject() {
|
||||
return false
|
||||
}
|
||||
if !s3a.dirHoldsOnlyHiddenEntries(ctx, client, bucket, dir+"/"+entry.Name, cursor) {
|
||||
if !s3a.dirHoldsOnlyHiddenEntriesInner(ctx, client, bucket, dir+"/"+entry.Name, budget, totalBudget, cursor) {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
@@ -250,3 +250,82 @@ func TestDeletedPrefixesDoNotConsumeMaxKeys(t *testing.T) {
|
||||
assert.False(t, cursor.isTruncated, "stepping over deleted prefixes must not truncate the page")
|
||||
assert.Zero(t, cursor.maxKeys, "only the live prefixes may spend the page budget")
|
||||
}
|
||||
|
||||
// TestPerPrefixBudgetNotShared is a regression test for the bug reported in issue #10847:
|
||||
// one large all-deleted prefix exhausts the shared probe budget, causing every later
|
||||
// prefix to be reported as visible even when all its objects are delete-marked.
|
||||
//
|
||||
// The fix gives each candidate prefix its own probe budget (hiddenProbeBudget, 10000 by
|
||||
// default). The test sets cursor.probeBudget = 3 to stay fast without touching the
|
||||
// package-level constant.
|
||||
func TestPerPrefixBudgetNotShared(t *testing.T) {
|
||||
// Build 5 delete-marked objects under "bigdeleted/" – more than the budget of 3.
|
||||
bigDeletedEntries := make([]*filer_pb.Entry, 0, 5)
|
||||
for i := 0; i < 5; i++ {
|
||||
bigDeletedEntries = append(bigDeletedEntries, deleteMarkedVersionsDir(
|
||||
"obj"+strconv.Itoa(i),
|
||||
))
|
||||
}
|
||||
|
||||
client := &testFilerClient{
|
||||
entriesByDir: map[string][]*filer_pb.Entry{
|
||||
"/buckets/test": {
|
||||
newDir("bigdeleted"),
|
||||
newDir("emptydeleted"),
|
||||
newDir("live"),
|
||||
},
|
||||
"/buckets/test/bigdeleted": bigDeletedEntries,
|
||||
"/buckets/test/emptydeleted": {deleteMarkedVersionsDir("obj")},
|
||||
"/buckets/test/live": {liveVersionsDir("obj")},
|
||||
},
|
||||
}
|
||||
|
||||
// probeBudget: 3 simulates the bug scenario with a tiny dataset; real default is
|
||||
// hiddenProbeBudget (10000).
|
||||
seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"},
|
||||
&ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true, probeBudget: 3})
|
||||
|
||||
// "bigdeleted" exhausted its own probe budget, so it is reported rather than
|
||||
// proven all-deleted.
|
||||
assert.Contains(t, seen, "bigdeleted", "a prefix whose probe budget ran out must be reported")
|
||||
|
||||
// "emptydeleted" must be hidden: all its objects are delete-marked and its probe
|
||||
// budget was not exhausted by "bigdeleted".
|
||||
assert.NotContains(t, seen, "emptydeleted", "all-deleted prefix after a budget-exhausting prefix must still be hidden")
|
||||
|
||||
// "live" must always appear.
|
||||
assert.Contains(t, seen, "live", "prefix with a live current version must be listed")
|
||||
}
|
||||
|
||||
// TestPerRequestProbeBudgetStillBoundsWork covers the request-wide ceiling:
|
||||
// deleted prefixes do not spend maxKeys, so a page can walk an unbounded
|
||||
// number of them. Once probedEntries exceeds the request total, a later
|
||||
// all-deleted prefix is reported rather than proven.
|
||||
func TestPerRequestProbeBudgetStillBoundsWork(t *testing.T) {
|
||||
deleted := func(n int) []*filer_pb.Entry {
|
||||
entries := make([]*filer_pb.Entry, 0, n)
|
||||
for i := 0; i < n; i++ {
|
||||
entries = append(entries, deleteMarkedVersionsDir("obj"+strconv.Itoa(i)))
|
||||
}
|
||||
return entries
|
||||
}
|
||||
|
||||
client := &testFilerClient{
|
||||
entriesByDir: map[string][]*filer_pb.Entry{
|
||||
"/buckets/test": {
|
||||
newDir("d1"),
|
||||
newDir("d2"),
|
||||
},
|
||||
"/buckets/test/d1": deleted(4),
|
||||
"/buckets/test/d2": deleted(4),
|
||||
},
|
||||
}
|
||||
|
||||
// Per-prefix budget (10) can cover either prefix alone, but the request
|
||||
// total (5) is spent probing "d1", so "d2" is reported without a full scan.
|
||||
seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"},
|
||||
&ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true, probeBudget: 10, probeTotalBudget: 5})
|
||||
|
||||
assert.NotContains(t, seen, "d1", "a fully probed all-deleted prefix stays hidden")
|
||||
assert.Contains(t, seen, "d2", "a prefix probed past the request total is reported")
|
||||
}
|
||||
Reference in new issue
Block a user