From a95f5474422f90246ea853f9745135c2224111de Mon Sep 17 00:00:00 2001
From: Leon Gao <liangg@uber.com>
Date: Sun, 31 Jan 2021 23:49:13 -0800
Subject: [PATCH] Adjust sequence when refreshing volumes to remove volumes
 first

---
 .../hadoop/hdfs/server/datanode/DataNode.java | 15 ++--
 .../datanode/TestDataNodeHotSwapVolumes.java  | 71 ++++++++++++++-----
 2 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
index 7fb729c5861e9..d624a434747d8 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
@@ -772,6 +772,14 @@ private void refreshVolumes(String newVolumes) throws IOException {
             - changedVolumes.deactivateLocations.size() <= 0) {
           throw new IOException("Attempt to remove all volumes.");
         }
+
+        try {
+          removeVolumes(changedVolumes.deactivateLocations);
+        } catch (IOException e) {
+          errorMessageBuilder.append(e.getMessage());
+          LOG.error("Failed to remove volume", e);
+        }
+
         if (!changedVolumes.newLocations.isEmpty()) {
           LOG.info("Adding new volumes: {}",
               Joiner.on(",").join(changedVolumes.newLocations));
@@ -817,13 +825,6 @@ public IOException call() {
           }
         }
 
-        try {
-          removeVolumes(changedVolumes.deactivateLocations);
-        } catch (IOException e) {
-          errorMessageBuilder.append(e.getMessage());
-          LOG.error("Failed to remove volume", e);
-        }
-
         if (errorMessageBuilder.length() > 0) {
           throw new IOException(errorMessageBuilder.toString());
         }
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
index 7efae8808c55b..91ce96a7ff91c 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeHotSwapVolumes.java
@@ -118,22 +118,7 @@ private void startDFSCluster(int numNameNodes, int numDataNodes)
   private void startDFSCluster(int numNameNodes, int numDataNodes,
       int storagePerDataNode) throws IOException {
     shutdown();
-    conf = new Configuration();
-    conf.setLong(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, BLOCK_SIZE);
-
-    /*
-     * Lower the DN heartbeat, DF rate, and recheck interval to one second
-     * so state about failures and datanode death propagates faster.
-     */
-    conf.setInt(DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY, 1);
-    conf.setInt(DFSConfigKeys.DFS_DF_INTERVAL_KEY, 1000);
-    conf.setInt(DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY,
-        1000);
-    /* Allow 1 volume failure */
-    conf.setInt(DFSConfigKeys.DFS_DATANODE_FAILED_VOLUMES_TOLERATED_KEY, 1);
-    conf.setTimeDuration(DFSConfigKeys.DFS_DATANODE_DISK_CHECK_MIN_GAP_KEY,
-        0, TimeUnit.MILLISECONDS);
-
+    conf = setConfiguration(new Configuration());
     MiniDFSNNTopology nnTopology =
         MiniDFSNNTopology.simpleFederatedTopology(numNameNodes);
 
@@ -145,6 +130,26 @@ private void startDFSCluster(int numNameNodes, int numDataNodes,
     cluster.waitActive();
   }
 
+  private Configuration setConfiguration(Configuration config) {
+    config.setLong(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, BLOCK_SIZE);
+    config.setLong(DFSConfigKeys.DFS_NAMENODE_MIN_BLOCK_SIZE_KEY, 1);
+
+    /*
+     * Lower the DN heartbeat, DF rate, and recheck interval to one second
+     * so state about failures and datanode death propagates faster.
+     */
+    config.setInt(DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY, 1);
+    config.setInt(DFSConfigKeys.DFS_DF_INTERVAL_KEY, 1000);
+    config.setInt(DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY,
+        1000);
+    /* Allow 1 volume failure */
+    config.setInt(DFSConfigKeys.DFS_DATANODE_FAILED_VOLUMES_TOLERATED_KEY, 1);
+    config.setTimeDuration(DFSConfigKeys.DFS_DATANODE_DISK_CHECK_MIN_GAP_KEY,
+        0, TimeUnit.MILLISECONDS);
+
+    return config;
+  }
+
   private void shutdown() {
     if (cluster != null) {
       cluster.shutdown();
@@ -1119,4 +1124,38 @@ public void testFullBlockReportAfterRemovingVolumes()
         any(StorageBlockReport[].class),
         any(BlockReportContext.class));
   }
+
+  @Test(timeout=60000)
+  public void testAddRemovedVolumeWithVolumeOnSameMount()
+      throws IOException, ReconfigurationException {
+    shutdown();
+    conf = setConfiguration(new Configuration());
+    conf.setBoolean(DFSConfigKeys.DFS_DATANODE_ALLOW_SAME_DISK_TIERING, true);
+    conf.setDouble(DFSConfigKeys
+        .DFS_DATANODE_RESERVE_FOR_ARCHIVE_DEFAULT_PERCENTAGE, 0.4);
+    cluster = new MiniDFSCluster.Builder(conf)
+        .numDataNodes(1)
+        .storagesPerDatanode(2)
+        .storageTypes(new StorageType[]{StorageType.DISK, StorageType.ARCHIVE})
+        .build();
+
+    DataNode dn = cluster.getDataNodes().get(0);
+    List<String> oldDirs = getDataDirs(dn);
+    List<String> newDirs = new ArrayList<>();
+    for (String s : oldDirs) {
+      if (s.contains("ARCHIVE")) {
+        s = s + "_replace";
+      }
+      newDirs.add(s);
+    }
+    // Replace should be successful.
+    String[] newVal = dn.reconfigurePropertyImpl(DFS_DATANODE_DATA_DIR_KEY,
+        String.join(",", newDirs)).split(",");
+    assertEquals(2, newVal.length);
+    for (String val : newVal) {
+      if (val.contains("ARCHIVE")) {
+        assertTrue(val.contains("_replace"));
+      }
+    }
+  }
 }