YARN-2843. Fixed NodeLabelsManager to trim inputs for hosts and labels so as to make them work correctly. Contributed by Wangda Tan.

(cherry picked from commit 0fd97f9c1989a793b882e6678285607472a3f75a)

Conflicts:
	hadoop-yarn-project/CHANGES.txt

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

@@ -57,6 +57,7 @@ import org.apache.hadoop.yarn.server.api.protocolrecords.RefreshSuperUserGroupsC
 import org.apache.hadoop.yarn.server.api.protocolrecords.RefreshUserToGroupsMappingsRequest;
 import org.apache.hadoop.yarn.server.api.protocolrecords.RemoveFromClusterNodeLabelsRequest;
 import org.apache.hadoop.yarn.server.api.protocolrecords.ReplaceLabelsOnNodeRequest;
+import org.apache.hadoop.yarn.util.ConverterUtils;
 
 import com.google.common.collect.ImmutableMap;
 
@@ -393,18 +394,7 @@ public class RMAdminCLI extends HAAdmin {
         throw new IOException("node name cannot be empty");
       }
 
-      String nodeName;
-      int port;
-      if (nodeIdStr.contains(":")) {
-        nodeName = nodeIdStr.substring(0, nodeIdStr.indexOf(":"));
-        port = Integer.valueOf(nodeIdStr.substring(nodeIdStr.indexOf(":") + 1));
-      } else {
-        nodeName = nodeIdStr;
-        port = 0;
-      }
-
-      NodeId nodeId = NodeId.newInstance(nodeName, port);
-
+      NodeId nodeId = ConverterUtils.toNodeIdWithDefaultPort(nodeIdStr);
       map.put(nodeId, new HashSet<String>());
 
       for (int i = 1; i < splits.length; i++) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/CommonNodeLabelsManager.java

@@ -344,6 +344,7 @@ public class CommonNodeLabelsManager extends AbstractService {
    */
   public void addLabelsToNode(Map<NodeId, Set<String>> addedLabelsToNode)
       throws IOException {
+    addedLabelsToNode = normalizeNodeIdToLabels(addedLabelsToNode);
     checkAddLabelsToNode(addedLabelsToNode);
     internalAddLabelsToNode(addedLabelsToNode);
   }
@@ -409,6 +410,8 @@ public class CommonNodeLabelsManager extends AbstractService {
    */
   public void removeFromClusterNodeLabels(Collection<String> labelsToRemove)
       throws IOException {
+    labelsToRemove = normalizeLabels(labelsToRemove);
+    
     checkRemoveFromClusterNodeLabels(labelsToRemove);
 
     internalRemoveFromClusterNodeLabels(labelsToRemove);
@@ -518,6 +521,8 @@ public class CommonNodeLabelsManager extends AbstractService {
   public void
       removeLabelsFromNode(Map<NodeId, Set<String>> removeLabelsFromNode)
           throws IOException {
+    removeLabelsFromNode = normalizeNodeIdToLabels(removeLabelsFromNode);
+    
     checkRemoveLabelsFromNode(removeLabelsFromNode);
 
     internalRemoveLabelsFromNode(removeLabelsFromNode);
@@ -590,6 +595,8 @@ public class CommonNodeLabelsManager extends AbstractService {
    */
   public void replaceLabelsOnNode(Map<NodeId, Set<String>> replaceLabelsToNode)
       throws IOException {
+    replaceLabelsToNode = normalizeNodeIdToLabels(replaceLabelsToNode);
+    
     checkReplaceLabelsOnNode(replaceLabelsToNode);
 
     internalReplaceLabelsOnNode(replaceLabelsToNode);
@@ -665,7 +672,7 @@ public class CommonNodeLabelsManager extends AbstractService {
     return NO_LABEL;
   }
 
-  private Set<String> normalizeLabels(Set<String> labels) {
+  private Set<String> normalizeLabels(Collection<String> labels) {
     Set<String> newLabels = new HashSet<String>();
     for (String label : labels) {
       newLabels.add(normalizeLabel(label));
@@ -732,4 +739,15 @@ public class CommonNodeLabelsManager extends AbstractService {
       nodeCollections.put(hostName, host);
     }
   }
+  
+  protected Map<NodeId, Set<String>> normalizeNodeIdToLabels(
+      Map<NodeId, Set<String>> nodeIdToLabels) {
+    Map<NodeId, Set<String>> newMap = new HashMap<NodeId, Set<String>>();
+    for (Entry<NodeId, Set<String>> entry : nodeIdToLabels.entrySet()) {
+      NodeId id = entry.getKey();
+      Set<String> labels = entry.getValue();
+      newMap.put(id, normalizeLabels(labels)); 
+    }
+    return newMap;
+  }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/ConverterUtils.java

@@ -167,7 +167,7 @@ public class ConverterUtils {
     }
     try {
       NodeId nodeId =
-          NodeId.newInstance(parts[0], Integer.parseInt(parts[1]));
+          NodeId.newInstance(parts[0].trim(), Integer.parseInt(parts[1]));
       return nodeId;
     } catch (NumberFormatException e) {
       throw new IllegalArgumentException("Invalid port: " + parts[1], e);

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/nodelabels/NodeLabelTestBase.java

@@ -19,7 +19,7 @@
 package org.apache.hadoop.yarn.nodelabels;
 
 import java.util.Collection;
-import java.util.Iterator;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 
@@ -49,12 +49,10 @@ public class NodeLabelTestBase {
 
   public static void assertCollectionEquals(Collection<String> c1,
       Collection<String> c2) {
-    Assert.assertEquals(c1.size(), c2.size());
-    Iterator<String> i1 = c1.iterator();
-    Iterator<String> i2 = c2.iterator();
-    while (i1.hasNext()) {
-      Assert.assertEquals(i1.next(), i2.next());
-    }
+    Set<String> s1 = new HashSet<String>(c1);
+    Set<String> s2 = new HashSet<String>(c2);
+    Assert.assertEquals(s1, s2);
+    Assert.assertTrue(s1.containsAll(s2));
   }
 
   public static <E> Set<E> toSet(E... elements) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/nodelabels/TestCommonNodeLabelsManager.java

@@ -258,4 +258,27 @@ public class TestCommonNodeLabelsManager extends NodeLabelTestBase {
     Assert.assertTrue(mgr.getClusterNodeLabels().isEmpty());
     assertCollectionEquals(mgr.lastRemovedlabels, Arrays.asList("p2", "p3"));
   }
+  
+  @Test(timeout = 5000) 
+  public void testTrimLabelsWhenAddRemoveNodeLabels() throws IOException {
+    mgr.addToCluserNodeLabels(toSet(" p1"));
+    assertCollectionEquals(mgr.getClusterNodeLabels(), toSet("p1"));
+    mgr.removeFromClusterNodeLabels(toSet("p1 "));
+    Assert.assertTrue(mgr.getClusterNodeLabels().isEmpty());
+  }
+  
+  @Test(timeout = 5000) 
+  public void testTrimLabelsWhenModifyLabelsOnNodes() throws IOException {
+    mgr.addToCluserNodeLabels(toSet(" p1", "p2"));
+    mgr.addLabelsToNode(ImmutableMap.of(toNodeId("n1"), toSet("p1 ", "p2")));
+    assertMapEquals(
+        mgr.getNodeLabels(),
+        ImmutableMap.of(toNodeId("n1"), toSet("p1", "p2")));
+    mgr.replaceLabelsOnNode(ImmutableMap.of(toNodeId("n1"), toSet(" p2")));
+    assertMapEquals(
+        mgr.getNodeLabels(),
+        ImmutableMap.of(toNodeId("n1"), toSet("p2")));
+    mgr.removeLabelsFromNode(ImmutableMap.of(toNodeId("n1"), toSet("  p2 ")));
+    Assert.assertTrue(mgr.getNodeLabels().isEmpty());
+  }
 }