HDDS-10714. Restrict reconciliation requests by datanode status - #11182
Open
rich7420 wants to merge 4 commits into
Open
HDDS-10714. Restrict reconciliation requests by datanode status#11182rich7420 wants to merge 4 commits into
rich7420 wants to merge 4 commits into
Conversation
ReconcileContainerEventHandler sent a reconcile command to every replica and listed every other replica as a peer, ignoring node status. Restrict participation using the rules from the Jira: stale, dead, decommissioned, and in-maintenance nodes are neither peers nor targets; decommissioning and entering-maintenance nodes can be peers but not targets so their data is reconciled off before they leave the cluster; healthy, in-service nodes are both. The handler now looks up each replica's NodeStatus and partitions the replicas into targets and peers accordingly, and skips a target that has no eligible peer to reconcile against. Covered by TestReconcileContainerEventHandler cases for the per-status partition, the no-eligible-peer skip, and an unknown-status node. Claude-Session: https://claude.ai/code/session_01FUpCUnmy6JzHPGvwMhyGzq
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Restricts SCM’s container reconciliation participants based on datanode health/operational status via NodeManager, and updates tests and wiring accordingly.
Changes:
- Inject
NodeManagerintoReconcileContainerEventHandlerand filter reconciliation targets/peers byNodeStatus. - Skip reconciliation when no eligible targets exist; allow “target-only” reconciliation when peers are empty.
- Extend
TestReconcileContainerEventHandlerwith status-based reconciliation scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/reconciliation/ReconcileContainerEventHandler.java | Adds NodeManager-driven filtering for reconciliation targets/peers and skip behavior. |
| hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java | Wires scmNodeManager into the updated reconciliation handler constructor. |
| hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/reconciliation/TestReconcileContainerEventHandler.java | Adds/updates tests validating reconciliation behavior across node statuses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+76
to
+98
| Set<DatanodeDetails> targets = new HashSet<>(); | ||
| Set<DatanodeDetails> peers = new HashSet<>(); | ||
| for (ContainerReplica replica : containerManager.getContainerReplicas(containerID)) { | ||
| DatanodeDetails datanode = replica.getDatanodeDetails(); | ||
| final NodeStatus status; | ||
| try { | ||
| status = nodeManager.getNodeStatus(datanode); | ||
| } catch (NodeNotFoundException ex) { | ||
| LOG.warn("Skipping datanode {} for reconciliation of container {} since its status is unknown.", | ||
| datanode, containerID); | ||
| continue; | ||
| } | ||
| if (!status.isHealthy()) { | ||
| continue; | ||
| } | ||
| // Transitioning nodes remain peers so other replicas can recover any unique data before the node leaves. | ||
| if (!status.isDecommissioned() && !status.isInMaintenance()) { | ||
| peers.add(datanode); | ||
| } | ||
| if (status.isInService()) { | ||
| targets.add(datanode); | ||
| } | ||
| } |
Comment on lines
+100
to
+107
| LOG.info("Reconcile container event triggered for container {} with targets {} and peers {}", | ||
| containerID, targets, peers); | ||
|
|
||
| LOG.info("Reconcile container event triggered for container {} with peers {}", containerID, allReplicaNodes); | ||
| if (targets.isEmpty()) { | ||
| LOG.warn("Skipping reconciliation for container {} since no eligible target datanodes are available.", | ||
| containerID); | ||
| return; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
SCM previously sent container reconciliation commands to every datanode with a replica and allowed every replica to be used as a peer, regardless of the datanode's health or operational state.
This change uses
NodeManagerstatus when selecting reconciliation participants:The existing container and replica eligibility rules are unchanged. Changes to reconciliation status reporting are out of scope.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10714
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/33510354889
10 x 10 flaky-test-check for
TestReconcileContainerEventHandler— 100/100 passed: https://github.com/rich7420/ozone/actions/runs/33515332863