Skip to content

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

Draft
F64116045 wants to merge 1 commit into
apache:masterfrom
F64116045:HDDS-16012
Draft

HDDS-16012. Detect replicas with equal BCSID and mismatched data checksums#11199
F64116045 wants to merge 1 commit 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.

@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)

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