[
https://issues.apache.org/jira/browse/HDDS-17000?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17000:
------------------------------
Attachment: MC-1-rewind-chunk-stream-on-failed-partial-read.patch
> Sequential read retried after a failed ReadChunk silently skips the bytes it
> had already copied from the chunk buffer
> ---------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17000
> URL: https://issues.apache.org/jira/browse/HDDS-17000
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Critical
> Attachments: MC-1-rewind-chunk-stream-on-failed-partial-read.patch,
> TestBugMC1PartialChunkReadRetry.java
>
>
> h3. Mechanism
> {{ChunkInputStream.read(byte[], int, int)}} and
> {{ChunkInputStream.read(ByteBuffer)}} can do two things in one call. They
> first copy the bytes still buffered from the previous ReadChunk into the
> caller's buffer, which moves the chunk position past them, and then send
> another ReadChunk for the rest. If that ReadChunk throws, the count of copied
> bytes is lost with the exception, but the chunk position stays moved.
> {{BlockInputStream.readWithStrategy}} then refreshes the pipeline and token
> and retries. The retry computes its length from the moved position, while the
> {{ByteArrayReader}} offset never advanced, so the retry writes the following
> bytes over the start of the caller's buffer and reports success. The
> {{numBytesRead != readLength}} check in {{BlockInputStream}} and
> {{KeyInputStream.checkPartBytesRead}} both pass. With {{ByteBufferReader}}
> the buffer position did advance, so the data lands in the right place but the
> count is short and the read fails with "Inconsistent read" instead of being
> retried.
> The positioned read path in the same class already resets the destination on
> retry ({{dst.position(startPosition)}} in {{BlockInputStream.readChunkAt}});
> the sequential path has no equivalent.
> h3. Trigger
> # Checksum verification is on ({{ozone.client.verify.checksum}}, default
> true), so a ReadChunk fetches whole checksum ranges (the bytes per checksum
> the key was written with, {{ozone.client.bytes.per.checksum}} of the writer,
> default 16KB) and a read that ends inside a range leaves the rest of it
> buffered.
> # A sequential read of a replicated key, with the default
> {{ozone.client.stream.readblock.enable}}=false, starts inside such a buffered
> range and extends past it within the same chunk, for example a read size that
> does not divide 16KB, or reads after a seek to an unaligned offset.
> # The ReadChunk for the part after the buffered range fails on every datanode
> of the pipeline with any {{IOException}} other than {{SCMSecurityException}}
> (which is rethrown without a retry), and a later retry succeeds. Examples: a
> short network outage on the client side on any cluster, a restart of the only
> datanode of a one replica key, or an expired block token on a secure cluster,
> which every datanode rejects with {{BLOCK_TOKEN_VERIFICATION_FAILED}} once a
> stream has been open longer than {{hdds.block.token.expiry.time}} (default
> 1d) and which the client is designed to refresh and retry. Retries are
> bounded by {{ozone.client.read.max.retries}} (default 3) with
> {{ozone.client.read.retry.interval}} (default 1s) between them.
> Not affected: positioned reads ({{readPositioned}}, {{read(long, ...)}}) and
> the streaming read path ({{StreamBlockInputStream}}, which does not use
> {{ChunkInputStream}}). EC keys are not affected by the silent variant because
> {{ECBlockInputStream}} reads its block streams through {{ByteBuffer}}; there
> the loud variant becomes a {{BadDataLocationException}}, so a plain token
> expiry makes the reader switch to a spare replica or to reconstruction (code
> reading only).
> Affected versions: the read loop and the retry are the same in every release
> tag I checked from 1.3.0 through 2.2.1 (1.3.0, 1.4.0, 1.4.1, 2.0.0, 2.1.0,
> 2.2.0, 2.2.1). Token expiry is a silent trigger since 1.4.0, where the retry
> uses the refreshed token (HDDS-9734). 1.2.0 has the same loop but retries
> only {{StorageContainerException}}.
> h3. Impact
> The {{byte[]}} read returns success with the wrong bytes: the bytes that were
> buffered (up to bytes per checksum minus one, so up to 16383 bytes with the
> defaults) are replaced by the following bytes, and the stream position is
> ahead by the same amount, so the caller reaches EOF that many bytes early.
> Only the usual WARN and INFO lines of a retried read are logged; nothing
> reports the skipped bytes and nothing re-checks the data afterwards. If the
> read range plus the skipped bytes reaches the end of the block, the retried
> read comes back short and {{KeyInputStream.checkPartBytesRead}} throws
> "Inconsistent read" instead. If all retries fail, the exception leaves the
> stream ahead by the copied bytes, so an application that catches it and keeps
> reading also skips them. Data stored on the datanodes is not affected.
> Through the {{ByteBuffer}} API the same trigger fails the read with an
> "Inconsistent read" {{IOException}} although the retry itself would have
> succeeded.
> h3. Reproduction
> [^TestBugMC1PartialChunkReadRetry.java] ({{hadoop-ozone/client}}) uses the
> real {{KeyInputStream}}, {{BlockInputStream}}, {{ChunkInputStream}},
> {{ContainerProtocolCalls}} and response validators with the default client
> config except a zero retry interval. The {{XceiverClientSpi}} is replaced by
> a simulated datanode at {{sendCommandAsync}} (so the gRPC client's own loop
> over the datanodes is not exercised): it serves GetBlock and ReadChunk with
> real CRC32 checksums from a key of two chunks, {1,2,3,4} and {5,6}, with 2
> bytes per checksum, and rejects the old token with
> {{BLOCK_TOKEN_VERIFICATION_FAILED}} once it is marked expired. The token
> refresh goes through the normal refresh function. After {{read(1)}} returns
> [1], the token expires and {{read(3)}} returns 3 with [3, 4, 5]; the position
> is 5 and the rest of the key reads as [6]. Without the expiry the same reads
> return [2, 3, 4] and then [5, 6]. The {{ByteBuffer}} variant fails with
> "Inconsistent read". The tests assert the defect, so a passing run means it
> is present. Not run on a real cluster.
> h3. Patch
> [^MC-1-rewind-chunk-stream-on-failed-partial-read.patch], against
> 1fb64c8e499161d1452ae6948ffd461fc9d2c302. It also applies to master at
> 8664dc4fc3c.
> When the ReadChunk inside {{ChunkInputStream.read(byte[], ...)}} or
> {{ChunkInputStream.read(ByteBuffer)}} fails after the call has already copied
> buffered bytes, the stream seeks back by that many bytes (and the
> {{ByteBuffer}} variant also resets the buffer position) before rethrowing. A
> failed {{ChunkInputStream}} read then leaves the chunk stream and the buffer
> where the call started, so the retry in {{BlockInputStream}} resumes from
> there, for both APIs. A read that fails before copying anything is unchanged,
> as is every read that succeeds. One visible change: when the retries are
> exhausted, the stream is now left at the start of the failed call instead of
> after the copied bytes, so a caller that catches the exception and keeps
> reading gets those bytes again instead of skipping them. Bytes from earlier
> chunks that a {{BlockInputStream}} read had already completed before the
> failure stay consumed, as before. {{LocalChunkInputStream}} (short circuit)
> inherits both methods. Returning the partial count instead of throwing was
> considered and rejected: {{BlockInputStream}} treats a short count as a non
> retried "Inconsistent read", so a token expiry would become a read failure.
> Covered in the existing {{TestBlockInputStream}}, which uses a real
> {{ChunkInputStream}} with only the ReadChunk call stubbed to fail with
> {{BLOCK_TOKEN_VERIFICATION_FAILED}}. {{retriedReadReturnsContiguousData}}
> fails it once, for {{byte[]}} and {{ByteBuffer}}, each after an unaligned
> first read (bytes left in the buffer) and an aligned one (nothing left).
> {{failedReadLeavesPositionUnchanged}} keeps it failing until the retries are
> exhausted and checks that the position and the buffer are where the call
> started and that the next read returns the right bytes. Without the change
> the unaligned {{byte[]}} case fails with the bytes from offset 20 in place of
> offset 1, the unaligned {{ByteBuffer}} case with "Inconsistent read ...
> readLength= 30 numBytesRead=11", and both exhausted retry cases with
> "expected: <1> but was: <20>" for the position. With the patch all six cases
> pass, as do the {{hdds-client}} storage and client io unit suites (168 tests,
> including the EC input stream tests) and the mini cluster suites
> {{TestChunkInputStream}} and {{TestKeyInputStream}}, and checkstyle is clean.
> With the patch the {{byte[]}} and {{ByteBuffer}} tests of the reproduction
> fail because the read returns [2, 3, 4].
> Found by TLA+ model checking and code review of the Ozone client read retry
> and failover paths under HDDS-15926, on commit
> 1fb64c8e499161d1452ae6948ffd461fc9d2c302. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-10536 and HDDS-10497 (block token expiry on long running
> HBase reads, which is one trigger here, fixed by refreshing the token),
> HDDS-9734 (the chunk stream uses the new token after the refresh, which is
> the retry where the bytes are lost), HDDS-8024 (retry other datanodes on
> ReadChunk failure) and HDDS-10632 ("Inconsistent read" with numBytesRead=-1
> on hsync'ed files, a different cause). 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]
