[
https://issues.apache.org/jira/browse/HDDS-16092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101948#comment-18101948
]
Ritesh Shukla commented on HDDS-16092:
--------------------------------------
Follow-up to my previous comment, which was fair to challenge: that experiment
ran against a *single-OM* MiniOzoneCluster, and it sampled the stored index
once, immediately after restart. Neither can establish anything about replay,
which is what the claimed consequence depends on. Re-ran it properly.
*Setup*: 3-OM HA cluster. Wrote two keys ten times each. Took a follower (not
the leader), read its persisted index, stopped it, rewrote TRANSACTION_INFO_KEY
offline from (t:1, i:41) back to (t:1, i:33), restarted just that OM, then
polled the index once a second instead of sampling once.
*Result*:
{noformat}
t+0s transaction info = (t:1, i:33) isStopped=false
t+1s transaction info = (t:1, i:42) isStopped=false
{noformat}
The follower restarted without error and its index went from the regressed 33
to 42 -- past where it had been before the regression -- inside a second. So in
HA the state is *self-correcting*: the node comes back, catches up, and no
failure occurs. Whether that catch-up is local replay of its own log or the
leader re-feeding entries, I did not separate; the observable outcome is the
same.
This confirms the previous conclusion rather than reversing it, and now does so
in the configuration that matters and with the mechanism actually exercised.
The "deterministic startup crash-loop" claim in the original description is
wrong, and the updateID guard in WithObjectID.Builder.validate does not fire on
this path.
It also sharpens what the bug is worth. In HA a regressed watermark appears to
heal on the next restart. The residual concern is the window before that: while
the node runs with a watermark below its data, its own record of what it
contains is wrong, and anything reading that record -- log purge decisions,
snapshot indexes reported to Ratis, an operator diagnosing state -- is reading
something false. That is still worth fixing, but it is not a crash and not data
loss, and the fix should be reviewed on those terms.
Single-node behaviour from the earlier comment stands as recorded: clean
restart, no index advance observed. Given the HA result shows an advance is
visible within a second when replay does occur, the single-node run most likely
had nothing to replay rather than having replayed silently.
> 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
> {{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]
