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

[F64116045]

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]

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]

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]

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]

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)

[F64116045]

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.

[devmadhuu]

@F64116045 This Recon behavior must be fixed in this PR to keep it consistent as this PR introduces inconsistency.

[F64116045]

Thanks @devmadhuu for the review.

Recon currently checks container health using its own replica data; it does not receive the mismatch state confirmed by SCM. So there are two ways to make the behavior consistent:

• Run the same two-scan check in Recon. This is the smaller change, but SCM and Recon may confirm at different times, and Recon must preserve the result across restarts.
• Send SCM’s confirmed result to Recon. The current RPC handles one container at a time, so this would likely require a batch RPC or persisted state.

With the first option, different scan timing could still make SCM and Recon temporarily disagree, so I’m not sure it fully addresses the consistency concern.

The scope of the second option would be much larger because we would need a batch API to pass confirmed mismatches and their removals from SCM to Recon. We would also need to handle incomplete transfers so Recon does not mistake a partial result for the complete state.

My preference is the first option. Recon already evaluates container health independently, and its scan does not necessarily run at the same time as SCM’s, so a short delay between their results is expected.

But I wonder which direction do you think is more appropriate here?

[devmadhuu]

I think Recon works on eventual consistency, so we should be fine with first option, but. logic should be consistent across SCM and Recon. Please go ahead with first option.

[F64116045]

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]

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]

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]

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.

[devmadhuu]

Thanks @F64116045 for the patch. Kindly make Recon behavior consistent as it was earlier in this PR.

[sodonnel]

From my reading, block deletion does not change the data checksum reported to SCM.

If this correct, then once a container is closed, the checksum should be fixed forever unless the container has a problem that is reconciled. This means the closed container either has no data checksum or the one and only data checksum.

Why then do we need to have the complexity of debouncing by recording scan 1 and then comparing with scan 2? What is that intended to guard against? I know it says mentioned debouncing in the Jira, but that was clearly generated by an AI and I am not sure it makes sense?

[F64116045]

I know it says mentioned debouncing in the Jira, but that was clearly generated by an AI and I am not sure it makes sense?

Thanks for questioning.

I went back and checked the cases it was meant to cover, including replicas that have not produced a checksum yet, Datanode reports arriving at different times, and checksum changes during scanning or reconciliation.

We already skip the comparison until every replica has reported a non-zero checksum.
Normal block deletion keeps the checksum unchanged.
Also if a different checksum arrives after that container has already been checked, the mismatch will be detected in the next scan

So I think you’re right that waiting for a second scan does not add any useful confirmation here.
I’ll remove the debounce, and thanks again for pointing this out.