[
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 race itself is reproduced; the drift is not.* The mechanism has been
demonstrated end to end on a
3-OM HA cluster with the real snapshot trigger and real write load -- 24
occurrences of the persisted
index moving backwards in ~30 seconds on unfixed code, and zero across 21.6
million samples with the
fix. See the reproduction comment. What remains inferred is what damage follows
from that state. 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
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.
> 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 race itself is reproduced; the drift is not.* The mechanism has been
> demonstrated end to end on a
> 3-OM HA cluster with the real snapshot trigger and real write load -- 24
> occurrences of the persisted
> index moving backwards in ~30 seconds on unfixed code, and zero across 21.6
> million samples with the
> fix. See the reproduction comment. What remains inferred is what damage
> follows from that state. 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]
