From 0b7fc5391cda6dbc22b73a9a89b3cd014768b289 Mon Sep 17 00:00:00 2001 From: Gergely Pollak Date: Wed, 2 Sep 2020 13:51:19 +0200 Subject: [PATCH 2/2] YARN-10415 Create a group matcher which checks ALL groups of the user Change-Id: I82475f3269169c9677311acc26752e631179f554 --- .../placement/CSMappingPlacementRule.java | 29 +++++---- .../placement/MappingRuleMatchers.java | 51 ++++++++++++++-- .../placement/VariableContext.java | 33 ++++++++-- .../placement/TestCSMappingPlacementRule.java | 40 +++++++++++++ .../placement/TestMappingRule.java | 2 + .../placement/TestMappingRuleMatchers.java | 60 ++++++++++++++++++- .../placement/TestVariableContext.java | 30 ++++++++++ 7 files changed, 221 insertions(+), 24 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java index 2929ae03466..fd588aad6b9 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java @@ -156,23 +156,24 @@ public boolean initialize(ResourceScheduler scheduler) throws IOException { return mappingRules.size() > 0; } - private String getPrimaryGroup(String user) throws IOException { - return groups.getGroupsSet(user).iterator().next(); - } - /** - * Traverse all secondary groups (as there could be more than one - * and position is not guaranteed) and ensure there is queue with - * the same name. + * Sets group related data for the provided variable context. + * Primary group is the first group returned by getGroups. + * To determine secondary group we traverse all groups + * (as there could be more than one and position is not guaranteed) and + * ensure there is queue with the same name. + * This method also sets the groups set for the variable context for group + * matching. + * @param vctx Variable context to be updated * @param user Name of the user - * @return Name of the secondary group if found, null otherwise * @throws IOException */ - private String getSecondaryGroup(String user) throws IOException { + private void setupGroupsForVaribleContext(VariableContext vctx, String user) + throws IOException { Set groupsSet = groups.getGroupsSet(user); String secondaryGroup = null; Iterator it = groupsSet.iterator(); - it.next(); + String primaryGroup = it.next(); while (it.hasNext()) { String group = it.next(); if (this.queueManager.getQueue(group) != null) { @@ -185,7 +186,10 @@ private String getSecondaryGroup(String user) throws IOException { LOG.debug("User {} is not associated with any Secondary " + "Group. Hence it may use the 'default' queue", user); } - return secondaryGroup; + + vctx.put("%primary_group", primaryGroup); + vctx.put("%secondary_group", secondaryGroup); + vctx.addSet("groups", groupsSet); } private VariableContext createVariableContext( @@ -195,9 +199,8 @@ private VariableContext createVariableContext( vctx.put("%user", user); vctx.put("%specified", asc.getQueue()); vctx.put("%application", asc.getApplicationName()); - vctx.put("%primary_group", getPrimaryGroup(user)); - vctx.put("%secondary_group", getSecondaryGroup(user)); vctx.put("%default", "root.default"); + setupGroupsForVaribleContext(vctx, user); vctx.setImmutables(immutableVariables); return vctx; diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingRuleMatchers.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingRuleMatchers.java index fdc239f1f01..d430c478f3d 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingRuleMatchers.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/MappingRuleMatchers.java @@ -19,6 +19,7 @@ package org.apache.hadoop.yarn.server.resourcemanager.placement; import java.util.Arrays; +import java.util.Set; /** * This class contains all the matcher and some helper methods to generate them. @@ -96,6 +97,48 @@ public String toString() { } } + /** + * VariableMatcher will check if a provided variable's value matches the + * provided value. The provided value might contain variables as well, which + * will get evaluated before the comparison. + */ + public static class GroupMatcher implements MappingRuleMatcher { + /** + * The group which should match the users's groups. + */ + private String group; + + GroupMatcher(String value) { + this.group = value; + } + + /** + * The method will replace all variables in the value, then compares this + * substituted value against the variable's value, if they match we return + * true. + * If the variable is null we always return false. + * @param variables The variable context, which contains all the variables + * @return true if the value matches the variable's value, false otherwise + */ + @Override + public boolean match(VariableContext variables) { + Set groups = variables.getSet("groups"); + + if (group == null || groups == null) { + return false; + } + + String substituted = variables.replaceVariables(group); + return groups.contains(substituted); + } + + @Override + public String toString() { + return "GroupMatcher{" + + "group='" + group + '\'' + + '}'; + } + } /** * AndMatcher is a basic boolean matcher which takes multiple other * matcher as it's arguments, and on match it checks if all of them are true. @@ -193,13 +236,13 @@ public static MappingRuleMatcher createUserMatcher(String userName) { } /** - * Convenience method to create a variable matcher which matches against the - * user's primary group. + * Convenience method to create a group matcher which matches against the + * groups of the user. * @param groupName The groupName to be matched - * @return VariableMatcher with %primary_group as the variable + * @return GroupMatcher */ public static MappingRuleMatcher createGroupMatcher(String groupName) { - return new VariableMatcher("%primary_group", groupName); + return new GroupMatcher(groupName); } /** diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java index 12adde21663..6052f8e436c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java @@ -20,10 +20,7 @@ import com.google.common.collect.ImmutableSet; -import java.util.Arrays; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; +import java.util.*; /** * This class is a key-value store for the variables and their respective values @@ -44,6 +41,12 @@ */ private Set immutableNames; + /** + * Some matchers may need to find a data in a set, which is not usable + * as a variable in substitutions, this store is for those sets. + */ + private Map> sets = new HashMap<>(); + /** * Checks if the provided variable is immutable. * @param name Name of the variable to check @@ -114,6 +117,27 @@ public String get(String name) { return ret == null ? "" : ret; } + /** + * Adds a set to the context, each name can only be added once. + * @param name Name which can be used to reference the collection + * @param set The set to be stored + */ + public void addSet(String name, Set set) { + if (sets.containsKey(name)) { + throw new IllegalStateException( + "Set '" + name + "' is already set!"); + } + sets.put(name, set); + } + + /** + * Returns the set referenced by the name. + * @param name Name of the set to be returned. + */ + public Set getSet(String name) { + return sets.get(name); + } + /** * Check if a variable is part of the context. * @param name Name of the variable to be checked @@ -195,5 +219,4 @@ public String replacePathVariables(String input) { return String.join(".", parts); } - } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestCSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestCSMappingPlacementRule.java index 5b47d34084f..ccbd839e1e3 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestCSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestCSMappingPlacementRule.java @@ -411,4 +411,44 @@ public void testAllowCreateFlag() throws IOException { engine, app, "charlie", "root.man.create"); } + + private MappingRule createGroupMapping(String group, String queue) { + MappingRuleMatcher matcher = MappingRuleMatchers.createGroupMatcher(group); + MappingRuleAction action = + (new MappingRuleActions.PlaceToQueueAction(queue, true)) + .setFallbackReject(); + return new MappingRule(matcher, action); + } + + /** + * "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"), + * "emily", ImmutableSet.of("user", "tester", "developer") + */ + @Test + public void testGroupMatching() throws IOException { + ArrayList rules = new ArrayList<>(); + + rules.add(createGroupMapping("p_alice", "root.man.p_alice")); + rules.add(createGroupMapping("developer", "root.man.developer")); + + //everybody is in the user group, this should catch all + rules.add(createGroupMapping("user", "root.man.user")); + + CSMappingPlacementRule engine = setupEngine(true, rules); + ApplicationSubmissionContext app = createApp("app"); + + assertPlace( + "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"); + } } \ No newline at end of file diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRule.java index c215c5be38f..ce7d0638368 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRule.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import com.google.common.collect.Sets; import org.apache.hadoop.util.StringUtils; import org.junit.Test; @@ -107,6 +108,7 @@ void evaluateLegacyStringTestcase( public void testLegacyEvaluation() { VariableContext matching = setupVariables( "bob", "developer", "users", "MR"); + matching.addSet("groups", Sets.newHashSet("developer")); VariableContext mismatching = setupVariables( "joe", "tester", "admins", "Spark"); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRuleMatchers.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRuleMatchers.java index d384f93884a..3f7772cf549 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRuleMatchers.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestMappingRuleMatchers.java @@ -18,6 +18,7 @@ package org.apache.hadoop.yarn.server.resourcemanager.placement; +import com.google.common.collect.Sets; import junit.framework.TestCase; import org.junit.Test; @@ -41,12 +42,14 @@ public void testVariableMatcher() { matchingContext.put("%primary_group", "developers"); matchingContext.put("%application", "TurboMR"); matchingContext.put("%custom", "Matching string"); + matchingContext.addSet("groups", Sets.newHashSet("developers")); VariableContext mismatchingContext = new VariableContext(); mismatchingContext.put("%user", "dave"); mismatchingContext.put("%primary_group", "testers"); mismatchingContext.put("%application", "Tester APP"); mismatchingContext.put("%custom", "Not matching string"); + mismatchingContext.addSet("groups", Sets.newHashSet("testers")); VariableContext emptyContext = new VariableContext(); @@ -184,16 +187,17 @@ public void testBoolOperatorMatchers() { VariableContext developerBob = new VariableContext(); developerBob.put("%user", "bob"); developerBob.put("%primary_group", "developers"); - + developerBob.addSet("groups", Sets.newHashSet("developers")); VariableContext testerBob = new VariableContext(); testerBob.put("%user", "bob"); testerBob.put("%primary_group", "testers"); + testerBob.addSet("groups", Sets.newHashSet("testers")); VariableContext testerDave = new VariableContext(); testerDave.put("%user", "dave"); testerDave.put("%primary_group", "testers"); - + testerDave.addSet("groups", Sets.newHashSet("testers")); VariableContext accountantDave = new VariableContext(); accountantDave.put("%user", "dave"); @@ -252,4 +256,56 @@ public void testToStrings() { ", " + var.toString() + "]}", or.toString()); } + @Test + public void testGroupMatching() { + VariableContext letterGroups = new VariableContext(); + letterGroups.addSet("groups", Sets.newHashSet("a", "b", "c")); + + VariableContext numberGroups = new VariableContext(); + numberGroups.addSet("groups", Sets.newHashSet("1", "2", "3")); + + VariableContext noGroups = new VariableContext(); + + MappingRuleMatcher matchA = + MappingRuleMatchers.createGroupMatcher("a"); + MappingRuleMatcher matchB = + MappingRuleMatchers.createGroupMatcher("b"); + MappingRuleMatcher matchC = + MappingRuleMatchers.createGroupMatcher("c"); + MappingRuleMatcher match1 = + MappingRuleMatchers.createGroupMatcher("1"); + MappingRuleMatcher match2 = + MappingRuleMatchers.createGroupMatcher("2"); + MappingRuleMatcher match3 = + MappingRuleMatchers.createGroupMatcher("3"); + MappingRuleMatcher matchNull = + MappingRuleMatchers.createGroupMatcher(null); + + //letter groups submission should match only the letters + assertTrue(matchA.match(letterGroups)); + assertTrue(matchB.match(letterGroups)); + assertTrue(matchC.match(letterGroups)); + assertFalse(match1.match(letterGroups)); + assertFalse(match2.match(letterGroups)); + assertFalse(match3.match(letterGroups)); + assertFalse(matchNull.match(letterGroups)); + + //numeric groups submission should match only the numbers + assertFalse(matchA.match(numberGroups)); + assertFalse(matchB.match(numberGroups)); + assertFalse(matchC.match(numberGroups)); + assertTrue(match1.match(numberGroups)); + assertTrue(match2.match(numberGroups)); + assertTrue(match3.match(numberGroups)); + assertFalse(matchNull.match(numberGroups)); + + //noGroups submission should not match anything + assertFalse(matchA.match(noGroups)); + assertFalse(matchB.match(noGroups)); + assertFalse(matchC.match(noGroups)); + assertFalse(match1.match(noGroups)); + assertFalse(match2.match(noGroups)); + assertFalse(match3.match(noGroups)); + assertFalse(matchNull.match(noGroups)); + } } \ No newline at end of file diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestVariableContext.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestVariableContext.java index d04e649b6a9..642e5e624d0 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestVariableContext.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/TestVariableContext.java @@ -21,7 +21,10 @@ import com.google.common.collect.ImmutableSet; import org.junit.Test; +import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; +import java.util.Set; import static org.junit.Assert.*; @@ -199,4 +202,31 @@ public void testVariableReplace() { assertEquals(expected, variables.replaceVariables(pattern))); } + @Test + public void testCollectionStore() { + VariableContext variables = new VariableContext(); + Set coll1 = new HashSet<>(); + Set coll2 = new HashSet<>(); + + coll1.add("Bob"); + coll1.add("Roger"); + coll2.add("Bob"); + + variables.addSet("set", coll1); + variables.addSet("sameset", coll1); + variables.addSet("list", coll2); + + try { + variables.addSet("set", coll1); + fail("Same name cannot be used multiple times to add collections"); + } catch (IllegalStateException e) { + //Exception expected + } + + assertSame(coll1, variables.getSet("set")); + assertSame(coll1, variables.getSet("sameset")); + assertSame(coll2, variables.getSet("list")); + assertNull(variables.getSet("Nothing")); + } + } -- 2.26.2