[
https://issues.apache.org/jira/browse/HDDS-17041?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17041:
------------------------------
Attachment: TestBugMC1NSSummaryRetryReplay.java
> Retry of a failed NSSummaryTask applies namespace summary deltas twice or
> skips them, so Recon file counts and sizes stay wrong
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17041
> URL: https://issues.apache.org/jira/browse/HDDS-17041
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Minor
> Attachments:
> MC-1-resume-nssummary-subtasks-from-committed-position.patch,
> TestBugMC1NSSummaryRetryReplay.java
>
>
> h3. Mechanism
> {{ReconTaskControllerImpl.processOMUpdateBatch}} retries a failed task once
> on the same event batch, passing the seek positions the failed attempt
> returned ({{TaskResult.getSubTaskSeekPositions}}). {{NSSummaryTask.process}}
> runs three sub-tasks ({{NSSummaryTaskWithFSO.processWithFSO}},
> {{NSSummaryTaskWithLegacy.processWithLegacy}},
> {{NSSummaryTaskWithOBS.processWithOBS}}) and records one seek position per
> sub-task. Each sub-task skips that many events and adds the remaining ones to
> the stored totals (read, add, write back), so a retry is only correct if the
> seek position equals the number of leading events whose deltas are already
> committed to the Recon DB. Three exits of the sub-tasks break that:
> * *A sub-task that succeeded reports the position it started from.* After its
> final flush it returns {{seekPos}}, which is 0 unless a threshold flush
> happened. If a sibling sub-task fails, the retry runs the successful sub-task
> again from that position and adds its committed deltas a second time.
> * *The position after a threshold flush is one too far.* When the in memory
> map reaches {{ozone.recon.nssummary.flush.db.max.threshold}} entries, the
> sub-task commits it and sets {{seekPos = eventCounter + 1}}. {{eventCounter}}
> already counts the event that was just applied, so the retry also skips the
> next event, which was never applied.
> * *An unchecked exception discards the flushed position.* The loops catch
> only {{IOException}}. Any other exception ends the sub-task, and
> {{NSSummaryTask.awaitSubTask}} records the seek position the attempt started
> from. The retry then applies again everything that the threshold flushes of
> the failed attempt had committed.
> In each case the retry succeeds, the controller moves the task's
> {{lastUpdatedSeqNumber}} to the end of the batch and does not set
> {{tasksFailed}}, so no rebuild is queued. Later batches add to the wrong
> totals.
> h3. Trigger
> A sub-task of {{NSSummaryTask}} has to fail once and succeed on the immediate
> retry. A failure that repeats makes the retry fail as well, which sets
> {{tasksFailed}}, and the full rebuild that follows replaces the wrong totals.
> An I/O or corruption error of the Recon RocksDB does not qualify, because
> {{RocksDatabase}} then closes the database and the retry fails too.
> * Double counting of a sibling: any batch in which one sub-task fails and
> another one succeeds. The batch size and the flush threshold do not matter.
> * Skipped event, and double counting after an unchecked exception: in
> addition, the failing sub-task must have done a threshold flush earlier in
> the same batch, that is the batch touches at least
> {{ozone.recon.nssummary.flush.db.max.threshold}} distinct summary entries in
> that sub-task (150000 in ozone-default.xml). In practice this needs an FSO
> bucket, or a Legacy bucket with file system paths, with a wide directory
> tree. OBS would need that many buckets in one batch.
> Argued from the code, not run: the sub-tasks look up buckets (OBS, Legacy)
> and Legacy parent directories in the Recon OM DB.
> {{OzoneManagerServiceProviderImpl.syncDataFromOM}} applies each delta to that
> DB before it queues the events, and keeps applying later deltas while the
> controller processes the queue, so the DB is ahead of the batch being
> processed and can change between the first attempt and the retry. If a bucket
> that the sub-task has not cached yet, or a Legacy parent directory, is
> deleted and recreated under the same name, and the first attempt runs after
> the delete is applied but the recreate is applied before the retry, the first
> attempt fails on the missing entry and the retry succeeds. The window is
> narrow, since the two attempts run back to back. Otherwise the failure
> repeats and the rebuild repairs the totals. (On such a retry the key is
> counted under the recreated bucket or directory, a separate inaccuracy that
> the patch does not change.) The reproductions inject the failure.
> The first two exits exist since HDDS-12062 (ozone-2.0.0 to ozone-2.2.1 by
> reading the tagged source, not run on a release). The third exists since
> HDDS-15335 (ozone-2.2.0, ozone-2.2.1). Before HDDS-15335 an unchecked
> exception was not retried at all.
> h3. Impact
> The namespace summary that Recon serves for {{/api/v1/namespace/summary}} and
> {{/api/v1/namespace/du}} keeps wrong file counts and sizes for the affected
> buckets and directories until a full rebuild. Nothing compares the
> incremental totals with a rebuild. No OM data is affected.
> h3. Reproduction
> Run on unmodified source. [^TestBugMC1NSSummaryRetryReplay.java]
> ({{hadoop-ozone/recon}}) builds the summary of one bucket per layout with a
> full rebuild, then sends one batch through the real
> {{ReconTaskControllerImpl.processReconEvent}}, which runs the real
> {{NSSummaryTask}} on the real Recon RocksDB and then its retry. The only
> change is a Mockito spy of the summary manager that throws once when the OBS
> bucket's summary is read. Each test also checks that the batch is recorded as
> applied and that {{hasTasksFailed}} is false. A passing test means the defect
> is present.
> * Legacy put plus OBS put, OBS fails once: the Legacy bucket ends with 3
> files, a rebuild gives 2.
> * Flush threshold 1, two OBS puts, the second fails once with
> {{IOException}}: the bucket ends with 2 files, a rebuild gives 3.
> * The same with {{IllegalStateException}}: the bucket ends with 4 files, a
> rebuild gives 3.
> With the patch all three tests fail because the totals equal the rebuild.
> h3. Patch
> [^MC-1-resume-nssummary-subtasks-from-committed-position.patch], against
> 0665932dd35da4d05087e02748af0a64eb846ad5. It also applies to master at
> 2264d4d48ac.
> The patch makes the seek position of each sub-task mean "events of this batch
> already committed", counted from the start of the batch. In the three
> sub-tasks: a successful run returns the end of the batch ({{itrPos +
> eventCounter}}), so a retry skips it; a threshold flush records {{itrPos +
> eventCounter}} instead of {{eventCounter + 1}}; and the per event {{catch}}
> also takes {{RuntimeException}}, so the sub-task returns its last flushed
> position the same way it already does for {{IOException}}. In FSO and Legacy
> the threshold flush moves inside that {{try}}, as it already is in OBS, so an
> unchecked exception from the flush is covered too. A failed flush commits
> nothing (the batch is atomic), so the last committed position is still right.
> Not covered: an unchecked exception from the final flush after the loop, an
> {{Error}} and an interrupt, which still fall back to the position the attempt
> started from. A failure that repeats still fails the retry and leads to the
> rebuild as before. The controller and {{NSSummaryTask}} are unchanged.
> Retrying only the failed sub-tasks would need extra state in
> {{NSSummaryTask}}, the seek position already carries what is needed.
> Covered by four new cases in the existing {{TestNSSummaryTask}}:
> {{testRetryDoesNotReapplySucceededSubTask}},
> {{testRetryAfterThresholdFlushResumesAfterIOException}},
> {{testRetryAfterThresholdFlushResumesAfterRuntimeException}} (OBS) and
> {{testRetryResumesAfterUncheckedExceptionInThresholdFlush}} (Legacy, the
> flush throws). They fail without the change ({{expected: 2 but was: 3}},
> {{expected: 3 but was: 2}}, {{expected: 3 but was: 4}}, {{expected: 3 but
> was: 4}}). Three existing assertions on the returned seek positions are
> updated to the new meaning ({{testIndependentSubTaskSeekPositions}} in
> {{TestNSSummaryTask}}, two in {{TestNSSummaryTaskWithFSO}}). With the change
> the Recon namespace summary, task controller, delta sync and namespace
> summary endpoint suites pass (287 test runs) and checkstyle is clean. The
> patch does not repair totals that are already wrong.
> Found by TLA+ model checking and code review of the Recon namespace summary
> and task sync paths under HDDS-15926, on commit
> 0665932dd35da4d05087e02748af0a64eb846ad5. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-12062 (resolved, introduced the seek position retry),
> HDDS-5146 (a failure between the Recon OM DB update and the task run loses a
> batch) and HDDS-16463 (sync the Recon DB before the cursor advances). The
> attached patch is a proposal for review. Generated with Specula (Claude Opus
> 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
