[
https://issues.apache.org/jira/browse/HDDS-16092?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ritesh Shukla updated HDDS-16092:
---------------------------------
Description:
h2. Summary
{{OzoneManagerStateMachine.takeSnapshotImpl}} writes {{TRANSACTION_INFO_KEY}}
with a direct, unbatched {{put}} while the double buffer writes the same key
inside its atomic batch. A snapshot that lands in the gap between the batch
commit and the applied-index advance computes its index from a stale value and
overwrites the batch's higher {{TransactionInfo}} with a lower one, leaving the
DB holding data for transactions its own watermark disclaims.
h2. The two writers
* Double buffer: {{putWithBatch}} at {{OzoneManagerDoubleBuffer.java:375-376}},
committed together with the transaction data at {{:379-381}}. The state
machine's applied index advances only *afterwards*, at {{:396}}
({{updateLastAppliedIndex.accept}}, wired to
{{OzoneManagerStateMachine::updateLastAppliedTermIndex}} at
{{OzoneManagerStateMachine.java:562}}).
* Snapshot: direct {{put}} at {{OzoneManagerStateMachine.java:604}}, using the
index computed at {{:599}} as {{max(applied, notified)}}.
h2. Interleaving
# *OMDoubleBufferFlushThread* commits transactions 101..105. The DB now
atomically holds their data and {{TransactionInfo = 105}}. It proceeds toward
{{accept(105)}}.
# *Ratis StateMachineUpdater* enters {{takeSnapshotImpl}} first. That method is
{{synchronized}} ({{:596}}) and so is {{updateLastAppliedTermIndex}} ({{:262}})
-- the same monitor -- so step 1's {{accept(105)}} blocks. The window is
therefore held open across the put and the {{flushDB}} that follows, rather
than being a momentary read blip.
# It reads {{applied = 100}}, computes {{snapshot = 100}}, and at {{:604}} puts
{{TransactionInfo = 100}}. That write is sequenced after the already-committed
batch, so *100 overwrites 105*.
End state: the DB contains the effects of 101..105 under a watermark of 100.
Ratis's snapshot index is also 100, so the log purge stops there and entries
101..105 survive in the raft log. On an idle cluster the inconsistency persists
until the next flush.
Note the asymmetry: {{updateLastAppliedTermIndex}} guards the *in-memory* index
against moving backwards ({{assertUpdateIncreasingly}}, {{:264}}). The
persisted twin has no equivalent guard, which is exactly the direction this
moves.
h2. Consequence
*Not established -- see the correction comment on this issue.* The original
text here claimed a deterministic startup crash-loop via the updateID guard in
{{WithObjectID.Builder.validate}}. An end-to-end experiment (stage a regressed
watermark on a real OM, restart it) did not reproduce that: the OM restarted
cleanly with data intact. The guard exists, but nothing verifies a restart
reaches it.
What is established is the state itself: the DB ends up holding transactions
that its own persisted index disclaims, so the record of what the DB contains
is wrong. That is worth fixing regardless, but no specific failure mode should
be used to argue severity or backport scope until someone establishes one.
h2. Trigger
The auto-snapshot path, enabled unconditionally at
{{OzoneManagerRatisServer.java:869}} with the threshold from
{{ozone.om.ratis.snapshot.auto.trigger.threshold}}.
h2. Suggested fix
Make the snapshot's write unable to lower the stored value: either skip the put
when the persisted index already exceeds the snapshot index, or route it
through the same batch/commit path the flush uses so the two writers are
ordered rather than racing.
h2. Provenance and caveats
Found while reviewing HDDS-16086 / [PR
#10943|https://github.com/apache/ozone/pull/10943], which touches the other
writer of this field. Line references are against master at commit
{{8415f3b876}}.
Every load-bearing citation above was read directly. The one assumption not
re-derived from source is standard RocksDB write ordering in step 3 -- that a
{{put}} issued after {{commitBatchOperation}} returns is sequenced after that
batch. No reproducer has been written yet; the interleaving is derived from the
code, not observed.
was:
h2. Summary
{{OzoneManagerStateMachine.takeSnapshotImpl}} writes {{TRANSACTION_INFO_KEY}}
with a direct, unbatched {{put}} while the double buffer writes the same key
inside its atomic batch. A snapshot that lands in the gap between the batch
commit and the applied-index advance computes its index from a stale value and
overwrites the batch's higher {{TransactionInfo}} with a lower one, leaving the
DB holding data for transactions its own watermark disclaims.
h2. The two writers
* Double buffer: {{putWithBatch}} at {{OzoneManagerDoubleBuffer.java:375-376}},
committed together with the transaction data at {{:379-381}}. The state
machine's applied index advances only *afterwards*, at {{:396}}
({{updateLastAppliedIndex.accept}}, wired to
{{OzoneManagerStateMachine::updateLastAppliedTermIndex}} at
{{OzoneManagerStateMachine.java:562}}).
* Snapshot: direct {{put}} at {{OzoneManagerStateMachine.java:604}}, using the
index computed at {{:599}} as {{max(applied, notified)}}.
h2. Interleaving
# *OMDoubleBufferFlushThread* commits transactions 101..105. The DB now
atomically holds their data and {{TransactionInfo = 105}}. It proceeds toward
{{accept(105)}}.
# *Ratis StateMachineUpdater* enters {{takeSnapshotImpl}} first. That method is
{{synchronized}} ({{:596}}) and so is {{updateLastAppliedTermIndex}} ({{:262}})
-- the same monitor -- so step 1's {{accept(105)}} blocks. The window is
therefore held open across the put and the {{flushDB}} that follows, rather
than being a momentary read blip.
# It reads {{applied = 100}}, computes {{snapshot = 100}}, and at {{:604}} puts
{{TransactionInfo = 100}}. That write is sequenced after the already-committed
batch, so *100 overwrites 105*.
End state: the DB contains the effects of 101..105 under a watermark of 100.
Ratis's snapshot index is also 100, so the log purge stops there and entries
101..105 survive in the raft log. On an idle cluster the inconsistency persists
until the next flush.
Note the asymmetry: {{updateLastAppliedTermIndex}} guards the *in-memory* index
against moving backwards ({{assertUpdateIncreasingly}}, {{:264}}). The
persisted twin has no equivalent guard, which is exactly the direction this
moves.
h2. Consequence
On restart, {{loadSnapshotInfoFromDB}} seeds the applied index at 100 and Ratis
replays 101..105 through full request execution, with no replay suppression at
any layer.
This is *not* silent data corruption. If any object in the window was touched
more than once, {{WithObjectID.Builder.validate()}} throws
{{IllegalArgumentException}} on the updateID regression before anything is
persisted ({{WithObjectID.java:121-131}}), and {{runCommand}}'s {{catch
(Throwable)}} terminates the OM. The reachable outcome is a deterministic
startup crash-loop -- persisting until manual repair on a single-node
deployment, or until the leader's log purge passes the window on an HA node,
after which restart heals via install-snapshot. If no object in the window was
multi-touched, the replay is benign and the next flush silently heals the
watermark.
h2. Trigger
The auto-snapshot path, enabled unconditionally at
{{OzoneManagerRatisServer.java:869}} with the threshold from
{{ozone.om.ratis.snapshot.auto.trigger.threshold}}.
h2. Suggested fix
Make the snapshot's write unable to lower the stored value: either skip the put
when the persisted index already exceeds the snapshot index, or route it
through the same batch/commit path the flush uses so the two writers are
ordered rather than racing.
h2. Provenance and caveats
Found while reviewing HDDS-16086 / [PR
#10943|https://github.com/apache/ozone/pull/10943], which touches the other
writer of this field. Line references are against master at commit
{{8415f3b876}}.
Every load-bearing citation above was read directly. The one assumption not
re-derived from source is standard RocksDB write ordering in step 3 -- that a
{{put}} issued after {{commitBatchOperation}} returns is sequenced after that
batch. No reproducer has been written yet; the interleaving is derived from the
code, not observed.
> takeSnapshotImpl's unbatched TransactionInfo put can regress the persisted
> transaction index below the DB's content
> -------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16092
> URL: https://issues.apache.org/jira/browse/HDDS-16092
> Project: Apache Ozone
> Issue Type: Bug
> Components: Ozone Manager
> Reporter: Ritesh Shukla
> Priority: Major
>
> h2. Summary
> {{OzoneManagerStateMachine.takeSnapshotImpl}} writes {{TRANSACTION_INFO_KEY}}
> with a direct, unbatched {{put}} while the double buffer writes the same key
> inside its atomic batch. A snapshot that lands in the gap between the batch
> commit and the applied-index advance computes its index from a stale value
> and overwrites the batch's higher {{TransactionInfo}} with a lower one,
> leaving the DB holding data for transactions its own watermark disclaims.
> h2. The two writers
> * Double buffer: {{putWithBatch}} at
> {{OzoneManagerDoubleBuffer.java:375-376}}, committed together with the
> transaction data at {{:379-381}}. The state machine's applied index advances
> only *afterwards*, at {{:396}} ({{updateLastAppliedIndex.accept}}, wired to
> {{OzoneManagerStateMachine::updateLastAppliedTermIndex}} at
> {{OzoneManagerStateMachine.java:562}}).
> * Snapshot: direct {{put}} at {{OzoneManagerStateMachine.java:604}}, using
> the index computed at {{:599}} as {{max(applied, notified)}}.
> h2. Interleaving
> # *OMDoubleBufferFlushThread* commits transactions 101..105. The DB now
> atomically holds their data and {{TransactionInfo = 105}}. It proceeds toward
> {{accept(105)}}.
> # *Ratis StateMachineUpdater* enters {{takeSnapshotImpl}} first. That method
> is {{synchronized}} ({{:596}}) and so is {{updateLastAppliedTermIndex}}
> ({{:262}}) -- the same monitor -- so step 1's {{accept(105)}} blocks. The
> window is therefore held open across the put and the {{flushDB}} that
> follows, rather than being a momentary read blip.
> # It reads {{applied = 100}}, computes {{snapshot = 100}}, and at {{:604}}
> puts {{TransactionInfo = 100}}. That write is sequenced after the
> already-committed batch, so *100 overwrites 105*.
> End state: the DB contains the effects of 101..105 under a watermark of 100.
> Ratis's snapshot index is also 100, so the log purge stops there and entries
> 101..105 survive in the raft log. On an idle cluster the inconsistency
> persists until the next flush.
> Note the asymmetry: {{updateLastAppliedTermIndex}} guards the *in-memory*
> index against moving backwards ({{assertUpdateIncreasingly}}, {{:264}}). The
> persisted twin has no equivalent guard, which is exactly the direction this
> moves.
> h2. Consequence
> *Not established -- see the correction comment on this issue.* The original
> text here claimed a deterministic startup crash-loop via the updateID guard
> in {{WithObjectID.Builder.validate}}. An end-to-end experiment (stage a
> regressed watermark on a real OM, restart it) did not reproduce that: the OM
> restarted cleanly with data intact. The guard exists, but nothing verifies a
> restart reaches it.
> What is established is the state itself: the DB ends up holding transactions
> that its own persisted index disclaims, so the record of what the DB contains
> is wrong. That is worth fixing regardless, but no specific failure mode
> should be used to argue severity or backport scope until someone establishes
> one.
> h2. Trigger
> The auto-snapshot path, enabled unconditionally at
> {{OzoneManagerRatisServer.java:869}} with the threshold from
> {{ozone.om.ratis.snapshot.auto.trigger.threshold}}.
> h2. Suggested fix
> Make the snapshot's write unable to lower the stored value: either skip the
> put when the persisted index already exceeds the snapshot index, or route it
> through the same batch/commit path the flush uses so the two writers are
> ordered rather than racing.
> h2. Provenance and caveats
> Found while reviewing HDDS-16086 / [PR
> #10943|https://github.com/apache/ozone/pull/10943], which touches the other
> writer of this field. Line references are against master at commit
> {{8415f3b876}}.
> Every load-bearing citation above was read directly. The one assumption not
> re-derived from source is standard RocksDB write ordering in step 3 -- that a
> {{put}} issued after {{commitBatchOperation}} returns is sequenced after that
> batch. No reproducer has been written yet; the interleaving is derived from
> the code, not observed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
