YARN-2762. Fixed RMAdminCLI to trim and check node-label related arguments before sending to RM. Contributed by Rohith Sharmaks

hadoop-yarn-project/CHANGES.txt

@@ -133,6 +133,9 @@ Release 2.7.0 - UNRELEASED
 
     YARN-2056. Disable preemption at Queue level (Eric Payne via jlowe)
 
+    YARN-2762. Fixed RMAdminCLI to trim and check node-label related arguments
+    before sending to RM. (Rohith Sharmaks via jianhe)
+
   OPTIMIZATIONS
 
   BUG FIXES

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/cli/RMAdminCLI.java

@@ -69,6 +69,10 @@ public class RMAdminCLI extends HAAdmin {
     RecordFactoryProvider.getRecordFactory(null);
   private boolean directlyAccessNodeLabelStore = false;
   static CommonNodeLabelsManager localNodeLabelsManager = null;
+  private static final String NO_LABEL_ERR_MSG =
+      "No cluster node-labels are specified";
+  private static final String NO_MAPPING_ERR_MSG =
+      "No node-to-labels mappings are specified";
 
   protected final static Map<String, UsageInfo> ADMIN_USAGE =
       ImmutableMap.<String, UsageInfo>builder()
@@ -332,18 +336,24 @@ public class RMAdminCLI extends HAAdmin {
     return localNodeLabelsManager;
   }
   
-  private int addToClusterNodeLabels(String args) throws IOException,
-      YarnException {
+  private Set<String> buildNodeLabelsSetFromStr(String args) {
     Set<String> labels = new HashSet<String>();
     for (String p : args.split(",")) {
-      labels.add(p);
+      if (!p.trim().isEmpty()) {
+        labels.add(p.trim());
+      }
     }
 
-    return addToClusterNodeLabels(labels);
+    if (labels.isEmpty()) {
+      throw new IllegalArgumentException(NO_LABEL_ERR_MSG);
+    }
+    return labels;
   }
 
-  private int addToClusterNodeLabels(Set<String> labels) throws IOException,
+  private int addToClusterNodeLabels(String args) throws IOException,
       YarnException {
+    Set<String> labels = buildNodeLabelsSetFromStr(args);
+
     if (directlyAccessNodeLabelStore) {
       getNodeLabelManagerInstance(getConf()).addToCluserNodeLabels(labels);
     } else {
@@ -358,10 +368,7 @@ public class RMAdminCLI extends HAAdmin {
 
   private int removeFromClusterNodeLabels(String args) throws IOException,
       YarnException {
-    Set<String> labels = new HashSet<String>();
-    for (String p : args.split(",")) {
-      labels.add(p);
-    }
+    Set<String> labels = buildNodeLabelsSetFromStr(args);
 
     if (directlyAccessNodeLabelStore) {
       getNodeLabelManagerInstance(getConf()).removeFromClusterNodeLabels(
@@ -377,7 +384,7 @@ public class RMAdminCLI extends HAAdmin {
     return 0;
   }
   
-  private Map<NodeId, Set<String>> buildNodeLabelsFromStr(String args)
+  private Map<NodeId, Set<String>> buildNodeLabelsMapFromStr(String args)
       throws IOException {
     Map<NodeId, Set<String>> map = new HashMap<NodeId, Set<String>>();
 
@@ -404,12 +411,15 @@ public class RMAdminCLI extends HAAdmin {
       }
     }
 
+    if (map.isEmpty()) {
+      throw new IllegalArgumentException(NO_MAPPING_ERR_MSG);
+    }
     return map;
   }
 
   private int replaceLabelsOnNodes(String args) throws IOException,
       YarnException {
-    Map<NodeId, Set<String>> map = buildNodeLabelsFromStr(args);
+    Map<NodeId, Set<String>> map = buildNodeLabelsMapFromStr(args);
     return replaceLabelsOnNodes(map);
   }
 
@@ -507,21 +517,21 @@ public class RMAdminCLI extends HAAdmin {
         exitCode = getGroups(usernames);
       } else if ("-addToClusterNodeLabels".equals(cmd)) {
         if (i >= args.length) {
-          System.err.println("No cluster node-labels are specified");
+          System.err.println(NO_LABEL_ERR_MSG);
           exitCode = -1;
         } else {
           exitCode = addToClusterNodeLabels(args[i]);
         }
       } else if ("-removeFromClusterNodeLabels".equals(cmd)) {
         if (i >= args.length) {
-          System.err.println("No cluster node-labels are specified");
+          System.err.println(NO_LABEL_ERR_MSG);
           exitCode = -1;
         } else {
           exitCode = removeFromClusterNodeLabels(args[i]);
         }
       } else if ("-replaceLabelsOnNode".equals(cmd)) {
         if (i >= args.length) {
-          System.err.println("No cluster node-labels are specified");
+          System.err.println(NO_MAPPING_ERR_MSG);
           exitCode = -1;
         } else {
           exitCode = replaceLabelsOnNodes(args[i]);

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/cli/TestRMAdminCLI.java

@@ -443,14 +443,31 @@ public class TestRMAdminCLI {
         new String[] { "-addToClusterNodeLabels",
             "-directlyAccessNodeLabelStore" };
     assertTrue(0 != rmAdminCLI.run(args));
+
+    // no labels, should fail at client validation
+    args = new String[] { "-addToClusterNodeLabels", " " };
+    assertTrue(0 != rmAdminCLI.run(args));
+
+    // no labels, should fail at client validation
+    args = new String[] { "-addToClusterNodeLabels", " , " };
+    assertTrue(0 != rmAdminCLI.run(args));
+
+    // successfully add labels
+    args =
+        new String[] { "-addToClusterNodeLabels", ",x,,",
+            "-directlyAccessNodeLabelStore" };
+    assertEquals(0, rmAdminCLI.run(args));
+    assertTrue(dummyNodeLabelsManager.getClusterNodeLabels().containsAll(
+        ImmutableSet.of("x")));
   }
   
   @Test
   public void testRemoveFromClusterNodeLabels() throws Exception {
     // Successfully remove labels
-    dummyNodeLabelsManager.addToCluserNodeLabels(ImmutableSet.of("x"));
+    dummyNodeLabelsManager.addToCluserNodeLabels(ImmutableSet.of("x", "y"));
     String[] args =
-        { "-removeFromClusterNodeLabels", "x", "-directlyAccessNodeLabelStore" };
+        { "-removeFromClusterNodeLabels", "x,,y",
+            "-directlyAccessNodeLabelStore" };
     assertEquals(0, rmAdminCLI.run(args));
     assertTrue(dummyNodeLabelsManager.getClusterNodeLabels().isEmpty());
     
@@ -463,6 +480,14 @@ public class TestRMAdminCLI {
         new String[] { "-removeFromClusterNodeLabels",
             "-directlyAccessNodeLabelStore" };
     assertTrue(0 != rmAdminCLI.run(args));
+
+    // no labels, should fail at client validation
+    args = new String[] { "-removeFromClusterNodeLabels", " " };
+    assertTrue(0 != rmAdminCLI.run(args));
+
+    // no labels, should fail at client validation
+    args = new String[] { "-removeFromClusterNodeLabels", ", " };
+    assertTrue(0 != rmAdminCLI.run(args));
   }
   
   @Test
@@ -487,6 +512,13 @@ public class TestRMAdminCLI {
         new String[] { "-replaceLabelsOnNode",
             "-directlyAccessNodeLabelStore" };
     assertTrue(0 != rmAdminCLI.run(args));
+
+    // no labels, should fail
+    args = new String[] { "-replaceLabelsOnNode", " " };
+    assertTrue(0 != rmAdminCLI.run(args));
+
+    args = new String[] { "-replaceLabelsOnNode", ", " };
+    assertTrue(0 != rmAdminCLI.run(args));
   }
 
   @Test