HADOOP-18091. S3A auditing leaks memory through ThreadLocal references #3930
Conversation
This file contains 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
|
note, does not yet update the docs |
|
needs unit tests of the basic map class and its operations |
steveloughran
commented
Jan 26, 2022
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/audit/MemoryHungryAuditor.java
Outdated
Show resolved
Hide resolved
...op-aws/src/test/java/org/apache/hadoop/fs/s3a/audit/TestActiveAuditManagerThreadLeakage.java
Outdated
Show resolved
Hide resolved
A map where references can be garbage collected. This makes it a best-effort map Change-Id: I4622f51f87cfe0fb50629e447f6828c8b5072c8c
if this is working, hope to use for iostats snapshots too Change-Id: I808ced6dc4bfbf2b961174dbdb544636a6253592
Creates managers in a separate thread, creates spans. runs out of memory Change-Id: Ic4ff382e4edfa37ec7feb46265ba5a63e1abb560
no longer prunes anything, however. Change-Id: I71f4f36dcc0900531441b870f3f5de79c64fb2ee
need to create lots of spans during the life of an auditor so the memory consumption is enough for GC to care. Change-Id: Iad3a350b8243031ad158c966e130bab77ab59fb3
* hadoop common TestWeakReferenceMap for map semantics and recreating of entries on demand. * TestActiveAuditManagerThreadLeakage test enhanced a lot with tests of demand creation etc * auditing docs brought up to date * CommonAuditContext docs cover memory leak risks, as does audit arch. the put() call will log call stack if logging at trace, to provide a way to debug problems, if they ever surface. * s3a committers explicitly remove jobid in commit/abort ops. (manifest committer should will do the same) * auditing is still turned off for consistency with Hadoop 3.3.2 its there for support calls, people can deploy it in production when they see benefits, but there's no cost/risk to others. Change-Id: I63ec9ec35d80d25d9e3cf1eedaca3aff595a06fa
If, somehow, spans were not being deactivated from threads which where then destroyed, then the map would fill up with weak references to those spans. They wouldn't stop GC-ing of the audit manager, but they would fill up the hash table. Now, when a span is deactivated. 1. its entry is removed from the map (not replaced with the unbounded one, as that will happen on demand) 2. when the unbounded span is active on a thread, its deactivate() call also removes it from the map. 3. every so often (PRUNE_THRESHOLD = 10000), a full prune of the reference map is performed. this will remove any null references. Because of 1 & 2, when valid or unbound spans are deactivated, the map shrinks. Because of 3, even if threads are destroyed with valid spans active, after a GC the fact that they have been GC'd will surface. This doesn't remove map entries with unbonded spans active as the strong references held to them means they get GC'd. We could do that with a prune() in the audit manager removing all entries where !isValid() is true. Change-Id: I62fc9d66216dd8fdafb22d8b57dcdd6d311915e3
Turning auditing back on. Change-Id: I46032e467cc399a0f292f756a4bf7d1fd5e30352
Change-Id: I4af0a49cc87a0998578c3170e5199665f7f572bc
Change-Id: I6b2c76131e520081c649a89a638b03b2398780e1
steveloughran
force-pushed
the
s3/HADOOP-18091-thread-map
branch
from
279a19c to
ec7fac6
Compare
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/auditing_architecture.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/auditing_architecture.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/auditing_architecture.md
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/fs/s3a/audit/impl/TestActiveAuditManagerThreadLeakage.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/fs/s3a/audit/impl/TestActiveAuditManagerThreadLeakage.java
Show resolved
Hide resolved
...s/src/test/java/org/apache/hadoop/fs/s3a/audit/impl/TestActiveAuditManagerThreadLeakage.java
Outdated
Show resolved
Hide resolved
| // but if memory allocation in test runs increase, it | ||
| // may cease to hold. in which case: create more | ||
| // audit managers | ||
| LOG.info("Totel prune count {}", pruneCount); |
...s/src/test/java/org/apache/hadoop/fs/s3a/audit/impl/TestActiveAuditManagerThreadLeakage.java
Outdated
Show resolved
Hide resolved
mehakmeet
reviewed
Feb 2, 2022
| @@ -111,6 +118,14 @@ | |||
| public static final String NOT_A_WRAPPED_SPAN | |||
| = "Span attached to request is not a wrapped span"; | |||
|
|
|||
| /** | |||
| * Arbritary threshold for triggering pruning on deactivation. | |||
| * non-atomic and non-blocking. | ||
| * @return the number of entries pruned. | ||
| */ | ||
| public int prune() { |
There was a problem hiding this comment.
Why do we need to prune for WeakReferences, thought the point of using them was that we don't need to manually do this, and they'll be cleared automatically through the gc? Maybe I'm not understanding this right, please let me know
There was a problem hiding this comment.
because the map will become full of threadId to null entries, which consumes small amounts of memory (one record per terminated thread). pruning just cleans it all out
| * @param key key of lost reference | ||
| */ | ||
| private void noteLost(final K key) { | ||
| // incrment local counter |
| return remove(curentThreadId()); | ||
| } | ||
|
|
||
| public long curentThreadId() { |
| _Auditing was disabled by default until a fix was implemented._ | ||
|
|
||
| The memory leak has been fixed using a new class `org.apache.hadoop.util.WeakReferenceMap` | ||
| to store a map of thread IDs to active spans. When the S3A fileystem is closed, |
| `org.apache.hadoop.fs.audit.CommonAuditContext` to `TRACE` | ||
| to log the origin of operations which add log entries. | ||
|
|
||
| This will produce a log entry whose strack trace will show the caller chain.f |
| } | ||
|
|
||
| /** | ||
| * Verifu that pruning takes place intermittently. |
| @@ -82,3 +82,6 @@ log4j.logger.org.apache.hadoop.fs.s3a.S3AStorageStatistics=INFO | |||
| #log4j.logger.org.apache.hadoop.fs.s3a.audit=DEBUG | |||
| # log request creation, span lifecycle and other low-level details | |||
| #log4j.logger.org.apache.hadoop.fs.s3a.audit=TRACE | |||
|
|
|||
| # uncomment this to trace where context entries are set | |||
| # log4j.logger.org.apache.hadoop.fs.audit.CommonAuditContext=TRACE | |||
|
@mehakmeet @mukund-thakur thanks for your feedback; will address this week |
* typos and nits * cleanup in test teardown * tuning docs Change-Id: I088f050fa34b097266b70f3e078da89e083dfdf9
|
🎊 +1 overall
This message was automatically generated. |
|
ok. just want to make sure i've got all those typos down |
Change-Id: I1809cec29d9354185d759e24e65688a44d50cb89
|
🎊 +1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Feb 10, 2022
#3930) Adds a new map type WeakReferenceMap, which stores weak references to values, and a WeakReferenceThreadMap subclass to more closely resemble a thread local type, as it is a map of threadId to value. Construct it with a factory method and optional callback for notification on loss and regeneration. WeakReferenceThreadMap<WrappingAuditSpan> activeSpan = new WeakReferenceThreadMap<>( (k) -> getUnbondedSpan(), this::noteSpanReferenceLost); This is used in ActiveAuditManagerS3A for span tracking. Relates to * HADOOP-17511. Add an Audit plugin point for S3A * HADOOP-18094. Disable S3A auditing by default. Contributed by Steve Loughran. Change-Id: Ibf7bb082fd47298f7ebf46d92f56e80ca9b2aaf8
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
apache#3930) Adds a new map type WeakReferenceMap, which stores weak references to values, and a WeakReferenceThreadMap subclass to more closely resemble a thread local type, as it is a map of threadId to value. Construct it with a factory method and optional callback for notification on loss and regeneration. WeakReferenceThreadMap<WrappingAuditSpan> activeSpan = new WeakReferenceThreadMap<>( (k) -> getUnbondedSpan(), this::noteSpanReferenceLost); This is used in ActiveAuditManagerS3A for span tracking. Relates to * HADOOP-17511. Add an Audit plugin point for S3A * HADOOP-18094. Disable S3A auditing by default. Contributed by Steve Loughran.
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…al references (apache#3930) Adds a new map type WeakReferenceMap, which stores weak references to values, and a WeakReferenceThreadMap subclass to more closely resemble a thread local type, as it is a map of threadId to value. Construct it with a factory method and optional callback for notification on loss and regeneration. WeakReferenceThreadMap<WrappingAuditSpan> activeSpan = new WeakReferenceThreadMap<>( (k) -> getUnbondedSpan(), this::noteSpanReferenceLost); This is used in ActiveAuditManagerS3A for span tracking. Relates to * HADOOP-17511. Add an Audit plugin point for S3A * HADOOP-18094. Disable S3A auditing by default. Contributed by Steve Loughran. Change-Id: Ibf7bb082fd47298f7ebf46d92f56e80ca9b2aaf8 Conflicts: hadoop-common-project/hadoop-common/src/main/resources/core-default.xml Change-Id: Iaa092c89ab0be16f816b56be78d795ce618a8656
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.
Adds a new map type WeakReferenceMap, which stores weak references to values,
and a WeakReferenceThreadMap subclass to more closely resemble a thread local
type, as it is a map of thread id to value.
construct it with a factory method and optional callback
for notification on loss and regeneration.
This is used in ActiveAuditManagerS3A for span tracking.
If a calling method has a reference to the span then even with gc the reference will
be valid, it's only if no more references are held that there will be problems.
This does mean that if S3 a code keeps references around then back references to the auditor are retained.
but those classes which can get returned and which do have spans (list iterators, input and output streams, ...)
And all have callbacks into the main S3a file system anyway.
Testing this has been fun; about as hard as the production code.
The good news: We can do this in a unit test relatively quickly.
We just create a sequence of audit managers and in each one
Schedule tasks across a thread pool to create spans.
By providing an auditor implementation whose class and spans use lots of memory
We can trigger OOM fast on the original code.
With the new structure this doesn't happen. Instead, and after 100+ iterations
GC calls trigger removal of the only-weakly-referenced spans.
I have run the Itest suites against s3 london and all is well; retesting.
my system is set to fail if any operation is ever executed out of a span, other than those which happened during copy operations when the S3 SDK transfer manager invokes them.
That is the sole risk I can see in this world: that if and external reference is not held then the thread reference will be discarded prematurely.
I don't see it happening in this case but will review carefully just to make sure.
How was this patch tested?
New test which triggers oom on the old code, but works now.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?