Skip to content

HDDS-16012. Detect replicas with equal BCSID and mismatched data checksums - #11199

Open
F64116045 wants to merge 2 commits into
apache:masterfrom
F64116045:HDDS-16012
Open

HDDS-16012. Detect replicas with equal BCSID and mismatched data checksums#11199
F64116045 wants to merge 2 commits into
apache:masterfrom
F64116045:HDDS-16012

Conversation

@F64116045

@F64116045 F64116045 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

SCM receives the BCSID and data checksum in container replica reports, but did not compare checksums across replicas.

This patch detects CLOSED RATIS containers whose replicas report the same BCSID but different non-empty data checksums. Replication Manager confirms a mismatch only when it appears in two consecutive complete scans.

Confirmed mismatches are exposed through a metric, logged once per continuous mismatch episode, and shown by ozone admin container info. The CLI receives the result determined by SCM through the existing GetContainerReplicas response. Recon uses the same comparison rule.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16012

How was this patch tested?

Added or updated unit tests covering:

  • checksum comparison;
  • Replication Manager debounce, metric, and warning behavior;
  • SCM protocol responses;
  • ozone admin container info; and
  • Recon mismatch reporting.

CI: https://github.com/F64116045/ozone/actions/runs/33977432622

@sodonnel

sodonnel commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

To start with, I think the new logic for checking and logging checksum mis-matches should got into a "health check" that gets added to the processing chain (perhaps at the end, when all replication and health issues have been addressed), but you could perhaps argue that this is an element of an unhealthy container that the data checksum now exposes.

Another reason to add this as a healthy check, is that we many want to trigger an action (replication, reconciliation etc) based on this, so the flow should fit into the structure which replication manger already has. Check the existing health checks, like RatisReplicationCheckHandler.

I also wonder if we should be adding details to the existing replicationManager report for this, which kind of works with the metrics system too.

@F64116045

Copy link
Copy Markdown
Contributor Author

Thanks @sodonnel for the suggestion! I agree this would fit better in the existing health-check chain, especially if it may trigger an action later. The handler can also record the result in ReplicationManagerReport.

One detail with placing the handler at the end is that the current chain stops once a handler handles a container.
For example, if RatisReplicationCheckHandler handles it as under or over replicated, the checksum handler would be skipped for that scan. This means the checksum check would only run after those replication issues no longer apply.

Just want to confirm does that match the intended behavior?

@sodonnel

sodonnel commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

I am not sure - putting it at the end was the first thing I thought of, but if its read only for now (aside from gathering metrics / report) then it can go into the chain where makes sense as long as it returns false so other subsequent parts run. Its simple to adjust the position in the processing chain after the Check Handler is written so we can always change that small part.

@F64116045

Copy link
Copy Markdown
Contributor Author

Its simple to adjust the position in the processing chain after the Check Handler is written so we can always change that small part.

That makes sense, thanks. So I’m planning to place the handler here for now:

.addNext(ecReplicationCheckHandler)
.addNext(checksumMismatchHandler)
.addNext(ratisReplicationCheckHandler)

The earlier handlers won’t stop the CLOSED RATIS containers this check applies to, and the checksum handler will return false so the rest of the chain can continue. Please let me know if you have any other concerns, thanks!

(BTW, since this refactor will replace much of the current implementation, I plan to amend the commit and force-push the updated patch after)

// Persist checksum mismatches in Recon's REPLICA_MISMATCH state.
if (container.getState() == CLOSED &&
container.getReplicationType() == RATIS &&
hasMismatch(replicas, ContainerReplica::getSequenceId,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recon still reports a mismatch immediately, as it did before. It does not use SCM’s two-scan debounce. This preserves Recon’s existing reporting behavior, but means Recon may report the mismatch before SCM confirms it and before the CLI displays it.

Please let me know if you have any concerns about keeping this behavior.

Comment on lines +56 to +65
for (T left : replicas) {
Long leftSequenceId = sequenceId.apply(left);
long leftDataChecksum = dataChecksum.applyAsLong(left);
for (T right : replicas) {
if (leftSequenceId.equals(sequenceId.apply(right)) &&
leftDataChecksum != dataChecksum.applyAsLong(right)) {
return true;
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered using a HashMap to make the comparison O(R), R is the replica count of one container.:

previous = checksumsByBcsId.putIfAbsent(bcsId, checksum);

However, this check runs across the CLOSED RATIS containers during every full Replication Manager scan. Using a map would create a lot of short-lived maps in quick succession.
Since RATIS normally has only three replicas, the current O(R²) loop does 3 × 3 comparisons per container and avoids those allocations.

@F64116045

F64116045 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

I’ve updated the patch based on the discussion. Checksum mismatches now use Replication Manager’s existing health-check and report flow instead of a separate processing and metrics path. The new check records the mismatch and lets the rest of the chain continue.

The PR is now larger than I expected, mainly because the result also needs to pass through the report, RPC, and CLI layers.
If this is difficult to review as one PR, please let me know, maybe we can split the health-check and reporting changes from the RPC and CLI changes.

@sodonnel

sodonnel commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

I have a question about the data checksum that is passed to SCM from the datanodes. A closed container cannot get any new writes - it can only get deletes. How does the data checksum change as deletes are processed? Do they result in the data checksum changing as each block is deleted, or is the datachecksum fixed at container close time based on the contents of the container at that time?

@F64116045

Copy link
Copy Markdown
Contributor Author

Thanks @sodonnel for the question.

From my reading, block deletion does not change the data checksum reported to SCM. The checksum is first generated when the container is closed.
When a block is deleted, its entry is retained in the checksum tree with the same block checksum:

Before deletion: blockID=N, checksum=X, deleted=false
After deletion:  blockID=N, checksum=X, deleted=true

The deletion path rebuilds the block entry from its existing chunk metadata and marks it as deleted.
The block checksum is calculated before the chunk-level entries are cleared.
Since the container checksum is calculated from all block checksums in the tree, the value X remains part of the calculation after deletion.
Therefore, block deletion preserves the current container checksum rather than generating a new value for each delete.

@F64116045
F64116045 marked this pull request as ready for review September 5, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants