YARN-4975. Fair Scheduler: exception thrown when a parent queue marked 'parent' has configured child queues

(Contributed by Yufei Gu via Daniel Templeton)

(cherry picked from commit f85b74ccf9f1c1c1444cc00750b03468cbf40fb9)

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

@@ -487,6 +487,7 @@ public class AllocationFileLoaderService extends AbstractService {
     Map<ReservationACL, AccessControlList> racls = new HashMap<>();
     NodeList fields = element.getChildNodes();
     boolean isLeaf = true;
+    boolean isReservable = false;
 
     for (int j = 0; j < fields.getLength(); j++) {
       Node fieldNode = fields.item(j);
@@ -558,7 +559,7 @@ public class AllocationFileLoaderService extends AbstractService {
         racls.put(ReservationACL.SUBMIT_RESERVATIONS,
                 new AccessControlList(text));
       } else if ("reservation".equals(field.getTagName())) {
-        isLeaf = false;
+        isReservable = true;
         reservableQueues.add(queueName);
         configuredQueues.get(FSQueueType.PARENT).add(queueName);
       } else if ("allowPreemptionFrom".equals(field.getTagName())) {
@@ -577,22 +578,21 @@ public class AllocationFileLoaderService extends AbstractService {
         isLeaf = false;
       }
     }
-    if (isLeaf) {
-      // if a leaf in the alloc file is marked as type='parent'
-      // then store it under 'parent'
-      if ("parent".equals(element.getAttribute("type"))) {
-        configuredQueues.get(FSQueueType.PARENT).add(queueName);
-      } else {
-        configuredQueues.get(FSQueueType.LEAF).add(queueName);
-      }
+
+    // if a leaf in the alloc file is marked as type='parent'
+    // then store it as a parent queue
+    if (isLeaf && !"parent".equals(element.getAttribute("type"))) {
+      configuredQueues.get(FSQueueType.LEAF).add(queueName);
     } else {
-      if ("parent".equals(element.getAttribute("type"))) {
-        throw new AllocationConfigurationException("Both <reservation> and " +
-            "type=\"parent\" found for queue " + queueName + " which is " +
-            "unsupported");
+      if (isReservable) {
+        throw new AllocationConfigurationException("The configuration settings"
+            + " for " + queueName + " are invalid. A queue element that "
+            + "contains child queue elements or that has the type='parent' "
+            + "attribute cannot also include a reservation element.");
       }
       configuredQueues.get(FSQueueType.PARENT).add(queueName);
     }
+
     // Set default acls if not defined
     // The root queue defaults to all access
     for (QueueACL acl : QueueACL.values()) {

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

@@ -618,6 +618,94 @@ public class TestAllocationFileLoaderService {
     allocLoader.reloadAllocations();
   }
 
+  @Test
+  public void testParentTagWithReservation() throws Exception {
+    Configuration conf = new Configuration();
+    conf.set(FairSchedulerConfiguration.ALLOCATION_FILE, ALLOC_FILE);
+
+    PrintWriter out = new PrintWriter(new FileWriter(ALLOC_FILE));
+    out.println("<?xml version=\"1.0\"?>");
+    out.println("<allocations>");
+    out.println("<queue name=\"parent\" type=\"parent\">");
+    out.println("<reservation>");
+    out.println("</reservation>");
+    out.println("</queue>");
+    out.println("</allocations>");
+    out.close();
+
+    AllocationFileLoaderService allocLoader = new AllocationFileLoaderService();
+    allocLoader.init(conf);
+    ReloadListener confHolder = new ReloadListener();
+    allocLoader.setReloadListener(confHolder);
+    try {
+      allocLoader.reloadAllocations();
+    } catch (AllocationConfigurationException ex) {
+      assertEquals(ex.getMessage(), "The configuration settings for root.parent"
+          + " are invalid. A queue element that contains child queue elements"
+          + " or that has the type='parent' attribute cannot also include a"
+          + " reservation element.");
+    }
+  }
+
+  @Test
+  public void testParentWithReservation() throws Exception {
+    Configuration conf = new Configuration();
+    conf.set(FairSchedulerConfiguration.ALLOCATION_FILE, ALLOC_FILE);
+
+    PrintWriter out = new PrintWriter(new FileWriter(ALLOC_FILE));
+    out.println("<?xml version=\"1.0\"?>");
+    out.println("<allocations>");
+    out.println("<queue name=\"parent\">");
+    out.println("<reservation>");
+    out.println("</reservation>");
+    out.println(" <queue name=\"child\">");
+    out.println(" </queue>");
+    out.println("</queue>");
+    out.println("</allocations>");
+    out.close();
+
+    AllocationFileLoaderService allocLoader = new AllocationFileLoaderService();
+    allocLoader.init(conf);
+    ReloadListener confHolder = new ReloadListener();
+    allocLoader.setReloadListener(confHolder);
+    try {
+      allocLoader.reloadAllocations();
+    } catch (AllocationConfigurationException ex) {
+      assertEquals(ex.getMessage(), "The configuration settings for root.parent"
+          + " are invalid. A queue element that contains child queue elements"
+          + " or that has the type='parent' attribute cannot also include a"
+          + " reservation element.");
+    }
+  }
+
+  @Test
+  public void testParentTagWithChild() throws Exception {
+    Configuration conf = new Configuration();
+    conf.set(FairSchedulerConfiguration.ALLOCATION_FILE, ALLOC_FILE);
+
+    PrintWriter out = new PrintWriter(new FileWriter(ALLOC_FILE));
+    out.println("<?xml version=\"1.0\"?>");
+    out.println("<allocations>");
+    out.println("<queue name=\"parent\" type=\"parent\">");
+    out.println(" <queue name=\"child\">");
+    out.println(" </queue>");
+    out.println("</queue>");
+    out.println("</allocations>");
+    out.close();
+
+    AllocationFileLoaderService allocLoader = new AllocationFileLoaderService();
+    allocLoader.init(conf);
+    ReloadListener confHolder = new ReloadListener();
+    allocLoader.setReloadListener(confHolder);
+    allocLoader.reloadAllocations();
+    AllocationConfiguration queueConf = confHolder.allocConf;
+    // Check whether queue 'parent' and 'child' are loaded successfully
+    assertTrue(queueConf.getConfiguredQueues().get(FSQueueType.PARENT)
+        .contains("root.parent"));
+    assertTrue(queueConf.getConfiguredQueues().get(FSQueueType.LEAF)
+        .contains("root.parent.child"));
+  }
+
   /**
    * Verify that you can't have the queue name with just a non breaking
    * whitespace in the allocations file.