[
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
Two code paths write the OM's persisted transaction index,
{{TRANSACTION_INFO_KEY}}, and they are not ordered against each other. A
snapshot can overwrite a higher index that a batch commit just stored, leaving
the DB holding transactions that its own index disclaims.
h2. The two writers
The double buffer writes the index *inside* the same RocksDB batch as the
transaction data ({{OzoneManagerDoubleBuffer.java:373-376}}), so data and index
become durable together at the commit ({{:379-381}}). It advances the state
machine's in-memory applied index only *afterwards* ({{:396}}).
{{OzoneManagerStateMachine.takeSnapshotImpl}} ({{:596-610}}) computes its index
from that in-memory applied value and writes the key with a direct, unbatched
{{put}} ({{:604}}), then forces it to disk with {{flushDB()}} ({{:605}}).
h2. The race
# *OMDoubleBufferFlushThread* commits a batch through index N. The DB now holds
that data and {{TransactionInfo = N}}, atomically.
# Before it reaches {{:396}}, the *Ratis StateMachineUpdater* enters
{{takeSnapshotImpl}} and reads the still-unadvanced applied index M, where M <
N.
# It writes {{TransactionInfo = M}}. That put is sequenced after the committed
batch, so *M overwrites N*, and {{flushDB()}} makes it durable.
The gap at step 2 is not a single instruction: between the commit returning and
the index advancing, the flush thread closes the batch, updates counters, runs
{{cleanupCache}} ({{:392}}) and releases semaphore permits ({{:394}}).
Two guards that look like they would prevent this do not:
* The wait loop in {{takeSnapshot()}} ({{:582-591}}) only runs while {{applied
< lastSkippedIndex}}. {{lastSkippedIndex}} advances solely in
{{notifyTermIndexUpdated}}, which Ratis calls only for non-state-machine
entries ({{RaftServerImpl.java:1884-1886}}) -- and the OM disables Ratis
log-metadata entries ({{OzoneManagerRatisServer.java:810}}). In steady state
the only such entry is the configuration entry written at leader election, so
the condition is permanently false and the loop never executes.
* {{max(applied, notified)}} at {{:599}} is, for the same reason, just
{{applied}}: {{lastNotifiedTermIndex}} is frozen at that configuration entry,
far below applied.
Ratis does not detect it either. Its only assertion is {{snapshot index >
appliedIndex}} ({{StateMachineUpdater.java:294-299}}), which is the opposite
direction. A too-low index is accepted silently, and the log purge simply stops
earlier -- which is safe in itself, and is why the condition leaves no trace.
h2. When it matters
Under sustained write load the next batch commit rewrites the key with a
correct higher index within milliseconds, so most occurrences are invisible and
self-repairing.
The case that does not self-repair is *graceful shutdown*.
{{TRIGGER_WHEN_STOP_ENABLED_DEFAULT}} is true in Ratis 3.2.1, so a snapshot is
taken on the way down ({{StateMachineUpdater.java:334-335}}), and the OM stops
the double buffer immediately afterwards
({{OzoneManagerStateMachine.java:752-755}}). If that stop-snapshot loses the
race, the regressed index is the *final* persisted value. The node then
restarts, {{loadSnapshotInfoFromDB}} ({{:709-724}}) seeds the applied index at
M, and Ratis replays M+1..N against a DB that already contains them.
That makes rolling restarts the realistic exposure: every OM shutdown is one
draw, and a rolling upgrade of a 3-OM cluster is three. The auto-snapshot
trigger (every 400000 applied indices, {{OzoneManagerRatisServer.java:869}})
fires far more often but is the self-repairing case.
h2. Consequence
Only the restarting OM replays; its peers do not. The OM has no
replay-idempotency guard -- searching for "replay" across
{{ozone-manager/src/main}} returns only comments -- and quota accounting is
read-modify-write ({{OMKeyCommitRequest.java:375,407}}), so replicas can drift
apart with no exception, no warning and no checksum.
*The severity of that drift is inferred, not demonstrated.* An experiment on a
3-OM cluster -- regress a follower's persisted index from (t:1, i:41) to (t:1,
i:33), restart only that OM, poll -- showed the follower restart without error
and catch its index up to (t:1, i:42) within a second. Replay ran and
succeeded. That experiment did not check whether the replayed transactions
caused damage, so the drift described above is traced structurally and has not
been reproduced.
A prior version of this description claimed a deterministic startup crash-loop
through the updateID guard in {{WithObjectID.Builder.validate}}. That was wrong
and is retracted; the comments carry both experiments.
h2. Fix
Order the two writers on a lock owned by the double buffer, and route the
snapshot's write through a monotonic {{persistIfNewer}} so it can never lower
the stored index. The snapshot then reports whatever value is actually stored,
so the DB row, the in-memory copy Ratis reads via {{getLatestSnapshot}}, and
the returned index cannot disagree.
A read-then-write check alone is insufficient: holding the state machine
monitor blocks the applied-index update, but a commit already in flight can
still land between the check and the write. The read must also bypass the table
cache ({{getSkipCache}}, as {{TransactionInfo.readTransactionInfo}} already
does for this key), because the value being compared against is written by a
batch commit that does not populate that cache.
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
*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.
> 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
> Labels: pull-request-available
>
> h2. Summary
> Two code paths write the OM's persisted transaction index,
> {{TRANSACTION_INFO_KEY}}, and they are not ordered against each other. A
> snapshot can overwrite a higher index that a batch commit just stored,
> leaving the DB holding transactions that its own index disclaims.
> h2. The two writers
> The double buffer writes the index *inside* the same RocksDB batch as the
> transaction data ({{OzoneManagerDoubleBuffer.java:373-376}}), so data and
> index become durable together at the commit ({{:379-381}}). It advances the
> state machine's in-memory applied index only *afterwards* ({{:396}}).
> {{OzoneManagerStateMachine.takeSnapshotImpl}} ({{:596-610}}) computes its
> index from that in-memory applied value and writes the key with a direct,
> unbatched {{put}} ({{:604}}), then forces it to disk with {{flushDB()}}
> ({{:605}}).
> h2. The race
> # *OMDoubleBufferFlushThread* commits a batch through index N. The DB now
> holds that data and {{TransactionInfo = N}}, atomically.
> # Before it reaches {{:396}}, the *Ratis StateMachineUpdater* enters
> {{takeSnapshotImpl}} and reads the still-unadvanced applied index M, where M
> < N.
> # It writes {{TransactionInfo = M}}. That put is sequenced after the
> committed batch, so *M overwrites N*, and {{flushDB()}} makes it durable.
> The gap at step 2 is not a single instruction: between the commit returning
> and the index advancing, the flush thread closes the batch, updates counters,
> runs {{cleanupCache}} ({{:392}}) and releases semaphore permits ({{:394}}).
> Two guards that look like they would prevent this do not:
> * The wait loop in {{takeSnapshot()}} ({{:582-591}}) only runs while
> {{applied < lastSkippedIndex}}. {{lastSkippedIndex}} advances solely in
> {{notifyTermIndexUpdated}}, which Ratis calls only for non-state-machine
> entries ({{RaftServerImpl.java:1884-1886}}) -- and the OM disables Ratis
> log-metadata entries ({{OzoneManagerRatisServer.java:810}}). In steady state
> the only such entry is the configuration entry written at leader election, so
> the condition is permanently false and the loop never executes.
> * {{max(applied, notified)}} at {{:599}} is, for the same reason, just
> {{applied}}: {{lastNotifiedTermIndex}} is frozen at that configuration entry,
> far below applied.
> Ratis does not detect it either. Its only assertion is {{snapshot index >
> appliedIndex}} ({{StateMachineUpdater.java:294-299}}), which is the opposite
> direction. A too-low index is accepted silently, and the log purge simply
> stops earlier -- which is safe in itself, and is why the condition leaves no
> trace.
> h2. When it matters
> Under sustained write load the next batch commit rewrites the key with a
> correct higher index within milliseconds, so most occurrences are invisible
> and self-repairing.
> The case that does not self-repair is *graceful shutdown*.
> {{TRIGGER_WHEN_STOP_ENABLED_DEFAULT}} is true in Ratis 3.2.1, so a snapshot
> is taken on the way down ({{StateMachineUpdater.java:334-335}}), and the OM
> stops the double buffer immediately afterwards
> ({{OzoneManagerStateMachine.java:752-755}}). If that stop-snapshot loses the
> race, the regressed index is the *final* persisted value. The node then
> restarts, {{loadSnapshotInfoFromDB}} ({{:709-724}}) seeds the applied index
> at M, and Ratis replays M+1..N against a DB that already contains them.
> That makes rolling restarts the realistic exposure: every OM shutdown is one
> draw, and a rolling upgrade of a 3-OM cluster is three. The auto-snapshot
> trigger (every 400000 applied indices, {{OzoneManagerRatisServer.java:869}})
> fires far more often but is the self-repairing case.
> h2. Consequence
> Only the restarting OM replays; its peers do not. The OM has no
> replay-idempotency guard -- searching for "replay" across
> {{ozone-manager/src/main}} returns only comments -- and quota accounting is
> read-modify-write ({{OMKeyCommitRequest.java:375,407}}), so replicas can
> drift apart with no exception, no warning and no checksum.
> *The severity of that drift is inferred, not demonstrated.* An experiment on
> a 3-OM cluster -- regress a follower's persisted index from (t:1, i:41) to
> (t:1, i:33), restart only that OM, poll -- showed the follower restart
> without error and catch its index up to (t:1, i:42) within a second. Replay
> ran and succeeded. That experiment did not check whether the replayed
> transactions caused damage, so the drift described above is traced
> structurally and has not been reproduced.
> A prior version of this description claimed a deterministic startup
> crash-loop through the updateID guard in {{WithObjectID.Builder.validate}}.
> That was wrong and is retracted; the comments carry both experiments.
> h2. Fix
> Order the two writers on a lock owned by the double buffer, and route the
> snapshot's write through a monotonic {{persistIfNewer}} so it can never lower
> the stored index. The snapshot then reports whatever value is actually
> stored, so the DB row, the in-memory copy Ratis reads via
> {{getLatestSnapshot}}, and the returned index cannot disagree.
> A read-then-write check alone is insufficient: holding the state machine
> monitor blocks the applied-index update, but a commit already in flight can
> still land between the check and the write. The read must also bypass the
> table cache ({{getSkipCache}}, as {{TransactionInfo.readTransactionInfo}}
> already does for this key), because the value being compared against is
> written by a batch commit that does not populate that cache.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
