[
https://issues.apache.org/jira/browse/HDDS-17547?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17547:
------------------------------
Affects Version/s: 2.2.1
2.0.0
1.4.0
2.1.2
2.1.0
1.4.1
2.1.1
1.3.0
2.2.0
> GrpcOmTransport can send to a follower while its failover provider names the
> leader, so concurrent failovers move the client away from the leader
> -------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17547
> URL: https://issues.apache.org/jira/browse/HDDS-17547
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 1.3.0, 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: Minor
> Attachments:
> MC-5-grpc-om-transport-send-to-provider-current-om.patch,
> TestBugMC5GrpcLeaderTargetDesync.java
>
>
> h3. Mechanism
> {{GrpcOmTransport}} keeps the address it sends leader requests to in its own
> {{host}} field, apart from the failover provider's current OM, which is what
> the retry policy ({{OMFailoverProxyProviderBase.getRetryPolicy}}) reasons
> about. In {{GrpcOmTransport.shouldRetry}} the failover
> ({{globalFailoverCount}} check, {{performFailover}}, increment) and the store
> {{host.set(getGrpcProxyAddress(getCurrentProxyOMNodeId()))}} are separate
> steps. When two threads fail over at the same time, an older thread can read
> the current OM, be preempted, and store it after a newer thread has already
> failed over to the leader and stored that. {{host}} then names a follower
> while the provider names the leader.
> Over gRPC a NotLeader carries no suggested leader, so the next NotLeader from
> that follower is blamed on the leader, the provider selects the next OM in
> round robin order, and every thread of the client is moved away from the
> leader until the ring brings it back.
> h3. Trigger
> Three OMs, round robin order r0, r1, r2, r2 is the leader and the client is
> on r0. Thread t1 gets NotLeader from r0, fails over to r1 and is preempted
> before it stores {{host}}. Thread t2 sends to r0, gets NotLeader, fails over
> to r2, stores {{host}} = r2 and succeeds. t1 stores {{host}} = r1. t1's retry
> goes to r1 while the provider names r2, and the NotLeader moves the provider
> to r0, then r1, then r2.
> The window is a few instructions wide; the reproduction holds one thread at
> that point.
> h3. Impact
> * Each occurrence costs the affected request N minus 1 extra NotLeader round
> trips to followers (2 with 3 OMs), and moves the shared provider briefly to a
> follower, so other requests in flight at that moment also hit followers. Each
> NotLeader is also logged at ERROR by {{GrpcOmTransport.submitRequest}}.
> * On a secure cluster a follower rejects every request sent to {{host}} with
> NotLeader (writes, and reads stamped with the leader read consistency
> {{DEFAULT}} or {{LINEARIZABLE_LEADER_ONLY}}), so the race gives no wrong
> result and no failed request there; a read that already carries a follower
> read hint may be served by a follower anyway, which is within what it asked
> for. The failover budget is counted per request (500 by default), so one
> occurrence does not come close to it. These points are from reading the code,
> not from a run.
> * Only deployments that set {{ozone.om.transport.class}} to the gRPC
> transport are affected; the default has been Hadoop RPC since HDDS-9446.
> * Related: HDDS-17462 (apache/ozone#11763) sends the suggested leader with a
> gRPC NotLeader. Once that lands, a follower that knows the leader sends the
> misdirected retry straight back to it and most of this cost goes away; the
> stale send target remains.
> * In the reproduction the stale request also costs t1 one full round backoff
> ({{ozone.client.wait.between.retries.millis}}, 2 s by default). That backoff
> is not caused by {{host}}: it comes from the retry policy handling a
> NotLeader that was sent before another thread's failover, and it remains with
> the patch.
> h3. Reproduction
> PASS on unmodified source, deterministic.
> [^TestBugMC5GrpcLeaderTargetDesync.java] ({{hadoop-ozone/common}}) runs three
> in process OM gRPC servers (the leader answers, the others reply NotLeader
> encoded as the OM gRPC server does) and sends {{ServiceList}} requests
> through {{GrpcOmTransport.submitRequest}} with default retry settings. The
> only intervention is timing: t1 is held in its first OM address lookup after
> its first NotLeader (on the unmodified code that is in {{shouldRetry}},
> between reading the current OM and storing {{host}}) while t2 fails over to
> the leader; the lookup result is not changed. Output with round robin order
> om2, om1, om0 and leader om0: after t2 reached om0, t1 sends to om1 while the
> provider names om0, then to om2, om1 and om0. A passing test means the defect
> is present. With the patch it fails: t1 sends once to om1 (the OM it had
> already picked) and then to om0.
> h3. Patch
> [^MC-5-grpc-om-transport-send-to-provider-current-om.patch], against
> efe0c7eba1527530d1001f6517b733576fdc6d61, which is also the current master.
> {{GrpcOmTransport}} no longer keeps {{host}}. {{submitRequestToLeader}} reads
> the address of the provider's current OM at each send, and the stores into
> {{host}} in {{start}}, {{shouldRetry}} and {{changeLeaderProxyForTest}} are
> removed. {{GrpcOmTransport}} can then no longer hold a stale send target. A
> send that races another thread's failover can still have its reply handled
> after the provider moved, but that failover has changed
> {{globalFailoverCount}}, so {{shouldRetry}} does not fail over again. The
> failover logic and the follower read path
> ({{GrpcOMFollowerReadFailoverProxyProvider}}, which sends to its own chosen
> OM) are unchanged.
> Covered by {{testRequestGoesToOmAfterFailoverByAnotherCaller}} in the
> existing {{TestS3GrpcOmTransport}}, single threaded: the client is on om0,
> the test fails the provider over to the leader om2 the way another thread's
> request does, then sends one request and expects it to reach om2 only.
> Without the change it fails with {{["om0", "om0", "om1", "om2"]}} (the
> request goes to the stale address, the NotLeader is blamed on om2 and the
> client goes round the ring; the order after om2 depends on the shuffled
> ring). With it {{TestS3GrpcOmTransport}},
> {{TestGrpcOmTransportConcurrentFailover}}, the OM failover and follower read
> proxy provider suites in {{hadoop-ozone/common}} (67 tests) and
> {{TestGrpcOmTransport}} in {{hadoop-ozone/s3gateway}} (4 tests) pass,
> checkstyle is clean.
> Not part of this patch: the {{globalFailoverCount}} check and increment
> around {{performFailover}} are also not one atomic step, so two threads could
> both fail over (argued, not run).
> Found by TLA+ model checking and code review of the OM follower read and read
> consistency routing paths under HDDS-15926, on commit
> efe0c7eba1527530d1001f6517b733576fdc6d61. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. 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]
