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