You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Tejaskriya (via GitHub)" <gi...@apache.org> on 2024/03/12 06:35:32 UTC
[PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Tejaskriya opened a new pull request, #6367:
URL: https://github.com/apache/ozone/pull/6367
## What changes were proposed in this pull request?
Many users test out Ozone in small clusters of 15 Datanodes or less. If a 15 DN cluster has some EC 10-4 containers, for example, then it's not possible to decommission more than 1 Datanode, because EC 10-4 requires at least 14 Datanodes. If someone tries to decommission 2 Datanodes, the decommissioning process is designed such that it will keep looping and checking every 30 seconds whether it's possible to take the two DNs offline. It will never fail, even though it's clearly not possible to take the Datanodes offline.
In this PR, the decommission is failed early if sufficient datanodes are not available based on the maximum replication factor of containers present in the cluster. It returns a corresponding DatanodeAdminError.
The detailed design doc can be found in the EPIC HDDS-10461
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10462
## How was this patch tested?
Tested manually in docker cluster. In the following cluster, RATIS-THREE is the maximum replication and it has 5 DNS:
```
bash-4.2$ ozone admin datanode decommission ozone-datanode-4 ozone-datanode-5 ozone-datanode-2
Started decommissioning datanode(s):
ozone-datanode-4
ozone-datanode-5
ozone-datanode-2
Error: AllHosts: Sufficient nodes are not available.
Some nodes could not enter the decommission workflow
```
-- will add unit tests and mark it _ready for review_ post that.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1533252567
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
Review Comment:
We also need to check that the Datanodes being decommissioned are initially in-service only and not any other state. We'd have to account for any non in-service DNs or DNs that we're getting `NodeNotFoundException` for in our calculation also.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
Review Comment:
We need to check that the Datanode being decommissioned are in-service only. Need to account for non in-service datanodes and datanodes that we're getting `NodeNotFoundException` for in our calculation as well.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeDecommissionManager.java:
##########
@@ -56,17 +75,50 @@ public class TestNodeDecommissionManager {
private NodeDecommissionManager decom;
private StorageContainerManager scm;
private NodeManager nodeManager;
+ private ContainerManager containerManager;
private OzoneConfiguration conf;
+ @TempDir
+ private File testDir;
+ private DBStore dbStore;
+ private SCMHAManager scmhaManager;
+ private SequenceIdGenerator sequenceIdGen;
+ private ContainerReplicaPendingOps pendingOpsMock;
@BeforeEach
void setup(@TempDir File dir) throws Exception {
conf = new OzoneConfiguration();
conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, dir.getAbsolutePath());
- nodeManager = createNodeManager(conf);
- decom = new NodeDecommissionManager(conf, nodeManager,
+ scm = HddsTestUtils.getScm(conf);
+ nodeManager = scm.getScmNodeManager();
+ final OzoneConfiguration ozConf = SCMTestUtils.getConf(testDir);
+ dbStore = DBStoreBuilder.createDBStore(
+ ozConf, new SCMDBDefinition());
+ scmhaManager = SCMHAManagerStub.getInstance(true);
+ sequenceIdGen = new SequenceIdGenerator(
+ ozConf, scmhaManager, SCMDBDefinition.SEQUENCE_ID.getTable(dbStore));
+ final PipelineManager pipelineManager =
+ new MockPipelineManager(dbStore, scmhaManager, nodeManager);
+ pipelineManager.createPipeline(RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE));
+ pendingOpsMock = mock(ContainerReplicaPendingOps.class);
+ containerManager = new ContainerManagerImpl(ozConf,
Review Comment:
I have not checked the test cases yet, but is it necessary to have the actual container manager implementation here? Can we not mock it instead? If we mock it, this test doesn't need to depend on the actual implementation.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
+ cif.getState().equals(HddsProtos.LifeCycleState.DELETING)) {
+ continue;
+ }
+ int reqNodes = cif.getReplicationConfig().getRequiredNodes();
+ if (reqNodes > minInService) {
+ minInService = reqNodes;
+ }
+ /* *below code would check the replication type and then get the factor,
+ but as we have a simpler way i.e., getRequiredNodes(), I don't think we need to care about the replication type
+
+ HddsProtos.ReplicationType replicationType = cif.getReplicationType();
+ if (replicationType.equals(HddsProtos.ReplicationType.RATIS)) {
+ maxRatis = cif.getReplicationFactor().getNumber();
+ } else if (replicationType.equals(HddsProtos.ReplicationType.EC) {
+ //cif.getReplicationConfig();
+ }*/
Review Comment:
Yes, `getRequiredNodes` is 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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1534044125
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
Review Comment:
It does give me some concern that this large iteration occurs in response to a user command. However, it does not hold any locks beyond blocking further decommission commands, so it is probably OK.
An alternative, is to have Replication Manager track the count of containers per replication_type (and expose them via metrics or the container report) and they we could query RM for the largest number of nodes needed. However the RM data is always stale (runs every 5 minutes), and it cannot tell you if the containers on "this DN" will block. Also RM may not have ran yet after startup.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1546051331
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeDecommissionManager.java:
##########
@@ -369,10 +410,208 @@ public void testNodeDecommissionManagerOnBecomeLeader() throws Exception {
assertEquals(decom.getMonitor().getTrackedNodes().size(), 3);
}
- private SCMNodeManager createNodeManager(OzoneConfiguration config)
- throws IOException, AuthenticationException {
- scm = HddsTestUtils.getScm(config);
- return (SCMNodeManager) scm.getScmNodeManager();
+ @Test
+ public void testInsufficientNodeDecommissionThrowsExceptionForRatis() throws
+ NodeNotFoundException, IOException {
+ when(containerManager.getContainer(any(ContainerID.class)))
+ .thenAnswer(invocation -> getMockContainer(RatisReplicationConfig
+ .getInstance(HddsProtos.ReplicationFactor.THREE), (ContainerID)invocation.getArguments()[0]));
+ List<DatanodeAdminError> error;
+ List<DatanodeDetails> dns = new ArrayList<>();
+
+ for (int i = 0; i < 5; i++) {
+ DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
+ dns.add(dn);
+ nodeManager.register(dn, null, null);
+ }
+
+ Set<ContainerID> idsRatis = new HashSet<>();
+ for (int i = 0; i < 5; i++) {
+ ContainerInfo container = containerManager.allocateContainer(
+ RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE), "admin");
+ idsRatis.add(container.containerID());
+ }
+
+ for (DatanodeDetails dn : nodeManager.getAllNodes().subList(0, 3)) {
+ nodeManager.setContainers(dn, idsRatis);
+ }
+
+ error = decom.decommissionNodes(Arrays.asList(dns.get(1).getIpAddress(),
+ dns.get(2).getIpAddress(), dns.get(3).getIpAddress(), dns.get(4).getIpAddress()), false);
+ assertTrue(error.get(0).getHostname().contains("AllHosts"));
+
+ error = decom.decommissionNodes(Arrays.asList(dns.get(1).getIpAddress(),
+ dns.get(2).getIpAddress(), dns.get(3).getIpAddress(), dns.get(4).getIpAddress()), true);
+ assertTrue(error.size() == 0);
Review Comment:
Intelllij: `'assertTrue()' can be simplified to 'assertEquals()' `
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
Review Comment:
Looks like these two lines are basically doing the same thing? Also, intellij suggests
```
List<DatanodeDetails> validDns = new ArrayList<>(dns);
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ }
+
+ for (DatanodeDetails dn : validDns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
Review Comment:
I don't think we need to add this exception to the list of errors here, since it'll get added later at line 340. This will avoid duplicate errors.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeDecommissionManager.java:
##########
@@ -369,10 +410,208 @@ public void testNodeDecommissionManagerOnBecomeLeader() throws Exception {
assertEquals(decom.getMonitor().getTrackedNodes().size(), 3);
}
- private SCMNodeManager createNodeManager(OzoneConfiguration config)
- throws IOException, AuthenticationException {
- scm = HddsTestUtils.getScm(config);
- return (SCMNodeManager) scm.getScmNodeManager();
+ @Test
+ public void testInsufficientNodeDecommissionThrowsExceptionForRatis() throws
Review Comment:
The newly added test cases look good. Can we also assert that the DNs have transitioned to the decommissioning state wherever relevant? Existing tests are doing that.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ }
+
+ for (DatanodeDetails dn : validDns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
Review Comment:
We should log this exception.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1546126995
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
Review Comment:
I missed that line during clean up. Thanks for catching it!
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1539064328
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
Review Comment:
We'll have to synchronize on the container info object as discussed in the design doc. For example, in `AbstractContainerReportHandler`:
```
synchronized (containerInfo) {
updateContainerStats(datanodeDetails, containerInfo, replicaProto);
if (!updateContainerState(datanodeDetails, containerInfo, replicaProto,
publisher)) {
updateContainerReplica(datanodeDetails, containerId, replicaProto);
}
}
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -528,10 +528,11 @@ public HddsProtos.Node queryNode(UUID uuid) throws IOException {
/**
* Attempts to decommission the list of nodes.
* @param nodes The list of hostnames or hostname:ports to decommission
+ * @param force boolean flag that skips fail-early checks and tries to decommission nodes
* @throws IOException
*/
@Override
- public List<DatanodeAdminError> decommissionNodes(List<String> nodes)
+ public List<DatanodeAdminError> decommissionNodes(List<String> nodes, boolean force)
Review Comment:
We need to send the `force` arguement in the actual proto message as well.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
+ cif.getState().equals(HddsProtos.LifeCycleState.DELETING)) {
+ continue;
+ }
+ int reqNodes = cif.getReplicationConfig().getRequiredNodes();
+ if ((inServiceTotal - numDecom) < reqNodes) {
+ return false;
+ }
+ if (reqNodes > minInService) {
Review Comment:
I think you forgot to remove `minInService`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -305,9 +312,18 @@ public DatanodeAdminMonitor getMonitor() {
}
public synchronized List<DatanodeAdminError> decommissionNodes(
- List<String> nodes) {
+ List<String> nodes, boolean force) {
List<DatanodeAdminError> errors = new ArrayList<>();
List<DatanodeDetails> dns = mapHostnamesToDatanodes(nodes, errors);
+ // add check for fail-early if force flag is not set
+ if (!force) {
+ boolean decommissionPossible = checkIfDecommissionPossible(dns, errors);
+ if (!decommissionPossible) {
+ LOG.error("Cannot decommission nodes as sufficient node are not available.");
Review Comment:
Let's mention hostnames of the DNs in the log at line 322 as well. Also I think it's best add a log before line 319 that says we're checking if decommission is possible and shows the value of `force`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
+ cif.getState().equals(HddsProtos.LifeCycleState.DELETING)) {
+ continue;
+ }
+ int reqNodes = cif.getReplicationConfig().getRequiredNodes();
+ if ((inServiceTotal - numDecom) < reqNodes) {
+ return false;
Review Comment:
Let's log all necessary details when returning false, at the INFO level. Such as the total in-service nodes, number of nodes to be decommissioned, valid nodes out of that, and the number of required nodes. And also the DN and the ContainerInfo because of which we're failing early.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
Review Comment:
We need to ignore non in-service DNs in this loop too. Let's add some tests for non in-service and `NodeNotFoundException` DNs.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java:
##########
@@ -217,11 +217,12 @@ List<HddsProtos.Node> queryNode(HddsProtos.NodeOperationalState opState,
* Allows a list of hosts to be decommissioned. The hosts are identified
* by their hostname and optionally port in the format foo.com:port.
* @param hosts A list of hostnames, optionally with port
+ * @param force boolean flag that forcefully tries to decommission nodes
Review Comment:
```suggestion
* @param force true to forcefully decommission Datanodes
```
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1545986133
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
Review Comment:
I have added a test for this too now using mocks. Could you please review it?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #6367:
URL: https://github.com/apache/ozone/pull/6367#issuecomment-2024442141
@Tejaskriya Is this ready for 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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1546062096
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ }
+
+ for (DatanodeDetails dn : validDns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
Review Comment:
Let's go with WARN.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1540693012
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
Review Comment:
I have moved the container related code under a synchronized block. Could you please take a look now?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1546127964
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeDecommissionManager.java:
##########
@@ -369,10 +410,208 @@ public void testNodeDecommissionManagerOnBecomeLeader() throws Exception {
assertEquals(decom.getMonitor().getTrackedNodes().size(), 3);
}
- private SCMNodeManager createNodeManager(OzoneConfiguration config)
- throws IOException, AuthenticationException {
- scm = HddsTestUtils.getScm(config);
- return (SCMNodeManager) scm.getScmNodeManager();
+ @Test
+ public void testInsufficientNodeDecommissionThrowsExceptionForRatis() throws
Review Comment:
Thank you for the review! I have added these checks in the tests. I have addressed the other comments as well. Could you please take a look at this PR again and approve the workflows if everything seems okay?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1538703474
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
Review Comment:
Yes, as you have pointed out, we cannot rely on the RM as using stale data for the fail-early check here as it would be dangerous. The only other way that came up was to iterate through the containers like in this PR
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1542914562
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
Review Comment:
Yeah, it's not very probable but it's possible that a node goes missing somehow just after the check. We should account for that possibility.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1542376836
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +384,57 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int minInService = -1, numDecom = dns.size();
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ }
+ }
+
+ for (DatanodeDetails dn : dns) {
Review Comment:
I have added test for checking nodes not-in-service. But in case of NodeNotFound, In the current logic, if we pass the hostname for a non-existent DN then it is caught in `mapHostnamesToDatanodes(nodes, errors)` and the DN is not added to the datanodeDetails list (dns) we iterate through for the checks. Do we need to still consider this scenario?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #6367:
URL: https://github.com/apache/ozone/pull/6367#issuecomment-2019562976
@sodonnel @siddhantsangwan Thank you for the reviews! I have addressed the comments left. Could you please take a look at the current changes?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1546056515
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +388,63 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ int numDecom = dns.size();
+ List<DatanodeDetails> validDns = dns.stream().collect(Collectors.toList());
+ Collections.copy(validDns, dns);
+ int inServiceTotal = nodeManager.getNodeCount(NodeStatus.inServiceHealthy());
+ for (DatanodeDetails dn : dns) {
+ try {
+ NodeStatus nodeStatus = getNodeStatus(dn);
+ NodeOperationalState opState = nodeStatus.getOperationalState();
+ if (opState != NodeOperationalState.IN_SERVICE) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ } catch (NodeNotFoundException ex) {
+ numDecom--;
+ validDns.remove(dn);
+ }
+ }
+
+ for (DatanodeDetails dn : validDns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
Review Comment:
At what level should this be logged? Would INFO be fine?
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #6367:
URL: https://github.com/apache/ozone/pull/6367#issuecomment-2031362496
Thanks everyone. Merged to master.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #6367:
URL: https://github.com/apache/ozone/pull/6367#issuecomment-2012485629
> but a user should be able to toggle this off using the decommission command.
Yea I think we need a --force flag or something to tell it to ignore the check. The reason is that a cluster could have a handful of EC 10-4, but some nodes need to be taken out anyway. The alternative to decommission is to just stop them, and that has to be done one node at a time and wait for replication to complete, so its slow.
If we have the --force flag, then it would gracefully decommission as much as it could, and all the Ratis or smaller EC replication ones. Then it would get stuck and the admin could make a decision to just stop them, knowing the remaining EC containers will not be able to be replicated.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1533252567
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
Review Comment:
We also need to check that the Datanodes being decommissioned are initially in-service only and not any other state. We'd have to account for any non in-service DNs or DNs that we're getting `NodeNotFoundException` for in our calculation also.
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #6367:
URL: https://github.com/apache/ozone/pull/6367#issuecomment-2024497318
> @Tejaskriya Is this ready for review?
@siddhantsangwan it is ready for review now. I have added few more checks in the same test cases with force flag set to true. The behaviour is as expected. I have also tested it manually in docker set-up, I have updated the results in the PR description under "How was this patch tested"
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1534032778
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
+ cif.getState().equals(HddsProtos.LifeCycleState.DELETING)) {
+ continue;
+ }
+ int reqNodes = cif.getReplicationConfig().getRequiredNodes();
+ if (reqNodes > minInService) {
Review Comment:
If you move the line `(nodeManager.getNodeCount(NodeStatus.inServiceHealthy()` to get the total nodes, you don't need to track the minInService. You can just fail early if there are not enough nodes, eg:
```
inService = odeManager.getNodeCount(NodeStatus.inServiceHealthy()) - dns.size();
for (DatanodeDetails dn : dns) {
...
if (cif.getReplicationConfig().getRequiredNodes() > inService) {
// fail early - no point in checking any further
return false;
}
}
```
```
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1534051824
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -368,6 +382,50 @@ public synchronized void startDecommission(DatanodeDetails dn)
}
}
+ private synchronized boolean checkIfDecommissionPossible(List<DatanodeDetails> dns, List<DatanodeAdminError> errors) {
+ // do we require method synchronization?
+ int minInService = -1; // maxRatis = -1, maxEc = -1;
+ for (DatanodeDetails dn : dns) {
+ Set<ContainerID> containers;
+ try {
+ containers = nodeManager.getContainers(dn);
+ } catch (NodeNotFoundException ex) {
+ LOG.warn("The host {} was not found in SCM. Ignoring the request to " +
+ "decommission it", dn.getHostName());
+ errors.add(new DatanodeAdminError(dn.getHostName(),
+ "The host was not found in SCM"));
+ continue; // ignore the DN and continue to next one
+ }
+ for (ContainerID cid : containers) {
+ ContainerInfo cif;
+ try {
+ cif = containerManager.getContainer(cid);
+ } catch (ContainerNotFoundException ex) {
+ continue; // ignore the container and continue to next one
+ }
+ if (cif.getState().equals(HddsProtos.LifeCycleState.DELETED) ||
+ cif.getState().equals(HddsProtos.LifeCycleState.DELETING)) {
+ continue;
+ }
+ int reqNodes = cif.getReplicationConfig().getRequiredNodes();
+ if (reqNodes > minInService) {
+ minInService = reqNodes;
+ }
+ /* *below code would check the replication type and then get the factor,
+ but as we have a simpler way i.e., getRequiredNodes(), I don't think we need to care about the replication type
+
+ HddsProtos.ReplicationType replicationType = cif.getReplicationType();
+ if (replicationType.equals(HddsProtos.ReplicationType.RATIS)) {
+ maxRatis = cif.getReplicationFactor().getNumber();
+ } else if (replicationType.equals(HddsProtos.ReplicationType.EC) {
+ //cif.getReplicationConfig();
+ }*/
Review Comment:
Yes, getRequiredNodes is the simplest way. Please remove the commented code too (I think you left it there just to ask the question which is perfectly fine).
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #6367:
URL: https://github.com/apache/ozone/pull/6367#discussion_r1538700923
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeDecommissionManager.java:
##########
@@ -56,17 +75,50 @@ public class TestNodeDecommissionManager {
private NodeDecommissionManager decom;
private StorageContainerManager scm;
private NodeManager nodeManager;
+ private ContainerManager containerManager;
private OzoneConfiguration conf;
+ @TempDir
+ private File testDir;
+ private DBStore dbStore;
+ private SCMHAManager scmhaManager;
+ private SequenceIdGenerator sequenceIdGen;
+ private ContainerReplicaPendingOps pendingOpsMock;
@BeforeEach
void setup(@TempDir File dir) throws Exception {
conf = new OzoneConfiguration();
conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, dir.getAbsolutePath());
- nodeManager = createNodeManager(conf);
- decom = new NodeDecommissionManager(conf, nodeManager,
+ scm = HddsTestUtils.getScm(conf);
+ nodeManager = scm.getScmNodeManager();
+ final OzoneConfiguration ozConf = SCMTestUtils.getConf(testDir);
+ dbStore = DBStoreBuilder.createDBStore(
+ ozConf, new SCMDBDefinition());
+ scmhaManager = SCMHAManagerStub.getInstance(true);
+ sequenceIdGen = new SequenceIdGenerator(
+ ozConf, scmhaManager, SCMDBDefinition.SEQUENCE_ID.getTable(dbStore));
+ final PipelineManager pipelineManager =
+ new MockPipelineManager(dbStore, scmhaManager, nodeManager);
+ pipelineManager.createPipeline(RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE));
+ pendingOpsMock = mock(ContainerReplicaPendingOps.class);
+ containerManager = new ContainerManagerImpl(ozConf,
Review Comment:
Thanks for the review, I have used mock for these tests now
--
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
Re: [PR] HDDS-10462. Fail Datanode Decommission Early [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan merged PR #6367:
URL: https://github.com/apache/ozone/pull/6367
--
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