You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "siddhantsangwan (via GitHub)" <gi...@apache.org> on 2023/09/08 15:59:54 UTC
[GitHub] [ozone] siddhantsangwan opened a new pull request, #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
siddhantsangwan opened a new pull request, #5261:
URL: https://github.com/apache/ozone/pull/5261
## What changes were proposed in this pull request?
Problem:
The legacy replication manager currently resolves mismatched replicas (those whose replica state do not match SCM's container state) by
1. Replicating the matching replicas until they are fully replicated.
2. Deleting the mismatched replicas.
This approach does not work when LRM is presented with the following small cluster situation:
SCM state: CLOSED.
5 datanodes in the cluster.
Replica states: CLOSED CLOSED QUASI QUASI QUASI.
LRM will not make progress because there is no datanode to add a closed replica to that does not already have a replica.
Changes proposed:
Try to delete an unhealthy replica (UNHEALTHY or QUASI_CLOSED) to free up a datanode for a healthy replica. We prefer deleting a replica with less sequence id than the container's. If the container is QUASI_CLOSED, then the replica to be deleted should not have a unique origin node id. Also, this replica should be on a healthy, in-service node.
We do this only if there isn't a pending delete, if there are at least 4 replicas, and if there is at least one replica which matches the container's lifecycle state.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9257
## How was this patch tested?
Yet to test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1321411776
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
Review Comment:
I'm avoiding this check as it'll also return true for cases like closed container with closing replica, quasi-closed container with closing replica etc. Instead, I'm now doing
```
if (containerState != LifeCycleState.QUASI_CLOSED &&
replica.getState() == State.QUASI_CLOSED) {
// a quasi_closed replica is a candidate only if its seq id is less
// than the container's
if (replica.getSequenceId() < containerSeq) {
deleteCandidates.add(replica);
}
} else if (replica.getState() == State.UNHEALTHY) {
deleteCandidates.add(replica);
}
}
```
which should do what we want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1321408680
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
+ }
+
+ if (containerState == LifeCycleState.CLOSED) {
+ deleteExcessLowestBcsIDs(container, deleteCandidates, 1);
+ return;
+ }
+
+ // if the container is quasi_closed, delete a replica only if it's seq id
+ // is less, and it doesn't have a unique origin node
+ if (containerState == LifeCycleState.QUASI_CLOSED) {
+ List<ContainerReplica> nonUniqueDeleteCandidates =
+ findNonUniqueDeleteCandidates(replicas, deleteCandidates);
+ deleteExcessLowestBcsIDs(container, nonUniqueDeleteCandidates, 1);
+ return;
Review Comment:
I've updated this part to only delete an UNHEALTHY replica when we have a quasi-closed container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 merged pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 merged PR #5261:
URL: https://github.com/apache/ozone/pull/5261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1320263332
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
+ }
+
+ if (containerState == LifeCycleState.CLOSED) {
+ deleteExcessLowestBcsIDs(container, deleteCandidates, 1);
+ return;
+ }
+
+ // if the container is quasi_closed, delete a replica only if it's seq id
+ // is less, and it doesn't have a unique origin node
+ if (containerState == LifeCycleState.QUASI_CLOSED) {
+ List<ContainerReplica> nonUniqueDeleteCandidates =
+ findNonUniqueDeleteCandidates(replicas, deleteCandidates);
+ deleteExcessLowestBcsIDs(container, nonUniqueDeleteCandidates, 1);
+ return;
Review Comment:
I think this part requires some modification. If we're deleting a quasi closed replica, we need to ensure it's sequence id is actually less than the container. Otherwise we should delete an UNHEALTHY replica.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #5261:
URL: https://github.com/apache/ozone/pull/5261#issuecomment-1713718020
I've addressed review comments and added a unit test for quasi-closed containers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1320978177
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
Review Comment:
Yes, this is just a defensive check. We shouldn't end up here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #5261:
URL: https://github.com/apache/ozone/pull/5261#issuecomment-1712105628
I am checking for at least 3 replicas in the latest commit, not 4. This is consistent with #5255.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1320439776
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
Review Comment:
In this case all the replicas are unhealthy. This is a defensive check and should not actually happen since this method is called from `handleUnderReplicatedHealthy` right?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
+ }
+
+ if (containerState == LifeCycleState.CLOSED) {
+ deleteExcessLowestBcsIDs(container, deleteCandidates, 1);
+ return;
+ }
+
+ // if the container is quasi_closed, delete a replica only if it's seq id
+ // is less, and it doesn't have a unique origin node
+ if (containerState == LifeCycleState.QUASI_CLOSED) {
+ List<ContainerReplica> nonUniqueDeleteCandidates =
+ findNonUniqueDeleteCandidates(replicas, deleteCandidates);
+ deleteExcessLowestBcsIDs(container, nonUniqueDeleteCandidates, 1);
+ return;
Review Comment:
We shouldn't need to delete a quasi-closed replica here. We would get here if we have at least one quasi-closed replica and unhealthy replicas on the rest of the nodes. The quasi-closed replica is under-replicated, which is why we are in `handleUnderReplicatedHealthy`, but it cannot be replicated because the rest of the nodes have unhealthy replicas. So this case should only need to delete unhealthy replicas. I think this would be handled automatically with my suggestion above.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
Review Comment:
Can't we do this check instead?
```suggestion
if (!compareState(containerState, replica.getState())) {
deleteCandidates.add(replica);
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -2283,11 +2395,15 @@ private void replicateAnyWithTopology(ContainerInfo container,
"replica found.",
container.containerID());
}
- } catch (IOException | IllegalStateException ex) {
+ } catch (IllegalStateException ex) {
LOG.warn("Exception while replicating container {}.",
container.getContainerID(), ex);
}
}
+//
+// private void removeUnhealthyDatanodeIfNeeded() {
+// if (getInflightDel())
+// }
Review Comment:
nit. commented code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #5261:
URL: https://github.com/apache/ozone/pull/5261#discussion_r1320452758
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1141,8 +1141,103 @@ private void handleUnderReplicatedHealthy(final ContainerInfo container,
List<ContainerReplica> replicationSources = getReplicationSources(container,
replicaSet.getReplicas(), State.CLOSED, State.QUASI_CLOSED);
// This method will handle topology even if replicasNeeded <= 0.
- replicateAnyWithTopology(container, replicationSources,
- placementStatus, replicasNeeded);
+ try {
+ replicateAnyWithTopology(container, replicationSources,
+ placementStatus, replicasNeeded);
+ } catch (SCMException e) {
+ if (e.getResult()
+ .equals(SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE) &&
+ replicasNeeded > 0) {
+ /*
+ If we reach here, the container is under replicated but placement
+ policy could not find any target Datanodes to host new replicas.
+ We can try unblocking under replication handling by removing any
+ unhealthy replicas. This will free up those datanodes, so they can host
+ healthy replicas.
+ */
+ deleteUnhealthyReplicaIfNeeded(container, replicaSet);
+ }
+ }
+ }
+
+ /**
+ * Finds and deletes an unhealthy replica (UNHEALTHY or QUASI_CLOSED) under
+ * certain conditions.
+ */
+ private void deleteUnhealthyReplicaIfNeeded(ContainerInfo container,
+ RatisContainerReplicaCount replicaCount) {
+ LOG.info("Finding an unhealthy replica to delete for container {} with " +
+ "replicas {} to unblock under replication handling.", container,
+ replicaCount.getReplicas());
+
+ int pendingDeletes = getInflightDel(container.containerID());
+ if (pendingDeletes > 0) {
+ LOG.debug("Container {} has {} pending deletes. Will not delete an " +
+ "unhealthy replica for this container.", container, pendingDeletes);
+ return;
+ }
+
+ List<ContainerReplica> replicas = replicaCount.getReplicas();
+ if (replicas.size() < 3) {
+ LOG.debug("Container {} has only {} replicas. Will not delete an " +
+ "unhealthy replica for this container.", container, replicas.size());
+ return;
+ }
+
+ LifeCycleState containerState = container.getState();
+ boolean foundMatchingReplica = false;
+ for (ContainerReplica replica : replicas) {
+ if (compareState(containerState, replica.getState())) {
+ foundMatchingReplica = true;
+ break;
+ }
+ }
+ if (!foundMatchingReplica) {
+ LOG.debug("No matching replica found for container {} with replicas " +
+ "{}. Will not delete any unhealthy replica for this container.",
+ container, replicas);
+ return;
+ }
+
+ List<ContainerReplica> deleteCandidates = new ArrayList<>();
+ // collect unhealthy replicas on in-service, healthy nodes
+ for (ContainerReplica replica : replicas) {
+ try {
+ NodeStatus nodeStatus =
+ nodeManager.getNodeStatus(replica.getDatanodeDetails());
+ if (!nodeStatus.isHealthy() || !nodeStatus.isInService()) {
+ continue;
+ }
+ } catch (NodeNotFoundException e) {
+ LOG.warn("Skipping replica {} when trying to unblock under " +
+ "replication handling.", replica, e);
+ continue;
+ }
+
+ if (replica.getState() == State.QUASI_CLOSED) {
+ deleteCandidates.add(replica);
+ } else if (replica.getState() == State.UNHEALTHY) {
+ deleteCandidates.add(replica);
+ }
+ }
+
+ if (containerState == LifeCycleState.CLOSED) {
+ deleteExcessLowestBcsIDs(container, deleteCandidates, 1);
+ return;
+ }
+
+ // if the container is quasi_closed, delete a replica only if it's seq id
+ // is less, and it doesn't have a unique origin node
+ if (containerState == LifeCycleState.QUASI_CLOSED) {
+ List<ContainerReplica> nonUniqueDeleteCandidates =
+ findNonUniqueDeleteCandidates(replicas, deleteCandidates);
+ deleteExcessLowestBcsIDs(container, nonUniqueDeleteCandidates, 1);
+ return;
Review Comment:
We shouldn't need to delete a quasi-closed replica here. We would get here if we have at least one quasi-closed replica and unhealthy replicas on the rest of the nodes. The quasi-closed replica is under-replicated, which is why we are in `handleUnderReplicatedHealthy`, but it cannot be replicated because the rest of the nodes have unhealthy replicas. So this case should only need to delete unhealthy replicas. I think this would be handled automatically with my [suggestion](https://github.com/apache/ozone/pull/5261#discussion_r1320440166) above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on pull request #5261: HDDS-9257. LegacyReplicationManager: Unhealthy replicas could block under replication handling
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #5261:
URL: https://github.com/apache/ozone/pull/5261#issuecomment-1715196705
@sodonnel @errose28 Using the common logic from `ReplicationManagerUtil` in this PR now. Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org