WIP: feat(executor): allocator-backed OOM guard behind an opt-in oom-guard feature#2033
Draft
andygrove wants to merge 2 commits into
Draft
WIP: feat(executor): allocator-backed OOM guard behind an opt-in oom-guard feature#2033andygrove wants to merge 2 commits into
andygrove wants to merge 2 commits into
Conversation
A task that exhausts its memory budget currently fails the entire job on
the first occurrence: `From<BallistaError> for FailedTask` maps everything
that is not an IO error to `FailedReason::ExecutionError`, and the
scheduler fails the stage outright on that reason with no retry.
A memory exhaustion is often transient. The retried task may land on a
less-loaded executor, or run once its peers on the same executor have
drained. Classify it as retriable so the scheduler reschedules it,
bounded by --task-max-failures, rather than failing the job outright.
Match the error through DataFusionError::find_root(), because DataFusion
routinely wraps errors in Context, and in Shared when propagating one
stream error to several output partitions (RepartitionExec). A bare
match on the outer error would miss most real cases.
The unpartitioned shuffle-write path flattened the error with
`format!("{e:?}")` before it reached the classifier, which destroyed the
type and made the new arm unreachable for exactly the stages most likely
to exhaust memory: the final stage, the broadcast-join build side, and
the CoalescePartitions and SortPreservingMerge input stages. Preserve the
DataFusionError instead of formatting it.
…d feature Executor memory accounting relies on voluntary MemoryPool reservations, which miss allocations made by Arrow buffers, join scratch space, and expression kernels. Real native memory therefore runs above what the pool believes is reserved, and the executor process can be OOM-killed. That is far worse than a single failed task: a dead executor takes all of its shuffle output with it, so every downstream stage that would have read from it raises a fetch-partition error, cascading into stage rollbacks and map-stage re-runs, potentially across other concurrent jobs. Add a global allocator that tracks the bytes actually handed out, and use that signal in two layers: - RealUsagePool, a MemoryPool decorator that rejects growth once real live usage plus the request would exceed the budget, so DataFusion spills rather than growing into an OOM. This is the layer that does the work. - MemoryGuardExec, a transparent pass-through plan node on each stage's data path that checks the budget between batches and fails that one task as a last resort. It debounces, so a transient spike that the pool gate is already resolving does not shed every task on the executor. The allocator itself only tracks: unwinding out of a GlobalAlloc is undefined behaviour, so enforcement happens at safe points instead. Live bytes are counted in cache-line-padded shards, which keeps the count exact across thread churn without a Drop-carrying thread-local (whose lazy storage would allocate from inside the allocator). The guard enforces on tracked live bytes rather than RSS. RSS is what the OOM killer reads, but an allocator does not promptly return freed pages, so a spill that genuinely released memory would leave RSS high and the guard latched on, failing every subsequent task. Failing tasks cannot reclaim memory that is already free. RSS is sampled for observability only, and a warning is logged when it diverges above the limit while tracked usage stays below it, which is the signal that the allocator is holding memory the guard cannot reclaim. The feature is off by default. It installs a tracking global allocator, so it is a build-time opt-in and the default build is unchanged, with no per-allocation overhead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Relates to #2031. Prior art: apache/datafusion-comet#4582, which explores the same idea for Comet. This diverges from that prototype in one important way — see "Deviations from the Comet prototype".
Rationale for this change
Executor memory accounting relies on voluntary
MemoryPoolreservations, which miss Arrow buffers, join scratch space, and expression-kernel temporaries. Real native memory therefore runs above what the pool believes is reserved, and the executor process can be OOM-killed.In Ballista that is far worse than one failed task: a dead executor takes all of its shuffle output with it, so every downstream stage that would have read from it raises
FetchPartitionError, cascading into stage rollbacks and map-stage re-runs — potentially across other concurrent jobs.What changes are included in this PR?
A new
oom-guardcargo feature (off by default) that installs a global allocator tracking the bytes actually handed out, and uses that signal in two layers:RealUsagePool— aMemoryPooldecorator that rejectstry_growwithResourcesExhaustedonce real live usage plus the request would exceed the budget, so DataFusion spills rather than growing into an OOM. This is the layer that does the useful work.MemoryGuardExec— a transparent pass-throughExecutionPlaninserted below each stage's shuffle writer. It checks the budget between batches and fails that one task as a last resort. It debounces (3 consecutive over-budget checks and 100 ms continuously over budget), so a transient spike that the pool gate is already resolving does not shed every task on the executor.The ceiling is
--memory-pool-size. If that flag is unset the guard tracks but never enforces.Two design points worth reviewer attention
The allocator only tracks; it never enforces. Unwinding out of a
GlobalAllocis documented undefined behaviour (core::alloc::GlobalAlloc: "It's undefined behavior if global allocators unwind... a panic from any of these functions may lead to memory unsafety"). So enforcement happens at safe points — memory-pool growth, and plan poll boundaries — instead. Live bytes are counted in cache-line-padded atomic shards, which keeps the count exact across thread churn without aDrop-carrying thread-local (whose lazy storage would allocate from inside the allocator).The guard enforces on tracked live bytes, not RSS. RSS is what the OOM killer reads, but allocators do not promptly return freed pages, so a spill that genuinely released memory would leave RSS high and the guard latched on, failing every subsequent task — turning a safety feature into an outage. Failing tasks cannot reclaim memory that is already free. RSS is sampled for observability only, and a warning is logged when it climbs above the limit while tracked usage stays below it, which is precisely the signal that the allocator is holding memory the guard cannot reclaim.
Deviations from the Comet prototype
The Comet prototype raises a panic from inside
GlobalAlloc::allocand catches it withcatch_unwind. I did not port that, for two reasons, and both apply to Comet as well:RepartitionExecspawns one per input partition, hash-join build side,SortPreservingMerge), and a panic raised inside one of those is swallowed by tokio as aJoinErrorand never reaches the catch site. Since the breaker disarms itself on trip, it would be permanently disarmed after its first real trip.Known limitations (please read before reviewing)
fetch_addto every allocation. A TPC-H benchmark (feature on vs off, under both AQE settings) is the next step and is the gate on whether this could ever be default-on. Until then this is a well-tested hypothesis.poll_nextis not caught. The pool gate is the first line of defence; the breaker only shrinks the blast radius. This feature reduces the probability of an executor dying; it does not prevent it.--memory-pool-sizechanges meaning in anoom-guardbuild: the ceiling is compared against all live process bytes (gRPC, tokio, object-store caches), not just query memory, so the pool gates somewhat earlier than the same value implies in a default build. Documented in the CLI help and the tuning guide. A separate--memory-guard-limitmay be the better answer; I would take guidance here.sort_shuffle/writer.rsdoeslet _ = reservation.try_grow(...), discarding pool rejection in favour of its own absolute byte counter, soRealUsagePoolcannot make it spill. The breaker still covers it. Wiring its spill decision to the pool result felt out of scope here.Are there any user-facing changes?
Only for those who opt in at build time (
cargo build --release --features oom-guard). The default build is unchanged: no allocator is installed and there is no per-allocation overhead. Documented indocs/source/user-guide/tuning-guide.mdandcargo-install.md.