YARN-8738. FairScheduler should not parse negative maxResources or minResources values as positive. (Contributed by Szilard Nemeth)

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

@@ -506,11 +506,13 @@ public class FairSchedulerConfiguration extends Configuration {
       String resourceValue = parts[1].trim();
       try {
         if (asPercent) {
-          configurableResource.setPercentage(resourceName,
-              findPercentage(resourceValue, ""));
+          double percentage = parseNewStyleResourceAsPercentage(value,
+              resourceValue);
+          configurableResource.setPercentage(resourceName, percentage);
         } else {
-          configurableResource.setValue(resourceName,
-              Long.parseLong(resourceValue));
+          long parsedValue = parseNewStyleResourceAsAbsoluteValue(value,
+              resourceValue, resourceName);
+          configurableResource.setValue(resourceName, parsedValue);
         }
       } catch (ResourceNotFoundException ex) {
         throw createConfigException(value, "The "
@@ -518,22 +520,43 @@ public class FairSchedulerConfiguration extends Configuration {
             + "recognized. Please check the value of "
             + YarnConfiguration.RESOURCE_TYPES + " in the Resource "
             + "Manager's configuration files.", ex);
-      } catch (NumberFormatException ex) {
-        // This only comes from Long.parseLong()
-        throw createConfigException(value, "The "
-            + "resource values must all be integers. \"" + resourceValue
-            + "\" is not an integer.", ex);
-      } catch (AllocationConfigurationException ex) {
-        // This only comes from findPercentage()
-        throw createConfigException(value, "The "
-            + "resource values must all be percentages. \""
-            + resourceValue + "\" is either not a number or does not "
-            + "include the '%' symbol.", ex);
       }
     }
     return configurableResource;
   }
 
+  private static double parseNewStyleResourceAsPercentage(
+      String value, String resourceValue)
+      throws AllocationConfigurationException {
+    try {
+      return findPercentage(resourceValue, "");
+    } catch (AllocationConfigurationException ex) {
+      throw createConfigException(value,
+          "The resource values must all be percentages. \""
+              + resourceValue + "\" is either not a non-negative number " +
+              "or does not include the '%' symbol.", ex);
+    }
+  }
+
+  private static long parseNewStyleResourceAsAbsoluteValue(String value,
+      String resourceValue, String resourceName)
+      throws AllocationConfigurationException {
+    final long parsedValue;
+    try {
+      parsedValue = Long.parseLong(resourceValue);
+    } catch (NumberFormatException e) {
+      throw createConfigException(value, "The "
+          + "resource values must all be integers. \"" + resourceValue
+          + "\" is not an integer.", e);
+    }
+    if (parsedValue < 0) {
+      throw new AllocationConfigurationException(
+          "Invalid value of " + resourceName +
+              ": " + parsedValue + ", value should not be negative!");
+    }
+    return parsedValue;
+  }
+
   private static ConfigurableResource parseOldStyleResourceAsPercentage(
           String value) throws AllocationConfigurationException {
     return new ConfigurableResource(
@@ -543,13 +566,36 @@ public class FairSchedulerConfiguration extends Configuration {
   private static ConfigurableResource parseOldStyleResource(String value)
           throws AllocationConfigurationException {
     final String lCaseValue = StringUtils.toLowerCase(value);
-    int memory = findResource(lCaseValue, "mb");
-    int vcores = findResource(lCaseValue, "vcores");
-
+    final int memory = parseOldStyleResourceMemory(lCaseValue);
+    final int vcores = parseOldStyleResourceVcores(lCaseValue);
     return new ConfigurableResource(
             BuilderUtils.newResource(memory, vcores));
   }
 
+  private static int parseOldStyleResourceMemory(String lCaseValue)
+      throws AllocationConfigurationException {
+    final int memory = findResource(lCaseValue, "mb");
+
+    if (memory < 0) {
+      throw new AllocationConfigurationException(
+          "Invalid value of memory: " + memory +
+              ", value should not be negative!");
+    }
+    return memory;
+  }
+
+  private static int parseOldStyleResourceVcores(String lCaseValue)
+      throws AllocationConfigurationException {
+    final int vcores = findResource(lCaseValue, "vcores");
+
+    if (vcores < 0) {
+      throw new AllocationConfigurationException(
+          "Invalid value of vcores: " + vcores +
+              ", value should not be negative!");
+    }
+    return vcores;
+  }
+
   private static double[] getResourcePercentage(
       String val) throws AllocationConfigurationException {
     int numberOfKnownResourceTypes = ResourceUtils
@@ -573,7 +619,7 @@ public class FairSchedulerConfiguration extends Configuration {
   private static double findPercentage(String val, String units)
       throws AllocationConfigurationException {
     final Pattern pattern =
-        Pattern.compile("((\\d+)(\\.\\d*)?)\\s*%\\s*" + units);
+        Pattern.compile("(-?(\\d+)(\\.\\d*)?)\\s*%\\s*" + units);
     Matcher matcher = pattern.matcher(val);
     if (!matcher.find()) {
       if (units.equals("")) {
@@ -584,7 +630,14 @@ public class FairSchedulerConfiguration extends Configuration {
             units);
       }
     }
-    return Double.parseDouble(matcher.group(1)) / 100.0;
+    double percentage = Double.parseDouble(matcher.group(1)) / 100.0;
+
+    if (percentage < 0) {
+      throw new AllocationConfigurationException("Invalid percentage: " +
+          val + ", percentage should not be negative!");
+    }
+
+    return percentage;
   }
 
   private static AllocationConfigurationException createConfigException(
@@ -608,7 +661,7 @@ public class FairSchedulerConfiguration extends Configuration {
   
   private static int findResource(String val, String units)
       throws AllocationConfigurationException {
-    final Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
+    final Pattern pattern = Pattern.compile("(-?\\d+)(\\.\\d*)?\\s*" + units);
     Matcher matcher = pattern.matcher(val);
     if (!matcher.find()) {
       throw new AllocationConfigurationException("Missing resource: " + units);

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

@@ -46,7 +46,9 @@ import org.apache.log4j.AppenderSkeleton;
 import org.apache.log4j.Level;
 import org.apache.log4j.spi.LoggingEvent;
 import org.junit.Assert;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 /**
  * Tests fair scheduler configuration.
@@ -103,6 +105,29 @@ public class TestFairSchedulerConfiguration {
     }
   }
 
+  @Rule
+  public ExpectedException exception = ExpectedException.none();
+
+  private void expectMissingResource(String resource) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("Missing resource: " + resource);
+  }
+
+  private void expectNegativePercentageOldStyle() {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("percentage should not be negative");
+  }
+
+  private void expectNegativePercentageNewStyle() {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("is either not a non-negative number");
+  }
+
+  private void expectNegativeValueOfResource(String resource) {
+    exception.expect(AllocationConfigurationException.class);
+    exception.expectMessage("Invalid value of " + resource);
+  }
+
   @Test
   public void testParseResourceConfigValue() throws Exception {
     Resource expected = BuilderUtils.newResource(5 * 1024, 2);
@@ -246,48 +271,267 @@ public class TestFairSchedulerConfiguration {
             + "test1 = 50 % ").getResource(clusterResource));
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testNoUnits() throws Exception {
+    expectMissingResource("mb");
     parseResourceConfigValue("1024");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testOnlyMemory() throws Exception {
+    expectMissingResource("vcores");
     parseResourceConfigValue("1024mb");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testOnlyCPU() throws Exception {
+    expectMissingResource("mb");
     parseResourceConfigValue("1024vcores");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testGibberish() throws Exception {
+    expectMissingResource("mb");
     parseResourceConfigValue("1o24vc0res");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testNoUnitsPercentage() throws Exception {
+    expectMissingResource("cpu");
     parseResourceConfigValue("95%, 50% memory");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testInvalidNumPercentage() throws Exception {
+    expectMissingResource("cpu");
     parseResourceConfigValue("95A% cpu, 50% memory");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testCpuPercentageMemoryAbsolute() throws Exception {
+    expectMissingResource("memory");
     parseResourceConfigValue("50% cpu, 1024 mb");
   }
 
-  @Test(expected = AllocationConfigurationException.class)
+  @Test
   public void testMemoryPercentageCpuAbsolute() throws Exception {
+    expectMissingResource("cpu");
     parseResourceConfigValue("50% memory, 2 vcores");
   }
 
   @Test
-  public void testAllocationIncrementMemoryDefaultUnit() throws Exception {
+  public void testMemoryPercentageNegativeValue() throws Exception {
+    expectNegativePercentageOldStyle();
+    parseResourceConfigValue("-10% memory, 50% cpu");
+  }
+
+  @Test
+  public void testCpuPercentageNegativeValue() throws Exception {
+    expectNegativePercentageOldStyle();
+    parseResourceConfigValue("10% memory, -10% cpu");
+  }
+
+  @Test
+  public void testMemoryAndCpuPercentageNegativeValue() throws Exception {
+    expectNegativePercentageOldStyle();
+    parseResourceConfigValue("-20% memory, -10% cpu");
+  }
+
+  @Test
+  public void testCpuPercentageMemoryAbsoluteCpuNegative() throws Exception {
+    expectMissingResource("memory");
+    parseResourceConfigValue("-50% cpu, 1024 mb");
+  }
+
+  @Test
+  public void testCpuPercentageMemoryAbsoluteMemoryNegative() throws Exception {
+    expectMissingResource("memory");
+    parseResourceConfigValue("50% cpu, -1024 mb");
+  }
+
+  @Test
+  public void testMemoryPercentageCpuAbsoluteCpuNegative() throws Exception {
+    expectMissingResource("cpu");
+    parseResourceConfigValue("50% memory, -2 vcores");
+  }
+
+  @Test
+  public void testMemoryPercentageCpuAbsoluteMemoryNegative() throws Exception {
+    expectNegativePercentageOldStyle();
+    parseResourceConfigValue("-50% memory, 2 vcores");
+  }
+
+
+  @Test
+  public void testAbsoluteVcoresNegative() throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("-2 vcores,5120 mb");
+  }
+
+  @Test
+  public void testAbsoluteMemoryNegative() throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("2 vcores,-5120 mb");
+  }
+
+  @Test
+  public void testAbsoluteVcoresNegativeWithSpaces() throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("-2 vcores, 5120 mb");
+  }
+
+  @Test
+  public void testAbsoluteMemoryNegativeWithSpaces() throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("2 vcores, -5120 mb");
+  }
+
+  @Test
+  public void testAbsoluteVcoresNegativeWithMoreSpaces() throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("5120mb   mb, -2    vcores");
+  }
+
+  @Test
+  public void testAbsoluteMemoryNegativeWithMoreSpaces() throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("-5120mb   mb, 2    vcores");
+  }
+
+  @Test
+  public void testAbsoluteVcoresNegativeFractional() throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("  5120.3 mb, -2.35 vcores  ");
+  }
+
+  @Test
+  public void testAbsoluteMemoryNegativeFractional() throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("  -5120.3 mb, 2.35 vcores  ");
+  }
+
+  @Test
+  public void testParseNewStyleResourceMemoryNegative() throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("memory-mb=-5120,vcores=2");
+  }
+
+  @Test
+  public void testParseNewStyleResourceVcoresNegative() throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("memory-mb=5120,vcores=-2");
+  }
+
+  @Test
+  public void testParseNewStyleResourceMemoryNegativeWithSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("memory-mb=-5120, vcores=2");
+  }
+
+  @Test
+  public void testParseNewStyleResourceVcoresNegativeWithSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("memory-mb=5120, vcores=-2");
+  }
+
+  @Test
+  public void testParseNewStyleResourceMemoryNegativeWithMoreSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue(" vcores = 2 ,  memory-mb = -5120 ");
+  }
+
+  @Test
+  public void testParseNewStyleResourceVcoresNegativeWithMoreSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue(" vcores = -2 ,  memory-mb = 5120 ");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithCustomResourceMemoryNegative()
+      throws Exception {
+    expectNegativeValueOfResource("memory");
+    parseResourceConfigValue("vcores=2,memory-mb=-5120,test1=4");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithCustomResourceVcoresNegative()
+      throws Exception {
+    expectNegativeValueOfResource("vcores");
+    parseResourceConfigValue("vcores=-2,memory-mb=-5120,test1=4");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithCustomResourceNegative()
+      throws Exception {
+    expectNegativeValueOfResource("test1");
+    parseResourceConfigValue("vcores=2,memory-mb=5120,test1=-4");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithCustomResourceNegativeWithSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("test1");
+    parseResourceConfigValue(" vcores = 2 , memory-mb = 5120 , test1 = -4 ");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithPercentagesVcoresNegative() throws
+      Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores=-75%,memory-mb=40%");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithPercentagesMemoryNegative() throws
+      Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores=75%,memory-mb=-40%");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithPercentagesVcoresNegativeWithSpaces()
+      throws Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores=-75%, memory-mb=40%");
+  }
+
+  @Test
+  public void testParseNewStyleResourceWithPercentagesMemoryNegativeWithSpaces()
+      throws Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores=75%, memory-mb=-40%");
+  }
+
+  @Test
+  public void
+  testParseNewStyleResourceWithPercentagesVcoresNegativeWithMoreSpaces()
+      throws Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores = -75%, memory-mb = 40%");
+  }
+
+  @Test
+  public void
+  testParseNewStyleResourceWithPercentagesMemoryNegativeWithMoreSpaces()
+      throws Exception {
+    expectNegativePercentageNewStyle();
+    parseResourceConfigValue("vcores = 75%, memory-mb = -40%");
+  }
+
+  @Test
+  public void
+  testParseNewStyleResourceWithPercentagesCustomResourceNegativeWithSpaces()
+      throws Exception {
+    expectNegativeValueOfResource("test1");
+    parseResourceConfigValue(" vcores = 2 , memory-mb = 5120 , test1 = -4 ");
+  }
+
+  @Test
+  public void testAllocationIncrementMemoryDefaultUnit() {
     Configuration conf = new Configuration();
     conf.set(YarnConfiguration.RESOURCE_TYPES + "." +
         ResourceInformation.MEMORY_MB.getName() +