YARN-10635. CSMapping rule can return paths with empty parts. Contributed by Gergely Pollak.

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java

@@ -194,6 +194,7 @@ public class CSMappingPlacementRule extends PlacementRule {
     String secondaryGroup = null;
     Iterator<String> it = groupsSet.iterator();
     String primaryGroup = it.next();
+
     while (it.hasNext()) {
       String group = it.next();
       if (this.queueManager.getQueue(group) != null) {
@@ -203,8 +204,7 @@ public class CSMappingPlacementRule extends PlacementRule {
     }
 
     if (secondaryGroup == null && LOG.isDebugEnabled()) {
-      LOG.debug("User {} is not associated with any Secondary " +
-          "Group. Hence it may use the 'default' queue", user);
+      LOG.debug("User {} is not associated with any Secondary group", user);
     }
 
     vctx.put("%primary_group", primaryGroup);
@@ -223,7 +223,15 @@ public class CSMappingPlacementRule extends PlacementRule {
     //To place queues specifically to default, users must use root.default
     if (!asc.getQueue().equals(YarnConfiguration.DEFAULT_QUEUE_NAME)) {
       vctx.put("%specified", asc.getQueue());
+    } else {
+      //Adding specified as empty will prevent it to be undefined and it won't
+      //try to place the application to a queue named '%specified', queue path
+      //validation will reject the empty path or the path with empty parts,
+      //so we sill still hit the fallback action of this rule if no queue
+      //is specified
+      vctx.put("%specified", "");
     }
+
     vctx.put("%application", asc.getApplicationName());
     vctx.put("%default", "root.default");
     try {
@@ -239,6 +247,12 @@ public class CSMappingPlacementRule extends PlacementRule {
   private String validateAndNormalizeQueue(
       String queueName, boolean allowCreate) throws YarnException {
     MappingQueuePath path = new MappingQueuePath(queueName);
+
+    if (path.hasEmptyPart()) {
+      throw new YarnException("Invalid path returned by rule: '" +
+          queueName + "'");
+    }
+
     String leaf = path.getLeafName();
     String parent = path.getParent();
 
@@ -335,14 +349,19 @@ public class CSMappingPlacementRule extends PlacementRule {
       MappingRule rule, VariableContext variables) {
     MappingRuleResult result = rule.evaluate(variables);
 
+    if (LOG.isDebugEnabled()) {
+      LOG.debug("Evaluated rule '{}' with result: '{}'", rule, result);
+    }
+
     if (result.getResult() == MappingRuleResultType.PLACE) {
       try {
         result.updateNormalizedQueue(validateAndNormalizeQueue(
             result.getQueue(), result.isCreateAllowed()));
       } catch (Exception e) {
-        LOG.info("Cannot place to queue '{}' returned by mapping rule. " +
-            "Reason: {}", result.getQueue(), e.getMessage());
         result = rule.getFallback();
+        LOG.info("Cannot place to queue '{}' returned by mapping rule. " +
+            "Reason: '{}' Fallback operation: '{}'",
+            result.getQueue(), e.getMessage(), result);
       }
     }
 
@@ -451,6 +470,12 @@ public class CSMappingPlacementRule extends PlacementRule {
       }
     }
 
+    if (LOG.isDebugEnabled()) {
+      LOG.debug("Placement final result '{}' for application '{}'",
+          (ret == null ? "null" : ret.getFullQueuePath()),
+          asc.getApplicationId());
+    }
+
     return ret;
   }
 

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingQueuePath.java

@@ -69,6 +69,21 @@ public class MappingQueuePath {
     }
   }
 
+  /**
+   * Simple helper method to determine if the path contains any empty parts.
+   * @return true if there is at least one empty part of the path
+   */
+  public boolean hasEmptyPart() {
+    String[] parts = getFullPath().split("\\.");
+    for (int i = 0; i < parts.length; i++) {
+      if (parts[i].equals("")) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
   /**
    * Getter for the parent part of the path.
    * @return Parent path of the queue, null if there is no parent.

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingRuleActions.java

@@ -66,13 +66,15 @@ public final class MappingRuleActions {
      * This method is the main logic of the action, it will replace all the
      * variables in the queuePattern with their respective values, then returns
      * a placementResult with the final queue name.
+     *
      * @param variables The variable context, which contains all the variables
      * @return The result of the action
      */
     @Override
     public MappingRuleResult execute(VariableContext variables) {
-      String substituted = variables.replacePathVariables(queuePattern);
-      return MappingRuleResult.createPlacementResult(substituted, allowCreate);
+        String substituted = variables.replacePathVariables(queuePattern);
+        return MappingRuleResult.createPlacementResult(
+            substituted, allowCreate);
     }
 
     /**

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestCSMappingPlacementRule.java

@@ -64,7 +64,7 @@ public class TestCSMappingPlacementRule {
       "alice", ImmutableSet.of("p_alice", "user", "developer"),
       "bob", ImmutableSet.of("p_bob", "user", "developer"),
       "charlie", ImmutableSet.of("p_charlie", "user", "tester"),
-      "dave", ImmutableSet.of("user", "tester"),
+      "dave", ImmutableSet.of("user"),
       "emily", ImmutableSet.of("user", "tester", "developer")
   );
 
@@ -90,6 +90,7 @@ public class TestCSMappingPlacementRule {
         .withQueue("root.disambiguous.deep.disambiuser.disambi")
         .withQueue("root.disambiguous.deep.group.developer")
         .withManagedParentQueue("root.disambiguous.deep.dman")
+        .withDynamicParentQueue("root.dynamic")
         .build();
 
     when(queueManager.getQueue(isNull())).thenReturn(null);
@@ -151,8 +152,9 @@ public class TestCSMappingPlacementRule {
   private void assertReject(String message, CSMappingPlacementRule engine,
       ApplicationSubmissionContext asc, String user) {
     try {
-      engine.getPlacementForApp(asc, user);
-      fail(message);
+      ApplicationPlacementContext apc = engine.getPlacementForApp(asc, user);
+      fail("Unexpected queue result: " + apc.getFullQueuePath() + " - " +
+          message);
     } catch (YarnException e) {
       //To prevent PlacementRule chaining present in PlacementManager
       //when an application is rejected an exception is thrown to make sure
@@ -483,14 +485,36 @@ public class TestCSMappingPlacementRule {
   }
 
   @Test
-  public void testGroupMatching() throws IOException {
+  public void testGroupTargetMatching() throws IOException {
     ArrayList<MappingRule> rules = new ArrayList<>();
 
-    rules.add(createGroupMapping("p_alice", "root.man.p_alice"));
-    rules.add(createGroupMapping("developer", "root.man.developer"));
+    rules.add(
+        new MappingRule(
+            MappingRuleMatchers.createUserMatcher("alice"),
+            (new MappingRuleActions.PlaceToQueueAction(
+                "root.man.%primary_group", true))
+                .setFallbackReject()));
+
+    rules.add(
+        new MappingRule(
+            MappingRuleMatchers.createUserMatcher("bob"),
+            (new MappingRuleActions.PlaceToQueueAction(
+                "root.dynamic.%secondary_group.%user", true))
+                .setFallbackReject()));
+
+    rules.add(
+        new MappingRule(
+            MappingRuleMatchers.createUserMatcher("charlie"),
+            (new MappingRuleActions.PlaceToQueueAction(
+                "root.man.%secondary_group", true))
+                .setFallbackReject()));
 
-    //everybody is in the user group, this should catch all
-    rules.add(createGroupMapping("user", "root.man.user"));
+    rules.add(
+        new MappingRule(
+            MappingRuleMatchers.createUserMatcher("dave"),
+            (new MappingRuleActions.PlaceToQueueAction(
+                "root.dynamic.%secondary_group.%user", true))
+                .setFallbackReject()));
 
     CSMappingPlacementRule engine = setupEngine(true, rules);
     ApplicationSubmissionContext app = createApp("app");
@@ -499,12 +523,15 @@ public class TestCSMappingPlacementRule {
         "Alice should be placed to root.man.p_alice based on her primary group",
         engine, app, "alice", "root.man.p_alice");
     assertPlace(
-        "Bob should be placed to root.man.developer based on his developer " +
-        "group", engine, app, "bob", "root.man.developer");
-    assertPlace(
-        "Charlie should be placed to root.man.user because he is not a " +
-        "developer nor in the p_alice group", engine, app, "charlie",
-        "root.man.user");
+        "Bob should be placed to root.dynamic.developer.bob based on his " +
+        "secondary group, since we have a queue named 'developer', bob " +
+        "identifies as a user with secondary_group 'developer'", engine, app,
+        "bob", "root.dynamic.developer.bob");
+    assertReject("Charlie should get rejected because he neither of his" +
+        "groups have an ambiguous queue, so effectively he has no secondary " +
+        "group", engine, app, "charlie");
+    assertReject("Dave should get rejected because he has no secondary group",
+        engine, app, "dave");
   }
 
   void assertConfigTestResult(List<MappingRule> rules) {