Siyao Meng created HDDS-17331:
---------------------------------
Summary: Concurrent node state change and replication streams
limit reconfiguration can leave the datanode replication pool at the wrong size
or throw
Key: HDDS-17331
URL: https://issues.apache.org/jira/browse/HDDS-17331
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
h3. Mechanism
{{ReplicationSupervisor.resize}} runs without a lock. It is called from the
command processor thread through {{nodeStateUpdated}} (a
{{SetNodeOperationalStateCommand}} from SCM, via
{{SetNodeOperationalStateCommandHandler}}) and from the reconfiguration thread
through {{setReplicationMaxStreams}} and {{setPerVolumePoolSize}}
({{HddsDatanodeService.reconfigReplicationStreamsLimit}} and
{{reconfigPerVolumeStreamsLimit}}). Each call reads the streams limit and the
node state and computes a pool size. The default pool updater from
{{ReplicationSupervisor.Builder.build}} then reads {{getCorePoolSize}} and
calls {{setMaximumPoolSize}} and {{setCorePoolSize}} as two separate steps. The
per volume pools are resized the same way by {{HddsServerUtil.setPoolSize}}.
When both threads resize at the same time, two things can go wrong:
* *Lost update.* One thread computes its size from a value the other thread is
about to change, and its setters land last. Both calls return normally, but the
pool ends at a size that does not match the final streams limit and node state.
* *Exception.* One thread resizes the pool between the other thread's two
setters, so the second setter violates {{core <= max}} and throws
{{IllegalArgumentException}}. The throwing thread skips the per volume resize.
From {{nodeStateUpdated}} the exception escapes
{{SetNodeOperationalStateCommandHandler.handle}} and {{CommandDispatcher}} logs
it at ERROR. From {{setReplicationMaxStreams}} it fails the reconfiguration,
although {{ReplicationServer}} has already been resized and the new limit is
already stored in {{ReplicationConfig}}. On the per volume pools the same
exception is caught and logged at WARN per pool by
{{VolumeReplicationThreadPools.setPoolSize}}.
Both can happen in the same race: the thread that throws may be the one that
computed the correct size, and the other thread's stale size then remains.
The race came with HDDS-10237, which made the supervisor pool reconfigurable at
runtime. Before that only the command processor thread resized it.
h3. Trigger
An SCM node state change for the datanode (decommission, maintenance, or the
way back to IN_SERVICE) handled at the same moment as an admin reconfiguration
of {{hdds.datanode.replication.streams.limit}} (or
{{hdds.datanode.replication.per.volume.streams.limit}}, only with
{{hdds.datanode.replication.per.volume.enabled}}, which is off by default) on
that datanode. The window is short: from the start of {{resize}} to its pool
setters for the lost update (one INFO log call in between), and between the two
setters for the exception. Both events are rare and independent, so this needs
an operator who changes the streams limit while the node state is changing, for
example to speed up a decommission that was just started.
h3. Impact
* The global replication executor uses an unbounded {{PriorityBlockingQueue}},
so it never runs more threads than its core size. After a lost update fewer
replication tasks run at once than configured (4 instead of 6 in the
reproduction), while the reconfiguration reported success.
* Any later resize recomputes from the current state and limits and repairs the
pool, but a resize only happens on a real node state change or a
reconfiguration to a different value. A repeated
{{SetNodeOperationalStateCommand}} with the same state does nothing
({{nodeStateUpdated}} returns early when the state is unchanged), and a
reconfiguration to the value already stored is skipped. During a decommission
the next state change (to DECOMMISSIONED) comes after the drain, which is the
phase the out of service scaling is meant to speed up. The reverse direction
lasts longest: a node back IN_SERVICE gets no further state change, so a stale
size stays until the next streams limit change or a restart.
* On the exception path the admin is told that the reconfiguration failed
although the new limit is already stored in {{ReplicationConfig}}, and
depending on the interleaving the pool has either the new size or the stale one
(both were run). Because the failed reconfiguration is not recorded, retrying
it applies the limit again (read from the code, not run). The datanode logs an
ERROR for the node state command. The persisted node state is not affected.
No data is lost. The effect is on replication throughput and on the feedback
the operator gets.
h3. Reproduction
Two self contained JUnit classes for {{hadoop-hdds/container-service}} (target
path and Maven command are in each class comment). Both build a real
{{ReplicationSupervisor}} on a real {{ThreadPoolExecutor}} configured like the
default one. The pool size updater, passed through the existing
{{executorThreadUpdater}} builder hook, performs the same steps as the default
updater and holds one thread until the other thread has finished its own resize
(or, with a fix, is blocked waiting for the first one). No product code is
changed. Both pass on unmodified source, which shows the bug.
* [^TestBugMC2ResizeLostUpdate.java]: streams limit 2, node IN_SERVICE.
{{nodeStateUpdated(DECOMMISSIONING)}} and {{setReplicationMaxStreams(3)}} both
return normally, and the pool ends at core 4 / max 4 where 3 x 2 = 6 is
required. A repeated {{nodeStateUpdated(DECOMMISSIONING)}} leaves it at 4, and
of 6 replication tasks submitted through {{addTask}} only 4 run at once while 2
wait in the queue.
* [^TestBugMC1ResizeIllegalArgument.java]: the {{IllegalArgumentException}}
escapes the real {{SetNodeOperationalStateCommandHandler.handle}} when a
reconfiguration (limit 2 to 1) resizes between the command's two setters. It
also escapes {{setReplicationMaxStreams(3)}} when a change from DECOMMISSIONING
to IN_SERVICE resizes between the reconfiguration's two setters, while the pool
and {{ReplicationConfig}} already reflect the new limit. That
{{ReconfigurationHandler}} then reports the reconfiguration as failed was read
from the code, not run.
* The combined case (the reconfiguration throws and the pool stays at the stale
4 / 4 where 6 is required) was run as a scratch test with the same hook and is
not attached.
Without any hook, a loop of 20000 iterations with two plain threads and the
unmodified default updater lost the update 4 times on unmodified source (pool
at 3/3 or 4/4, and the kept case ran 3 of 6 tasks at once), and never with the
patch. That run showed no exception; the exception without a hook was seen only
under extra CPU load in the original investigation. These loops are slow and
probabilistic and are not attached.
h3. Patch
[^MC-1-serialize-replication-supervisor-pool-resize.patch], against
07512078fe639f387acd52104491d5a7f3edb84c. It also applies to master at
2851f5a3057.
{{nodeStateUpdated}}, {{setReplicationMaxStreams}} and {{setPerVolumePoolSize}}
become {{synchronized}}. They are the only callers of {{resize}}, and the node
state and the limits are only written inside them, so each resize now reads the
latest state and limits and runs both setters before the next one starts. One
change removes both the lost update and the exception. Nothing called under the
lock takes another lock that is held while waiting for the supervisor, and the
only other user of that monitor is the counter initialization for a new task
type in {{initCounters}}. The lock also keeps {{maxQueueSize}}, which
{{resize}} writes from the same stale read, consistent with the final state.
Behaviour without concurrent calls is unchanged.
Covered by {{concurrentNodeStateChangeAndReconfigurationApplyLatestPoolSize}}
in the existing {{TestReplicationSupervisor}}, for both directions (IN_SERVICE
to DECOMMISSIONING and back, while the limit goes from 2 to 3). Without the
change it fails with "Expecting AtomicInteger(4) to have value: 6" (2 instead
of 3 for the way back). With it {{TestReplicationSupervisor}} (56 tests), the
command handler tests in {{hadoop-hdds/container-service}},
{{TestPerVolumePushReplication}} and {{TestOzoneIntegrationNonHA}} on a mini
cluster (340 tests, including the datanode reconfiguration tests) pass, and
checkstyle is clean. With the patch both reproduction classes fail, because the
second resize waits for the first, nothing throws and the pool ends at the
required size.
Found by TLA+ model checking and code review of the datanode
ReplicationSupervisor thread pool resize and metrics paths under HDDS-15926, on
commit 07512078fe639f387acd52104491d5a7f3edb84c. Checked against HDDS issues
and apache/ozone pull requests for duplicates before filing. Related but not a
duplicate: HDDS-8554 (resolved, setter order when a single thread shrinks the
pool). The attached patch is a proposal for review. Generated with Specula
(Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
