[
https://issues.apache.org/jira/browse/HDDS-17443?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gargi Jaiswal reassigned HDDS-17443:
------------------------------------
Assignee: Gargi Jaiswal
> Domain socket Receiver logs an NPE for every response without a block file
> and sends no reply to a GetBlock that is not granted access
> --------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17443
> URL: https://issues.apache.org/jira/browse/HDDS-17443
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.3.0
> Reporter: Siyao Meng
> Assignee: Gargi Jaiswal
> Priority: Minor
> Attachments:
> CR-1-reply-to-getblock-without-block-file-in-domain-socket-receiver.patch,
> TestBugCR1NullBlockFileClose.java,
> TestBugMC2GetBlockWithoutFlagNoResponse.java
>
>
> h3. Mechanism
> {{Receiver.ProcessRequestTask.run}} attaches a block file to the response
> only for a GetBlock that returns SUCCESS, and both places that use the file
> assume this is the same as "the response has a file":
> * {{Receiver.sendResponse}} closes {{entry.getFile()}} in its {{finally}}
> block without a null check, and catches only {{IOException}}. For every
> response without a file (Echo, a GetBlock that does not succeed, the
> UNSUPPORTED_REQUEST reply) the close throws a {{NullPointerException}} after
> the response has been flushed. The exception skips
> {{ContainerMetrics.incContainerLocalOpsLatencies}} and is logged by
> {{ProcessRequestTask.run}} as "Failed to processRequest" at ERROR with the
> stack trace.
> * {{KeyValueHandler.handleGetBlock}} opens and parks a block file only when
> the request sets {{requestShortCircuitAccess}} and the domain socket server
> is started, and returns SUCCESS with {{shortCircuitAccessGranted=false}}
> otherwise. {{ProcessRequestTask.run}} fetches the file for every SUCCESS
> GetBlock, {{Objects.requireNonNull}} throws, the exception is only logged,
> and no response is sent.
> HDDS-11627 closed the block's stream only when there was one; HDDS-12883
> brought the close back for the block file without that null check. The
> non-null requirement on the file dates from HDDS-11627.
> h3. Trigger
> * Logged NPE: every Echo, and every short-circuit GetBlock that does not
> succeed. The most frequent one is the routine fallback when the local
> datanode has not applied the PutBlock yet ({{UNKNOWN_BCSID}}, see the comment
> in {{BlockInputStream.getBlockDataUsingClient}}). Run for {{UNKNOWN_BCSID}}
> and {{NO_SUCH_BLOCK}}; other error results such as {{CONTAINER_NOT_FOUND}} or
> a token failure take the same path.
> * No reply: a GetBlock on the domain socket without
> {{requestShortCircuitAccess}}. The field is optional and defaults to false,
> so this is a valid request, but the in-tree reader
> ({{BlockInputStream.getBlockDataUsingSCClient}}) always sets it. By code
> reading the same happens when the domain socket server is stopped between the
> block lookup and the {{isStarted()}} check (not run); there {{closeAllPeers}}
> disconnects the client anyway.
> h3. Impact
> * Every Echo and every failed short-circuit GetBlock adds an ERROR with a
> stack trace to the datanode log, and {{localLatencyNsEcho}} and
> {{localLatencyNsGetBlock}} miss those samples ({{numLocalEcho}} and
> {{numLocalGetBlock}} are correct). The client receives a correct response and
> falls back to gRPC as designed. Reproduced. No in-tree reader sends Echo on
> the domain socket (the Echo load generator in freon uses gRPC), so in
> practice the errors come from failed GetBlocks.
> * A client that sends a GetBlock without the flag gets no reply and waits for
> its own read timeout. Only that request is affected: the read thread goes
> back to the pool and the same connection serves the next request normally
> (reproduced with a single read thread). Other connections and clients are not
> affected. No in-tree client sends such a request, so today this only matters
> for other clients of the protocol.
> h3. Reproduction
> [^TestBugCR1NullBlockFileClose.java] and
> [^TestBugMC2GetBlockWithoutFlagNoResponse.java]
> ({{hadoop-ozone/integration-test}}) run a real {{XceiverServerDomainSocket}}
> with {{HddsDispatcher}} and {{KeyValueHandler}} and send requests framed as
> the short-circuit client frames them. They need the native libhadoop
> ({{-Phadoop-native-lib}}) and change no source. Both pass on unmodified
> source, so a passing test means the defect is present.
> * The first sends an Echo, a GetBlock with BCSID 5 against a replica at 0
> ({{UNKNOWN_BCSID}}) and a GetBlock of a missing block ({{NO_SUCH_BLOCK}}).
> All three get the right response, and the datanode logs three
> {{NullPointerException: Cannot invoke "java.io.RandomAccessFile.close()"}}
> errors. {{numLocalGetBlock}} is 3 with 1 latency sample (the granted control
> request) and {{numLocalEcho}} is 1 with 0 samples.
> * The second sends a GetBlock with the flag (answered with its descriptor),
> then the same GetBlock without the flag: no reply within the 3 s read
> timeout, and the datanode logs {{NullPointerException: Failed to get block
> file for block}}. A third GetBlock on the same connection is answered
> normally.
> h3. Patch
> [^CR-1-reply-to-getblock-without-block-file-in-domain-socket-receiver.patch],
> against 69d77beff60b9106e2517c36a7a1b2cc726af568. It also applies to master
> at 1acf315d546.
> Two changes in {{Receiver}}: {{sendResponse}} closes the block file only when
> there is one, and {{ProcessRequestTask.run}} fetches the parked file only
> when the GetBlock response grants short-circuit access. A SUCCESS GetBlock
> that grants none is then sent as the handler built it, with its BlockData and
> without a descriptor, and by code reading the in-tree client takes its
> existing "Short-circuit access is denied" path (fails the request, disables
> short-circuit for the configured interval) and reads through gRPC.
> {{KeyValueHandler.handleGetBlock}} grants access exactly when it parks a
> file, so the handler is unchanged. The two changes go together: with only the
> second one, the reply that is now sent would hit the first problem right
> after it is flushed. The new tests pass the read executor argument of the
> {{XceiverServerDomainSocket}} constructor like the existing tests; after the
> change in HDDS-17442, which removes that argument, they drop it as well.
> Covered by {{testResponseWithoutBlockFile}} and
> {{testGetBlockWithoutShortCircuitAccess}} in the existing
> {{TestXceiverServerDomainSocket}}. Without the change the first fails because
> the Receiver logged "Failed to processRequest" for the Echo and the failed
> GetBlock, and the second fails with {{SocketTimeoutException}}. With it
> {{TestXceiverServerDomainSocket}}, {{TestXceiverClientManagerSC}},
> {{TestLocalChunkInputStream}} and {{TestDomainSocketFactory}} (19 tests) and
> {{TestKeyValueHandler}} (43 tests) pass, checkstyle is clean, and both
> reproductions fail because the error is gone and the reply arrives.
> Found by TLA+ model checking and code review of the datanode short-circuit
> read paths under HDDS-15926, on commit
> 69d77beff60b9106e2517c36a7a1b2cc726af568. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-12883 (added the close of the block file after it is sent),
> HDDS-16918 (fallback to gRPC is not logged on the datanode), HDDS-16335
> (concurrent access on the client side of the domain socket). 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]
