[
https://issues.apache.org/jira/browse/HDDS-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17331:
------------------------------
Attachment: TestBugMC2ResizeLostUpdate.java
> 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
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Priority: Minor
> Attachments: MC-1-serialize-replication-supervisor-pool-resize.patch,
> TestBugMC1ResizeIllegalArgument.java, TestBugMC2ResizeLostUpdate.java
>
>
> 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]
