YARN-7291. Better input parsing for resource in allocation file. Contributed by Zoltan Siegl

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java

@@ -20,10 +20,12 @@
 import static org.apache.hadoop.yarn.util.resource.ResourceUtils.RESOURCE_REQUEST_VALUE_PATTERN;
 
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import com.google.common.collect.Lists;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.apache.hadoop.classification.InterfaceAudience.Private;
@@ -216,6 +218,15 @@ public class FairSchedulerConfiguration extends Configuration {
 
   private static final String INVALID_RESOURCE_DEFINITION_PREFIX =
           "Error reading resource config--invalid resource definition: ";
+  private static final String RESOURCE_PERCENTAGE_PATTERN =
+      "^(-?(\\d+)(\\.\\d*)?)\\s*%\\s*";
+  private static final String RESOURCE_VALUE_PATTERN =
+      "^(-?\\d+)(\\.\\d*)?\\s*";
+  /**
+   * For resources separated by spaces instead of a comma.
+   */
+  private static final String RESOURCES_WITH_SPACES_PATTERN =
+      "-?\\d+(?:\\.\\d*)?\\s*[a-z]+\\s*";
 
   public FairSchedulerConfiguration() {
     super();
@@ -507,7 +518,7 @@ private static ConfigurableResource parseNewStyleResource(String value,
       try {
         if (asPercent) {
           double percentage = parseNewStyleResourceAsPercentage(value,
-              resourceValue);
+              resourceName, resourceValue);
           configurableResource.setPercentage(resourceName, percentage);
         } else {
           long parsedValue = parseNewStyleResourceAsAbsoluteValue(value,
@@ -526,10 +537,10 @@ private static ConfigurableResource parseNewStyleResource(String value,
   }
 
   private static double parseNewStyleResourceAsPercentage(
-      String value, String resourceValue)
+      String value, String resource, String resourceValue)
       throws AllocationConfigurationException {
     try {
-      return findPercentage(resourceValue, "");
+      return findPercentage(resourceValue, resource);
     } catch (AllocationConfigurationException ex) {
       throw createConfigException(value,
           "The resource values must all be percentages. \""
@@ -563,18 +574,39 @@ private static ConfigurableResource parseOldStyleResourceAsPercentage(
             getResourcePercentage(StringUtils.toLowerCase(value)));
   }
 
-  private static ConfigurableResource parseOldStyleResource(String value)
+  private static ConfigurableResource parseOldStyleResource(String input)
           throws AllocationConfigurationException {
-    final String lCaseValue = StringUtils.toLowerCase(value);
+    final String lowerCaseInput = StringUtils.toLowerCase(input);
-    final int memory = parseOldStyleResourceMemory(lCaseValue);
+    String[] resources = lowerCaseInput.split(",");
-    final int vcores = parseOldStyleResourceVcores(lCaseValue);
+
+    if (resources.length != 2) {
+      resources = findOldStyleResourcesInSpaceSeparatedInput(lowerCaseInput);
+      if (resources.length != 2) {
+        throw new AllocationConfigurationException(
+            "Cannot parse resource values from input: " + input);
+      }
+    }
+    final int memory = parseOldStyleResourceMemory(resources);
+    final int vcores = parseOldStyleResourceVcores(resources);
     return new ConfigurableResource(
             BuilderUtils.newResource(memory, vcores));
   }
 
-  private static int parseOldStyleResourceMemory(String lCaseValue)
+  private static String[] findOldStyleResourcesInSpaceSeparatedInput(
+      String input) {
+    final Pattern pattern = Pattern.compile(RESOURCES_WITH_SPACES_PATTERN);
+    final Matcher matcher = pattern.matcher(input);
+
+    List<String> resources = Lists.newArrayList();
+    while (matcher.find()) {
+      resources.add(matcher.group(0));
+    }
+    return resources.toArray(new String[0]);
+  }
+
+  private static int parseOldStyleResourceMemory(String[] resources)
       throws AllocationConfigurationException {
-    final int memory = findResource(lCaseValue, "mb");
+    final int memory = findResource(resources, "mb");
 
     if (memory < 0) {
       throw new AllocationConfigurationException(
@@ -584,9 +616,9 @@ private static int parseOldStyleResourceMemory(String lCaseValue)
     return memory;
   }
 
-  private static int parseOldStyleResourceVcores(String lCaseValue)
+  private static int parseOldStyleResourceVcores(String[] resources)
       throws AllocationConfigurationException {
-    final int vcores = findResource(lCaseValue, "vcores");
+    final int vcores = findResource(resources, "vcores");
 
     if (vcores < 0) {
       throw new AllocationConfigurationException(
@@ -596,45 +628,62 @@ private static int parseOldStyleResourceVcores(String lCaseValue)
     return vcores;
   }
 
-  private static double[] getResourcePercentage(
+  private static double[] getResourcePercentage(String val)
-      String val) throws AllocationConfigurationException {
+      throws AllocationConfigurationException {
     int numberOfKnownResourceTypes = ResourceUtils
         .getNumberOfCountableResourceTypes();
     double[] resourcePercentage = new double[numberOfKnownResourceTypes];
-    String[] strings = val.split(",");
+    String[] values = val.split(",");
 
-    if (strings.length == 1) {
+    if (values.length == 1) {
-      double percentage = findPercentage(strings[0], "");
+      double percentage = findPercentage(values, "");
       for (int i = 0; i < numberOfKnownResourceTypes; i++) {
         resourcePercentage[i] = percentage;
       }
     } else {
-      resourcePercentage[0] = findPercentage(val, "memory");
+      resourcePercentage[0] = findPercentage(values, "memory");
-      resourcePercentage[1] = findPercentage(val, "cpu");
+      resourcePercentage[1] = findPercentage(values, "cpu");
     }
 
     return resourcePercentage;
   }
 
-  private static double findPercentage(String val, String units)
+  private static double findPercentage(String resourceValue, String resource)
       throws AllocationConfigurationException {
-    final Pattern pattern =
+    return findPercentageInternal(resource, resourceValue, false);
-        Pattern.compile("(-?(\\d+)(\\.\\d*)?)\\s*%\\s*" + units);
+  }
-    Matcher matcher = pattern.matcher(val);
+
-    if (!matcher.find()) {
+  private static double findPercentage(String[] resourceValues, String resource)
-      if (units.equals("")) {
+      throws AllocationConfigurationException {
+    String resourceValue = findResourceFromValues(resourceValues, resource);
+    return findPercentageInternal(resource, resourceValue, true);
+  }
+
+  private static double findPercentageInternal(String resource,
+      String resourceValue, boolean includeResourceInPattern)
+      throws AllocationConfigurationException {
+    final Pattern pattern;
+    if (includeResourceInPattern) {
+      pattern = Pattern.compile(RESOURCE_PERCENTAGE_PATTERN + resource);
+    } else {
+      pattern = Pattern.compile(RESOURCE_PERCENTAGE_PATTERN);
+    }
+
+    Matcher matcher = pattern.matcher(resourceValue);
+    if (!matcher.matches()) {
+      if (resource.equals("")) {
         throw new AllocationConfigurationException("Invalid percentage: " +
-            val);
+            resourceValue);
       } else {
-        throw new AllocationConfigurationException("Missing resource: " +
+        throw new AllocationConfigurationException("Invalid percentage of " +
-            units);
+            resource + ": " + resourceValue);
       }
     }
     double percentage = Double.parseDouble(matcher.group(1)) / 100.0;
 
     if (percentage < 0) {
       throw new AllocationConfigurationException("Invalid percentage: " +
-          val + ", percentage should not be negative!");
+          resourceValue + ", percentage should not be negative!");
     }
 
     return percentage;
@@ -659,13 +708,26 @@ public long getUpdateInterval() {
     return getLong(UPDATE_INTERVAL_MS, DEFAULT_UPDATE_INTERVAL_MS);
   }
   
-  private static int findResource(String val, String units)
+  private static int findResource(String[] resourceValues, String resource)
       throws AllocationConfigurationException {
-    final Pattern pattern = Pattern.compile("(-?\\d+)(\\.\\d*)?\\s*" + units);
+    String resourceValue = findResourceFromValues(resourceValues, resource);
-    Matcher matcher = pattern.matcher(val);
+    final Pattern pattern = Pattern.compile(RESOURCE_VALUE_PATTERN +
+        resource);
+    Matcher matcher = pattern.matcher(resourceValue);
     if (!matcher.find()) {
-      throw new AllocationConfigurationException("Missing resource: " + units);
+      throw new AllocationConfigurationException("Invalid value of " +
+          (resource.equals("mb") ? "memory" : resource) + ": " + resourceValue);
     }
     return Integer.parseInt(matcher.group(1));
   }
+
+  private static String findResourceFromValues(String[] resourceValues,
+      String resource) throws AllocationConfigurationException {
+    for (String resourceValue : resourceValues) {
+      if (resourceValue.contains(resource)) {
+        return resourceValue.trim();
+      }
+    }
+    throw new AllocationConfigurationException("Missing resource: " + resource);
+  }
 }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFairSchedulerConfiguration.java

@@ -86,6 +86,28 @@ private void expectMissingResource(String resource) {
     exception.expectMessage("Missing resource: " + resource);
   }
 
+  private void expectUnparsableResource(String resource) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("Cannot parse resource values from input: "
+        + resource);
+  }
+
+  private void expectInvalidResource(String resource) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("Invalid value of " + resource + ": ");
+  }
+
+  private void expectInvalidResourcePercentage(String resource) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("Invalid percentage of " + resource + ": ");
+  }
+
+  private void expectInvalidResourcePercentageNewStyle(String value) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("\"" + value + "\" is either " +
+        "not a non-negative number");
+  }
+
   private void expectNegativePercentageOldStyle() {
     exception.expect(AllocationConfigurationException.class);
     exception.expectMessage("percentage should not be negative");
@@ -106,6 +128,8 @@ public void testParseResourceConfigValue() throws Exception {
     Resource expected = BuilderUtils.newResource(5 * 1024, 2);
     Resource clusterResource = BuilderUtils.newResource(10 * 1024, 4);
 
+    assertEquals(expected,
+        parseResourceConfigValue("5120 mb 2 vcores").getResource());
     assertEquals(expected,
         parseResourceConfigValue("2 vcores, 5120 mb").getResource());
     assertEquals(expected,
@@ -246,26 +270,30 @@ public void testParseResourceConfigValue() throws Exception {
 
   @Test
   public void testNoUnits() throws Exception {
-    expectMissingResource("mb");
+    String value = "1024";
-    parseResourceConfigValue("1024");
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
   }
 
   @Test
   public void testOnlyMemory() throws Exception {
-    expectMissingResource("vcores");
+    String value = "1024mb";
-    parseResourceConfigValue("1024mb");
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
   }
 
   @Test
   public void testOnlyCPU() throws Exception {
-    expectMissingResource("mb");
+    String value = "1024vcores";
-    parseResourceConfigValue("1024vcores");
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
   }
 
   @Test
   public void testGibberish() throws Exception {
-    expectMissingResource("mb");
+    String value = "1o24vc0res";
-    parseResourceConfigValue("1o24vc0res");
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
   }
 
   @Test
@@ -276,7 +304,7 @@ public void testNoUnitsPercentage() throws Exception {
 
   @Test
   public void testInvalidNumPercentage() throws Exception {
-    expectMissingResource("cpu");
+    expectInvalidResourcePercentage("cpu");
     parseResourceConfigValue("95A% cpu, 50% memory");
   }
 
@@ -292,6 +320,44 @@ public void testMemoryPercentageCpuAbsolute() throws Exception {
     parseResourceConfigValue("50% memory, 2 vcores");
   }
 
+  @Test
+  public void testDuplicateVcoresDefinitionAbsolute() throws Exception {
+    expectInvalidResource("vcores");
+    parseResourceConfigValue("1024 mb, 2 4 vcores");
+  }
+
+  @Test
+  public void testDuplicateMemoryDefinitionAbsolute() throws Exception {
+    expectInvalidResource("memory");
+    parseResourceConfigValue("2048 1024 mb, 2 vcores");
+  }
+
+  @Test
+  public void testDuplicateVcoresDefinitionPercentage() throws Exception {
+    expectInvalidResourcePercentage("cpu");
+    parseResourceConfigValue("50% memory, 50% 100%cpu");
+  }
+
+  @Test
+  public void testDuplicateMemoryDefinitionPercentage() throws Exception {
+    expectInvalidResourcePercentage("memory");
+    parseResourceConfigValue("50% 80% memory, 100%cpu");
+  }
+
+  @Test
+  public void testParseNewStyleDuplicateMemoryDefinitionPercentage()
+      throws Exception {
+    expectInvalidResourcePercentageNewStyle("40% 80%");
+    parseResourceConfigValue("vcores = 75%, memory-mb = 40% 80%");
+  }
+
+  @Test
+  public void testParseNewStyleDuplicateVcoresDefinitionPercentage()
+      throws Exception {
+    expectInvalidResourcePercentageNewStyle("75% 65%");
+    parseResourceConfigValue("vcores = 75% 65%, memory-mb = 40%");
+  }
+
   @Test
   public void testMemoryPercentageNegativeValue() throws Exception {
     expectNegativePercentageOldStyle();
@@ -334,7 +400,6 @@ public void testMemoryPercentageCpuAbsoluteMemoryNegative() throws Exception {
     parseResourceConfigValue("-50% memory, 2 vcores");
   }
 
-
   @Test
   public void testAbsoluteVcoresNegative() throws Exception {
     expectNegativeValueOfResource("vcores");
@@ -383,6 +448,26 @@ public void testAbsoluteMemoryNegativeFractional() throws Exception {
     parseResourceConfigValue("  -5120.3 mb, 2.35 vcores  ");
   }
 
+  @Test
+  public void testOldStyleResourcesSeparatedBySpaces() throws Exception {
+    parseResourceConfigValue("2 vcores, 5120 mb");
+  }
+
+  @Test
+  public void testOldStyleResourcesSeparatedBySpacesInvalid() throws Exception {
+    String value = "2 vcores 5120 mb 555 mb";
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
+  }
+
+  @Test
+  public void testOldStyleResourcesSeparatedBySpacesInvalidUppercaseUnits()
+      throws Exception {
+    String value = "2 vcores 5120 MB 555 GB";
+    expectUnparsableResource(value);
+    parseResourceConfigValue(value);
+  }
+
   @Test
   public void testParseNewStyleResourceMemoryNegative() throws Exception {
     expectNegativeValueOfResource("memory");