Siyao Meng created HDDS-17678:
---------------------------------
Summary: PutBlock piggybacked on WriteChunk does not update the
BCSID saved in the Ratis snapshot, so the restart check misses its loss
Key: HDDS-17678
URL: https://issues.apache.org/jira/browse/HDDS-17678
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: MC-1-update-bcsid-map-for-piggybacked-putblock.patch,
TestBugMC1PiggybackedHsyncBcsidRestart.java
h3. Mechanism
With {{ozone.client.stream.putblock.piggybacking}}, the client sends the block
metadata inside the WriteChunk request for the last partial chunk of a flush,
hsync or close, and {{KeyValueHandler.handleWriteChunk}} commits the block when
the entry is applied (COMMIT_DATA stage). The commit goes through
{{BlockManagerImpl.persistPutBlock}} like a plain PutBlock and advances the
container BCSID through an unsynced RocksDB batch.
{{HddsDispatcher.updateBCSID}} only updates {{container2BCSIDMap}} for
{{PutBlock}} and {{PutSmallFile}}. The piggybacked commit is a {{WriteChunk}},
so the map keeps the BCSID of the last plain PutBlock (or 0).
{{ContainerStateMachine.takeSnapshot}} persists only that map, and on restart
{{ContainerSet.buildMissingContainerSetAndValidate}} marks a container
UNHEALTHY only if its DB BCSID is below the map value. The check added by
HDDS-1843 therefore cannot detect that a piggybacked commit covered by the
snapshot was lost.
h3. Trigger
# A client sets {{ozone.client.hbase.enhancements.allowed}} and
{{ozone.client.stream.putblock.piggybacking}} (both default false). The
datanode accepts a piggybacked WriteChunk regardless of server configuration.
# The client writes a key on a Ratis pipeline and flushes, hsyncs or closes it
with a partial last chunk, so the PutBlock is piggybacked. The piggybacked
commit is applied on a datanode; no plain PutBlock follows on that container on
that datanode.
# A Ratis snapshot is taken on that datanode after the apply.
# The datanode loses the unsynced RocksDB WAL tail (power loss or OS crash)
while the snapshot file, which is forced to disk, survives.
# On restart the DB BCSID is not below the snapshot value, the check passes,
and log replay starts after the snapshot index, so the commit is not reapplied.
h3. Impact
The replica stays OPEN (and later CLOSED) without the block data the client was
told is durable after hsync or close, and it is not marked UNHEALTHY. The same
loss with piggybacking off marks the replica UNHEALTHY at restart, as intended
by HDDS-1843. The other replicas still hold the data, so the visible effect is
lost redundancy that nothing reports. Possible downstream effects, not
verified: SCM counting the CLOSED replica as healthy, and a GetBlock with an
older BCSID returning the shorter block from this replica.
h3. Reproduction
Fails on the unfixed code, deterministic, unmodified source.
[^TestBugMC1PiggybackedHsyncBcsidRestart.java]
({{hadoop-ozone/integration-test}}) starts 3 datanodes, writes and hsyncs a key
on RATIS/THREE, resets the container BCSID in one replica's container DB to 0
(the lost write injection {{TestValidateBCSIDOnRestart}} uses), and restarts
that datanode, whose stop takes a snapshot at the last applied index. It runs
with piggybacking on and off and expects the replica to be UNHEALTHY after the
restart. With piggybacking off it passes. With piggybacking on it fails with
{{expected: UNHEALTHY but was: OPEN}}. The same test with the key closed
instead of hsynced (not attached) fails the same way, which confirms the close
path. A real power loss was not run.
h3. Patch
[^MC-1-update-bcsid-map-for-piggybacked-putblock.patch], against
ec5ed5f7ea0e4b406aee53aae4906616819cba2b. It also applies to master at
7294fc2da18.
{{HddsDispatcher.updateBCSID}} also updates {{container2BCSIDMap}} for a
successful WriteChunk that carries a block in the COMMIT_DATA stage, which is
where the piggybacked block is committed. The map then holds the same value as
after a plain PutBlock and the existing restart check applies. WriteChunk
without a block and the WRITE_DATA stage are unchanged.
Covered by {{testPiggybackedPutBlockUpdatesContainer2BCSIDMap}} in the existing
{{TestHddsDispatcher}}, which fails without the change: the map still holds 0
instead of 2 ({{[1L=0L (expected: 2L)]}}). With it {{TestHddsDispatcher}},
{{TestKeyValueHandler}}, {{TestContainerStateMachineLeader}},
{{TestContainerStateMachineFollower}}, {{TestBlockManagerImpl}} (156 tests),
{{TestValidateBCSIDOnRestart}} and {{TestHSync}} (43 tests) pass, and both
cases of the reproduction pass.
Found by TLA+ model checking and code review of the datanode container state
machine apply and block commit paths 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]
