[GitHub] [hadoop] KevinWikant commented on a change in pull request #3675: HDFS-16303. Improve handling of datanode lost while decommissioning

[GitBox <gi...@apache.org>]

KevinWikant commented on a change in pull request #3675:
URL: https://github.com/apache/hadoop/pull/3675#discussion_r754456231



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
##########
@@ -1654,4 +1658,139 @@ public Boolean get() {
 
     cleanupFile(fileSys, file);
   }
+
+  /**
+   * Test DatanodeAdminManager logic to re-queue unhealthy decommissioning nodes
+   * which are blocking the decommissioning of healthy nodes.
+   * Force the tracked nodes set to be filled with nodes lost while decommissioning,
+   * then decommission healthy nodes & validate they are decommissioned eventually.
+   */
+  @Test(timeout = 120000)
+  public void testRequeueUnhealthyDecommissioningNodes() throws Exception {
+    // Allow 3 datanodes to be decommissioned at a time
+    getConf().setInt(DFSConfigKeys.DFS_NAMENODE_DECOMMISSION_MAX_CONCURRENT_TRACKED_NODES, 3);
+    // Disable the normal monitor runs
+    getConf()
+        .setInt(MiniDFSCluster.DFS_NAMENODE_DECOMMISSION_INTERVAL_TESTING_KEY, Integer.MAX_VALUE);
+
+    // Start cluster with 6 datanodes
+    startCluster(1, 6);
+    final FSNamesystem namesystem = getCluster().getNamesystem();
+    final BlockManager blockManager = namesystem.getBlockManager();
+    final DatanodeManager datanodeManager = blockManager.getDatanodeManager();
+    final DatanodeAdminManager decomManager = datanodeManager.getDatanodeAdminManager();
+    assertEquals(6, getCluster().getDataNodes().size());
+
+    // 3 datanodes will be "live" datanodes that are expected to be decommissioned eventually
+    final List<DatanodeDescriptor> liveNodes = getCluster().getDataNodes().subList(3, 6).stream()
+        .map(dn -> getDatanodeDesriptor(namesystem, dn.getDatanodeUuid()))
+        .collect(Collectors.toList());
+    assertEquals(3, liveNodes.size());
+
+    // 3 datanodes will be "dead" datanodes that are expected to never be decommissioned
+    final List<DatanodeDescriptor> deadNodes = getCluster().getDataNodes().subList(0, 3).stream()
+        .map(dn -> getDatanodeDesriptor(namesystem, dn.getDatanodeUuid()))
+        .collect(Collectors.toList());
+    assertEquals(3, deadNodes.size());
+
+    // Need to create some data or "isNodeHealthyForDecommissionOrMaintenance"
+    // may unexpectedly return true for a dead node
+    writeFile(getCluster().getFileSystem(), new Path("/tmp/test1"), 1, 100);
+
+    // Cause the 3 "dead" nodes to be lost while in state decommissioning
+    // and fill the tracked nodes set with those 3 "dead" nodes
+    ArrayList<DatanodeInfo> decommissionedNodes = Lists.newArrayList();
+    int expectedNumTracked = 0;
+    for (final DatanodeDescriptor deadNode : deadNodes) {
+      assertEquals(0, decomManager.getNumPendingNodes());
+      assertEquals(expectedNumTracked, decomManager.getNumTrackedNodes());
+
+      // Start decommissioning the node
+      takeNodeOutofService(0, deadNode.getDatanodeUuid(), 0, decommissionedNodes,
+          AdminStates.DECOMMISSION_INPROGRESS);
+      decommissionedNodes.add(deadNode);
+
+      // Stop the datanode so that it is lost while decommissioning
+      getCluster().stopDataNode(deadNode.getXferAddr());
+      deadNode.setLastUpdate(213); // Set last heartbeat to be in the past
+
+      // Wait for the decommissioning datanode to become unhealthy
+      // & to be added to "pendingNodes"
+      final int finalExpectedNumTracked = expectedNumTracked;
+      GenericTestUtils.waitFor(() ->
+          !BlockManagerTestUtil.isNodeHealthyForDecommissionOrMaintenance(blockManager, deadNode)
+              && decomManager.getNumTrackedNodes() == finalExpectedNumTracked
+              && decomManager.getNumPendingNodes() == 1, 500, 30000);
+
+      // Run DatanodeAdminManager.Monitor & validate the node becomes tracked
+      BlockManagerTestUtil.recheckDecommissionState(datanodeManager);
+      expectedNumTracked++;
+      assertEquals(0, decomManager.getNumPendingNodes());
+      assertEquals(expectedNumTracked, decomManager.getNumTrackedNodes());
+    }
+
+    // Validate the 3 "dead" nodes are not removed from the tracked nodes set
+    // after several seconds of operation
+    final Duration checkDuration = Duration.ofSeconds(5);
+    Instant checkUntil = Instant.now().plus(checkDuration);
+    while (Instant.now().isBefore(checkUntil)) {
+      BlockManagerTestUtil.recheckDecommissionState(datanodeManager);
+      assertEquals(0, decomManager.getNumPendingNodes());
+      assertEquals(3, decomManager.getNumTrackedNodes());
+      Thread.sleep(500);
+    }
+
+    // Validate that the "live" nodes are considered healthy
+    GenericTestUtils.waitFor(() -> {
+      try {
+        return liveNodes.stream().allMatch(
+            dn -> BlockManagerTestUtil.isNodeHealthyForDecommissionOrMaintenance(blockManager, dn));
+      } catch (Exception e) {
+        LOG.warn("Exception running isNodeHealthyForDecommissionOrMaintenance", e);
+        return false;
+      }
+    }, 500, 30000);
+
+    // Start decommissioning 2 "live" datanodes
+    for (final DatanodeDescriptor liveNode : liveNodes.subList(0, 2)) {
+      takeNodeOutofService(0, liveNode.getDatanodeUuid(), 0, decommissionedNodes,
+          AdminStates.DECOMMISSION_INPROGRESS);
+      decommissionedNodes.add(liveNode);
+    }
+
+    // Validate that the live datanodes are put into the pending decommissioning queue
+    GenericTestUtils.waitFor(
+        () -> decomManager.getNumTrackedNodes() == 3 && decomManager.getNumPendingNodes() == 2, 500,
+        30000);
+
+    // Validate that 2 of the "dead" nodes are re-queued.
+    // Need to comment out this check because DatanodeAdminBackoffMonitor
+    // manages datanodes with different logic than DatanodeAdminDefaultMonitor.
+    /*
+    BlockManagerTestUtil.recheckDecommissionState(datanodeManager);
+    assertEquals(4, decomManager.getNumPendingNodes());
+    assertEquals(1, decomManager.getNumTrackedNodes());
+    */

Review comment:
       This assertion passes consistently for "TestDecommission", but fails for "TestDecommissionWithBackoffMonitor"
   
   I will investigate why its failing for "TestDecommissionWithBackoffMonitor" & get back to you




-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org