[
https://issues.apache.org/jira/browse/HDDS-17669?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17669:
------------------------------
Attachment: TestBugCR5InterruptedWriteRetries.java
> Interrupting a Ratis key write, hsync or close that waits for a watch for
> commit loses the interrupt, and the data is retried on a new block
> --------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17669
> URL: https://issues.apache.org/jira/browse/HDDS-17669
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 2.0.0, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1, 2.2.2
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> CR-5-restore-interrupt-flag-in-block-output-stream.patch,
> TestBugCR5InterruptedWriteRetries.java
>
>
> h3. Mechanism
> {{KeyOutputStream.handleRetry}} stops retrying a failed block when the thread
> is interrupted ("Interrupted while trying for retry"). Three catch sites in
> {{BlockOutputStream}} turn an {{InterruptedException}} into an
> {{IOException}} without restoring the interrupt flag, unlike the other four
> in the same class: {{waitForAllPendingFlushes}}, {{writeOnRetry}} and
> {{allocateNewBufferIfNeeded}}. {{Future.get}} or {{Lock.lockInterruptibly}}
> has already cleared the flag, so it is lost.
> Every {{close()}} and {{hsync()}} of a Ratis key waits in
> {{BlockOutputStream.handleFlushInternal}} for the watch for commit of its own
> PutBlock, and a {{write}} on a full buffer pool waits in {{handleFullBuffer}}
> for the watch of its last PutBlock. If the thread is interrupted during
> either wait, the catch site there restores the flag and fails the block
> stream. {{KeyOutputStream.handleExceptionInternal}} then waits in
> {{waitForAllPendingFlushes}} for the same pending watch. That wait sees the
> flag, throws {{InterruptedException}}, clears the flag, and the resulting
> {{IOException}} is ignored there. The failure handling goes on, the guard in
> {{handleRetry}} no longer sees the interrupt, and the buffered data is
> written again to a new block.
> h3. Trigger
> # Interrupt a thread in {{close()}} or {{hsync()}} of a Ratis key while it
> waits for the watch for commit of its PutBlock ({{Future.cancel(true)}},
> {{ExecutorService.shutdownNow}}, a framework task kill). No datanode error
> and no slow follower are needed.
> # Or interrupt a thread in {{write}} while it waits on a full buffer pool for
> a slow watch for commit.
> The same happens when the interrupt arrives while the failure handling of an
> earlier error (for example a PutBlock timeout) waits for a pending watch of
> the same block, or while a retry waits for its watch in {{writeOnRetry}}.
> h3. Impact
> * An interrupted {{close()}} returns normally and commits the key. An
> interrupted {{hsync()}} or {{write}} returns normally, and a later
> {{close()}} commits the key with all the data. In each case the interrupt
> flag is cleared and OM allocated a new block for the data, and the old
> pipeline is added to the stream's exclude list. The cancellation is ignored,
> and the caller has no way to see that it was interrupted (run, see
> Reproduction).
> * No data loss or wrong data was observed. The key that is committed contains
> everything the application wrote.
> * Argued from the code, not run: the failure handling continues while the old
> block's watch is still pending, so a late watch reply can race with the
> retry. Without the interrupt this cannot occur, because
> {{waitForAllPendingFlushes}} returns only after every pending watch has
> completed.
> h3. Reproduction
> PASS on unmodified source, deterministic, no cluster, about 4 s.
> [^TestBugCR5InterruptedWriteRetries.java] ({{hadoop-ozone/client}}) uses the
> real {{KeyOutputStream}}, {{RatisBlockOutputStream}} and {{CommitWatcher}},
> with a scripted {{XceiverClientSpi}} whose watches for commit on the first
> pipeline stay pending, and a mocked OM.
> * {{interruptWhileCloseWaitsForWatch}}: after a 100 byte write, {{close()}}
> blocks on the watch. After the interrupt it returns normally, the flag is
> clear, one new block was allocated, and the key is committed with 100 bytes.
> * {{interruptWhileHsyncWaitsForWatch}}: the same for {{hsync()}}. A later
> {{close()}} commits 100 bytes.
> * {{interruptWhileWriteWaitsOnFullBuffer}}: a 16 KB write fills the four 4 KB
> buffers and blocks. After the interrupt it returns normally, the flag is
> clear, one new block was allocated, and {{close()}} commits 16,384 bytes.
> * {{interruptWhileFailureHandlingWaitsForPendingFlushes}}: a 4 KB write
> leaves its watch pending, the PutBlock of the next 4 KB fails, and a third
> write blocks in {{waitForAllPendingFlushes}}. After the interrupt the same
> happens, while the old watch is still pending.
> * {{withoutInterruptFailureHandlingWaitsForPendingWatch}}: control without
> the interrupt. The failure handling waits until the old watch is answered,
> then the key commits with all 8,292 bytes.
> A passing test means the defect is present.
> h3. Patch
> [^CR-5-restore-interrupt-flag-in-block-output-stream.patch], against
> ec5ed5f7ea0e4b406aee53aae4906616819cba2b. It also applies to master at
> b683fd3f294.
> The three catch sites call {{Thread.currentThread().interrupt()}} before
> {{handleInterruptedException}}, as the other catch sites of
> {{BlockOutputStream}} do. With the patch an interrupted {{write()}},
> {{hsync()}} or {{close()}} fails with the thread's interrupt flag kept and
> the key is not committed, so an application that interrupts writer threads
> will now see that failure where the call used to succeed silently. The
> exception is the one the existing guard in {{handleRetry}} already throws:
> the original error, not an {{InterruptedIOException}}.
> Covered by three tests in the existing {{TestBlockOutputStreamCorrectness}},
> one per catch site, and {{testInterruptedWriteFailsWithoutRetry}} in the
> existing {{TestKeyOutputStream}}, which interrupts a write blocked on a full
> buffer pool and checks that it fails, keeps the flag and asks OM for no new
> block. All four fail without the change because the flag is cleared. With it,
> the hdds-client storage unit tests (225), the ozone-client unit tests (650)
> and the mini cluster suites {{TestCommitWatcher}},
> {{TestBlockOutputStreamWithFailures}} and {{TestFailureHandlingByClient}} (28
> tests) pass, and checkstyle is clean. With the patch the four interrupt tests
> of the reproduction fail, because the call now throws, the flag is set, no
> block is allocated and nothing is committed. The control still passes.
> Related: HDDS-17648 (a watch that never completes when the Ratis client is
> closed) leaves {{waitForAllPendingFlushes}} waiting forever. Until that is
> fixed an interrupt is the only way out of the wait, and today it turns into a
> silent retry. The patches touch different files.
> Found by code review of the Ozone client Ratis block write and failure
> handling paths, as part of the TLA+ verification effort under HDDS-15926, on
> commit ec5ed5f7ea0e4b406aee53aae4906616819cba2b. 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]
