HDFS-13853. RBF: RouterAdmin update cmd is overwriting the entry not updating the existing. Contributed by Ayush Saxena.

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java

@@ -22,8 +22,10 @@ import java.net.InetSocketAddress;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.LinkedHashMap;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 import org.apache.hadoop.classification.InterfaceAudience.Private;
 import org.apache.hadoop.conf.Configuration;
@@ -138,9 +140,10 @@ public class RouterAdmin extends Configured implements Tool {
           + "[-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] "
           + "-owner <owner> -group <group> -mode <mode>]";
     } else if (cmd.equals("-update")) {
-      return "\t[-update <source> <nameservice1, nameservice2, ...> "
-          + "<destination> "
-          + "[-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] "
+      return "\t[-update <source>"
+          + " [<nameservice1, nameservice2, ...> <destination>] "
+          + "[-readonly true|false] [-faulttolerant true|false]"
+          + " [-order HASH|LOCAL|RANDOM|HASH_ALL] "
           + "-owner <owner> -group <group> -mode <mode>]";
     } else if (cmd.equals("-rm")) {
       return "\t[-rm <source>]";
@@ -294,6 +297,8 @@ public class RouterAdmin extends Configured implements Tool {
       } else if ("-update".equals(cmd)) {
         if (updateMount(argv, i)) {
           System.out.println("Successfully updated mount point " + argv[i]);
+          System.out.println(
+              "WARN: Changing order/destinations may lead to inconsistencies");
         } else {
           exitCode = -1;
         }
@@ -366,6 +371,10 @@ public class RouterAdmin extends Configured implements Tool {
         e.printStackTrace();
         debugException = ex;
       }
+    } catch (IOException ioe) {
+      exitCode = -1;
+      System.err.println(cmd.substring(1) + ": " + ioe.getLocalizedMessage());
+      printUsage(cmd);
     } catch (Exception e) {
       exitCode = -1;
       debugException = e;
@@ -473,17 +482,7 @@ public class RouterAdmin extends Configured implements Tool {
     mount = normalizeFileSystemPath(mount);
     // Get the existing entry
     MountTableManager mountTable = client.getMountTableManager();
-    GetMountTableEntriesRequest getRequest =
-        GetMountTableEntriesRequest.newInstance(mount);
-    GetMountTableEntriesResponse getResponse =
-        mountTable.getMountTableEntries(getRequest);
-    List<MountTable> results = getResponse.getEntries();
-    MountTable existingEntry = null;
-    for (MountTable result : results) {
-      if (mount.equals(result.getSourcePath())) {
-        existingEntry = result;
-      }
-    }
+    MountTable existingEntry = getMountEntry(mount, mountTable);
 
     if (existingEntry == null) {
       // Create and add the entry if it doesn't exist
@@ -579,100 +578,81 @@ public class RouterAdmin extends Configured implements Tool {
    * @throws IOException If there is an error.
    */
   public boolean updateMount(String[] parameters, int i) throws IOException {
-    // Mandatory parameters
     String mount = parameters[i++];
-    String[] nss = parameters[i++].split(",");
-    String dest = parameters[i++];
-
-    // Optional parameters
-    boolean readOnly = false;
-    boolean faultTolerant = false;
-    String owner = null;
-    String group = null;
-    FsPermission mode = null;
-    DestinationOrder order = null;
-    while (i < parameters.length) {
-      if (parameters[i].equals("-readonly")) {
-        readOnly = true;
-      } else if (parameters[i].equals("-faulttolerant")) {
-        faultTolerant = true;
-      } else if (parameters[i].equals("-order")) {
-        i++;
-        try {
-          order = DestinationOrder.valueOf(parameters[i]);
-        } catch(Exception e) {
-          System.err.println("Cannot parse order: " + parameters[i]);
-        }
-      } else if (parameters[i].equals("-owner")) {
-        i++;
-        owner = parameters[i];
-      } else if (parameters[i].equals("-group")) {
-        i++;
-        group = parameters[i];
-      } else if (parameters[i].equals("-mode")) {
-        i++;
-        short modeValue = Short.parseShort(parameters[i], 8);
-        mode = new FsPermission(modeValue);
-      } else {
-        printUsage("-update");
-        return false;
-      }
-
-      i++;
-    }
-
-    return updateMount(mount, nss, dest, readOnly, faultTolerant, order,
-        new ACLEntity(owner, group, mode));
-  }
-
-  /**
-   * Update a mount table entry.
-   *
-   * @param mount Mount point.
-   * @param nss Nameservices where this is mounted to.
-   * @param dest Destination path.
-   * @param readonly If the mount point is read only.
-   * @param order Order of the destination locations.
-   * @param aclInfo the ACL info for mount point.
-   * @return If the mount point was updated.
-   * @throws IOException Error updating the mount point.
-   */
-  public boolean updateMount(String mount, String[] nss, String dest,
-      boolean readonly, boolean faultTolerant,
-      DestinationOrder order, ACLEntity aclInfo)
-      throws IOException {
     mount = normalizeFileSystemPath(mount);
     MountTableManager mountTable = client.getMountTableManager();
-
-    // Create a new entry
-    Map<String, String> destMap = new LinkedHashMap<>();
-    for (String ns : nss) {
-      destMap.put(ns, dest);
-    }
-    MountTable newEntry = MountTable.newInstance(mount, destMap);
-
-    newEntry.setReadOnly(readonly);
-    newEntry.setFaultTolerant(faultTolerant);
-
-    if (order != null) {
-      newEntry.setDestOrder(order);
-    }
-
-    // Update ACL info of mount table entry
-    if (aclInfo.getOwner() != null) {
-      newEntry.setOwnerName(aclInfo.getOwner());
+    MountTable existingEntry = getMountEntry(mount, mountTable);
+    if (existingEntry == null) {
+      throw new IOException(mount + " doesn't exist.");
     }
+    // Check if the destination needs to be updated.
 
-    if (aclInfo.getGroup() != null) {
-      newEntry.setGroupName(aclInfo.getGroup());
+    if (!parameters[i].startsWith("-")) {
+      String[] nss = parameters[i++].split(",");
+      String dest = parameters[i++];
+      Map<String, String> destMap = new LinkedHashMap<>();
+      for (String ns : nss) {
+        destMap.put(ns, dest);
+      }
+      final List<RemoteLocation> locations = new LinkedList<>();
+      for (Entry<String, String> entry : destMap.entrySet()) {
+        String nsId = entry.getKey();
+        String path = normalizeFileSystemPath(entry.getValue());
+        RemoteLocation location = new RemoteLocation(nsId, path, mount);
+        locations.add(location);
+      }
+      existingEntry.setDestinations(locations);
     }
-
-    if (aclInfo.getMode() != null) {
-      newEntry.setMode(aclInfo.getMode());
+    try {
+      while (i < parameters.length) {
+        switch (parameters[i]) {
+        case "-readonly":
+          i++;
+          existingEntry.setReadOnly(getBooleanValue(parameters[i]));
+          break;
+        case "-faulttolerant":
+          i++;
+          existingEntry.setFaultTolerant(getBooleanValue(parameters[i]));
+          break;
+        case "-order":
+          i++;
+          try {
+            existingEntry.setDestOrder(DestinationOrder.valueOf(parameters[i]));
+            break;
+          } catch (Exception e) {
+            throw new Exception("Cannot parse order: " + parameters[i]);
+          }
+        case "-owner":
+          i++;
+          existingEntry.setOwnerName(parameters[i]);
+          break;
+        case "-group":
+          i++;
+          existingEntry.setGroupName(parameters[i]);
+          break;
+        case "-mode":
+          i++;
+          short modeValue = Short.parseShort(parameters[i], 8);
+          existingEntry.setMode(new FsPermission(modeValue));
+          break;
+        default:
+          printUsage("-update");
+          return false;
+        }
+        i++;
+      }
+    } catch (IllegalArgumentException iae) {
+      throw iae;
+    } catch (Exception e) {
+      String msg = "Unable to parse arguments: " + e.getMessage();
+      if (e instanceof ArrayIndexOutOfBoundsException) {
+        msg = "Unable to parse arguments: no value provided for "
+            + parameters[i - 1];
+      }
+      throw new IOException(msg);
     }
-
     UpdateMountTableEntryRequest updateRequest =
-        UpdateMountTableEntryRequest.newInstance(newEntry);
+        UpdateMountTableEntryRequest.newInstance(existingEntry);
     UpdateMountTableEntryResponse updateResponse =
         mountTable.updateMountTableEntry(updateRequest);
     boolean updated = updateResponse.getStatus();
@@ -682,6 +662,45 @@ public class RouterAdmin extends Configured implements Tool {
     return updated;
   }
 
+  /**
+   * Parse string to boolean.
+   * @param value the string to be parsed.
+   * @return parsed boolean value.
+   * @throws Exception if other than true|false is provided.
+   */
+  private boolean getBooleanValue(String value) throws Exception {
+    if (value.equalsIgnoreCase("true")) {
+      return true;
+    } else if (value.equalsIgnoreCase("false")) {
+      return false;
+    }
+    throw new IllegalArgumentException("Invalid argument: " + value
+        + ". Please specify either true or false.");
+  }
+
+  /**
+   * Gets the mount table entry.
+   * @param mount name of the mount entry.
+   * @param mountTable the mount table.
+   * @return corresponding mount entry.
+   * @throws IOException in case of failure to retrieve mount entry.
+   */
+  private MountTable getMountEntry(String mount, MountTableManager mountTable)
+      throws IOException {
+    GetMountTableEntriesRequest getRequest =
+        GetMountTableEntriesRequest.newInstance(mount);
+    GetMountTableEntriesResponse getResponse =
+        mountTable.getMountTableEntries(getRequest);
+    List<MountTable> results = getResponse.getEntries();
+    MountTable existingEntry = null;
+    for (MountTable result : results) {
+      if (mount.equals(result.getSourcePath())) {
+        existingEntry = result;
+      }
+    }
+    return existingEntry;
+  }
+
   /**
    * Remove mount point.
    *

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java

@@ -536,8 +536,8 @@ public class TestRouterAdminCLI {
     argv = new String[] {"-update", src, nsId};
     assertEquals(-1, ToolRunner.run(admin, argv));
     assertTrue("Wrong message: " + out, out.toString().contains(
-        "\t[-update <source> <nameservice1, nameservice2, ...> <destination> "
-            + "[-readonly] [-faulttolerant] "
+        "\t[-update <source> [<nameservice1, nameservice2, ...> <destination>] "
+            + "[-readonly true|false] [-faulttolerant true|false] "
             + "[-order HASH|LOCAL|RANDOM|HASH_ALL] "
             + "-owner <owner> -group <group> -mode <mode>]"));
     out.reset();
@@ -581,9 +581,9 @@ public class TestRouterAdminCLI {
         + "\t[-add <source> <nameservice1, nameservice2, ...> <destination> "
         + "[-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] "
         + "-owner <owner> -group <group> -mode <mode>]\n"
-        + "\t[-update <source> <nameservice1, nameservice2, ...> "
-        + "<destination> "
-        + "[-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] "
+        + "\t[-update <source> [<nameservice1, nameservice2, ...> "
+        + "<destination>] [-readonly true|false]"
+        + " [-faulttolerant true|false] [-order HASH|LOCAL|RANDOM|HASH_ALL] "
         + "-owner <owner> -group <group> -mode <mode>]\n" + "\t[-rm <source>]\n"
         + "\t[-ls <path>]\n"
         + "\t[-getDestination <path>]\n"
@@ -902,23 +902,15 @@ public class TestRouterAdminCLI {
 
   @Test
   public void testUpdateNonExistingMountTable() throws Exception {
-    System.setOut(new PrintStream(out));
+    System.setErr(new PrintStream(err));
     String nsId = "ns0";
     String src = "/test-updateNonExistingMounttable";
     String dest = "/updateNonExistingMounttable";
     String[] argv = new String[] {"-update", src, nsId, dest};
-    assertEquals(0, ToolRunner.run(admin, argv));
-
-    stateStore.loadCache(MountTableStoreImpl.class, true);
-    GetMountTableEntriesRequest getRequest =
-        GetMountTableEntriesRequest.newInstance(src);
-    GetMountTableEntriesResponse getResponse =
-        client.getMountTableManager().getMountTableEntries(getRequest);
-    // Ensure the destination updated successfully
-    MountTable mountTable = getResponse.getEntries().get(0);
-    assertEquals(src, mountTable.getSourcePath());
-    assertEquals(nsId, mountTable.getDestinations().get(0).getNameserviceId());
-    assertEquals(dest, mountTable.getDestinations().get(0).getDest());
+    // Update shall fail if the mount entry doesn't exist.
+    assertEquals(-1, ToolRunner.run(admin, argv));
+    assertTrue(err.toString(), err.toString()
+        .contains("update: /test-updateNonExistingMounttable doesn't exist."));
   }
 
   @Test
@@ -997,6 +989,106 @@ public class TestRouterAdminCLI {
     assertEquals(newDest, mountTable.getDestinations().get(0).getDest());
   }
 
+  @Test
+  public void testUpdateChangeAttributes() throws Exception {
+    // Add a mount table firstly
+    String nsId = "ns0";
+    String src = "/mount";
+    String dest = "/dest";
+    String[] argv = new String[] {"-add", src, nsId, dest, "-readonly",
+        "-order", "HASH_ALL"};
+    assertEquals(0, ToolRunner.run(admin, argv));
+
+    stateStore.loadCache(MountTableStoreImpl.class, true);
+    GetMountTableEntriesRequest getRequest =
+        GetMountTableEntriesRequest.newInstance(src);
+    GetMountTableEntriesResponse getResponse =
+        client.getMountTableManager().getMountTableEntries(getRequest);
+    // Ensure mount table added successfully
+    MountTable mountTable = getResponse.getEntries().get(0);
+    assertEquals(src, mountTable.getSourcePath());
+
+    // Update the destination
+    String newNsId = "ns0";
+    String newDest = "/newDestination";
+    argv = new String[] {"-update", src, newNsId, newDest};
+    assertEquals(0, ToolRunner.run(admin, argv));
+
+    stateStore.loadCache(MountTableStoreImpl.class, true);
+    getResponse =
+        client.getMountTableManager().getMountTableEntries(getRequest);
+    // Ensure the destination updated successfully and other attributes are
+    // preserved.
+    mountTable = getResponse.getEntries().get(0);
+    assertEquals(src, mountTable.getSourcePath());
+    assertEquals(newNsId,
+        mountTable.getDestinations().get(0).getNameserviceId());
+    assertEquals(newDest, mountTable.getDestinations().get(0).getDest());
+    assertTrue(mountTable.isReadOnly());
+    assertEquals("HASH_ALL", mountTable.getDestOrder().toString());
+
+    // Update the attribute.
+    argv = new String[] {"-update", src, "-readonly", "false"};
+    assertEquals(0, ToolRunner.run(admin, argv));
+
+    stateStore.loadCache(MountTableStoreImpl.class, true);
+    getResponse =
+        client.getMountTableManager().getMountTableEntries(getRequest);
+
+    // Ensure the attribute updated successfully and destination and other
+    // attributes are preserved.
+    mountTable = getResponse.getEntries().get(0);
+    assertEquals(src, mountTable.getSourcePath());
+    assertEquals(newNsId,
+        mountTable.getDestinations().get(0).getNameserviceId());
+    assertEquals(newDest, mountTable.getDestinations().get(0).getDest());
+    assertFalse(mountTable.isReadOnly());
+    assertEquals("HASH_ALL", mountTable.getDestOrder().toString());
+
+  }
+
+  @Test
+  public void testUpdateErrorCase() throws Exception {
+    // Add a mount table firstly
+    String nsId = "ns0";
+    String src = "/mount";
+    String dest = "/dest";
+    String[] argv = new String[] {"-add", src, nsId, dest, "-readonly",
+        "-order", "HASH_ALL"};
+    assertEquals(0, ToolRunner.run(admin, argv));
+    stateStore.loadCache(MountTableStoreImpl.class, true);
+
+    // Check update for non-existent mount entry.
+    argv = new String[] {"-update", "/noMount", "-readonly", "false"};
+    System.setErr(new PrintStream(err));
+    assertEquals(-1, ToolRunner.run(admin, argv));
+    assertTrue(err.toString(),
+        err.toString().contains("update: /noMount doesn't exist."));
+    err.reset();
+
+    // Check update if non true/false value is passed for readonly.
+    argv = new String[] {"-update", src, "-readonly", "check"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+    assertTrue(err.toString(), err.toString().contains("update: "
+        + "Invalid argument: check. Please specify either true or false."));
+    err.reset();
+
+    // Check update with missing value is passed for faulttolerant.
+    argv = new String[] {"-update", src, "ns1", "/tmp", "-faulttolerant"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+    assertTrue(err.toString(),
+        err.toString().contains("update: Unable to parse arguments:"
+            + " no value provided for -faulttolerant"));
+    err.reset();
+
+    // Check update with invalid order.
+    argv = new String[] {"-update", src, "ns1", "/tmp", "-order", "Invalid"};
+    assertEquals(-1, ToolRunner.run(admin, argv));
+    assertTrue(err.toString(), err.toString().contains(
+        "update: Unable to parse arguments: Cannot parse order: Invalid"));
+    err.reset();
+  }
+
   @Test
   public void testUpdateReadonlyUserGroupPermissionMountable()
       throws Exception {
@@ -1022,7 +1114,7 @@ public class TestRouterAdminCLI {
     // Update the readonly, owner, group and permission
     String testOwner = "test_owner";
     String testGroup = "test_group";
-    argv = new String[] {"-update", src, nsId, dest, "-readonly",
+    argv = new String[] {"-update", src, nsId, dest, "-readonly", "true",
         "-owner", testOwner, "-group", testGroup, "-mode", "0455"};
     assertEquals(0, ToolRunner.run(admin, argv));
 

hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSCommands.md

@@ -429,7 +429,7 @@ Usage:
 
       hdfs dfsrouteradmin
           [-add <source> <nameservice1, nameservice2, ...> <destination> [-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] -owner <owner> -group <group> -mode <mode>]
-          [-update <source> <nameservice1, nameservice2, ...> <destination> [-readonly] [-faulttolerant] [-order HASH|LOCAL|RANDOM|HASH_ALL] -owner <owner> -group <group> -mode <mode>]
+          [-update <source> [<nameservice1, nameservice2, ...> <destination>] [-readonly true|false] [-faulttolerant true|false] [-order HASH|LOCAL|RANDOM|HASH_ALL] -owner <owner> -group <group> -mode <mode>]
           [-rm <source>]
           [-ls <path>]
           [-getDestination <path>]
@@ -444,7 +444,7 @@ Usage:
 | COMMAND\_OPTION | Description |
 |:---- |:---- |
 | `-add` *source* *nameservices* *destination* | Add a mount table entry or update if it exists. |
-| `-update` *source* *nameservices* *destination* | Update a mount table entry or create one if it does not exist. |
+| `-update` *source* *nameservices* *destination* | Update a mount table entry attribures. |
 | `-rm` *source* | Remove mount point of specified path. |
 | `-ls` *path* | List mount points under specified path. |
 | `-getDestination` *path* | Get the subcluster where a file is or should be created. |