Siyao Meng created HDDS-17432:
---------------------------------
Summary: Stream PutBlock that passed the open check can commit
into a container after it is closed and its checksum and close report were taken
Key: HDDS-17432
URL: https://issues.apache.org/jira/browse/HDDS-17432
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-2-recheck-container-state-under-lock-for-stream-putblock.patch,
TestBugMC2StreamPutBlockCommittedAfterClose.java
h3. Mechanism
With {{ozone.client.datastream.putblock.without.raft.enabled}}, PutBlocks reach
the datanode through the data stream instead of the Raft log: as a stream
command in the middle of the stream ({{ContainerStateMachine.streamCommand}})
and when the stream closes ({{ContainerStateMachine.streamPutBlock}}). They are
not ordered with the Raft-applied CloseContainer.
{{KeyValueHandler.handlePutBlock}} calls {{checkContainerOpen}} (which accepts
OPEN and CLOSING) and later commits the block through
{{BlockManagerImpl.persistPutBlock}} without holding the container lock.
{{KeyValueHandler.closeContainer}} holds the container write lock only while it
syncs the DB and moves the replica to CLOSED; it then builds the data checksum
from the block table and sends the close ICR.
A stream PutBlock that passed the check before the close can therefore commit
after all of that. The CLOSED replica gains a block row and its block count
changes, while the close ICR and the stored data checksum describe the replica
without it. For the mid stream PutBlock,
{{KeyValueContainer.addToPendingPutBlockCache}} then fails because the close
cleared the pending block set, and the handler returns
{{CONTAINER_INTERNAL_ERROR}}. {{HddsDispatcher}} treats that as a replica
failure on a container that is no longer open and throws
{{IllegalStateException}}, so the stream command fails for a PutBlock that was
committed. The stream close PutBlock gets {{SUCCESS}}.
h3. Trigger
# Datanodes with {{hdds.container.ratis.datastream.enabled=true}} and a client
with {{ozone.client.datastream.putblock.without.raft.enabled=true}} (both
default false) write through the data stream.
# SCM closes the container while a stream PutBlock for it is between the open
check and the commit on one replica.
No fault is needed. The window is narrow (from the open check to the RocksDB
batch commit). Without a timing hook a tight loop hit it rarely (1 in 3000
attempts); the stale close report and checksum were only shown with a timing
hook.
h3. Impact
* The close ICR reports a key count and data checksum that do not include the
block, and the stale checksum is kept across a restart. The next full container
report refreshes the key count and the data scanner rebuilds the checksum
(every 7 days by default).
* Mid stream PutBlock: if the other replicas rejected it, the late replica
keeps a block row that no key references, because the client wrote the data
again to a new block. Until the scanner runs the replicas report the same
checksum; afterwards Recon reports a replica mismatch, and an admin reconcile
copies the row to the peers. This is the same kind of leftover an abandoned
write leaves through the Raft path. (Argued from the code, not run on a
cluster.)
* Stream close PutBlock: if the other replicas rejected it, the late replica is
the only one that holds the block, and the client is told the write succeeded
only because of HDDS-17431, which ignores the rejection on the other replicas.
* I found no wrong result visible to a client or OM from this race alone. After
the failed mid stream command the client excludes the pipeline and writes the
data to a new block.
* PutBlocks over gRPC (EC) can race a direct close from
{{CloseContainerCommandHandler}} in the same way, with the same transient
effect on the close ICR and checksum. This patch does not change that path.
(Argued from the code, not run.)
h3. Reproduction
PASS, deterministic, unmodified source.
[^TestBugMC2StreamPutBlockCommittedAfterClose.java]
({{hadoop-hdds/container-service}}) drives a real {{KeyValueHandler}} and
container. The PutBlock enters {{handlePutBlock}} with the op
{{ContainerStateMachine}} uses for each path, and the close goes through
{{KeyValueHandler.closeContainer}}. The only hook is a Mockito spy on the real
{{BlockManager}} that starts the close when the PutBlock reaches the commit and
lets the commit run once the close has finished. For both paths it shows a
CLOSED replica holding the block while its close ICR says keyCount 0 and its
stored checksum is that of an empty container. A passing test means the defect
is present. It calls the handler directly, so for the mid stream PutBlock it
sees {{CONTAINER_INTERNAL_ERROR}} rather than the {{IllegalStateException}}
that {{HddsDispatcher}} turns it into.
h3. Patch
[^MC-2-recheck-container-state-under-lock-for-stream-putblock.patch], against
7212465c0bb19615708c144238317db51c2a696d. It also applies to master at
ae71e2dd50c.
For stream PutBlocks only, {{handlePutBlock}} repeats {{checkContainerOpen}}
under the container read lock and commits while holding it. Close, quasi close
and mark unhealthy change the state under the container write lock, so a stream
PutBlock is now either committed before the close takes its checksum and ICR,
or rejected with {{CLOSED_CONTAINER_IO}} without writing anything. Raft-applied
PutBlocks and the gRPC path take no new lock; the block file is still closed
outside the lock.
The mid stream part does not depend on anything else: {{streamCommand}} already
fails on a non success result, and the patch turns today's
{{IllegalStateException}} and leftover row into a plain {{CLOSED_CONTAINER_IO}}
with no row. The stream close part depends on HDDS-17431:
{{ContainerStateMachine.streamPutBlock}} ignores the PutBlock result, so a late
stream close PutBlock that this patch now rejects would leave the block off
that replica while the client is told the write succeeded. Land this patch with
or after the fix for HDDS-17431.
HDDS-16828's proposed patch takes the container read lock at the same place in
{{handlePutBlock}} for its finalized block check. Whichever lands second should
put both checks in one locked section; with HDDS-16828 in, this fix reduces to
repeating {{checkContainerOpen}} inside it.
Covered by two tests in the existing {{TestKeyValueHandler}}, each for both
stream paths. {{testStreamPutBlockRacingCloseIsNotCommittedAfterClose}} runs
the close while the PutBlock is at the commit and asserts that the PutBlock
succeeds and the checksum taken at close covers its block; without the change
it fails for both paths.
{{testStreamPutBlockRejectedWhenContainerClosesBeforeCommit}} closes the
container after the first open check and before the lock is taken, and asserts
{{CLOSED_CONTAINER_IO}} and no block; with the re-check under the lock removed
it fails for both paths. With the change {{TestKeyValueHandler}},
{{TestKeyValueHandlerWithUnhealthyContainer}}, {{TestKeyValueContainer}},
{{TestBlockManagerImpl}}, {{TestHddsDispatcher}},
{{TestKeyValueStreamDataChannel}}, {{TestCloseContainerCommandHandler}} and the
container state machine leader and follower tests pass,
{{TestBlockDataStreamOutput}} and {{TestContainerStateMachineStream}} pass on a
mini cluster (with and without the stream PutBlock), and checkstyle is clean.
With the patch the reproduction fails, because the PutBlock is committed before
the close and the close ICR counts it.
Found by TLA+ model checking and code review of the datanode streaming PutBlock
paths under HDDS-15926, on commit 7212465c0bb19615708c144238317db51c2a696d.
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]
