Siyao Meng created HDDS-17394:
---------------------------------
Summary: 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
Reporter: Siyao Meng
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]
