You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/09/05 19:25:18 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #5246: HDDS-9125. Decommissioning blocked because of under replicated EC containers
sodonnel opened a new pull request, #5246:
URL: https://github.com/apache/ozone/pull/5246
## What changes were proposed in this pull request?
The situation is well documented under the heading 'Situation 4' in https://docs.google.com/document/d/1ebuSwJZkw4wMWWCHinDvRCfNbeFD4kcHMyIN6Q6wD9g/edit?usp=sharing. This happens because of limitations in rack scatter policy + replication manager flow.
An example (summary) of this situation:
Suppose there are 5 racks and 6 DNs, such that any one rack will have 2 DNs. 5 replicas of an EC container are scattered across each of the 5 racks (so that there's only 1 replica on each rack). Now, if any of the Datanodes from any rack where there's only 1 DN on that rack is decommissioned, under replication handling will be blocked.
In this PR, if a rack has only excluded nodes, we reduce the cluster rack count by 1 for each rack in this case. So if we have a 5 rack clusters, and have replicas like:
```
/rack1/dn1 - replica
/rack1/dn2 - free
/rack2/dn3 - replica
/rack2/dn4 - free
/rack3/dn5 - replica
/rack3/dn6 - free
/rack4/dn7 - replica
/rack4/dn8 - free
/rack5/dn9 - replica decommissioning.
```
When this is passed to the placement policy, DNs 1, 3, 5 and 7 will be listed as used. 9 will be excluded. Before this PR, the placement policy will fail to find a new node, as it needs to place them over 5 racks and it cannot. After this PR, the new logic will reduce the required racks by 1, and the new replica will be placed on one of the other free nodes.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9125
## How was this patch tested?
Unit tests added to reproduce the issue and test the change.
--
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] sodonnel commented on a diff in pull request #5246: HDDS-9125. Decommissioning blocked because of under replicated EC containers
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5246:
URL: https://github.com/apache/ozone/pull/5246#discussion_r1316910699
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -364,6 +372,57 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
return result;
}
+ /**
+ * Given a list of excluded nodes, check if the rack for each excluded node is
+ * empty after removing the excluded nodes. If it is empty, then the rack
+ * contains only excluded nodes, and we return a list of these racks.
+ * @param excludedNodes List of excluded nodes
+ * @param usedRacksCntMap Map of used racks and their used node count
+ * @return List of racks that contain only excluded nodes or an empty list
+ */
+ private List<Node> findRacksWithOnlyExcludedNodes(
+ List<DatanodeDetails> excludedNodes, Map<Node, Integer> usedRacksCntMap) {
+ if (excludedNodes == null || excludedNodes.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<Node> unavailableRacks = new ArrayList<>();
+ Set<Node> excludedNodeRacks = new HashSet<>();
+ for (Node node : excludedNodes) {
+ Node rack = networkTopology.getAncestor(node, RACK_LEVEL);
+ if (rack != null && !usedRacksCntMap.containsKey(rack)) {
+ // Rack for an excluded node and no used nodes on this rack.
+ excludedNodeRacks.add(rack);
+ }
+ }
+ Set<Node> exc = new HashSet<>(excludedNodes);
+ for (Node rack : excludedNodeRacks) {
+ // Dead nodes are removed from the topology, so the node may have a null
+ // rack, or the topology may not know about it, depending on the timing
+ // of when it got removed / excluded.
+ if (rack == null) {
+ continue;
+ }
+ // If a node is removed from the cluster (eg goes dead), but the client
+ // already added it to the exclude list, then the rack may not be in the
+ // topology any longer. See test
+ // testExcludedNodesOverlapsOutOfServiceNodes
+ String rackPath = rack.getNetworkFullPath();
+ if (networkTopology.getNode(rackPath) == null) {
+ continue;
+ }
+
+ Node node = networkTopology.chooseRandom(rack.getNetworkFullPath(), exc);
Review Comment:
Surely it must do, otherwise its pretty much useless. If there are nodes for it to pick, it should pick one or its a bug. There are no restrictions placed on the nodes when selecting them from the topology, any restrictions based on in_service or free space are applied after we select a node.
--
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] sodonnel merged pull request #5246: HDDS-9125. Decommissioning blocked because of under replicated EC containers
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel merged PR #5246:
URL: https://github.com/apache/ozone/pull/5246
--
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 #5246: HDDS-9125. Decommissioning blocked because of under replicated EC containers
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5246:
URL: https://github.com/apache/ozone/pull/5246#discussion_r1316777451
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -364,6 +372,57 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
return result;
}
+ /**
+ * Given a list of excluded nodes, check if the rack for each excluded node is
+ * empty after removing the excluded nodes. If it is empty, then the rack
+ * contains only excluded nodes, and we return a list of these racks.
+ * @param excludedNodes List of excluded nodes
+ * @param usedRacksCntMap Map of used racks and their used node count
+ * @return List of racks that contain only excluded nodes or an empty list
+ */
+ private List<Node> findRacksWithOnlyExcludedNodes(
+ List<DatanodeDetails> excludedNodes, Map<Node, Integer> usedRacksCntMap) {
+ if (excludedNodes == null || excludedNodes.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<Node> unavailableRacks = new ArrayList<>();
+ Set<Node> excludedNodeRacks = new HashSet<>();
+ for (Node node : excludedNodes) {
+ Node rack = networkTopology.getAncestor(node, RACK_LEVEL);
+ if (rack != null && !usedRacksCntMap.containsKey(rack)) {
+ // Rack for an excluded node and no used nodes on this rack.
+ excludedNodeRacks.add(rack);
+ }
+ }
+ Set<Node> exc = new HashSet<>(excludedNodes);
+ for (Node rack : excludedNodeRacks) {
+ // Dead nodes are removed from the topology, so the node may have a null
+ // rack, or the topology may not know about it, depending on the timing
+ // of when it got removed / excluded.
+ if (rack == null) {
+ continue;
+ }
+ // If a node is removed from the cluster (eg goes dead), but the client
+ // already added it to the exclude list, then the rack may not be in the
+ // topology any longer. See test
+ // testExcludedNodesOverlapsOutOfServiceNodes
+ String rackPath = rack.getNetworkFullPath();
+ if (networkTopology.getNode(rackPath) == null) {
+ continue;
+ }
+
+ Node node = networkTopology.chooseRandom(rack.getNetworkFullPath(), exc);
Review Comment:
We need to know if the `chooseRandom` API guarantees that a node will be returned if it's available.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -364,6 +372,57 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
return result;
}
+ /**
+ * Given a list of excluded nodes, check if the rack for each excluded node is
+ * empty after removing the excluded nodes. If it is empty, then the rack
+ * contains only excluded nodes, and we return a list of these racks.
+ * @param excludedNodes List of excluded nodes
+ * @param usedRacksCntMap Map of used racks and their used node count
+ * @return List of racks that contain only excluded nodes or an empty list
+ */
+ private List<Node> findRacksWithOnlyExcludedNodes(
+ List<DatanodeDetails> excludedNodes, Map<Node, Integer> usedRacksCntMap) {
+ if (excludedNodes == null || excludedNodes.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<Node> unavailableRacks = new ArrayList<>();
+ Set<Node> excludedNodeRacks = new HashSet<>();
+ for (Node node : excludedNodes) {
+ Node rack = networkTopology.getAncestor(node, RACK_LEVEL);
+ if (rack != null && !usedRacksCntMap.containsKey(rack)) {
+ // Rack for an excluded node and no used nodes on this rack.
+ excludedNodeRacks.add(rack);
+ }
+ }
+ Set<Node> exc = new HashSet<>(excludedNodes);
+ for (Node rack : excludedNodeRacks) {
+ // Dead nodes are removed from the topology, so the node may have a null
+ // rack, or the topology may not know about it, depending on the timing
+ // of when it got removed / excluded.
+ if (rack == null) {
+ continue;
+ }
Review Comment:
Is this check needed given that we're checking for null at lines 393 and 411?
--
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] sodonnel commented on a diff in pull request #5246: HDDS-9125. Decommissioning blocked because of under replicated EC containers
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5246:
URL: https://github.com/apache/ozone/pull/5246#discussion_r1316917304
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -364,6 +372,57 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
return result;
}
+ /**
+ * Given a list of excluded nodes, check if the rack for each excluded node is
+ * empty after removing the excluded nodes. If it is empty, then the rack
+ * contains only excluded nodes, and we return a list of these racks.
+ * @param excludedNodes List of excluded nodes
+ * @param usedRacksCntMap Map of used racks and their used node count
+ * @return List of racks that contain only excluded nodes or an empty list
+ */
+ private List<Node> findRacksWithOnlyExcludedNodes(
+ List<DatanodeDetails> excludedNodes, Map<Node, Integer> usedRacksCntMap) {
+ if (excludedNodes == null || excludedNodes.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<Node> unavailableRacks = new ArrayList<>();
+ Set<Node> excludedNodeRacks = new HashSet<>();
+ for (Node node : excludedNodes) {
+ Node rack = networkTopology.getAncestor(node, RACK_LEVEL);
+ if (rack != null && !usedRacksCntMap.containsKey(rack)) {
+ // Rack for an excluded node and no used nodes on this rack.
+ excludedNodeRacks.add(rack);
+ }
+ }
+ Set<Node> exc = new HashSet<>(excludedNodes);
+ for (Node rack : excludedNodeRacks) {
+ // Dead nodes are removed from the topology, so the node may have a null
+ // rack, or the topology may not know about it, depending on the timing
+ // of when it got removed / excluded.
+ if (rack == null) {
+ continue;
+ }
Review Comment:
Yes, you are correct. The Rack must be non null in the map, as we check it before inserting it. There is no need to check again 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