[
https://issues.apache.org/jira/browse/HDDS-17394?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17394:
------------------------------
Attachment: CR-4-thread-safe-reconfiguration-complete-callbacks.patch
> Registering a reconfiguration callback while a reconfiguration is completing
> skips the remaining callbacks and the status listener
> ----------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17394
> URL: https://issues.apache.org/jira/browse/HDDS-17394
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> CR-4-thread-safe-reconfiguration-complete-callbacks.patch,
> TestBugCR4CallbackRegisteredDuringReconfiguration.java
>
>
> h3. Mechanism
> {{ReconfigurationHandler}} keeps its complete callbacks in a plain
> {{ArrayList}}. {{ReconfigurationHandler.registerCompleteCallback}} appends to
> it without a lock, and {{ReconfigurationHandler.triggerCompleteCallbacks}}
> iterates it on the reconfiguration task thread (while holding
> {{ReconfigurableBase}}'s {{reconfigLock}}, which registration does not take).
> An append during that loop makes the iterator throw
> {{ConcurrentModificationException}} on its next step. {{ReconfigurableBase}}
> catches it and logs "Reconfiguration complete callback threw exception" at
> WARN, so every callback after the one that was running is skipped, and so is
> the status listener.
> The task has already recorded every changed property as successful and set
> the new values on the live configuration before the callbacks run. {{ozone
> admin reconfig status}} therefore reports success, and running the
> reconfiguration again finds no change, so the skipped callbacks are not run
> again.
> The sibling collections in the class are a {{ConcurrentHashMap}} and a
> {{ConcurrentSkipListSet}}, and
> {{ReconfigurableBase.addReconfigurationCompleteCallback}} synchronizes, so
> the plain list looks like an oversight.
> h3. Trigger
> In the OM the only registration that can happen while the process runs is in
> the {{DirectoryDeletingService}} constructor.
> {{OzoneManager.installCheckpoint}} constructs a new one ({{keyManager.start}}
> or {{reloadOMState}}) whenever a follower installs a checkpoint from the
> leader. Nothing orders that with a reconfiguration task an admin started on
> that same follower with {{ozone admin reconfig --service=OM --address=<that
> OM host:port> start}}: the install holds the OM monitor, which the
> reconfiguration thread does not take, and stopping the RPC server does not
> stop a task that is already running.
> The datanode and SCM register their callbacks before their RPC servers start,
> so they are not affected today.
> h3. Impact
> Which callbacks are lost depends on when the append lands. The OM list holds,
> in order, the SCM proxy reload callback (only with an SCM service ID), the
> tracing callback, and one callback per {{DirectoryDeletingService}} ever
> constructed.
> * Usually the skipped callbacks are those of directory deleting services that
> the install has already stopped, plus the status listener, which only logs.
> Running those callbacks would restart the stopped services (HDDS-17296), so
> skipping them is harmless, and the new instance reads the new values from the
> OM configuration. The visible effect is the WARN with the stack trace.
> * If the same reconfiguration also changes the SCM node list, and the append
> lands while the SCM proxies are being reloaded, the tracing callback is
> skipped too, so a tracing change reported as successful is not applied until
> the next restart or reconfiguration that changes a tracing key. Argued from
> the code and shown with the real handler in the reproduction, not run with a
> real checkpoint install.
> h3. Reproduction
> PASS, deterministic, unmodified source.
> [^TestBugCR4CallbackRegisteredDuringReconfiguration.java]
> ({{hadoop-hdds/framework}}) drives a real {{ReconfigurationHandler}} and its
> reconfiguration task. The first callback registers another callback from a
> second thread and waits for it, which stands in for the
> {{DirectoryDeletingService}} constructor on the install path. The callback
> registered before the reconfiguration and the status listener are not called,
> the WARN with {{ConcurrentModificationException}} is logged, the status
> reports the property change as successful, and a second reconfiguration finds
> no change and still does not call the skipped callback. A passing test means
> the defect is present.
> h3. Patch
> [^CR-4-thread-safe-reconfiguration-complete-callbacks.patch], against
> 9c3e5477ffb9f41da639051aaa5e9d7bd8b53fe4. It also applies to master at
> c521a16af75.
> The list becomes a {{CopyOnWriteArrayList}}. Its iterator works on the
> callbacks registered when the loop started, so a concurrent registration
> cannot throw, every earlier callback runs, and the new callback runs from the
> next reconfiguration on. A service created during a reconfiguration already
> reads the new values, so it does not need the current run.
> Covered by {{runsAllCallbacksWhenOneIsRegisteredDuringReconfiguration}} in
> the existing {{TestReconfigurationHandler}}: a callback registers another one
> from a second thread during a real reconfiguration; the test checks that the
> earlier callback and the status listener run, and that the new callback runs
> in the next reconfiguration. Without the change neither the earlier callback
> nor the status listener is called. With it {{TestReconfigurationHandler}} and
> {{TestOzoneIntegrationNonHA}} (340 tests, including the OM, SCM and datanode
> reconfiguration and reconfig shell suites) pass, and checkstyle is clean.
> With the patch the reproduction fails at its first assertion, because the
> later callback and the listener are called.
> The race needs an install that constructs a {{DirectoryDeletingService}}; the
> patch attached to HDDS-17296 replaces that per instance registration with one
> registered at OM construction, which removes the OM trigger. This change
> keeps the handler safe for any registration at runtime.
> Found by code review of the BackgroundService restart and shutdown paths and
> the reconfiguration callback handling, as part of the TLA+ verification
> effort under HDDS-15926, on commit 9c3e5477ffb9f41da639051aaa5e9d7bd8b53fe4.
> Checked against HDDS issues and apache/ozone pull requests for duplicates
> before filing. Related but not duplicates: HDDS-12554 (added the callback
> list), HDDS-17296 (stale directory deleting service callbacks after an
> install). 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]
