[
https://issues.apache.org/jira/browse/HDDS-17279?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17279:
------------------------------
Attachment:
MC-7-synchronize-scm-endpoint-add-remove-with-report-fan-out.patch
> Adding or removing an SCM on a running datanode can make report producers
> throw, losing reports and stopping command status reports until restart
> -------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17279
> URL: https://issues.apache.org/jira/browse/HDDS-17279
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.2.0, 2.2.1
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-7-synchronize-scm-endpoint-add-remove-with-report-fan-out.patch,
> TestBugMC7AddEndpointRace.java
>
>
> h3. Mechanism
> {{StateContext}} keeps one queue per SCM endpoint for incremental reports,
> container actions and pipeline actions, plus a plain {{HashSet}} of
> endpoints. The producers ({{addIncrementalReport}}, {{addContainerAction}},
> {{addContainerActionIfAbsent}}, {{addPipelineActionIfAbsent}}) iterate that
> set and look up each endpoint's queue. {{StateContext.addEndpoint}} adds the
> endpoint to the set before it creates the queues, and neither {{addEndpoint}}
> nor {{removeEndpoint}} takes the locks the producers hold. A producer running
> at the same time therefore gets {{null}} for the new (or just removed)
> endpoint and throws a {{NullPointerException}}, or throws
> {{ConcurrentModificationException}} from the set iterator. The endpoints
> after it in the iteration miss that report or action. The queue size getters
> read by {{DatanodeQueueMetrics}} iterate the same maps without the locks, so
> a metrics read can also throw {{ConcurrentModificationException}}.
> {{ReportPublisher.publishReport}} catches only {{IOException}}, and the
> publishers run with {{scheduleAtFixedRate}}. When the exception is thrown
> inside {{CommandStatusReportPublisher}}, the scheduled task ends and is never
> rescheduled. The command statuses it had just removed from {{cmdStatusMap}}
> reach no SCM, and no command status report is sent to any SCM until the
> datanode restarts. {{refreshFullReport}} iterates the plain {{HashMap}}
> {{isFullReportReadyToBeSent}} that {{addEndpoint}} inserts into, so the full
> report publishers can end the same way (argued from the code, not run).
> h3. Trigger
> An Ozone administrator adds or removes an SCM with {{ozone admin reconfig}}
> of {{ozone.scm.nodes.<service>}} on a running datanode
> ({{HddsDatanodeService.reconfigScmNodes}}, added by HDDS-13890), while
> another thread is fanning out a report or action. Producers include container
> state changes (ICRs), pipeline reports, container and pipeline actions, and
> the periodic command status report (default every 60s). The window is the few
> microseconds {{addEndpoint}} or {{removeEndpoint}} take, so a single
> reconfiguration hits it rarely.
> h3. Impact
> * The existing SCMs miss the report or action that hit the window (for
> example an ICR).
> * If the command status report publisher hits it, command status reports stop
> for the life of the process. They are how SCM learns that block deletion
> commands completed, so SCM gets no more block deletion acknowledgements from
> this datanode until it restarts, and completed command statuses accumulate in
> {{cmdStatusMap}} (argued from the code, not run).
> * If a full report publisher hits it, SCM gets no more refreshed node reports
> (capacity and usage) or periodic full container reports, depending on the
> publisher, from this datanode until it restarts (argued from the code, not
> run).
> * A throw inside container request handling (the close container action that
> {{HddsDispatcher}} adds for a write to a full or unhealthy container, or the
> ICR sent when a write creates a container) fails that write and marks the
> Ratis container state machine unhealthy, which closes the pipeline on this
> datanode. The client retries on another pipeline (argued from the code, not
> run).
> * No crash of the datanode process and no data loss.
> h3. Reproduction
> Deterministic, on unmodified source: [^TestBugMC7AddEndpointRace.java]
> ({{hadoop-hdds/container-service}}). It uses the public {{StateContext}} and
> {{CommandStatusReportPublisher}} APIs as production calls them. The window is
> held open by adding an endpoint whose {{hashCode}} parks the reconfiguration
> thread on the first map update after the endpoint entered the set. The report
> interval is lowered to 100 ms. Both tests assert the correct behaviour and
> fail on the unmodified code: {{addIncrementalReport}} throws
> {{NullPointerException}}, and the scheduled {{CommandStatusReportPublisher}}
> task ends after its run. With the patch both pass.
> h3. Patch
> [^MC-7-synchronize-scm-endpoint-add-remove-with-report-fan-out.patch],
> against b10a7d427bdf83807a7e48a1be507d3b02d5a503. It also applies to master
> at e0de543cbe9.
> Each producer now iterates the values of the per endpoint map it writes to,
> under the lock that already guards that map, instead of iterating the
> separate endpoint set. {{addEndpoint}} and {{removeEndpoint}} update those
> maps under the same locks, and {{isFullReportReadyToBeSent}} becomes a
> {{ConcurrentHashMap}}. The queue size getters used by
> {{DatanodeQueueMetrics}} now read those maps under the same locks too. No new
> lock and no change in behaviour when the SCM set does not change.
> {{DatanodeQueueMetrics}} has the same pattern on its own maps (iterated in
> {{getMetrics}}, written by its {{addEndpoint}} and {{removeEndpoint}}) and is
> not covered by this patch.
> Covered by {{testEndpointChangeDuringReportFanOut}} in the existing
> {{TestStateContext}}: the test thread fans out reports and actions and reads
> the queue sizes for 10000 rounds while another thread keeps adding and
> removing an endpoint, then checks that the existing endpoint got every report
> and action. Without the change it failed in 4 of 4 runs with a
> {{ConcurrentModificationException}} (a {{NullPointerException}} is also
> possible); with only the getters left unlocked it failed in 3 of 3 runs in a
> getter; with the patch it passes in about 3s. {{TestStateContext}},
> {{TestHddsDispatcher}}, {{TestHeartbeatEndpointTask}},
> {{TestReportPublisher}}, {{TestReportManager}}, {{TestRunningDatanodeState}}
> and the mini cluster test {{TestDatanodeSCMNodesReconfiguration}} pass,
> checkstyle is clean.
> Found by TLA+ model checking and code review of the datanode registration and
> heartbeat paths under HDDS-15926, on commit
> b10a7d427bdf83807a7e48a1be507d3b02d5a503. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-13890 (added the runtime SCM add and remove) and HDDS-15369
> (datanode shutdown on {{ozone.scm.nodes}} reconfiguration). The attached
> patch is a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
