[
https://issues.apache.org/jira/browse/HDDS-16807?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-16807:
------------------------------
Attachment: TestBugMC4ReconcileDiskBalancerMove.java
> DiskBalancer can move a container that is being reconciled, the repairs are
> lost and SCM gets a report from the retired replica
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-16807
> URL: https://issues.apache.org/jira/browse/HDDS-16807
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-4-skip-diskbalancer-move-of-container-being-reconciled.patch,
> TestBugMC4ReconcileDiskBalancerMove.java
>
>
> h3. Mechanism
> {{ContainerController.reconcileContainer}} looks the container up once.
> {{KeyValueHandler.reconcileContainerInternal}} then repairs that object,
> without holding the container lock, and ends with an incremental container
> report (ICR) built from it. {{DiskBalancerService.DiskBalancerTask}} copies a
> container to another volume under the container read lock, which does not
> exclude those writes, replaces the entry in the container set
> ({{ContainerSet.updateContainer}}) and marks the source replica DELETED.
> Neither side knows about the other: {{DefaultContainerChoosingPolicy}} does
> not skip a container that is being reconciled, and the states DiskBalancer
> moves (CLOSED, QUASI_CLOSED) are the ones reconciliation works on. A repair
> that is written after the copy has read that part of the replica exists only
> in the source replica, which is deleted.
> HDDS-15066 handled the replaced container object for delete, mark unhealthy,
> close and block deletion with {{ContainerSet.getContainerWithWriteLock}}.
> Reconciliation was not covered.
> h3. Trigger
> # DiskBalancer is running on a datanode (an admin has started it).
> # An admin runs {{ozone admin container reconcile}} for a container that has
> a replica on that datanode.
> # DiskBalancer picks that container while the reconciliation is running, or
> the reconciliation starts while the move is copying.
> The outcome depends on the order:
> * The reconciliation ends after the move has copied the data and before the
> move replaces the replica: the closing ICR reports the repaired replica,
> which is then replaced by the copy from before the repair (run). This order
> has the widest window, from the moment the copy has read the repaired part
> until the replica is replaced, which is the rest of the copy and the import
> (read from the code, not measured).
> * The move completes before the reconciliation writes: every repair fails
> with "is not in closed state, Container state is DELETED" and the closing ICR
> reports state DELETED for a container whose live replica is CLOSED (run).
> * The reconciliation ends in the few statements between
> {{ContainerSet.updateContainer}} and the source being marked DELETED: the
> remaining repairs go to the old replica and the closing ICR is built from it
> (run, the test holds the move at this point). Without that hold the source is
> marked DELETED right away and the writes fail as in the second order (read
> from the code).
> h3. Impact
> * The repair is lost and nothing repeats it, reconciliation runs only on an
> admin request. In the first order (and the narrow third) the datanode logs
> "Completed reconciliation ... 1 blocks were updated. Data checksum updated"
> although the replica that remains is not repaired. In the second order it
> logs the failed chunk writes. The replica is left as it was before the
> command, so no data is lost relative to that.
> * SCM receives a report that does not describe the live replica. In the first
> order it is the repaired BCSID and data checksum, and no report follows the
> move (run, datanode side only, the run has no SCM), so SCM would keep that
> view until the next full container report
> ({{hdds.container.report.interval}}, default 60m; read from the code, not
> run).
> * In the other two orders the scan that the reconciliation requests at its
> end runs on the live replica and sends a correct ICR when that scan completes
> (run with a 12 KB container; the on demand scanner is throttled to 5 MB/s per
> volume by default,
> {{hdds.container.scrub.on.demand.volume.bytes.per.second}}). For state
> DELETED, SCM drops the replica from its view until the next report for it, so
> the container can look under replicated meanwhile (read from the code, not
> run).
> The code is the same in 2.2.0 and 2.2.1 (read from the release tags, not run
> there).
> h3. Reproduction
> PASS, deterministic, unmodified source.
> [^TestBugMC4ReconcileDiskBalancerMove.java]
> ({{hadoop-hdds/container-service}}) has one datanode with two volumes whose
> replica has blocks 0 and 1 at BCSID 1, and a peer that also has block 2 at
> BCSID 2. It uses the real {{KeyValueHandler}}, {{ContainerSet}},
> {{ContainerController}}, on demand scanner and the move task handed out by
> {{DiskBalancerService.getTasks}}. Only the calls to the peer are mocked, as
> in {{TestContainerReconciliationWithMockDatanodes}}, and the ICRs are
> recorded instead of sent. The only timing aids are two existing test hooks.
> In the first test {{KeyValueContainer.setInjector}} holds the move after its
> copy while the reconciliation runs from start to end: the only ICR reports
> BCSID 2 and the peer's data checksum, the move then puts the copy in place,
> the live replica has blocks 0 and 1 at BCSID 1, block 2 exists only in the
> DELETED replica, and no further ICR is sent. In the second test the move is
> started from the peer's first GetBlock call and
> {{DiskBalancerService.setInjector}} holds it right after the container set
> points to the copy: the closing ICR comes from the retired replica with BCSID
> 2 and the live replica is unrepaired. In the third test the move completes at
> that GetBlock call: the closing ICR reports state DELETED while the live
> replica is CLOSED and unrepaired. A passing test means the defect is present.
> h3. Patch
> [^MC-4-skip-diskbalancer-move-of-container-being-reconciled.patch], against
> 1e528b97aed3eef1be6846f1f0767c1ef98a883a. It also applies to master at
> 8ca44914771.
> {{KeyValueHandler.reconcileContainer}} resolves the container with
> {{ContainerSet.getContainerWithWriteLock}}, marks it as being reconciled
> under that lock and releases the lock before it starts. A move in progress
> holds the read lock until it has replaced the replica, so the reconciliation
> waits for it and works on the replica the move left in the container set.
> {{DiskBalancerTask}} checks the mark under its read lock, next to its
> existing state check, and skips the move. The container is eligible again
> once the reconciliation has ended. The mark is in memory only. The write lock
> is not held during the reconciliation, so export, delete and the scanner are
> not blocked by it.
> Behaviour that changes outside the race: the start of a reconciliation now
> waits for a running export or move of that container (while it waits, new
> readers of that container lock queue behind it, as for the other users of
> {{getContainerWithWriteLock}}), and a reconcile command whose container is
> deleted before the reconciliation starts, for example while it waits, now
> fails with {{CONTAINER_NOT_FOUND}} instead of running on the deleted object.
> Covered by {{moveSkipsContainerBeingReconciled}},
> {{reconcileWaitsForMoveInProgress}} and
> {{reconcileAppliesOnDestVolumeContainer}} in the existing
> {{TestDiskBalancerWithConcurrentBackgroundTasks}}. Without the change all
> three fail for all 8 schema and layout combinations (the container is moved
> during the reconciliation, the reconciliation does not wait for the move, and
> the ICR is sent for the stale replica). With it that suite (56 tests) and the
> DiskBalancer, {{KeyValueHandler}}, container and reconciliation unit suites
> pass (715 tests, 6 skipped by the suite), {{TestDiskBalancer}} and
> {{TestDiskBalancerDuringDecommissionAndMaintenance}} pass on a mini cluster,
> checkstyle is clean. {{TestContainerCommandReconciliation}} has not been run
> with the patch (its mini cluster did not start in the test environment). With
> the patch all three tests of the reproduction fail, because the move is
> skipped or the reconciliation waits for it, and the replica is repaired in
> place.
> Found by TLA+ model checking and code review of the container reconciliation
> paths in the datanode and SCM under HDDS-15926, on commit
> 1e528b97aed3eef1be6846f1f0767c1ef98a883a. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. 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]
