[
https://issues.apache.org/jira/browse/HDDS-17268?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17268:
------------------------------
Attachment: TestBugMC4StaleLeaderCommandBeforeTermInit.java
> Datanode ignores the heartbeat SCM term before term init and updates it
> without the lock, so a stale SCM leader's command can run
> ---------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17268
> URL: https://issues.apache.org/jira/browse/HDDS-17268
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-4-use-heartbeat-term-before-term-init-and-lock-term-update.patch,
> TestBugMC4StaleLeaderCommandBeforeTermInit.java,
> TestBugMC5HeartbeatTermRegression.java
>
>
> h3. Mechanism
> The datanode fences commands of a stale SCM leader with
> {{StateContext.termOfLeaderSCM}} (HDDS-3988): {{getNextCommand}} drops a
> command whose term is lower than the latest term seen. HDDS-7621 added the
> term to heartbeat responses so that the datanode also follows a new leader
> that sends no commands. Two defects in
> {{StateContext.updateTermOfLeaderSCM(long)}}, the method that applies the
> heartbeat term, weaken that fence.
> * *Heartbeat term ignored before init.* The method returns without doing
> anything while {{termOfLeaderSCM}} is empty, which it is after every datanode
> start. {{initTermOfLeaderSCM}} later sets the term from the queued commands
> only. If the first commands come from the old leader, the datanode settles on
> the old term and runs them, although it had already received the new term in
> heartbeat responses. The javadoc of {{initTermOfLeaderSCM}} says the datanode
> must "detect latest leader SCM before handling any SCMCommand, so that it
> won't be disturbed by stale leader SCM".
> * *Unlocked check then set.* {{HeartbeatEndpointTask.processResponse}} calls
> the method without {{StateContext.lock}}, while {{addCommand}} and
> {{getNextCommand}} call it under the lock. Each SCM endpoint has its own
> heartbeat thread. Two responses with terms 2 and 3 processed at the same time
> can leave the term at 2 after 3 was applied, and {{getNextCommand}} then
> returns a queued term 2 command instead of dropping it.
> An old leader does deliver commands of its term. It stamps the term when it
> creates a command and can create them until Ratis notifies it that it lost
> leadership. {{CommandQueue}} is not cleared on step down, and
> {{SCMNodeManager.processHeartbeat}} hands queued commands out on followers
> too, without a term in the response.
> h3. Trigger
> Heartbeat term ignored before init: a datanode starts (or restarts) while SCM
> leadership has moved from SCM A (term 1) to SCM B (term 2), and A still has
> commands of term 1 queued for that datanode, or A has not yet stepped down. B
> answers the datanode's heartbeats with term 2 but has no command for it yet.
> A's command is then the first in the datanode's queue.
> Unlocked update: two SCMs answer the same datanode as leader at the same time
> (an old leader at term 2 that has not stepped down yet, the new leader at
> term 3), the two responses are processed concurrently and the term 2 write
> lands between the read and the write of the term 3 update.
> h3. Impact
> * A command decided by the old leader runs on the datanode after the new
> leader took over, for example a force delete of a container replica. In the
> reproduction a force delete of term 1 deleted the replica after the datanode
> had received five heartbeat responses with term 2. The new leader did not
> decide that delete and does not know about it: when an SCM becomes leader,
> {{ReplicationManager.notifyStatusChanged}} clears the pending replication and
> delete ops, with the comment "They are no longer needed as the DN will
> discard any commands when the term changes". Its own over replication
> handling can therefore delete another replica of the same container, which is
> the double delete of HDDS-16449, there described as masked by this datanode
> term gate. Shown for one replica, multiple replicas and the resulting
> redundancy were not exercised. Re-replication restores redundancy afterwards.
> * The damage is bounded in time. ReplicationManager commands carry a datanode
> deadline ({{hdds.scm.replication.event.timeout}} 12m minus
> {{hdds.scm.replication.event.timeout.datanode.offset}} 6m) after which
> {{DeleteContainerCommandHandler}} and {{ReplicationSupervisor}} drop them, so
> a stale ReplicationManager delete runs only within about 6 minutes of its
> creation, and the new leader's ReplicationManager waits
> {{hdds.scm.wait.time.after.safemode.exit}} (5m) after it becomes ready before
> it acts. The reproduction sends a command without a deadline.
> * With the race, the term goes back from 3 to 2 until the next heartbeat of
> the new leader (default interval 30s), and term 2 commands queued in that
> time are dispatched. The race needs two SCMs answering as leader at once, so
> it is rare.
> * A datanode that already has its term is not affected by the first defect;
> it drops the same command ("Detect and drop a SCMCommand ... from stale
> leader SCM").
> h3. Reproduction
> Both attached classes pass on unmodified source when the bug is present
> (target path and Maven command in each class comment). Not run with real SCMs
> or a real SCM failover.
> * [^TestBugMC4StaleLeaderCommandBeforeTermInit.java]
> ({{hadoop-hdds/container-service}}): a real {{DatanodeStateMachine}} daemon
> over Hadoop RPC against three {{ScmTestMock}} SCMs that put their term into
> the heartbeat response only while they believe they lead, like
> {{SCMDatanodeProtocolServer.sendHeartbeat}}. Two cases: the old leader still
> believes it leads, or it is a follower that still holds the command. In both,
> the term 1 force delete removes the container (about 15s).
> * [^TestBugMC5HeartbeatTermRegression.java]
> ({{hadoop-hdds/container-service}}): two real {{HeartbeatEndpointTask}}s on
> their own threads against mocked SCM proxies (term 2 with a close container
> command, term 3 without). A Mockito spy on the {{StateContext}} holds the
> term 2 write between its check and its write until the term 3 update has
> finished, which makes the race deterministic. The term ends at 2 and
> {{getNextCommand}} returns the term 2 command.
> h3. Patch
> [^MC-4-use-heartbeat-term-before-term-init-and-lock-term-update.patch],
> against b10a7d427bdf83807a7e48a1be507d3b02d5a503. It also applies to master
> at e0de543cbe9.
> While the term is not initialized, {{updateTermOfLeaderSCM(long)}} records
> the highest term it is given, and {{initTermOfLeaderSCM}} uses the larger of
> that and the highest queued command term. The method takes
> {{StateContext.lock}}, the lock its other two callers already hold, which
> also keeps recording and init atomic. {{termOfLeaderSCM}} becomes volatile
> for the command handlers that read it from their own threads. Nothing changes
> once the term is set, and init still happens at the same moment.
> This narrows the window but does not close it: if the datanode initializes
> its term before it has processed a heartbeat response of the new leader (init
> only needs a majority of SCMs in HEARTBEAT state), a command of the old term
> is still accepted. Waiting for a leader heartbeat term before init would
> close it, but would stop command processing when no SCM sends a term (an SCM
> older than HDDS-7621, or no leader), so it is left out. Clearing
> {{CommandQueue}} when an SCM steps down would remove the follower route on
> the SCM side.
> Covered by {{dropsCommandOfStaleLeaderIfNewerTermWasSeenBeforeInit}},
> {{initsTermFromCommandIfOlderTermWasSeenBeforeInit}} (negative case) and
> {{concurrentTermUpdatesKeepHighestTerm}} in the existing
> {{TestStateContext}}. Without the change the first fails with the term 1
> command returned and the third with term 2 instead of 3. With it
> {{TestStateContext}}, {{TestHeartbeatEndpointTask}},
> {{TestHeartbeatEndpointTaskDnsRefresh}},
> {{TestDeleteContainerCommandHandler}}, {{TestReplicationSupervisor}},
> {{TestRunningDatanodeState}} and {{TestCloseContainerCommandHandler}} pass
> (113 tests), checkstyle is clean, and both attached reproductions fail
> because the stale command is dropped.
> 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-3988 (introduced the term fencing), HDDS-7621 (added the
> heartbeat term and the early return), HDDS-5330 (stale commands in sub queues
> after the term is set), HDDS-16449 (redundant delete after an SCM leadership
> transfer, masked by the datanode term gate) and HDDS-16804
> ({{ReplicationSupervisor}} loses a command of the new leader behind an equal
> task of the old leader). 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]
