Siyao Meng created HDDS-16807:
---------------------------------
Summary: 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
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]
