[
https://issues.apache.org/jira/browse/HDDS-16092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101984#comment-18101984
]
Ritesh Shukla commented on HDDS-16092:
--------------------------------------
Reproduced end to end on a 3-OM HA cluster. No staged state, no injected delay,
no artificial DB edit -- the real Ratis auto-snapshot trigger racing the real
double-buffer flush under real client write load.
*Method*: 3 OMs, 1 datanode, two client threads writing continuously. The only
setting changed from production is
{{ozone.om.ratis.snapshot.auto.trigger.threshold}}, dropped from its 400000
default to 50. That does not create the race -- it turns one draw per 400000
transactions into a draw every few transactions, so the existing window gets
sampled thousands of times in a short run. A watcher thread per OM polls the
persisted TRANSACTION_INFO_KEY with {{getSkipCache}} and records any instance
of the index moving *backwards*. A persisted watermark decreasing is
self-evidently wrong, so the detector needs no knowledge of the true applied
index.
*Unfixed code* -- 24 occurrences within ~30 seconds, on two of the three OMs
(the run stops early on first detection):
{noformat}
REPRO HIT: omNode-2: persisted index went BACKWARDS 50 -> 49
REPRO HIT: omNode-3: persisted index went BACKWARDS 50 -> 49
REPRO HIT: omNode-2: persisted index went BACKWARDS 50 -> 49
... 24 total
{noformat}
*Fixed code*, same test, full run:
{noformat}
REPRO SUMMARY: writes=2533 samples=21635614 regressions=0
REPRO RESULT: NOT REPRODUCED in this run
{noformat}
Both hits landed on followers rather than the leader, which matches
expectation: every OM runs its own StateMachineUpdater and its own flush
daemon, so all three are independent draws, and followers carry only the
replicated stream.
*What this settles and what it does not.* The mechanism is now demonstrated
rather than argued: the persisted transaction index really does move backwards
on a real HA OM, and the fix really does prevent it across 21.6 million
samples. The downstream consequence -- replay on restart causing quota drift or
replica divergence -- remains *inferred*. This experiment proves the bad state
occurs; it does not prove what damage follows from it.
> 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]
