[
https://issues.apache.org/jira/browse/HDDS-16092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101934#comment-18101934
]
Ritesh Shukla commented on HDDS-16092:
--------------------------------------
Correction to the Consequence section above -- I ran the end-to-end check and
it did not reproduce what I described.
*Experiment*: single-OM MiniOzoneCluster; wrote two keys five times each so
objects were multi-touched; stopped the OM; rewrote TRANSACTION_INFO_KEY
offline from (t:1, i:22) back to (t:1, i:16) -- i.e. staged exactly the state
this bug produces, a watermark behind data the DB already holds; restarted the
OM.
*Result*: the OM restarted cleanly. No replay failure, no crash loop. The
stored transaction info read (t:1, i:16) after restart and key1 read back as
its latest value.
So the claim that the reachable outcome is "a deterministic startup crash-loop"
via the updateID guard in WithObjectID.Builder.validate is *not supported*. I
verified that guard exists in the source, but never verified that a restart
actually reaches it, and this experiment suggests it does not -- at least not
in a single-OM cluster at this scale. I have not established why: the entries
may not be replayed at all (Ratis may take a snapshot on graceful stop and
resume from its own bookkeeping rather than the DB's), or the replay may be
idempotent for these operations. I did not chase it further.
*What this changes*: the mechanism in the description stands -- the two writers
are unordered and the persisted index can move backwards over committed data;
that part is verified by code and pinned by unit tests. The consequence should
be read as *unknown*, not as the crash-loop I wrote. A regressed watermark
still means the DB's own record of which transactions it contains is wrong,
which is worth fixing on its own terms, but I no longer have evidence for a
specific failure mode and would not use one to argue severity or backport scope.
Anyone picking this up should treat establishing the real consequence as open
work.
> 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
> 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
