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