HDDS-16012. Detect replicas with equal BCSID and mismatched data checksums - #11199
HDDS-16012. Detect replicas with equal BCSID and mismatched data checksums#11199F64116045 wants to merge 2 commits into
Conversation
|
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. |
|
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 One detail with placing the handler at the end is that the current chain stops once a handler handles a container. Just want to confirm does that match the intended behavior? |
|
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. |
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) |
a96fa18 to
5adc3f0
Compare
| // Persist checksum mismatches in Recon's REPLICA_MISMATCH state. | ||
| if (container.getState() == CLOSED && | ||
| container.getReplicationType() == RATIS && | ||
| hasMismatch(replicas, ContainerReplica::getSequenceId, |
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
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. |
|
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? |
|
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. The deletion path rebuilds the block entry from its existing chunk metadata and marks it as deleted. |
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 existingGetContainerReplicasresponse. 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:
ozone admin container info; andCI: https://github.com/F64116045/ozone/actions/runs/33977432622